Re: [PATCH v2 11/15] RISC-V: Linux ABI

[Palmer Dabbelt <palmer at dabbelt dot com>]

On Wed, 20 Dec 2017 09:33:36 PST (-0800), joseph@codesourcery.com wrote:
> On Tue, 19 Dec 2017, Palmer Dabbelt wrote:
>
> > diff --git a/sysdeps/unix/sysv/linux/riscv/____longjmp_chk.S b/sysdeps/unix/sysv/linux/riscv/____longjmp_chk.S
> > new file mode 100644
> > index 000000000000..e903c7e0df2a
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/riscv/____longjmp_chk.S
> > @@ -0,0 +1,2 @@
> > +#define __longjmp ____longjmp_chk
> > +#include <__longjmp.S>
>
> I'm not clear how this is meant to achieve the ____longjmp_chk semantics,
> which involve comparing current and saved stack pointers to avoid jumping
> into a frame that has returned.  (See
> sysdeps/unix/sysv/linux/generic/____longjmp_chk.c for a C version of the
> required logic.)
>
> > diff --git a/sysdeps/unix/sysv/linux/riscv/libthread_db.abilist b/sysdeps/unix/sysv/linux/riscv/libthread_db.abilist
>
> You need a full set of abilist baselines, not just this one.

Thanks.  I think I have them all now, they'll be included as part of the v3.

> > +extern int __riscv_flush_icache (void *start, void *end, unsigned long flags);
>
> Should use __start, __end, __flags, unless you really intend start, end
> and flags to be part of the public API for this header.

Sorry, I'm a bit confused by this.  Our intent is that this function (along 
with its arguments) can be called from user code, and will remain part of the 
public ABI forever.  I looked around a bit, would something like

   #ifdef __USE_MISC
   extern int riscv_flush_icache (void *__start, void *__end, unsigned long __flags);
   #endif

be correct?

> > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
>
> This header does not look appropriately namespace-clean.  Please make sure
> that all the conform/ tests pass, conditioning contents as necessary.
> It's also missing some preprocessor indentation.

Yep, it's not.  It'll be fixed for the v3.

Thanks!