On Thu, 2020-06-18 at 13:22 -0700, Nick Desaulniers wrote: > On Tue, Jun 16, 2020 at 3:36 PM 'Nathan Huckleberry' via Clang Built > Linux <clang-built-li...@googlegroups.com> wrote: > > > > Since clang does not push pc and sp in function prologues, the current > > implementation of unwind_frame does not work. By using the previous > > frame's lr/fp instead of saved pc/sp we get valid unwinds on clang-built > > kernels. > > > > The bounds check on next frame pointer must be changed as well since > > there are 8 less bytes between frames. > > > > This fixes /proc/<pid>/stack. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/912 > > Signed-off-by: Nathan Huckleberry <nh...@google.com> > > Thanks for the patch, Nathan! When I looked into this, I found the > latest ARM AAPCS [0] specifically says (with `it` referring to `a > platform`: "It may permit the frame pointer register to be used as a > general-purpose callee-saved register, but provide a platform-specific > mechanism for external agents to reliably locate the chain of frame > records." While it's good that's acknowledged in the documentation, > the current wording is relaxed in order to not force current > implementations to converge. This has the unfortunate side effect of > making finding the frame pointer toolchain dependendent, hence this > patch and your previous commit 6dc5fd93b2f1 ("ARM: 8900/1: > UNWINDER_FRAME_POINTER implementation for Clang"). Being more > specific in the documentation would force at least one implementation > to change, and I think that would also imply an ABI break. So I can > see it both ways, though I still would prefer that the language pin > this down, even if we had to change LLVM. Just providing additional > context for folks on the thread. > > This should also have a reported by tag from Miles, in v2. > > Reported-by: Miles Chen <miles.c...@mediatek.com> > > Miles mentioned to me that he tested it, but maybe Miles can confirm > that publicly on-list via an explicit Tested-by: tag?
Thanks for the patch. Tested-by: Miles Chen <miles.c...@mediatek.com> > > This would be useful for us to have in stable; otherwise we'll have to > carry out of tree in Android and CrOS, which I'd rather not do. Via > Documentation/process/stable-kernel-rules.rst, if you add this tag to > V2, that will greatly simplify submitting this to stable: > Cc: sta...@vger.kernel.org > > Miles also showed me the behavior of this patch for different kernel > versions, which varies anywhere from empty or single entry traces to > panics, so this is pretty important that this works for Clang builds. > > [0] https://static.docs.arm.com/ihi0042/i/aapcs32.pdf > > > --- > > arch/arm/kernel/stacktrace.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c > > index cc726afea023..76ea4178a55c 100644 > > --- a/arch/arm/kernel/stacktrace.c > > +++ b/arch/arm/kernel/stacktrace.c > > @@ -22,6 +22,19 @@ > > * A simple function epilogue looks like this: > > * ldm sp, {fp, sp, pc} > > * > > + * When compiled with clang, pc and sp are not pushed. A simple function > > + * prologue looks like this when built with clang: > > + * > > + * stmdb {..., fp, lr} > > + * add fp, sp, #x > > + * sub sp, sp, #y > > + * > > + * A simple function epilogue looks like this when built with clang: > > + * > > + * sub sp, fp, #x > > + * ldm {..., fp, pc} > > + * > > + * > > * Note that with framepointer enabled, even the leaf functions have the > > same > > * prologue and epilogue, therefore we can ignore the LR value in this > > case. > > */ > > @@ -34,6 +47,16 @@ int notrace unwind_frame(struct stackframe *frame) > > low = frame->sp; > > high = ALIGN(low, THREAD_SIZE); > > > > +#ifdef CONFIG_CC_IS_CLANG > > + /* check current frame pointer is within bounds */ > > + if (fp < low + 4 || fp > high - 4) > > The patch LGTM; maybe Russell or Catalin could triple check this > bounds check? Assuming there's no issue, you can include on a v2 my > reviewed by: > > Reviewed-by: Nick Desaulniers <ndesaulni...@google.com> > > I'd probably wait the remainder of a week before sending a v2 to > collect additional feedback. Thank you again. > > > + return -EINVAL; > > + > > + frame->sp = frame->fp; > > + frame->fp = *(unsigned long *)(fp); > > + frame->pc = frame->lr; > > + frame->lr = *(unsigned long *)(fp + 4); > > +#else > > /* check current frame pointer is within bounds */ > > if (fp < low + 12 || fp > high - 4) > > return -EINVAL; > > @@ -42,6 +65,7 @@ int notrace unwind_frame(struct stackframe *frame) > > frame->fp = *(unsigned long *)(fp - 12); > > frame->sp = *(unsigned long *)(fp - 8); > > frame->pc = *(unsigned long *)(fp - 4); > > +#endif > > > > return 0; > > } > > -- > > 2.27.0.290.gba653c62da-goog > > > > -- >
