Re: [RFA 5/6] Remove unused variables

["Maciej W. Rozycki" <macro at imgtec dot com>]

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