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?


Thanks
Bernd.