Re: [PATCH] Print registers not saved in the frame as "<not saved>", instead of "<optimized out>".

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

On 05/09/2013 5:29 PM, Pedro Alves wrote:
> Hi guys,
> 
> Getting back to this, trying to make progress.
> 
> On 08/19/2013 11:24 AM, Andrew Burgess wrote:
>> On 16/08/2013 7:41 PM, Pedro Alves wrote:
>>> On 08/12/2013 09:01 PM, Mark Kettenis wrote:
>>>>> Date: Mon, 12 Aug 2013 14:55:04 +0100
>>>>> From: Pedro Alves <palves@redhat.com>
>>>>>
>>>>> On 08/12/2013 02:31 PM, Andrew Burgess wrote:
>>>>>> On 06/08/2013 7:39 PM, Pedro Alves wrote:
>>>>>>> On 08/06/2013 04:41 PM, Mark Kettenis wrote:
>>>>>>>>> Date: Tue, 6 Aug 2013 14:49:03 +0100
>>>>>>>>> From: "Andrew Burgess" <aburgess@broadcom.com>
>>>>>>>
>>>>>>>>> 3. My understanding was that values lost due to the ABI of a call site
>>>>>>>>> were recorded as optimized out.  For evidence I would present
>>>>>>>>> dwarf2_frame_prev_register, and how DWARF2_FRAME_REG_UNDEFINED is handled.
>>>>>>>>>
>>>>>>>>> For these reasons I believe my patch should still be considered, what do
>>>>>>>>> you think?
>>>>>>>>
>>>>>>>> I think that registers are either available or unavailble.  A register
>>>>>>>> being unavailble implies that a variable that is supposed to live in
>>>>>>>> such a register may have been optimized out.  Whether GDB's pseudo
>>>>>>>> variables that respresent registers are considered unavailable or
>>>>>>>> optimized out in that case is arguable.
>>>>>>>
>>>>>>> I think improving consistency as in Andrew's patch is good.
>>>>>>
>>>>>> Given almost a week has passed with no further feedback I plan to
>>>>>> commit this patch tomorrow unless there's any further discussion to be had.
>>>>>
>>>>> TBC, note my opinion doesn't get to overrule Mark's.  Consensus
>>>>> works much better, and Mark does have deep knowledge of all
>>>>> ABI/pseudo registers/etc. gdb things.
>>>>> That said, Mark, if you still disagree, please counter argue,
>>>>> otherwise, we'll just have to assume you do agree with the
>>>>> rationales and clarifications.
>>>>
>>>> Can't say I agree.  It simply doesn't make sense for registers to be
>>>> "optimized out".  I guess there are two reasons why GDB can't display
>>>> the contents of a register in a frame:
>>>>
>>>> 1. The register contents aren't made available by the debugging
>>>>    interface, i.e. ptrace(2) or the remote stub doesn't tell us.
>>>>
>>>> 2. The register wasn't saved before calling another function.
>>>>
>>>> I guess after Andrew's chnages 1) would be shown as <unavailable> and
>>>> 2) would become <optimized out>.  But in the latter case something
>>>> like <not saved> would make more sense.
>>>>
>>>> That said, Pedro, you're pretty much the expert for this area of GDB.
>>>> So If you think Andrew should go ahead with this, feel free to ignore
>>>> me.
>>>
>>> This is a tough call.  I do agree that "optimized out" for registers
>>> is a bit confusing.  However, we already do print "<optimized out>" in
>>> other places, such as when printing expressions, and consistency
>>> is good.  If we did add a distinction, I agree with Andrew that it should
>>> be done in a more systematic way.  However, I'm not really sure we need
>>> much machinery.  Wouldn't something like:
>>>
>>> void
>>> val_print_optimized_out (const struct value *val, struct ui_file *stream)
>>> {
>>>   if (value_lval_const (val) == lval_register)
>>>     fprintf_filtered (stream, _("<not saved>"));
>>>   else
>>>     fprintf_filtered (stream, _("<optimized out>"));
>>> }
>>>
>>> work?  What could be the register value cases that would print
>>> "not saved" that we'd still want to print "optimized out" ?
>>
>> The only case I can immediately think of where this would cause a
>> problem would be for computed locations, (lval_computed).  The easy
>> answer would be (in that case) the blame the compiler - why say the
>> location is in a register if that register is volatile - but sadly I see
>> this way too often.
> 
> Hmm, OK, but then lval_computed values with that change won't
> ever show "<not saved>", due to the lval_register check.  IOW,
> we'd have to do something else in addition to lval_computed values
> to make them print something other than the current <optimized out>.
> 
> However, I've come to think there's a really simple rule to
> follow here -- We should only ever print <not saved> for values
> that represent machine/pseudo registers.  IOW, $pc, $rax, etc.
> If the debug info happens to describe a variable as being located
> in some optimized out register, we should still print
> <optimized out>.  The previous version of the patch failed that:
> 
>  (gdb) PASS: gdb.dwarf2/dw2-op-out-param.exp: continue to breakpoint: Stop in breakpt for test int_param_single_reg_loc
>   bt
>   #0  0x000000000040058f in breakpt ()
>  -#1  0x00000000004005a2 in int_param_single_reg_loc (operand0=<optimized out>, operand1=0xdeadbe00deadbe01, operand2=<optimized out>)
>  +#1  0x00000000004005a2 in int_param_single_reg_loc (operand0=<not saved>, operand1=0xdeadbe00deadbe01, operand2=<not saved>)
>   #2  0x0000000000400577 in main ()
> 
> It didn't really make a lot of sense.  This new version doesn't have
> that change anymore.
> 
> That simple rule suggests that whatever the internal representation,
> we should be easily able to have a single central point where to tag
> such values.  In fact, I think that already exists in value_of_register.
> 
>> However, exchanging what I see as the current larger inconsistency, for
>> this much smaller one seems like a good deal to me, especially if it
>> gets this patch unblocked...
> 
> Alright, what do you (all) think of of this (supposedly finished) patch
> on top of yours (Andrew's) then?


Looks good to me.  Thanks for this.

Andrew