This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: [ARM] "svc" insn check at irrelevant address in ARM unwind info sniffer

> > I will start by saying that I see you point. If we're in a frame whose
> > first instruction is a call, then we could be seeing the same issue.
> > I would also argue that innermost-ness is important, here, assuming
> > that we agree that the ARM info is only correct at the point of call.
> > So, strictly speaking, we should not even be attempting to use it
> > for innermost frame.
> Yes, that is true.

> > Should I work on a version of the patch that merges the two ideas?
> > Or do you stand with just checking "get_frame_pc (this_frame) >
> > func_start"? I confess I know ARM unwind info just enough to get by,
> > so I trust your judgement on this.
> Let us start from the simple thing, just checking
> "get_frame_pc (this_frame) > func_start".  If we need to check
> innermost-ness for other problems in the future, we can add it then.

I have had a little bit of time to think this over while in the plane,
and I am not convinced that checking for "get_frame_pc (this_frame) >
func_start" is really treating the symptom rather than the ailment.

Here is how I understand things: The code is trying to figure out
if our frame corresponds to a system call or not. For that, it searches
the instruction at "get_frame_pc (this_frame) - 2" (or -4 for non-thumb).
To me, the reason for the -2 or the -4, is that it implicitly makes
the assumption that "get_frame_pc (this_frame)" is a *return address*.
So, it has to check the instruction immediately before that return
address. That assumption is not valid for the inner-most address.

Imagine we have a function whose code has been compiled into:

    svc #imm
    insn #2    <<<-- breakpoint here

... and we're stopped at the breakpiont.  In this case, get_frame_pc
will return the address in "insn #2", and therefore see an "svc"
instruction just before it, and conclude that we're stuck on a system
call, which is not true.

If we really want to go simple, perhaps what we can do is just
the following (untested):

    @@ -2787,6 +2787,11 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self,

       /* See if we have an ARM exception table entry covering this address.  */
       addr_in_block = get_frame_address_in_block (this_frame);
    +  /* Add a comment explaining why here... */
    +  if (get_frame_pc (this_frame) != addr_in_block)
    +    return 0;
       entry = arm_find_exidx_entry (addr_in_block, &exidx_region);
       if (!entry)
         return 0;

... but the downside of it is that it shunts the code that decides
to use the ARM table when there is no minimal symbol for our address.

     Before we make this decision, however, we check whether we
     actually have *symbol* information for the current frame.
     If not, prologue parsing would not work anyway, so we might
     as well use the exception table and hope for the best.  */
  if (find_pc_partial_function (addr_in_block, NULL, &func_start, NULL))

So, in my opinion, the patch I initially propose is still best.
When you look at the patch with whitespace changes removed (attached),
it is actually fairly tiny.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 73f26b9..f755076 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2818,7 +2818,17 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self,
       /* We also assume exception information is valid if we're currently
 	 blocked in a system call.  The system library is supposed to
-	 ensure this, so that e.g. pthread cancellation works.  */
+	 ensure this, so that e.g. pthread cancellation works.
+	 But before verifying the instruction at the point of call, make
+	 sure this_frame is actually making a call (or, said differently,
+	 that it is not the innermost frame).  For that, we compare
+	 this_frame's PC vs this_frame's addr_in_block. If equal, it means
+	 there is no call (otherwise, the PC would be the return address,
+	 which is the instruction after the call).  */
+      if (get_frame_pc (this_frame) != addr_in_block)
+	{
 	  if (arm_frame_is_thumb (this_frame))
 	      LONGEST insn;
@@ -2837,6 +2847,7 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self,
 		  && (insn & 0x0f000000) == 0x0f000000 /* svc */)
 		exc_valid = 1;
+	}
       /* Bail out if we don't know that exception information is valid.  */
       if (!exc_valid)

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]