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

login
register
mail settings
Submitter Bernd Edlinger
Date Oct. 20, 2019, 6:48 a.m.
Message ID <VI1PR03MB452814244371F04E2848AB68E46E0@VI1PR03MB4528.eurprd03.prod.outlook.com>
Download mbox | patch
Permalink /patch/35179/
State New
Headers show

Comments

Bernd Edlinger - Oct. 20, 2019, 6:48 a.m.
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.
Bernd Edlinger - Oct. 26, 2019, 8:06 a.m.
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.
>
Simon Marchi - Oct. 27, 2019, 1:52 a.m.
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
Simon Marchi - Oct. 27, 2019, 2:17 a.m.
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
Bernd Edlinger - Oct. 30, 2019, 8:05 p.m.
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.
Simon Marchi - Oct. 30, 2019, 8:18 p.m.
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

Patch

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