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:

    ...stuff...
    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.

Thanks!
  

Joel Brobecker <brobecker@adacore.com> writes:

> 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*.

Yeah, that is a good point...

> 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:
>
>     ...stuff...
>     svc #imm
>     insn #2    <<<-- breakpoint here
>

... and this is a good example too.

> ... 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.
>


>
> 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.

so, your initial patch is OK to me.
  

> so, your initial patch is OK to me.

Thanks, Yao. Patch now pushed.