Re: [PATCH 5/5] Add frame pointer unwinding for aarch64

[Mark Wielaard <mark at klomp dot org>]

On Tue, 2017-04-25 at 14:49 +0200, Mark Wielaard wrote:
> +bool
> +EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ ((unused)),
> +                 ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc,
> +                 ebl_pid_memory_read_t *readfunc, void *arg,
> +                 bool *signal_framep __attribute__ ((unused)))
> +{
> +  Dwarf_Word fp, lr, sp;
> +
> +  if (!getfunc(LR_REG, 1, &lr, arg))
> +    return false;
> +
> +  if (!getfunc(FP_REG, 1, &fp, arg))
> +    fp = 0;
> +
> +  if (!getfunc(SP_REG, 1, &sp, arg))
> +    sp = 0;
> +
> +  Dwarf_Word newPc, newLr, newFp, newSp;
> +
> +  // The initial frame is special. We are expected to return lr directly in this case, and we'll
> +  // come back to the same frame again in the next round.
> +  if ((pc & 0x1) == 0)
> +    {
> +      newLr = lr;
> +      newFp = fp;
> +      newSp = sp;
> +    }
> +  else
> +    {
> +      if (!readfunc(fp + LR_OFFSET, &newLr, arg))
> +        newLr = 0;
> +
> +      if (!readfunc(fp + FP_OFFSET, &newFp, arg))
> +        newFp = 0;
> +
> +      newSp = fp + SP_OFFSET;
> +    }
> +
> +  newPc = newLr & (~0x1);
> +  if (!setfunc(-1, 1, &newPc, arg))
> +    return false;

My question is about this "initial frame". In our testcase we don't have
this case since the backtrace starts in a function that has some CFI.
But I assume you have some tests that rely on this behavior.

The first question is how/why the (pc & 0x1) == 0 check works?
Why is that the correct check?

Secondly, if it really is the initial (or signal frame) we are after,
should we pass in into bool *signal_framep argument. Currently we don't

Lastly could you instead of returning the frame itself with just the pc
adjusted do that directly?

  if ((pc & 0x1) == 0)
    lr = lr & (~0x1);

And then use the code in the else clause always?

All the above might simply be me not understanding the significance of
the initial frame.

Cheers,

Mark