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

[Pedro Alves <palves at redhat dot com>]

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?

-- 
Pedro Alves