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

[Simon Marchi <simark@simark.ca>]

On 2019-12-15 1:18 p.m., Bernd Edlinger wrote:
> 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.

Ok, I'll push this one.  In case you want an account, the form is there, you
can mention me as the person who approved your access.

https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Simon

https://sourceware.org/cgi-bin/pdw/ps_form.cgi