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

[Simon Marchi <simark@simark.ca>]

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.

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).

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.

Thanks!

Simon