Re: [PATCH] Always print call-clobbered registers in outer frames.

["Andrew Burgess" <aburgess@broadcom.com>]

On 24/09/2013 1:06 PM, Pedro Alves wrote:
> On 09/20/2013 12:03 PM, Andrew Burgess wrote:
>> On 19/09/2013 8:15 PM, Pedro Alves wrote:
>>> Like this...
>>>
>>> --------------
>>> Subject: [PATCH] Always print call-clobbered registers in outer frames.
>>>
>>> With older GCCs, when some variable lived on a call-clobbered register
>>> just before a call, GCC would mark such register as undefined across
>>> the function call, with DW_CFA_undefined.  This is interpreted by the
>>> debugger as meaning the variable is optimized out at that point.  That
>>> is, if the user does "up", and tries to print the variable.
>>>
>>> Newer GCCs stopped doing that.  They now just don't emit a location
>>> for the variable, resulting in GDB printing "<optimized out>" all the
>>> same.  (See <https://sourceware.org/ml/gdb-patches/2012-08/msg00787.html>.)
>>>
>>> The difference is that with binaries produced by those older GCCs, GDB
>>> will also print the registers themselves (info registers / p $reg) as
>>> "<optimized out>".  This is confusing.  
>>
>> I agree with you 100% on this, however, I feel we're merging two
>> arguments together here.  The reason gdb prints <optimised out> is
>> really because values only support 3 states: available, unavailable, and
>> optimized-out, we use optimized-out for the DW_CFA_undefined state.
>>
>> What we really need is a new state, lets call it not-saved, we don't
>> necessarily need to have extra state inside the value object to model
>> this, we might (as in your original patch) be able to derive this state,
>> but this is a state that a register can be in.
> 
> Luckily, I already have a patch that does that.  :-)
> 
>>> This patch makes GDB always follow this rule (which is what the
>>> heuristic unwinders usually do by default):
>>>
>>>  The values of call-clobbered registers in the outer frame, if not
>>>  saved by the caller, are defined as being the values the registers
>>>  would have if the inner frame was to return immediately.
>>
>> You're right that most targets seem to follow this rule, which seems odd
>> to me, however I think that the rs600, s390, sh, and tic6x targets
>> don't, they set the call-clobbered registers to DW_CFA_undefined in
>> their dwarf2_frame_set_init_reg functions, this seems much more sensible
>> to me, assuming that call-clobbered registers have not been used seems
>> .... odd.
> 
> Okay.  Reading more code, and investigating the history behind it
> convinced me that that is indeed the direction GDB was currently
> heading.
> 
>>
>>>
>>> The documentation is updated to more clearly explain this.
>>>
>>> IOW, ignore DW_CFA_undefined _when printing frame registers_, but
>>> not when printing variables.  This translates to, if value of a frame
>>> register, comes out as optimized out (that's what "not saved"
>>> lval_register values end up as), fetch it from the next frame.
>>
>> I really think this is going to confuse users, we're basically saying
>> that call-clobbered registers are assumed to never be clobbered .... we
>> might get away with this from very shallow call-stacks, but for very
>> deep stacks this seems very unlikely, in which case, a user switches to
>> some outer stack and does "info registers", how does he know which
>> registers gdb has carefully retrieved and are correct, and which are
>> just random values fetched from some inner frame?
> 
> I had toyed with something like Doug suggested.  See my
> other reply to Mark.




> 
>> My question then is, what makes you believe the inner, possibly
>> call-clobbered value is right?
> 
> Nothing, of course.  It's just a choice -- either assume it's right,
> and somehow warn the user it may not be right through some other means;
> or, assume it's not right, and hide the value from the user.  If
> GDB is wrong, the user can still fetch the value, though it'll take
> more steps (up+p $reg+down, etc.).  It might still be a good idea to
> provide an easier way to "flatten" the not-saved registers as
> convenience for that, not sure).

I guess the thing I'm struggling with is why would we /ever/ assume the
value in an inner frame is correct?  Sure, for very shallow stacks on
machines with a non-small register set there's a reasonable chance the
value is correct, but in any other case I seriously don't see how taking
the value from an inner frame is any better than picking a random number.

> 
> (skipping answering some of the other points as I believe I've answered
> them in my other reply to Mark).
> 
>>> -However, @value{GDBN} must deduce where registers are saved, from the machine
>>> -code generated by your compiler.  If some registers are not saved, or if
>>> -@value{GDBN} is unable to locate the saved registers, the selected stack
>>> -frame makes no difference.
>>> +Most ABIs reserve some registers as ``scratch'' registers that don't
>>> +need to be saved by the caller (a.k.a. caller-saved or call-clobbered
>>> +registers).  It may therefore not be possible for @value{GDBN} to know
>>
>> I think here you should say "Most ABIs reserve some registers as
>> ``scratch'' registers that don't need to be saved by the callee"
>> (callee, not caller).
> 
> Indeed.
> 
>>
>>> +the value a register had before the call (in other words, in the outer
>>> +frame), if the register value has since been changed by the callee.
>>> +@value{GDBN} tries to deduce where registers are saved, from the debug
>>> +info, unwind info, or the machine code generated by your compiler.  
>>
>> I don't think this is correct either, we try to figure out where callee
>> saved registers are stored, but these are not the scratch registers, if
>> the scratch registers are needed by the caller then the caller will save
>> them, but the callee will just go ahead and use them.  Similarly the
>> DWARF only tells us about callee saved registers.  The caller saved
>> registers are now not mentioned in the DWARF as gcc has made sure that
>> no variables are live in these registers.
> 
> Hmm.  You're right, but only if you see a sequence/connection between
> the sentences.  They're different paragraphs -- the second sentence is
> mostly preexisting, and I didn't mean to imply the GDB tries to figure
> out where caller-saved/scratch registers are.  I'll rephrase it (in
> this or the <not saved> patch, whatever we end up with.  Thanks.
> 
>>
>>>                                                                       If
>>> +some register is not saved, or if @value{GDBN} is unable to locate the
>>> +saved register, @value{GDBN} will assume the register in the outer
>>> +frame had the same location and value it has in the inner frame. In
>>> +other words, the values of call-clobbered registers in the outer
>>> +frame, if not saved by the caller, are defined as being the values the
>>> +registers would have if the inner frame was to return immediately.
>>> +This is usually harmless, but note however that if you change a
>>> +register in the outer frame, you may also be affecting the inner
>>> +frame.
>>
>> I'd just like to pick up on the "harmless" here, I agree that it would
>> be "harmless" in the sense that if we did a return in gdb it would be
>> harmless; the register is callee clobbered, the caller either does not
>> care what is in the register after the call, or has code to restore the
>> value that it does care about. 
> 
> Right, that's what I was thinking.  It'd be better to write that out
> explicitly.
> 
>> From a debugging point of few though, I
>> suspect showing the wrong value might be far from harmless, 
> 
> No sure why.

I think that if you print it "they" (users) will use it, assuming it's
correct.  Given that I don't think it's correct I think we're likely to
be giving users miss-information.

Are there any other examples within gdb that you can think of (I can't)
where we print information that we know is very likely wrong? (Obviously
I mean deliberately here rather than being mislead by invalid debug
information).

> 
>> and if this
>> patch is merged we need to draw attention to the fact that the more
>> "outer" the frame it you're looking at, the more likely call-clobbered
>> registers are to be wrong.
> 
> Blah, what a rat hole.  :-P :-)
> 
> Please guys, re-confirm to me the direction you prefer seeing
> GDB follow, even after my reasoning further explained (I'm okay
> with either approach), and I'll update the corresponding patch's
> documentation bits with the suggestions you've sent.

I think the original "<not-saved>" patch was great and a good
improvement on where we are right now, I'd much rather see that merged
than the "values-from-inner-frame" patch.

Thanks for your continued effort :)

Andrew