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

[Joseph Myers <joseph at codesourcery dot com>]

On Mon, 25 Dec 2017, Palmer Dabbelt wrote:

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

You quoted this comment of mine without responding to it.  I don't see any 
sign of it having been addressed in the latest version of the port.

> > > +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?

There should be no __USE_MISC conditional, since this is in a header not 
defined by any standard.

But the parameter names should start with __, yes, unless it is 
specifically intended and documented that it's invalid for a user to do:

#define start something else
#include <sys/cachectl.h>

(and I don't see any reason why you'd want to define that to be invalid).

I see that in the latest port version you've renamed the function to 
__riscv_flush_icache.  Whether that's appropriate depends on whether this 
function is intended to be used in libraries such as libgcc with namespace 
concerns - normally public APIs do not have the leading __ on the name of 
the public interface, but an __ name is needed in certain cases if the 
function is expected to be called from somewhere like libgcc.

I think this machine-specific interface, whatever the function ends up 
being called, needs documenting in the glibc manual - presumably in 
platform.texi, where sys/platform/ppc.h is already documented.

-- 
Joseph S. Myers
joseph@codesourcery.com