[PATCHv2] Make "skip" work on inline frames

  On 10/27/19 3:17 AM, Simon Marchi wrote:
> On 2019-10-26 9:52 p.m., Simon Marchi wrote:
>> On 2019-10-20 2:48 a.m., Bernd Edlinger wrote:
>>> On 10/19/19 6:38 AM, Bernd Edlinger wrote:
>>>> Hmm,
>>>>
>>>> I noticed that the patch does not yet handle
>>>> the step <count> correctly, the count is decremented
>>>> although the inline frame is skipped and should not be
>>>> counted...
>>>>
>>>> Thus I will need to change at least this:
>>>>
>>>> --- a/gdb/infcmd.c
>>>> +++ b/gdb/infcmd.c
>>>> @@ -1122,7 +1122,6 @@ prepare_one_step (struct step_command_fsm *sm)
>>>>               set_running (resume_ptid, 1);
>>>>  
>>>>               step_into_inline_frame (tp);
>>>> -             sm->count--;
>>>>  
>>>>               sal = find_frame_sal (frame);
>>>>               sym = get_frame_function (frame);
>>>> @@ -1132,13 +1131,17 @@ prepare_one_step (struct step_command_fsm *sm)
>>>>  
>>>>               if (sal.line == 0
>>>>                   || !function_name_is_marked_for_skip (fn, sal))
>>>> -               return prepare_one_step (sm);
>>>> +               {
>>>> +                 sm->count--;
>>>> +                 return prepare_one_step (sm);
>>>> +               }
>>>>             }
>>>>  
>>>>
>>>
>>> Attached is an updated patch that fixes this issue,
>>> and also adds the following after step_into_inline_frame ():
>>>
>>> frame = get_current_frame ();
>>>
>>> That I consider safer, since this function calls reinit_frame_cache ().
>>> It was probably just by chance that this did not seem to cause any
>>> problems for me.
>>>
>>>
>>> Thanks
>>> Bernd.
>>
>> Hi Bernd,
>>
>> Sorry for the delay.  I'll start looking at this patch, but I first need to play with
>> it a bit first and get more familiar with that area of the code.
>>
>> In the mean time, I looked for your name in the copyright assignment list, and don't find
>> it.  I think this patch is large enough to warrant one  Do you already have one in place?
>> If not, please follow instructions here:
>>
>> https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future
>>
>> Simon
> 
> Oh, and I noticed that the patch doesn't come with a test, we'll need one before getting
> the patch in.  There are already some skip tests at testsuite/gdb.base/skip*.exp, so I
> could very well imagine a new test named gdb.base/skip-inline.exp.
> 
> See these pages for details on how to write and run tests:
> 
> - https://sourceware.org/gdb/wiki/GDBTestcaseCookbook
> - https://sourceware.org/gdb/wiki/TestingGDB
> 
> If you can't manage to make a test, at the very least please provide a minimal reproducer
> so somebody else will be able to translate that into a test.
> 

While the legal stuff will probably need more time,
I quickly wrote a test case for this.  Hope this helps to understand
how the patch works.

Attached you'll find a test case for the skip of inline functions,
I also added a test for the glitch in the first version of the patch.
(counting step over skipped inlined functions wrong)


Thanks
Bernd.


> Thanks,
> 
> Simon
>
  

On 10/30/19 9:56 PM, Bernd Edlinger wrote:
> +if { [prepare_for_testing "failed to prepare" "skip2" \
> +                          {skip2.c skip1.c } \
> +                          {debug nowarnings optimize=-O2}] } {

Instead of -O2, could you make this use -O0 (the default),
and then use attribute((always_inline)) to force inlining? 
We do that in some tests.  E.g., gdb.opt/inline-locals.c.

Quickly skimming the patch, I noticed a number of duplicated
test names.  See:
 https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Thanks,
Pedro Alves
  

On 2019-10-31 12:42 p.m., Pedro Alves wrote:
> On 10/30/19 9:56 PM, Bernd Edlinger wrote:
>> +if { [prepare_for_testing "failed to prepare" "skip2" \
>> +                          {skip2.c skip1.c } \
>> +                          {debug nowarnings optimize=-O2}] } {
> 
> Instead of -O2, could you make this use -O0 (the default),
> and then use attribute((always_inline)) to force inlining? 
> We do that in some tests.  E.g., gdb.opt/inline-locals.c.

I think that's a good suggestion, but just be aware that there used to be
some problems with always_inline, e.g.:

https://sourceware.org/bugzilla/show_bug.cgi?id=13263
https://sourceware.org/bugzilla/show_bug.cgi?id=12429

I'm not sure if those are still valid.  If they are, it might be more difficult
that expected to use always_inline.

Simon
  

On 10/31/19 4:53 PM, Simon Marchi wrote:
> On 2019-10-31 12:42 p.m., Pedro Alves wrote:
>> On 10/30/19 9:56 PM, Bernd Edlinger wrote:
>>> +if { [prepare_for_testing "failed to prepare" "skip2" \
>>> +                          {skip2.c skip1.c } \
>>> +                          {debug nowarnings optimize=-O2}] } {
>>
>> Instead of -O2, could you make this use -O0 (the default),
>> and then use attribute((always_inline)) to force inlining? 
>> We do that in some tests.  E.g., gdb.opt/inline-locals.c.
> 
> I think that's a good suggestion, but just be aware that there used to be
> some problems with always_inline, e.g.:

I don't think always_inline changes anything in the debug info special,
it just tells the compiler to inline the function even at -O0, which is
what we're after.

> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=13263

This one seems to be about attribute((artificial)), and the desire
to not step into such functions automatically:

   "Given that I marked the function always_inline and artificial, I
    was expecting not to step into its body."

> https://sourceware.org/bugzilla/show_bug.cgi?id=12429
> 

This one seems like a generic inlining issue.

> I'm not sure if those are still valid.  If they are, it might be more difficult
> that expected to use always_inline.
I don't think always_inline is anything special compared to inlining
because of -O2.

Thanks,
Pedro Alves