[PATCH] Make "skip" work on inline frames

[PATCH] Make "skip" work on inline frames

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

Hi,

I noticed that skip is not working well on inlined frames which may
happen in optimized gcc stage3 binary, where step stops at functions
which are marked for skip, whenever they happen to be in-lined, where
it is ignored if the frame is marked for skip, thus currently
skipped frames are only checked when the skipped function
is not inlined, which is usually only the case in non-optimized builds.


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch --]
[-- Type: text/x-patch; name="0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch", Size: 4418 bytes --]

From 6f24c762bf45d7b1b2d2e9285dfe2250b654b8ea Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 18 Oct 2019 14:28:45 +0200
Subject: [PATCH] Check all inline frames if they are marked for skip.

This makes the skip command work in optimized builds,
where skipped functions may be inlined.
Previously that was only working when stepping into
a non-inlined function.
---
 gdb/infcmd.c | 15 ++++++++++++++-
 gdb/infrun.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 7105774..9e4de66 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -58,6 +58,7 @@
 #include "thread-fsm.h"
 #include "top.h"
 #include "interps.h"
+#include "skip.h"
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
@@ -1112,6 +1113,9 @@ prepare_one_step (struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
+	      const char *fn = NULL;
+	      symtab_and_line sal;
+	      struct symbol *sym;
 
 	      /* Pretend that we've ran.  */
 	      resume_ptid = user_visible_resume_ptid (1);
@@ -1119,7 +1123,16 @@ prepare_one_step (struct step_command_fsm *sm)
 
 	      step_into_inline_frame (tp);
 	      sm->count--;
-	      return prepare_one_step (sm);
+
+	      sal = find_frame_sal (frame);
+	      sym = get_frame_function (frame);
+
+	      if (sym != NULL)
+		fn = SYMBOL_PRINT_NAME (sym);
+
+	      if (sal.line == 0
+		  || !function_name_is_marked_for_skip (fn, sal))
+		return prepare_one_step (sm);
 	    }
 
 	  pc = get_frame_pc (frame);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 07aebfa..04c1eee 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4041,6 +4041,45 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
   return 0;
 }
 
+/* Look for an inline frame that is marked for skip.
+   If PREV_FRAME is TRUE start at the previous frame,
+   otherwise start at the current frame.  Stop at the
+   first non-inline frame, or at the frame where the
+   step started.  */
+
+static bool
+inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
+{
+  struct frame_info *frame = get_current_frame ();
+
+  if (prev_frame)
+    frame = get_prev_frame (frame);
+
+  for (; frame != NULL; frame = get_prev_frame (frame))
+    {
+      const char *fn = NULL;
+      symtab_and_line sal;
+      struct symbol *sym;
+
+      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+	break;
+      if (get_frame_type (frame) != INLINE_FRAME)
+	break;
+
+      sal = find_frame_sal (frame);
+      sym = get_frame_function (frame);
+
+      if (sym != NULL)
+	fn = SYMBOL_PRINT_NAME (sym);
+
+      if (sal.line != 0
+	  && function_name_is_marked_for_skip (fn, sal))
+	return true;
+    }
+
+  return false;
+}
+
 /* If the event thread has the stop requested flag set, pretend it
    stopped for a GDB_SIGNAL_0 (i.e., as if it stopped due to
    target_stop).  */
@@ -6531,7 +6570,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  tmp_sal))
+						  tmp_sal)
+	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
@@ -6697,7 +6737,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  if (call_sal.line == ecs->event_thread->current_line
 	      && call_sal.symtab == ecs->event_thread->current_symtab)
-	    step_into_inline_frame (ecs->event_thread);
+	    {
+	      step_into_inline_frame (ecs->event_thread);
+	      if (inline_frame_is_marked_for_skip (false, ecs->event_thread))
+		{
+		  keep_going (ecs);
+		  return;
+		}
+	    }
 
 	  end_stepping_range (ecs);
 	  return;
@@ -6731,7 +6778,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepping through inlined function\n");
 
-      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL)
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+	  || inline_frame_is_marked_for_skip (false, ecs->event_thread))
 	keep_going (ecs);
       else
 	end_stepping_range (ecs);
-- 
1.9.1

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

[Bernd Edlinger]

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);
+               }
            }
 

So I'll send a new patch in a moment.


Thanks.

On 10/18/19 2:52 PM, Bernd Edlinger wrote:
> Hi,
> 
> I noticed that skip is not working well on inlined frames which may
> happen in optimized gcc stage3 binary, where step stops at functions
> which are marked for skip, whenever they happen to be in-lined, where
> it is ignored if the frame is marked for skip, thus currently
> skipped frames are only checked when the skipped function
> is not inlined, which is usually only the case in non-optimized builds.
> 
> 
> Thanks
> Bernd.
> 

[PATCHv2] Make "skip" work on inline frames

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch --]
[-- Type: text/x-patch; name="0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch", Size: 4461 bytes --]

From fa00b1890e525b4e8e9a8397bddfa9963c253080 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 18 Oct 2019 14:28:45 +0200
Subject: [PATCH] Check all inline frames if they are marked for skip.

This makes the skip command work in optimized builds,
where skipped functions may be inlined.
Previously that was only working when stepping into
a non-inlined function.
---
 gdb/infcmd.c | 20 ++++++++++++++++++--
 gdb/infrun.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 7105774..f4c183c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -58,6 +58,7 @@
 #include "thread-fsm.h"
 #include "top.h"
 #include "interps.h"
+#include "skip.h"
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
@@ -1112,14 +1113,29 @@ prepare_one_step (struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
+	      const char *fn = NULL;
+	      symtab_and_line sal;
+	      struct symbol *sym;
 
 	      /* Pretend that we've ran.  */
 	      resume_ptid = user_visible_resume_ptid (1);
 	      set_running (resume_ptid, 1);
 
 	      step_into_inline_frame (tp);
-	      sm->count--;
-	      return prepare_one_step (sm);
+
+	      frame = get_current_frame ();
+	      sal = find_frame_sal (frame);
+	      sym = get_frame_function (frame);
+
+	      if (sym != NULL)
+		fn = SYMBOL_PRINT_NAME (sym);
+
+	      if (sal.line == 0
+		  || !function_name_is_marked_for_skip (fn, sal))
+		{
+		  sm->count--;
+		  return prepare_one_step (sm);
+		}
 	    }
 
 	  pc = get_frame_pc (frame);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 07aebfa..04c1eee 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4041,6 +4041,45 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
   return 0;
 }
 
+/* Look for an inline frame that is marked for skip.
+   If PREV_FRAME is TRUE start at the previous frame,
+   otherwise start at the current frame.  Stop at the
+   first non-inline frame, or at the frame where the
+   step started.  */
+
+static bool
+inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
+{
+  struct frame_info *frame = get_current_frame ();
+
+  if (prev_frame)
+    frame = get_prev_frame (frame);
+
+  for (; frame != NULL; frame = get_prev_frame (frame))
+    {
+      const char *fn = NULL;
+      symtab_and_line sal;
+      struct symbol *sym;
+
+      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+	break;
+      if (get_frame_type (frame) != INLINE_FRAME)
+	break;
+
+      sal = find_frame_sal (frame);
+      sym = get_frame_function (frame);
+
+      if (sym != NULL)
+	fn = SYMBOL_PRINT_NAME (sym);
+
+      if (sal.line != 0
+	  && function_name_is_marked_for_skip (fn, sal))
+	return true;
+    }
+
+  return false;
+}
+
 /* If the event thread has the stop requested flag set, pretend it
    stopped for a GDB_SIGNAL_0 (i.e., as if it stopped due to
    target_stop).  */
@@ -6531,7 +6570,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  tmp_sal))
+						  tmp_sal)
+	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
@@ -6697,7 +6737,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  if (call_sal.line == ecs->event_thread->current_line
 	      && call_sal.symtab == ecs->event_thread->current_symtab)
-	    step_into_inline_frame (ecs->event_thread);
+	    {
+	      step_into_inline_frame (ecs->event_thread);
+	      if (inline_frame_is_marked_for_skip (false, ecs->event_thread))
+		{
+		  keep_going (ecs);
+		  return;
+		}
+	    }
 
 	  end_stepping_range (ecs);
 	  return;
@@ -6731,7 +6778,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepping through inlined function\n");
 
-      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL)
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+	  || inline_frame_is_marked_for_skip (false, ecs->event_thread))
 	keep_going (ecs);
       else
 	end_stepping_range (ecs);
-- 
1.9.1

[PING] [PATCHv2] Make "skip" work on inline frames

[Bernd Edlinger]

Hi,

I wanted to ping for this patch here:
http://sourceware.org/ml/gdb-patches/2019-10/msg00685.html


Thanks
Bernd.


On 10/20/19 8:48 AM, 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.
> 

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

[Simon Marchi]

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

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

[Simon Marchi]

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.

Thanks,

Simon

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

[Bernd Edlinger]

On 10/27/19 2:52 AM, 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:
> 

There should be an assignment on file, although it is signed by my employer
Softing Industrial Automation GmbH on Oct 25 2012 and countersigned by
John Sullivan on Dec 17 2012
The work that is intended to be covered by this assignment is mine.

I am also the maintainer of the GNU Mempool package:
https://www.gnu.org/software/mempool/
so I should be known to gnu.org, but maybe something got lost.

Is this assignment sufficient for contributing to gdb?


Thanks
Bernd.

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

[Simon Marchi]

On 2019-10-30 4:05 p.m., Bernd Edlinger wrote:
> There should be an assignment on file, although it is signed by my employer
> Softing Industrial Automation GmbH on Oct 25 2012 and countersigned by
> John Sullivan on Dec 17 2012
> The work that is intended to be covered by this assignment is mine.
> 
> I am also the maintainer of the GNU Mempool package:
> https://www.gnu.org/software/mempool/
> so I should be known to gnu.org, but maybe something got lost.
> 
> Is this assignment sufficient for contributing to gdb?
> 
> 
> Thanks
> Bernd.
> 

Hi Bernd,

The only assignment I see under Softing Industrial Automation is for GNU eCos, with
the date 2012-12-20, which fits with the date you gave.  Unfortunately, the assignments
are per-project, so you'll need to be covered by one for GDB too.

I also don't find anything for Mempool in the copyright.list file, which is strange.
If you are contributing code to a GNU project, you should normally have signed such
an copyright assignment for it in the past.  Do you remember doing so for Mempool?

Simon

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]

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
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-a-test-case-for-skip-with-inlined-functions.patch --]
[-- Type: text/x-patch; name="0001-Add-a-test-case-for-skip-with-inlined-functions.patch", Size: 5030 bytes --]

From bae4d08c6a69314b0073ee7d93254076e6574e55 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, 30 Oct 2019 21:35:22 +0100
Subject: [PATCH] Add a test case for skip with inlined functions

---
 gdb/testsuite/gdb.base/skip2.c   | 64 ++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/skip2.exp | 67 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/skip2.c
 create mode 100644 gdb/testsuite/gdb.base/skip2.exp

diff --git a/gdb/testsuite/gdb.base/skip2.c b/gdb/testsuite/gdb.base/skip2.c
new file mode 100644
index 0000000..295aa23
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.c
@@ -0,0 +1,64 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+int bar (void);
+int baz (int);
+void skip1_test_skip_file_and_function (void);
+void test_skip_file_and_function (void);
+
+static int
+foo (void)
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  volatile int x;
+
+  /* step immediately into the inlined code */
+  x = baz (foo ());
+
+  /* step first over non-inline code, this involves a different code path */
+  x = 0; x = baz (foo ());
+
+  test_skip_file_and_function ();
+
+  return 0;
+}
+
+static void
+test_skip (void)
+{
+}
+
+static void
+end_test_skip_file_and_function (void)
+{
+  abort ();
+}
+
+void
+test_skip_file_and_function (void)
+{
+  test_skip ();
+  skip1_test_skip_file_and_function ();
+  end_test_skip_file_and_function ();
+}
diff --git a/gdb/testsuite/gdb.base/skip2.exp b/gdb/testsuite/gdb.base/skip2.exp
new file mode 100644
index 0000000..db40601
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.exp
@@ -0,0 +1,67 @@
+#   Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib completion-support.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" "skip2" \
+                          {skip2.c skip1.c } \
+                          {debug nowarnings optimize=-O2}] } {
+    return -1
+}
+
+set srcfile skip2.c
+set srcfile1 skip1.c
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+# Create a skiplist entry for a specified file and function.
+
+gdb_test "skip function foo" "Function foo will be skipped when stepping\."
+
+gdb_test "step" ".*" "step in the main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "step in the main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "step into baz, since foo will be skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "step in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "step back to main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "step into baz, since foo will be skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "step in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "step back to main"
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+gdb_test "step" ".*" "step in the main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "step in the main"
+gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "step into baz, since foo will be skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "step back to main"
+gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "step into baz, since foo will be skipped"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "step back to main"
-- 
1.9.1

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

[Pedro Alves]

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

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

[Simon Marchi]

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

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

[Pedro Alves]

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

[PATCHv3] Make "skip" work on inline frames

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2022 bytes --]

On 10/31/19 7:00 PM, Pedro Alves wrote:
> 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
> 

Ah, thanks for that hint!

always_inline works quite well.

The debug session started (using gcc 4.8.4) with -O2 -g on the line with "{" in main,
and with -O0 -g at the first real statement, so I had to remove one step.
I did not worry about it initially, but now I agree that would have caused trouble.


I attached both parts of the patch, the fist part unchanged from previous.
The test case now makes sure to not repeat the names.


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch --]
[-- Type: text/x-patch; name="0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch", Size: 4465 bytes --]

From fa00b1890e525b4e8e9a8397bddfa9963c253080 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 18 Oct 2019 14:28:45 +0200
Subject: [PATCH 1/2] Check all inline frames if they are marked for skip.

This makes the skip command work in optimized builds,
where skipped functions may be inlined.
Previously that was only working when stepping into
a non-inlined function.
---
 gdb/infcmd.c | 20 ++++++++++++++++++--
 gdb/infrun.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 7105774..f4c183c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -58,6 +58,7 @@
 #include "thread-fsm.h"
 #include "top.h"
 #include "interps.h"
+#include "skip.h"
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
@@ -1112,14 +1113,29 @@ prepare_one_step (struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
+	      const char *fn = NULL;
+	      symtab_and_line sal;
+	      struct symbol *sym;
 
 	      /* Pretend that we've ran.  */
 	      resume_ptid = user_visible_resume_ptid (1);
 	      set_running (resume_ptid, 1);
 
 	      step_into_inline_frame (tp);
-	      sm->count--;
-	      return prepare_one_step (sm);
+
+	      frame = get_current_frame ();
+	      sal = find_frame_sal (frame);
+	      sym = get_frame_function (frame);
+
+	      if (sym != NULL)
+		fn = SYMBOL_PRINT_NAME (sym);
+
+	      if (sal.line == 0
+		  || !function_name_is_marked_for_skip (fn, sal))
+		{
+		  sm->count--;
+		  return prepare_one_step (sm);
+		}
 	    }
 
 	  pc = get_frame_pc (frame);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 07aebfa..04c1eee 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4041,6 +4041,45 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
   return 0;
 }
 
+/* Look for an inline frame that is marked for skip.
+   If PREV_FRAME is TRUE start at the previous frame,
+   otherwise start at the current frame.  Stop at the
+   first non-inline frame, or at the frame where the
+   step started.  */
+
+static bool
+inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
+{
+  struct frame_info *frame = get_current_frame ();
+
+  if (prev_frame)
+    frame = get_prev_frame (frame);
+
+  for (; frame != NULL; frame = get_prev_frame (frame))
+    {
+      const char *fn = NULL;
+      symtab_and_line sal;
+      struct symbol *sym;
+
+      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+	break;
+      if (get_frame_type (frame) != INLINE_FRAME)
+	break;
+
+      sal = find_frame_sal (frame);
+      sym = get_frame_function (frame);
+
+      if (sym != NULL)
+	fn = SYMBOL_PRINT_NAME (sym);
+
+      if (sal.line != 0
+	  && function_name_is_marked_for_skip (fn, sal))
+	return true;
+    }
+
+  return false;
+}
+
 /* If the event thread has the stop requested flag set, pretend it
    stopped for a GDB_SIGNAL_0 (i.e., as if it stopped due to
    target_stop).  */
@@ -6531,7 +6570,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  tmp_sal))
+						  tmp_sal)
+	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
@@ -6697,7 +6737,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  if (call_sal.line == ecs->event_thread->current_line
 	      && call_sal.symtab == ecs->event_thread->current_symtab)
-	    step_into_inline_frame (ecs->event_thread);
+	    {
+	      step_into_inline_frame (ecs->event_thread);
+	      if (inline_frame_is_marked_for_skip (false, ecs->event_thread))
+		{
+		  keep_going (ecs);
+		  return;
+		}
+	    }
 
 	  end_stepping_range (ecs);
 	  return;
@@ -6731,7 +6778,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepping through inlined function\n");
 
-      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL)
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+	  || inline_frame_is_marked_for_skip (false, ecs->event_thread))
 	keep_going (ecs);
       else
 	end_stepping_range (ecs);
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-a-test-case-for-skip-with-inlined-functions.patch --]
[-- Type: text/x-patch; name="0002-Add-a-test-case-for-skip-with-inlined-functions.patch", Size: 5381 bytes --]

From d8fc12de59c47c94ef42f91e0793554df1e4d714 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, 30 Oct 2019 21:35:22 +0100
Subject: [PATCH 2/2] Add a test case for skip with inlined functions

---
 gdb/testsuite/gdb.base/skip2.c   | 64 ++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/skip2.exp | 80 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/skip2.c
 create mode 100644 gdb/testsuite/gdb.base/skip2.exp

diff --git a/gdb/testsuite/gdb.base/skip2.c b/gdb/testsuite/gdb.base/skip2.c
new file mode 100644
index 0000000..d1abd45
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.c
@@ -0,0 +1,64 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+int bar (void);
+int baz (int);
+void skip1_test_skip_file_and_function (void);
+void test_skip_file_and_function (void);
+
+__attribute__((__always_inline__)) static inline int
+foo (void)
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  volatile int x;
+
+  /* step immediately into the inlined code */
+  x = baz (foo ());
+
+  /* step first over non-inline code, this involves a different code path */
+  x = 0; x = baz (foo ());
+
+  test_skip_file_and_function ();
+
+  return 0;
+}
+
+static void
+test_skip (void)
+{
+}
+
+static void
+end_test_skip_file_and_function (void)
+{
+  abort ();
+}
+
+void
+test_skip_file_and_function (void)
+{
+  test_skip ();
+  skip1_test_skip_file_and_function ();
+  end_test_skip_file_and_function ();
+}
diff --git a/gdb/testsuite/gdb.base/skip2.exp b/gdb/testsuite/gdb.base/skip2.exp
new file mode 100644
index 0000000..a59dd0f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.exp
@@ -0,0 +1,80 @@
+#   Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib completion-support.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" "skip2" \
+                          {skip2.c skip1.c } \
+                          {debug nowarnings}] } {
+    return -1
+}
+
+set srcfile skip2.c
+set srcfile1 skip1.c
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+# Create a skiplist entry for a specified file and function.
+
+gdb_test "skip function foo" "Function foo will be skipped when stepping\."
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+gdb_test "step" ".*" "step again into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+gdb_test "step" ".*" "step in the baz, again"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
+gdb_test "step" ".*" "step back to main, again"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "double step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+    gdb_test "step" ".*" "step back to main"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 2" ".*" "step again into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+    gdb_test "step" ".*" "step back to main, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "triple step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 3" ".*" "step over baz"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 3" ".*" "step over baz, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
-- 
1.9.1

[PATCHv4] Make "skip" work on inline frames

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

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.

On 10/31/19 8:19 PM, Bernd Edlinger wrote:
> On 10/31/19 7:00 PM, Pedro Alves wrote:
>> 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
>>
> 
> Ah, thanks for that hint!
> 
> always_inline works quite well.
> 
> The debug session started (using gcc 4.8.4) with -O2 -g on the line with "{" in main,
> and with -O0 -g at the first real statement, so I had to remove one step.
> I did not worry about it initially, but now I agree that would have caused trouble.
> 
> 
> I attached both parts of the patch, the fist part unchanged from previous.
> The test case now makes sure to not repeat the names.
> 
> 
> Thanks
> Bernd.
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch --]
[-- Type: text/x-patch; name="0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch", Size: 4454 bytes --]

From fade08bee3b10ea412f4a11c588b219e1fbffc63 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 18 Oct 2019 14:28:45 +0200
Subject: [PATCH 1/2] Check all inline frames if they are marked for skip

This makes the skip command work in optimized builds,
where skipped functions may be inlined.
Previously that was only working when stepping into
a non-inlined function.
---
 gdb/infcmd.c | 20 ++++++++++++++++++--
 gdb/infrun.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 2a25346..af66eaf 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -52,6 +52,7 @@
 #include "thread-fsm.h"
 #include "top.h"
 #include "interps.h"
+#include "skip.h"
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
@@ -1106,14 +1107,29 @@ prepare_one_step (struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
+	      const char *fn = NULL;
+	      symtab_and_line sal;
+	      struct symbol *sym;
 
 	      /* Pretend that we've ran.  */
 	      resume_ptid = user_visible_resume_ptid (1);
 	      set_running (resume_ptid, 1);
 
 	      step_into_inline_frame (tp);
-	      sm->count--;
-	      return prepare_one_step (sm);
+
+	      frame = get_current_frame ();
+	      sal = find_frame_sal (frame);
+	      sym = get_frame_function (frame);
+
+	      if (sym != NULL)
+		fn = sym->print_name ();
+
+	      if (sal.line == 0
+		  || !function_name_is_marked_for_skip (fn, sal))
+		{
+		  sm->count--;
+		  return prepare_one_step (sm);
+		}
 	    }
 
 	  pc = get_frame_pc (frame);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3718674..ab5306a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4035,6 +4035,45 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
   return 0;
 }
 
+/* Look for an inline frame that is marked for skip.
+   If PREV_FRAME is TRUE start at the previous frame,
+   otherwise start at the current frame.  Stop at the
+   first non-inline frame, or at the frame where the
+   step started.  */
+
+static bool
+inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
+{
+  struct frame_info *frame = get_current_frame ();
+
+  if (prev_frame)
+    frame = get_prev_frame (frame);
+
+  for (; frame != NULL; frame = get_prev_frame (frame))
+    {
+      const char *fn = NULL;
+      symtab_and_line sal;
+      struct symbol *sym;
+
+      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+	break;
+      if (get_frame_type (frame) != INLINE_FRAME)
+	break;
+
+      sal = find_frame_sal (frame);
+      sym = get_frame_function (frame);
+
+      if (sym != NULL)
+	fn = sym->print_name ();
+
+      if (sal.line != 0
+	  && function_name_is_marked_for_skip (fn, sal))
+	return true;
+    }
+
+  return false;
+}
+
 /* If the event thread has the stop requested flag set, pretend it
    stopped for a GDB_SIGNAL_0 (i.e., as if it stopped due to
    target_stop).  */
@@ -6525,7 +6564,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  tmp_sal))
+						  tmp_sal)
+	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
@@ -6691,7 +6731,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  if (call_sal.line == ecs->event_thread->current_line
 	      && call_sal.symtab == ecs->event_thread->current_symtab)
-	    step_into_inline_frame (ecs->event_thread);
+	    {
+	      step_into_inline_frame (ecs->event_thread);
+	      if (inline_frame_is_marked_for_skip (false, ecs->event_thread))
+		{
+		  keep_going (ecs);
+		  return;
+		}
+	    }
 
 	  end_stepping_range (ecs);
 	  return;
@@ -6725,7 +6772,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepping through inlined function\n");
 
-      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL)
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+	  || inline_frame_is_marked_for_skip (false, ecs->event_thread))
 	keep_going (ecs);
       else
 	end_stepping_range (ecs);
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-a-test-case-for-skip-with-inlined-functions.patch --]
[-- Type: text/x-patch; name="0002-Add-a-test-case-for-skip-with-inlined-functions.patch", Size: 5381 bytes --]

From 4b8c278b30898ab37db3dcfb73d2b8d1f1773569 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, 30 Oct 2019 21:35:22 +0100
Subject: [PATCH 2/2] Add a test case for skip with inlined functions

---
 gdb/testsuite/gdb.base/skip2.c   | 64 ++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/skip2.exp | 80 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/skip2.c
 create mode 100644 gdb/testsuite/gdb.base/skip2.exp

diff --git a/gdb/testsuite/gdb.base/skip2.c b/gdb/testsuite/gdb.base/skip2.c
new file mode 100644
index 0000000..d1abd45
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.c
@@ -0,0 +1,64 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+int bar (void);
+int baz (int);
+void skip1_test_skip_file_and_function (void);
+void test_skip_file_and_function (void);
+
+__attribute__((__always_inline__)) static inline int
+foo (void)
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  volatile int x;
+
+  /* step immediately into the inlined code */
+  x = baz (foo ());
+
+  /* step first over non-inline code, this involves a different code path */
+  x = 0; x = baz (foo ());
+
+  test_skip_file_and_function ();
+
+  return 0;
+}
+
+static void
+test_skip (void)
+{
+}
+
+static void
+end_test_skip_file_and_function (void)
+{
+  abort ();
+}
+
+void
+test_skip_file_and_function (void)
+{
+  test_skip ();
+  skip1_test_skip_file_and_function ();
+  end_test_skip_file_and_function ();
+}
diff --git a/gdb/testsuite/gdb.base/skip2.exp b/gdb/testsuite/gdb.base/skip2.exp
new file mode 100644
index 0000000..a59dd0f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.exp
@@ -0,0 +1,80 @@
+#   Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib completion-support.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" "skip2" \
+                          {skip2.c skip1.c } \
+                          {debug nowarnings}] } {
+    return -1
+}
+
+set srcfile skip2.c
+set srcfile1 skip1.c
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+# Create a skiplist entry for a specified file and function.
+
+gdb_test "skip function foo" "Function foo will be skipped when stepping\."
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+gdb_test "step" ".*" "step again into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+gdb_test "step" ".*" "step in the baz, again"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
+gdb_test "step" ".*" "step back to main, again"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "double step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+    gdb_test "step" ".*" "step back to main"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 2" ".*" "step again into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+    gdb_test "step" ".*" "step back to main, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "triple step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 3" ".*" "step over baz"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 3" ".*" "step over baz, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
-- 
1.9.1

[PING] [PATCHv4] Make "skip" work on inline frames

[Bernd Edlinger]

Ping...


On 11/24/19 12:22 PM, 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.
> 
> On 10/31/19 8:19 PM, Bernd Edlinger wrote:
>> On 10/31/19 7:00 PM, Pedro Alves wrote:
>>> 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
>>>
>>
>> Ah, thanks for that hint!
>>
>> always_inline works quite well.
>>
>> The debug session started (using gcc 4.8.4) with -O2 -g on the line with "{" in main,
>> and with -O0 -g at the first real statement, so I had to remove one step.
>> I did not worry about it initially, but now I agree that would have caused trouble.
>>
>>
>> I attached both parts of the patch, the fist part unchanged from previous.
>> The test case now makes sure to not repeat the names.
>>
>>
>> Thanks
>> Bernd.
>>

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

[Simon Marchi]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb.log --]
[-- Type: text/x-log; name="gdb.log", Size: 11436 bytes --]

Test run by simark on Sun Dec  1 21:33:16 2019
Native configuration is x86_64-pc-linux-gnu

		=== gdb tests ===

Schedule of variations:
    unix

Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
Using /home/simark/src/binutils-gdb/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
Running /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.exp ...
get_compiler_info: gcc-9-2-0
Executing on host: gcc  -fno-stack-protector  -fdiagnostics-color=never -w -c -g  -o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip20.o /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c    (timeout = 300)
spawn -ignore SIGHUP gcc -fno-stack-protector -fdiagnostics-color=never -w -c -g -o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip20.o /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c
Executing on host: gcc  -fno-stack-protector  -fdiagnostics-color=never -w -c -g  -o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip21.o /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c    (timeout = 300)
spawn -ignore SIGHUP gcc -fno-stack-protector -fdiagnostics-color=never -w -c -g -o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip21.o /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c
Executing on host: gcc  -fno-stack-protector /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip20.o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip21.o  -fdiagnostics-color=never -w -g  -lm   -o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip2    (timeout = 300)
spawn -ignore SIGHUP gcc -fno-stack-protector /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip20.o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip21.o -fdiagnostics-color=never -w -g -lm -o /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip2
spawn /home/simark/build/binutils-gdb/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /home/simark/build/binutils-gdb/gdb/testsuite/../data-directory
warning: Found custom handler for signal 7 (Bus error) preinstalled.
warning: Found custom handler for signal 8 (Floating point exception) preinstalled.
warning: Found custom handler for signal 11 (Segmentation fault) preinstalled.
Some signal dispositions inherited from the environment (SIG_DFL/SIG_IGN)
won't be propagated to spawned programs.
GNU gdb (GDB) 9.0.50.20191202-git
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) set height 0
(gdb) set width 0
(gdb) dir
Reinitialize source path to empty? (y or n) y
Source directories searched: $cdir:$cwd
(gdb) dir /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base
Source directories searched: /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base:$cdir:$cwd
(gdb) kill
The program is not being run.
(gdb) file /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip2
Reading symbols from /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip2...
(gdb) delete breakpoints
(gdb) info breakpoints
No breakpoints or watchpoints.
(gdb) break main
Breakpoint 1 at 0x1141: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c, line 37.
(gdb) run 
Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip2 

Breakpoint 1, main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
37	  x = baz (foo ());
(gdb) skip function foo
Function foo will be skipped when stepping.
(gdb) PASS: gdb.base/skip2.exp: skip function foo
bt
#0  main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: in the main
step
baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:27
27	  return a + 1;
(gdb) PASS: gdb.base/skip2.exp: step into baz, since foo will be skipped
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:27
#1  0x000055555555514d in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: in the baz, since foo was skipped
step
28	}
(gdb) PASS: gdb.base/skip2.exp: step in the baz
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:28
#1  0x000055555555514d in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: still in the baz
step
main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
37	  x = baz (foo ());
(gdb) PASS: gdb.base/skip2.exp: step back to main
bt
#0  main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: again in the main
step
40	  x = 0; x = baz (foo ());
(gdb) PASS: gdb.base/skip2.exp: step again into baz, since foo will be skipped
bt
#0  main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:40
(gdb) FAIL: gdb.base/skip2.exp: again in the baz
step
baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:27
27	  return a + 1;
(gdb) PASS: gdb.base/skip2.exp: step in the baz, again
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:27
#1  0x0000555555555163 in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:40
(gdb) PASS: gdb.base/skip2.exp: still in the baz, again
step
28	}
(gdb) PASS: gdb.base/skip2.exp: step back to main, again
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:28
#1  0x0000555555555163 in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:40
(gdb) FAIL: gdb.base/skip2.exp: again back to main
delete breakpoints
Delete all breakpoints? (y or n) y
(gdb) info breakpoints
No breakpoints or watchpoints.
(gdb) break main
Breakpoint 2 at 0x555555555141: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c, line 37.
(gdb) run 
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip2 

Breakpoint 2, main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
37	  x = baz (foo ());
(gdb) bt
#0  main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: double step: in the main
step 2
28	}
(gdb) PASS: gdb.base/skip2.exp: double step: step into baz, since foo will be skipped
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:28
#1  0x000055555555514d in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: double step: still in the baz
step
main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
37	  x = baz (foo ());
(gdb) PASS: gdb.base/skip2.exp: double step: step back to main
bt
#0  main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: double step: again in the main
step 2
baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:27
27	  return a + 1;
(gdb) PASS: gdb.base/skip2.exp: double step: step again into baz, since foo will be skipped
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:27
#1  0x0000555555555163 in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:40
(gdb) PASS: gdb.base/skip2.exp: double step: again in the baz
step
28	}
(gdb) PASS: gdb.base/skip2.exp: double step: step back to main, again
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:28
#1  0x0000555555555163 in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:40
(gdb) FAIL: gdb.base/skip2.exp: double step: again back to main
delete breakpoints
Delete all breakpoints? (y or n) y
(gdb) info breakpoints
No breakpoints or watchpoints.
(gdb) break main
Breakpoint 3 at 0x555555555141: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c, line 37.
(gdb) run 
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/skip2/skip2 

Breakpoint 3, main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
37	  x = baz (foo ());
(gdb) bt
#0  main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: triple step: in the main
step 3
main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
37	  x = baz (foo ());
(gdb) PASS: gdb.base/skip2.exp: triple step: step over baz
bt
#0  main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:37
(gdb) PASS: gdb.base/skip2.exp: triple step: again in the main
step 3
28	}
(gdb) PASS: gdb.base/skip2.exp: triple step: step over baz, again
bt
#0  baz (a=1) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip1.c:28
#1  0x0000555555555163 in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.c:40
(gdb) FAIL: gdb.base/skip2.exp: triple step: again back to main
testcase /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/skip2.exp completed in 1 seconds

		=== gdb Summary ===

# of expected passes		24
# of unexpected failures	4
Executing on host: /home/simark/build/binutils-gdb/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /home/simark/build/binutils-gdb/gdb/testsuite/../data-directory --version    (timeout = 300)
spawn -ignore SIGHUP /home/simark/build/binutils-gdb/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /home/simark/build/binutils-gdb/gdb/testsuite/../data-directory --version
warning: Found custom handler for signal 7 (Bus error) preinstalled.
warning: Found custom handler for signal 8 (Floating point exception) preinstalled.
warning: Found custom handler for signal 11 (Segmentation fault) preinstalled.
Some signal dispositions inherited from the environment (SIG_DFL/SIG_IGN)
won't be propagated to spawned programs.
GNU gdb (GDB) 9.0.50.20191202-git
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
/home/simark/build/binutils-gdb/gdb/gdb version  11 -nw -nx -data-directory /home/simark/build/binutils-gdb/gdb/testsuite/../data-directory 

runtest completed at Sun Dec  1 21:33:17 2019

[PATCHv5] Make "skip" work on inline frames

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch --]
[-- Type: text/x-patch; name="0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch", Size: 4454 bytes --]

From 00db11badb0ec45ae5086165d6afeabf31d56637 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 18 Oct 2019 14:28:45 +0200
Subject: [PATCH 1/2] Check all inline frames if they are marked for skip

This makes the skip command work in optimized builds,
where skipped functions may be inlined.
Previously that was only working when stepping into
a non-inlined function.
---
 gdb/infcmd.c | 20 ++++++++++++++++++--
 gdb/infrun.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 2a25346..af66eaf 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -52,6 +52,7 @@
 #include "thread-fsm.h"
 #include "top.h"
 #include "interps.h"
+#include "skip.h"
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
@@ -1106,14 +1107,29 @@ prepare_one_step (struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
+	      const char *fn = NULL;
+	      symtab_and_line sal;
+	      struct symbol *sym;
 
 	      /* Pretend that we've ran.  */
 	      resume_ptid = user_visible_resume_ptid (1);
 	      set_running (resume_ptid, 1);
 
 	      step_into_inline_frame (tp);
-	      sm->count--;
-	      return prepare_one_step (sm);
+
+	      frame = get_current_frame ();
+	      sal = find_frame_sal (frame);
+	      sym = get_frame_function (frame);
+
+	      if (sym != NULL)
+		fn = sym->print_name ();
+
+	      if (sal.line == 0
+		  || !function_name_is_marked_for_skip (fn, sal))
+		{
+		  sm->count--;
+		  return prepare_one_step (sm);
+		}
 	    }
 
 	  pc = get_frame_pc (frame);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6a346d6..7ddd21d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4034,6 +4034,45 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
   return 0;
 }
 
+/* Look for an inline frame that is marked for skip.
+   If PREV_FRAME is TRUE start at the previous frame,
+   otherwise start at the current frame.  Stop at the
+   first non-inline frame, or at the frame where the
+   step started.  */
+
+static bool
+inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
+{
+  struct frame_info *frame = get_current_frame ();
+
+  if (prev_frame)
+    frame = get_prev_frame (frame);
+
+  for (; frame != NULL; frame = get_prev_frame (frame))
+    {
+      const char *fn = NULL;
+      symtab_and_line sal;
+      struct symbol *sym;
+
+      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+	break;
+      if (get_frame_type (frame) != INLINE_FRAME)
+	break;
+
+      sal = find_frame_sal (frame);
+      sym = get_frame_function (frame);
+
+      if (sym != NULL)
+	fn = sym->print_name ();
+
+      if (sal.line != 0
+	  && function_name_is_marked_for_skip (fn, sal))
+	return true;
+    }
+
+  return false;
+}
+
 /* If the event thread has the stop requested flag set, pretend it
    stopped for a GDB_SIGNAL_0 (i.e., as if it stopped due to
    target_stop).  */
@@ -6524,7 +6563,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  tmp_sal))
+						  tmp_sal)
+	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
@@ -6690,7 +6730,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  if (call_sal.line == ecs->event_thread->current_line
 	      && call_sal.symtab == ecs->event_thread->current_symtab)
-	    step_into_inline_frame (ecs->event_thread);
+	    {
+	      step_into_inline_frame (ecs->event_thread);
+	      if (inline_frame_is_marked_for_skip (false, ecs->event_thread))
+		{
+		  keep_going (ecs);
+		  return;
+		}
+	    }
 
 	  end_stepping_range (ecs);
 	  return;
@@ -6724,7 +6771,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepping through inlined function\n");
 
-      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL)
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+	  || inline_frame_is_marked_for_skip (false, ecs->event_thread))
 	keep_going (ecs);
       else
 	end_stepping_range (ecs);
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-a-test-case-for-skip-with-inlined-functions.patch --]
[-- Type: text/x-patch; name="0002-Add-a-test-case-for-skip-with-inlined-functions.patch", Size: 7435 bytes --]

From 32627e64b9b7b437c5f6d46d1138ecba09816273 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, 30 Oct 2019 21:35:22 +0100
Subject: [PATCH 2/2] Add a test case for skip with inlined functions

Fixed an issue in skip.exp that made it fail with recent
gcc versions, while already at it.
---
 gdb/testsuite/gdb.base/skip-inline.c   | 64 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/skip-inline.exp | 80 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/skip.exp        | 12 +++--
 3 files changed, 153 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/skip-inline.c
 create mode 100644 gdb/testsuite/gdb.base/skip-inline.exp

diff --git a/gdb/testsuite/gdb.base/skip-inline.c b/gdb/testsuite/gdb.base/skip-inline.c
new file mode 100644
index 0000000..e562149
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip-inline.c
@@ -0,0 +1,64 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+int bar (void);
+int baz (int);
+void skip1_test_skip_file_and_function (void);
+void test_skip_file_and_function (void);
+
+__attribute__((__always_inline__)) static inline int
+foo (void)
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  volatile int x;
+
+  /* step immediately into the inlined code */
+  baz (foo ());
+
+  /* step first over non-inline code, this involves a different code path */
+  x = 0; x = baz (foo ());
+
+  test_skip_file_and_function ();
+
+  return 0;
+}
+
+static void
+test_skip (void)
+{
+}
+
+static void
+end_test_skip_file_and_function (void)
+{
+  abort ();
+}
+
+void
+test_skip_file_and_function (void)
+{
+  test_skip ();
+  skip1_test_skip_file_and_function ();
+  end_test_skip_file_and_function ();
+}
diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp
new file mode 100644
index 0000000..9d490bd
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip-inline.exp
@@ -0,0 +1,80 @@
+#   Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib completion-support.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" "skip-inline" \
+                          {skip-inline.c skip1.c } \
+                          {debug nowarnings}] } {
+    return -1
+}
+
+set srcfile skip-inline.c
+set srcfile1 skip1.c
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+# Create a skiplist entry for a specified file and function.
+
+gdb_test "skip function foo" "Function foo will be skipped when stepping\."
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+gdb_test "step" ".*" "step again into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+gdb_test "step" ".*" "step in the baz, again"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
+gdb_test "step" ".*" "step back to main, again"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "double step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+    gdb_test "step" ".*" "step back to main"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 2" ".*" "step again into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+    gdb_test "step" ".*" "step back to main, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "triple step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 3" ".*" "step over baz"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 3" ".*" "step over baz, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp
index d763194..0fd44e0 100644
--- a/gdb/testsuite/gdb.base/skip.exp
+++ b/gdb/testsuite/gdb.base/skip.exp
@@ -142,7 +142,9 @@ with_test_prefix "step after disabling 3" {
 
     gdb_test "step" "bar \\(\\) at.*" "step 1"
     gdb_test "step" ".*" "step 2"; # Return from foo()
-    gdb_test "step" "foo \\(\\) at.*" "step 3"
+    # 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"
     gdb_test "step" ".*" "step 4"; # Return from bar()
     gdb_test "step" "main \\(\\) at.*" "step 5"
 }
@@ -261,7 +263,9 @@ with_test_prefix "step using -fu for baz" {
     gdb_test_no_output "skip enable 7"
     gdb_test "step" "bar \\(\\) at.*" "step 1"
     gdb_test "step" ".*" "step 2"; # Return from bar()
-    gdb_test "step" "foo \\(\\) at.*" "step 3"
+    # 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"
     gdb_test "step" ".*" "step 4"; # Return from foo()
     gdb_test "step" "main \\(\\) at.*" "step 5"
 }
@@ -276,7 +280,9 @@ with_test_prefix "step using -rfu for baz" {
     gdb_test_no_output "skip enable 8"
     gdb_test "step" "bar \\(\\) at.*" "step 1"
     gdb_test "step" ".*" "step 2"; # Return from bar()
-    gdb_test "step" "foo \\(\\) at.*" "step 3"
+    # 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"
     gdb_test "step" ".*" "step 4"; # Return from foo()
     gdb_test "step" "main \\(\\) at.*" "step 5"
 }
-- 
1.9.1

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

[Simon Marchi]

On 2019-12-02 11:47 a.m., Bernd Edlinger wrote:
> 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?

Yes, I confirm it passes now, thanks!

Simon

[PING] [PATCHv5] Make "skip" work on inline frames

[Bernd Edlinger]

Ping...

The latest version of this patch can be found here:
https://sourceware.org/ml/gdb-patches/2019-12/msg00047.html


Thanks
Bernd.

On 12/2/19 5:47 PM, 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.
> 

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

[Simon Marchi]

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

[PATCHv6] Make "skip" work on inline frames

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

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.

[-- Attachment #2: changelog.txt --]
[-- Type: text/plain, Size: 420 bytes --]

gdb:
2019-12-15  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* infcmd.c (prepare_one_step): Step over skipped inline functions.
	* infrun.c (inline_frame_is_marked_for_skip): New helper function.
	(process_event_stop_test): Keep stepping over skipped inline functions.

gdb/testsuite:
2019-12-15  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gdb.base/skip-inline.c: New file.
	* gdb.base/skip-inline.exp: New file.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch --]
[-- Type: text/x-patch; name="0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch", Size: 4454 bytes --]

From ed6be155e5cbf560b2fc20735fbe8f5b37271e75 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 18 Oct 2019 14:28:45 +0200
Subject: [PATCH 1/2] Check all inline frames if they are marked for skip

This makes the skip command work in optimized builds,
where skipped functions may be inlined.
Previously that was only working when stepping into
a non-inlined function.
---
 gdb/infcmd.c | 20 ++++++++++++++++++--
 gdb/infrun.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 2a25346..af66eaf 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -52,6 +52,7 @@
 #include "thread-fsm.h"
 #include "top.h"
 #include "interps.h"
+#include "skip.h"
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
@@ -1106,14 +1107,29 @@ prepare_one_step (struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
+	      const char *fn = NULL;
+	      symtab_and_line sal;
+	      struct symbol *sym;
 
 	      /* Pretend that we've ran.  */
 	      resume_ptid = user_visible_resume_ptid (1);
 	      set_running (resume_ptid, 1);
 
 	      step_into_inline_frame (tp);
-	      sm->count--;
-	      return prepare_one_step (sm);
+
+	      frame = get_current_frame ();
+	      sal = find_frame_sal (frame);
+	      sym = get_frame_function (frame);
+
+	      if (sym != NULL)
+		fn = sym->print_name ();
+
+	      if (sal.line == 0
+		  || !function_name_is_marked_for_skip (fn, sal))
+		{
+		  sm->count--;
+		  return prepare_one_step (sm);
+		}
 	    }
 
 	  pc = get_frame_pc (frame);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6a346d6..7ddd21d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4034,6 +4034,45 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
   return 0;
 }
 
+/* Look for an inline frame that is marked for skip.
+   If PREV_FRAME is TRUE start at the previous frame,
+   otherwise start at the current frame.  Stop at the
+   first non-inline frame, or at the frame where the
+   step started.  */
+
+static bool
+inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
+{
+  struct frame_info *frame = get_current_frame ();
+
+  if (prev_frame)
+    frame = get_prev_frame (frame);
+
+  for (; frame != NULL; frame = get_prev_frame (frame))
+    {
+      const char *fn = NULL;
+      symtab_and_line sal;
+      struct symbol *sym;
+
+      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+	break;
+      if (get_frame_type (frame) != INLINE_FRAME)
+	break;
+
+      sal = find_frame_sal (frame);
+      sym = get_frame_function (frame);
+
+      if (sym != NULL)
+	fn = sym->print_name ();
+
+      if (sal.line != 0
+	  && function_name_is_marked_for_skip (fn, sal))
+	return true;
+    }
+
+  return false;
+}
+
 /* If the event thread has the stop requested flag set, pretend it
    stopped for a GDB_SIGNAL_0 (i.e., as if it stopped due to
    target_stop).  */
@@ -6524,7 +6563,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  tmp_sal))
+						  tmp_sal)
+	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
@@ -6690,7 +6730,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  if (call_sal.line == ecs->event_thread->current_line
 	      && call_sal.symtab == ecs->event_thread->current_symtab)
-	    step_into_inline_frame (ecs->event_thread);
+	    {
+	      step_into_inline_frame (ecs->event_thread);
+	      if (inline_frame_is_marked_for_skip (false, ecs->event_thread))
+		{
+		  keep_going (ecs);
+		  return;
+		}
+	    }
 
 	  end_stepping_range (ecs);
 	  return;
@@ -6724,7 +6771,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepping through inlined function\n");
 
-      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL)
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+	  || inline_frame_is_marked_for_skip (false, ecs->event_thread))
 	keep_going (ecs);
       else
 	end_stepping_range (ecs);
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-Add-a-test-case-for-skip-with-inlined-functions.patch --]
[-- Type: text/x-patch; name="0002-Add-a-test-case-for-skip-with-inlined-functions.patch", Size: 5368 bytes --]

From 7bee2af24e4f21ae449f604ac549492d5fcab1a4 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, 30 Oct 2019 21:35:22 +0100
Subject: [PATCH 2/2] Add a test case for skip with inlined functions

---
 gdb/testsuite/gdb.base/skip-inline.c   | 64 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/skip-inline.exp | 78 ++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/skip-inline.c
 create mode 100644 gdb/testsuite/gdb.base/skip-inline.exp

diff --git a/gdb/testsuite/gdb.base/skip-inline.c b/gdb/testsuite/gdb.base/skip-inline.c
new file mode 100644
index 0000000..e562149
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip-inline.c
@@ -0,0 +1,64 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+int bar (void);
+int baz (int);
+void skip1_test_skip_file_and_function (void);
+void test_skip_file_and_function (void);
+
+__attribute__((__always_inline__)) static inline int
+foo (void)
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  volatile int x;
+
+  /* step immediately into the inlined code */
+  baz (foo ());
+
+  /* step first over non-inline code, this involves a different code path */
+  x = 0; x = baz (foo ());
+
+  test_skip_file_and_function ();
+
+  return 0;
+}
+
+static void
+test_skip (void)
+{
+}
+
+static void
+end_test_skip_file_and_function (void)
+{
+  abort ();
+}
+
+void
+test_skip_file_and_function (void)
+{
+  test_skip ();
+  skip1_test_skip_file_and_function ();
+  end_test_skip_file_and_function ();
+}
diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp
new file mode 100644
index 0000000..4ab9ecf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip-inline.exp
@@ -0,0 +1,78 @@
+#   Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" "skip-inline" \
+			  {skip-inline.c skip1.c } \
+			  {debug nowarnings}] } {
+    return -1
+}
+
+set srcfile skip-inline.c
+set srcfile1 skip1.c
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+# Create a skiplist entry for a specified file and function.
+
+gdb_test "skip function foo" "Function foo will be skipped when stepping\."
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+gdb_test "step" ".*" "step again into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+gdb_test "step" ".*" "step in the baz, again"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
+gdb_test "step" ".*" "step back to main, again"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "double step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+    gdb_test "step" ".*" "step back to main"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 2" ".*" "step again into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+    gdb_test "step" ".*" "step back to main, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "triple step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 3" ".*" "step over baz"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 3" ".*" "step over baz, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
-- 
1.9.1

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

[Simon Marchi]

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?

Simon

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

[Bernd Edlinger]

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
> 

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

[Simon Marchi]

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

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

[Bernd Edlinger]

On 12/17/19 3:00 AM, Simon Marchi wrote:
> 
> 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.
> 

Will do.

Thanks a lot for you review.


Bernd.