Re: [PATCH v3 16/19] RISC-V: Linux ABI

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 26 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>

Same comment as before about how this doesn't actually seem to implement 
____longjmp_chk semantics for stack checks which include using 
sigaltstack.

> diff --git a/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h b/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h

> +struct sigcontext {
> +  /* gregs[0] holds the program counter. */
> +  unsigned long gregs[32];
> +  unsigned long long fpregs[66] __attribute__ ((aligned(16)));

Should use __aligned__ (even though this struct isn't part of a standard, 
there's no reason to take the identifier aligned from the user's 
namespace).  And missing space before "(16)".

> +int
> +__riscv_flush_icache (void *start, void *end, unsigned long flags)
> +{
> +  static volatile func_type cached_func;
> +
> +  func_type func = cached_func;
> +
> +  if (!func)
> +    cached_func = func = __lookup_riscv_flush_icache ();

I think we now prefer to explicitly use relaxed atomic loads and stores 
for such static variables that could be accessed from multiple threads at 
once (that shouldn't change the code generated).

The question of whether there should be a leading __ on the function name 
has been discussed separately.

> diff --git a/sysdeps/unix/sysv/linux/riscv/init-first.c b/sysdeps/unix/sysv/linux/riscv/init-first.c

> +long (*VDSO_SYMBOL(getcpu)) (unsigned *, unsigned *, void *) attribute_hidden;
> +long (*VDSO_SYMBOL(gettimeofday)) (struct timeval *, void *) attribute_hidden;
> +long (*VDSO_SYMBOL(clock_gettime)) (clockid_t, struct timespec *)
> +    attribute_hidden;
> +long (*VDSO_SYMBOL(clock_getres)) (clockid_t, struct timespec *)
> +    attribute_hidden;

long int, unsigned int.

> diff --git a/sysdeps/unix/sysv/linux/riscv/libc-vdso.h b/sysdeps/unix/sysv/linux/riscv/libc-vdso.h

> +extern long (*VDSO_SYMBOL(getcpu)) (unsigned *, unsigned *, void *)
> +    attribute_hidden;
> +extern long (*VDSO_SYMBOL(gettimeofday)) (struct timeval *, void *)
> +    attribute_hidden;
> +extern long (*VDSO_SYMBOL(clock_gettime)) (clockid_t, struct timespec *)
> +    attribute_hidden;
> +extern long (*VDSO_SYMBOL(clock_getres)) (clockid_t, struct timespec *)
> +    attribute_hidden;

Likewise.

> +void
> +__makecontext (ucontext_t *ucp, void (*func) (void), int argc,
> +	       long a0, long a1, long a2, long a3, long a4, ...)
> +{
> +  extern void __start_context (void) attribute_hidden;
> +  long i, sp;

long int, and likewise elsewhere in this function.

> +  /* Set up the stack. */
> +  sp = ((long)ucp->uc_stack.ss_sp + ucp->uc_stack.ss_size) & ALMASK;

Should have spaces in casts, (long int) ucp->uc_stack.ss_sp, and likewise 
elsewhere in this function.

> +  if (__builtin_expect (argc > 5, 0))

__glibc_unlikely, as elsewhere.

> diff --git a/sysdeps/unix/sysv/linux/riscv/register-dump.h b/sysdeps/unix/sysv/linux/riscv/register-dump.h

> +      hexvalue (ctx->uc_mcontext.gregs[i], regvalue, __WORDSIZE/4);

Missing spaces, __WORDSIZE / 4.

> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/cachectl.h b/sysdeps/unix/sysv/linux/riscv/sys/cachectl.h

> +extern int __riscv_flush_icache (void *start, void *end, unsigned long flags);

As discussed elsewhere, need __ on parameter names.

> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h

> +/* Don't rely on this, the interface is currently messed up and may need to
> +   be broken to be fixed.  */

You need to make sure you get the interface right, at least at the ABI 
level, before the port goes in.

> +#ifdef __USE_MISC
> +# define __ctx(fld) fld
> +#else
> +# define __ctx(fld) __ ## fld
> +#endif

The use of __ctx in other sys/ucontext.h headers was a matter of keeping 
maximum API compatibility with existing users, that might expect fields to 
be present with given names without leading __ in the default case (when 
__USE_MISC is defined), while having POSIX compatibility in the strict 
POSIX case when the field names without __ are not allowed.

Since you don't have existing users, that issue does not apply here.  I 
think it would be best for all the fields not permitted by POSIX to have 
leading __ unconditionally, so that you expose the same API for those 
fields both with and without __USE_MISC, and don't need __ctx.

> +typedef unsigned long greg_t;

unsigned long int.  Likewise elsewhere in this file.

> +struct __riscv_d_ext_state
> +  {
> +    unsigned long long __ctx(f[32]);

unsigned long long int (or uint64_t).  Likewise elsewhere in this file.

> +    /*
> +     * Reserved for expansion of sigcontext structure.  Currently zeroed
> +     * upon signal, and must be zero upon sigreturn.
> +     */

Stray leading '*' on comment lines.

> +    unsigned int __ctx(reserved)[3];

Reserved space should definitely use the __glibc_reserved* naming 
convention, unconditionally, no __ctx.

> +    /* There's some padding here to allow sigset_t to be expanded in the
> +     * future.  Though this is unlikely, other architectures put uc_sigmask
> +     * at the end of this structure and explicitly state it can be
> +     * expanded, so we didn't want to box ourselves in here. */
> +    char               __unused[1024 / 8 - sizeof (sigset_t)];
> +    /* We can't put uc_sigmask at the end of this structure because we need
> +     * to be able to expand sigcontext in the future.  For example, the
> +     * vector ISA extension will almost certainly add ISA state.  We want
> +     * to ensure all user-visible ISA state can be saved and restored via a
> +     * ucontext, so we're putting this at the end in order to allow for
> +     * infinite extensibility.  Since we know this will be extended and we
> +     * assume sigset_t won't be extended an extreme amount, we're
> +     * prioritizing this. */

Comment style, and use of __glibc_reserved, again.

-- 
Joseph S. Myers
joseph@codesourcery.com