Re: [PATCHv6] Make "skip" work on inline frames

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 12/15/19 2:12 PM, Simon Marchi wrote:
> On 2019-12-15 6:25 a.m., Bernd Edlinger wrote:
>> On 12/15/19 1:46 AM, Simon Marchi wrote:
>>> On 2019-12-02 11:47 a.m., Bernd Edlinger wrote:
>>>> On 12/2/19 3:34 AM, Simon Marchi wrote:
>>>>> On 2019-11-24 6:22 a.m., Bernd Edlinger wrote:
>>>>>> This is just a minor update on the patch
>>>>>> since the function SYMBOL_PRINT_NAME was removed with
>>>>>> commit 987012b89bce7f6385ed88585547f852a8005a3f
>>>>>> I replaced it with sym->print_name (), otherwise the
>>>>>> patch is unchanged.
>>>>>
>>>>> Hi Bernd,
>>>>>
>>>>> Sorry, I had lost this in the mailing list noise.
>>>>>
>>>>> I played a bit with the patch and different cases of figure.  I am not able to understand
>>>>> the purpose of each of your changes (due to the complexity of that particular code), but
>>>>> I didn't find anything that stood out as wrong to me.  Pedro might be able to do a more
>>>>> in-depth review of the event handling code.
>>>>>
>>>>> If the test tests specifically skipping of inline functions, I'd name it something more
>>>>> descriptive than "skip2.exp", maybe "skip-inline.exp"?
>>>>>
>>>>> Unfortunately, your test doesn't pass on my computer (gcc 9.2.0), but neither does the
>>>>> gdb.base/skip.exp.  I am attaching the gdb.log when running your test, if it can help.
>>>>>
>>>>> Simon
>>>>>
>>>>
>>>> Hi Simon,
>>>>
>>>> I only tested that with gcc-4.8, and both test cases worked with that gcc version.
>>>>
>>>> I tried now with gcc-trunk version from a few days ago, and I think I see
>>>> what you mean.
>>>>
>>>> skip2.c (now skip-inline.c) can be fixed by removing the assignment
>>>> to x in the first line, which is superfluous (and copied from skip.c).
>>>> But skip.c cannot be fixed this way.  I only see a chance to allow
>>>> the stepping back to main and then to foo happen.
>>>>
>>>> Does this modified test case work for you?
>>>>
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>
>>> Hi Bernd,
>>>
>>> Thanks for fixing the skip.exp test at the same time.  I'd prefer if this was done as a
>>> separate patch though, since it's an issue separate from the inline stepping issue you
>>> were originally tackling.
>>
>> Okay, I split that out as a separate patch now.
>>
>>>
>>> So the patch looks good to me if you remove those bits, and fix the following nits:
>>>
>>> - Remove "load_lib completion-support.exp" from the test.
>>> - The indentation in the .exp should use tabs for multiple of 8 columns, instead of just spaces (like you did in the .c).
>>>
>>
>> Done.  Also added changelog text, which I forgot previously.
>>
>>> A comment I would have on the bits in skip.exp:
>>>
>>>     # with recent gcc we jump once back to main before entering foo here
>>>     # if that happens try to step a second time
>>>     gdb_test "step" "foo \\(\\) at.*" "step 3" "main \\(\\) at .*" "step"
>>>
>>> It's usually not helpful to say "with recent gcc", since it doesn't mean much, especially
>>> when reading this 10 years from now.  Instead, mention the specific gcc version this was
>>> observed with.  Also, begin the sentence with a capital letter and finish with a period.
>>>
>>
>> Done.
>>
>>
>> Is it OK for trunk?
> 
> That LGTM.  I just remembered that your copyright assignment status was unclear, but I looked
> up your name and saw that you filed one recently.
> 
> Would you like me to continue pushing your patches for you, or would you prefer to get push
> access, so you are able to do so when they are approved?
> 

Either way is fine for me.


Thanks
Bernd.

> Simon
>