| Message ID | 87furdnubd.fsf@tromey.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 13, 2016 at 9:42 PM, Tom Tromey <tom@tromey.com> wrote: >>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes: > > Yao> check_typedef has side effects, IIUC, so we can't remove the call to it. > > Tom> Here's a new version with this restored. > > Yao> It is good to me. > > After rebasing, this patch needed a small update -- jit.c now has some > newly unused variables. > > Please review, thanks. > It is good to me.
On Wed, 13 Jul 2016, Tom Tromey wrote: > commit c097ec493b76209e6cf830ffca9b36fe2c2643fc > Author: Tom Tromey <tom@tromey.com> > Date: Mon Jun 6 14:18:30 2016 -0600 > > Remove unused variables > > This patch removes set-but-unused variables. This holds all the > removals I consider to be simple and relatively uncontroversial. I see you got to committing it actually now, before I got to the MIPS part (sorry about it), already proposed by Trevor Saunders previously BTW. > 2016-07-13 Tom Tromey <tom@tromey.com> > > * mips-tdep.c (micromips_scan_prologue): Remove "frame_addr". > (mips_o32_push_dummy_call): Remove "stack_used_p". TBH I disagree the changes to `micromips_scan_prologue' are uncontroversial -- it looks to me like the presence of `frame_addr', clearly copied over from `mips32_scan_prologue', is a sign of broken virtual frame pointer tracking code, which has not been completed for frames produced by microMIPS code. Obviously they need to follow the same rules WRT `alloca' calls as frames made with regular MIPS code, as there's nothing special defined in the ABI for microMIPS code. Of course one could argue that keeping broken code (though in a manner harmless to irrelevant cases) has little value, but at least it serves as a reminder to do something about it sometime. I'll see if I can add the missing piece regardless sometime, though regrettably I can't give it a high priority. Overall with recent and less so improvements to GCC's and other compilers' optimizers I think these heuristic unwinders have hardly any value nowadays, they seem unable to get through function prologues containing arbitrary instructions thrown there by the scheduler. This is very annoying in a common case where you interrupt a debuggee in the middle of a sleeping syscall, with no way to backtrace through stripped system shared libraries. What I think could work is combining PDR records with reduced heuristic unwinders, which in that case do not actually need to search for frame offsets as they're already provided in PDR records. So for non-leaf frames there's really nothing to do beyond interpreting these records, because by the time a nested function call has been made, the caller's frame has already been fully populated. For leaf frames it's a tad more complicated, because the records do not carry information as to where in the prologue individual slots are initialised (i.e. registers stored). But for that you do not have to teach the unwinder of the whole instruction set, it just needs to know these actually used for register saves, which there are a small number only. And the frame offset does not have to be decoded from the instruction stream either, as it's already in the PDR record. One could argue DWARF records are the modern and therefore natural choice for this, but typically they're gone with stripping unless special care is taken and records are removed selectively. I doubt such care is taken in the field on a regular basis -- i.e. people just run `strip' or maybe even `gcc -s', or use the `install-strip' Makefile target, which again just boils down to `strip' typically -- though I'd be happy to be proved wrong. PDR records are always kept OTOH and they are really tiny, so they don't consume much disk space either. Unlike DWARF records they're also usually present even in handcoded assembly, as they're set up with the `.frame', `.mask' and `.fmask' pseudo-ops, the former of which is actually required for MIPS PIC code to assemble at all, and PDR generation is on by default in GAS. The only drawback is some toolchain configurations may disable them explicitly with the `-mno-pdr' GAS option in the GCC driver. NB, on the contrary the changes to `mips_o32_push_dummy_call' look good to me as they stand, without reservation. FWIW, Maciej
> On Jul 20, 2016, at 2:36 PM, Maciej W. Rozycki <macro@imgtec.com> wrote: > > ... > Overall with recent and less so improvements to GCC's and other > compilers' optimizers I think these heuristic unwinders have hardly any > value nowadays, they seem unable to get through function prologues > containing arbitrary instructions thrown there by the scheduler. This is > very annoying in a common case where you interrupt a debuggee in the > middle of a sleeping syscall, with no way to backtrace through stripped > system shared libraries. My experience is that the heuristic unwinders can be made to handle a lot of what's thrown at them now, but it takes quite a lot of extra heuristics to do so. I have much of this on an internal version. Should I look into making them available? One thing I've done that may not be generally interesting: make the unwinders work in the kernel (NetBSD) and able to unwind across exception frames so you can use kernel debugging and see the stack all the way into the calling process. I haven't found this all that interesting in online debugging, but it has sometimes been useful in analyzing kernel crash dumps. paul
Maciej> Of course one could argue that keeping broken code (though in a manner Maciej> harmless to irrelevant cases) has little value, but at least it serves as Maciej> a reminder to do something about it sometime. I think on the whole it's preferable to file a bug for such cases; or to comment out the offending code and put in an explanation. The warnings catch real bugs; but they are not as useful if they are noisy. Tom
On Wed, 20 Jul 2016, Paul_Koning@Dell.com wrote: > > Overall with recent and less so improvements to GCC's and other > > compilers' optimizers I think these heuristic unwinders have hardly any > > value nowadays, they seem unable to get through function prologues > > containing arbitrary instructions thrown there by the scheduler. This is > > very annoying in a common case where you interrupt a debuggee in the > > middle of a sleeping syscall, with no way to backtrace through stripped > > system shared libraries. > > My experience is that the heuristic unwinders can be made to handle a > lot of what's thrown at them now, but it takes quite a lot of extra > heuristics to do so. I have much of this on an internal version. > Should I look into making them available? Absolutely! > One thing I've done that may not be generally interesting: make the > unwinders work in the kernel (NetBSD) and able to unwind across > exception frames so you can use kernel debugging and see the stack all > the way into the calling process. I haven't found this all that > interesting in online debugging, but it has sometimes been useful in > analyzing kernel crash dumps. I think it's a separate matter -- and for post-mortem debugging (and even live debugging e.g. with QEMU's integrated debug stub or if you're a lucky one who has JTAG probe hardware to hand) you can actually implement an exception frame sniffer/unwinder so that GDB can examine it automagically. As an example see mips-sde-tdep.c, handling exception frames from the old Algorithmics/MTI SDE toolkit/board support package. If you'd like to add a similar handler for the NetBSD kernel, then I'll gladly accept it. In a typical user app debug scenario you have cases where you want to interrupt the program and see where it is, examine its local state, often in the function that made the C library call which ended up in the syscall just interrupted. In the absence of either debug information all the way through the syscall's entry point or special support it does not work however with ABIs such as these used with the MIPS processor, where the structure of the stack frame is variable and you cannot backtrace by just taking the value from the frame pointer register and using it recursively to fetch previous frame pointers from the stack. Unlike say with x86 or Power, where you may not be able to get complete information about the innermost or some intermediate frames, but at least you can backtrace far enough to reach a frame associated with a function from your actual program being debugged and be able to fully examine the state there as well as within any previous frames. Based on my personal experience with debugging software I think it is a serious shortcoming of the MIPS backend, so if you have a way to improve the current situation, then by any means please share it and make people's life easier. It'll be a huge step forward even if sometime in the future we may get ourselves other means, such as what I have outlined in my previous e-mail or maybe yet something else. Maciej
On Wed, 20 Jul 2016, Tom Tromey wrote: > Maciej> Of course one could argue that keeping broken code (though in a manner > Maciej> harmless to irrelevant cases) has little value, but at least it serves as > Maciej> a reminder to do something about it sometime. > > I think on the whole it's preferable to file a bug for such cases; or to > comment out the offending code and put in an explanation. The warnings > catch real bugs; but they are not as useful if they are noisy. Good point, I have filed PR tdep/20406 to track this issue. Maciej
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index e92203b..5ae3141 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,26 @@ 2016-07-13 Tom Tromey <tom@tromey.com> + * mips-tdep.c (micromips_scan_prologue): Remove "frame_addr". + (mips_o32_push_dummy_call): Remove "stack_used_p". + * aarch64-tdep.c (aarch64_record_data_proc_imm): Remove + "insn_bit28". + * rust-lang.c (rust_print_type): Remove "len". + * rust-exp.y (super_name): Remove "current_len". + * python/py-framefilter.c (py_print_type): Remove "type". + * mdebugread.c (parse_partial_symbols): Remove + "past_first_source_file". + <N_SO>: Remove "valu", "first_so_symnum", "prev_textlow_not_set". + * m2-valprint.c (m2_print_unbounded_array): Remove + "content_type". + (m2_val_print): Remove "i". + * linespec.c (unexpected_linespec_error): Remove "cleanup". + * f-valprint.c (f_val_print): Remove "i". + * elfread.c (elf_symtab_read): Remove "offset". + * dwarf2-frame.c (dwarf2_fetch_cfa_info): Remove "addr_size". + * jit.c (jit_dealloc_cache): Remove "i" and "frame_arch". + +2016-07-13 Tom Tromey <tom@tromey.com> + * arch-utils.c (default_skip_permanent_breakpoint): Remove "bp_insn". * disasm.c (do_assembly_only): Remove "num_displayed". diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index e5ce13e..e97e2f4 100644 --- a/gdb/aarch64-tdep.c +++ b/gdb/aarch64-tdep.c @@ -2981,11 +2981,10 @@ aarch64_record_data_proc_reg (insn_decode_record *aarch64_insn_r) static unsigned int aarch64_record_data_proc_imm (insn_decode_record *aarch64_insn_r) { - uint8_t reg_rd, insn_bit28, insn_bit23, insn_bits24_27, setflags; + uint8_t reg_rd, insn_bit23, insn_bits24_27, setflags; uint32_t record_buf[4]; reg_rd = bits (aarch64_insn_r->aarch64_insn, 0, 4); - insn_bit28 = bit (aarch64_insn_r->aarch64_insn, 28); insn_bit23 = bit (aarch64_insn_r->aarch64_insn, 23); insn_bits24_27 = bits (aarch64_insn_r->aarch64_insn, 24, 27); diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c index 2f6355a..11258ea 100644 --- a/gdb/dwarf2-frame.c +++ b/gdb/dwarf2-frame.c @@ -908,7 +908,6 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc, struct dwarf2_fde *fde; CORE_ADDR text_offset; struct dwarf2_frame_state fs; - int addr_size; memset (&fs, 0, sizeof (struct dwarf2_frame_state)); @@ -923,7 +922,6 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc, fs.data_align = fde->cie->data_alignment_factor; fs.code_align = fde->cie->code_alignment_factor; fs.retaddr_column = fde->cie->return_address_register; - addr_size = fde->cie->addr_size; /* Check for "quirks" - known bugs in producers. */ dwarf2_frame_find_quirks (&fs, fde); diff --git a/gdb/elfread.c b/gdb/elfread.c index d4400bb..e90466b 100644 --- a/gdb/elfread.c +++ b/gdb/elfread.c @@ -233,7 +233,6 @@ elf_symtab_read (struct objfile *objfile, int type, asymbol *sym; long i; CORE_ADDR symaddr; - CORE_ADDR offset; enum minimal_symbol_type ms_type; /* Name of the last file symbol. This is either a constant string or is saved on the objfile's filename cache. */ @@ -263,8 +262,6 @@ elf_symtab_read (struct objfile *objfile, int type, continue; } - offset = ANOFFSET (objfile->section_offsets, - gdb_bfd_section_index (objfile->obfd, sym->section)); if (type == ST_DYNAMIC && sym->section == bfd_und_section_ptr && (sym->flags & BSF_FUNCTION)) diff --git a/gdb/f-valprint.c b/gdb/f-valprint.c index 08215e2..e1a677e 100644 --- a/gdb/f-valprint.c +++ b/gdb/f-valprint.c @@ -218,7 +218,6 @@ f_val_print (struct type *type, const gdb_byte *valaddr, int embedded_offset, { struct gdbarch *gdbarch = get_type_arch (type); enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); - unsigned int i = 0; /* Number of characters printed. */ int printed_field = 0; /* Number of fields printed. */ struct type *elttype; CORE_ADDR addr; @@ -293,8 +292,8 @@ f_val_print (struct type *type, const gdb_byte *valaddr, int embedded_offset, { if (want_space) fputs_filtered (" ", stream); - i = val_print_string (TYPE_TARGET_TYPE (type), NULL, addr, -1, - stream, options); + val_print_string (TYPE_TARGET_TYPE (type), NULL, addr, -1, + stream, options); } return; } diff --git a/gdb/jit.c b/gdb/jit.c index 2b6cf77..0a9806e 100644 --- a/gdb/jit.c +++ b/gdb/jit.c @@ -1165,12 +1165,8 @@ static void jit_dealloc_cache (struct frame_info *this_frame, void *cache) { struct jit_unwind_private *priv_data = (struct jit_unwind_private *) cache; - struct gdbarch *frame_arch; - int i; gdb_assert (priv_data->regcache != NULL); - frame_arch = get_frame_arch (priv_data->this_frame); - regcache_xfree (priv_data->regcache); xfree (priv_data); } diff --git a/gdb/linespec.c b/gdb/linespec.c index 7162163..ccedec8 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -1519,10 +1519,9 @@ unexpected_linespec_error (linespec_parser *parser) || token.type == LSTOKEN_KEYWORD) { char *string; - struct cleanup *cleanup; string = copy_token_string (token); - cleanup = make_cleanup (xfree, string); + make_cleanup (xfree, string); throw_error (GENERIC_ERROR, _("malformed linespec error: unexpected %s, \"%s\""), token_type_strings[token.type], string); diff --git a/gdb/m2-valprint.c b/gdb/m2-valprint.c index 2d3a32f..a53aa84 100644 --- a/gdb/m2-valprint.c +++ b/gdb/m2-valprint.c @@ -162,13 +162,11 @@ m2_print_unbounded_array (struct type *type, const gdb_byte *valaddr, struct ui_file *stream, int recurse, const struct value_print_options *options) { - struct type *content_type; CORE_ADDR addr; LONGEST len; struct value *val; type = check_typedef (type); - content_type = TYPE_TARGET_TYPE (TYPE_FIELD_TYPE (type, 0)); addr = unpack_pointer (TYPE_FIELD_TYPE (type, 0), (TYPE_FIELD_BITPOS (type, 0) / 8) + @@ -316,7 +314,6 @@ m2_val_print (struct type *type, const gdb_byte *valaddr, int embedded_offset, const struct value_print_options *options) { struct gdbarch *gdbarch = get_type_arch (type); - unsigned int i = 0; /* Number of characters printed. */ unsigned len; struct type *elttype; CORE_ADDR addr; @@ -355,7 +352,6 @@ m2_val_print (struct type *type, const gdb_byte *valaddr, int embedded_offset, LA_PRINT_STRING (stream, TYPE_TARGET_TYPE (type), valaddr + embedded_offset, len, NULL, 0, options); - i = len; } else { diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c index c46e880..a6a2efe 100644 --- a/gdb/mdebugread.c +++ b/gdb/mdebugread.c @@ -2368,7 +2368,6 @@ parse_partial_symbols (struct objfile *objfile) SYMR sh; struct partial_symtab *pst; int textlow_not_set = 1; - int past_first_source_file = 0; /* List of current psymtab's include files. */ const char **psymtab_include_list; @@ -2957,16 +2956,8 @@ parse_partial_symbols (struct objfile *objfile) case N_SO: { - CORE_ADDR valu; static int prev_so_symnum = -10; - static int first_so_symnum; const char *p; - int prev_textlow_not_set; - - valu = sh.value + ANOFFSET (objfile->section_offsets, - SECT_OFF_TEXT (objfile)); - - prev_textlow_not_set = textlow_not_set; /* A zero value is probably an indication for the SunPRO 3.0 compiler. dbx_end_psymtab explicitly tests @@ -2974,19 +2965,12 @@ parse_partial_symbols (struct objfile *objfile) if (sh.value == 0 && gdbarch_sofun_address_maybe_missing (gdbarch)) - { - textlow_not_set = 1; - valu = 0; - } + textlow_not_set = 1; else textlow_not_set = 0; - past_first_source_file = 1; - if (prev_so_symnum != symnum - 1) { /* Here if prev stab wasn't N_SO. */ - first_so_symnum = symnum; - if (pst) { pst = (struct partial_symtab *) 0; diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index 6098f71..4e4d79e 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -2939,7 +2939,6 @@ micromips_scan_prologue (struct gdbarch *gdbarch, int non_prologue_insns = 0; long frame_offset = 0; /* Size of stack frame. */ long frame_adjust = 0; /* Offset of FP from SP. */ - CORE_ADDR frame_addr = 0; /* Value of $30, used as frame pointer. */ int prev_delay_slot = 0; int in_delay_slot; CORE_ADDR prev_pc; @@ -3068,7 +3067,6 @@ micromips_scan_prologue (struct gdbarch *gdbarch, else if (sreg == MIPS_SP_REGNUM && dreg == 30) /* (D)ADDIU $fp, $sp, imm */ { - frame_addr = sp + offset; frame_adjust = offset; frame_reg = 30; } @@ -3144,10 +3142,7 @@ micromips_scan_prologue (struct gdbarch *gdbarch, dreg = b5s5_reg (insn); if (sreg == MIPS_SP_REGNUM && dreg == 30) /* MOVE $fp, $sp */ - { - frame_addr = sp; - frame_reg = 30; - } + frame_reg = 30; else if ((sreg & 0x1c) != 0x4) /* MOVE reg, $a0-$a3 */ this_non_prologue_insn = 1; @@ -5502,8 +5497,6 @@ mips_o32_push_dummy_call (struct gdbarch *gdbarch, struct value *function, } while (len > 0) { - /* Remember if the argument was written to the stack. */ - int stack_used_p = 0; int partial_len = (len < MIPS32_REGSIZE ? len : MIPS32_REGSIZE); if (mips_debug) @@ -5518,7 +5511,6 @@ mips_o32_push_dummy_call (struct gdbarch *gdbarch, struct value *function, promoted to int before being stored? */ int longword_offset = 0; CORE_ADDR addr; - stack_used_p = 1; if (mips_debug) { @@ -5960,8 +5952,6 @@ mips_o64_push_dummy_call (struct gdbarch *gdbarch, struct value *function, && len % MIPS64_REGSIZE != 0); while (len > 0) { - /* Remember if the argument was written to the stack. */ - int stack_used_p = 0; int partial_len = (len < MIPS64_REGSIZE ? len : MIPS64_REGSIZE); if (mips_debug) @@ -5976,7 +5966,6 @@ mips_o64_push_dummy_call (struct gdbarch *gdbarch, struct value *function, promoted to int before being stored? */ int longword_offset = 0; CORE_ADDR addr; - stack_used_p = 1; if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG) { if ((typecode == TYPE_CODE_INT diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c index aa25911..6692ac5 100644 --- a/gdb/python/py-framefilter.c +++ b/gdb/python/py-framefilter.c @@ -211,13 +211,12 @@ py_print_type (struct ui_out *out, struct value *val) TRY { - struct type *type; struct ui_file *stb; struct cleanup *cleanup; stb = mem_fileopen (); cleanup = make_cleanup_ui_file_delete (stb); - type = check_typedef (value_type (val)); + check_typedef (value_type (val)); type_print (value_type (val), "", stb, -1); ui_out_field_stream (out, "type", stb); do_cleanups (cleanup); diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y index aeb6058..456ffe5 100644 --- a/gdb/rust-exp.y +++ b/gdb/rust-exp.y @@ -968,17 +968,15 @@ super_name (const struct rust_op *ident, unsigned int n_supers) int i; int len; VEC (int) *offsets = NULL; - unsigned int current_len, previous_len; + unsigned int current_len; struct cleanup *cleanup; cleanup = make_cleanup (VEC_cleanup (int), &offsets); current_len = cp_find_first_component (scope); - previous_len = 0; while (scope[current_len] != '\0') { VEC_safe_push (int, offsets, current_len); gdb_assert (scope[current_len] == ':'); - previous_len = current_len; /* The "::". */ current_len += 2; current_len += cp_find_first_component (scope diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c index 17b20c6..3deb525 100644 --- a/gdb/rust-lang.c +++ b/gdb/rust-lang.c @@ -888,7 +888,6 @@ rust_print_type (struct type *type, const char *varstring, { fputs_filtered (TYPE_TAG_NAME (type), stream); fputs_filtered (" ", stream); - len = strlen (TYPE_TAG_NAME (type)); } fputs_filtered ("{\n", stream);
>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes: Yao> check_typedef has side effects, IIUC, so we can't remove the call to it. Tom> Here's a new version with this restored. Yao> It is good to me. After rebasing, this patch needed a small update -- jit.c now has some newly unused variables. Please review, thanks. Tom commit c097ec493b76209e6cf830ffca9b36fe2c2643fc Author: Tom Tromey <tom@tromey.com> Date: Mon Jun 6 14:18:30 2016 -0600 Remove unused variables This patch removes set-but-unused variables. This holds all the removals I consider to be simple and relatively uncontroversial. 2016-07-13 Tom Tromey <tom@tromey.com> * mips-tdep.c (micromips_scan_prologue): Remove "frame_addr". (mips_o32_push_dummy_call): Remove "stack_used_p". * aarch64-tdep.c (aarch64_record_data_proc_imm): Remove "insn_bit28". * rust-lang.c (rust_print_type): Remove "len". * rust-exp.y (super_name): Remove "current_len". * python/py-framefilter.c (py_print_type): Remove "type". * mdebugread.c (parse_partial_symbols): Remove "past_first_source_file". <N_SO>: Remove "valu", "first_so_symnum", "prev_textlow_not_set". * m2-valprint.c (m2_print_unbounded_array): Remove "content_type". (m2_val_print): Remove "i". * linespec.c (unexpected_linespec_error): Remove "cleanup". * f-valprint.c (f_val_print): Remove "i". * elfread.c (elf_symtab_read): Remove "offset". * dwarf2-frame.c (dwarf2_fetch_cfa_info): Remove "addr_size". * jit.c (jit_dealloc_cache): Remove "i" and "frame_arch".