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

["Andrew Burgess" <aburgess at broadcom dot com>]

On 16/09/2013 8:05 PM, Pedro Alves wrote:
> On 09/05/2013 05:35 PM, Andrew Burgess wrote:
>> 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.
> 
> Thanks.  Could you apply your patch then?
> 

Committed. Thanks for your time.

Andrew