Re: [PATCH v4 13/17] RISC-V: Linux ABI

[Joseph Myers <joseph at codesourcery dot com>]

On Sat, 13 Jan 2018, Palmer Dabbelt wrote:

> +static int __riscv_flush_icache_syscall (void *start, void *end,
> +					 unsigned long int flags)
> +{
> +#ifdef __NR_riscv_flush_icache
> +	return INLINE_SYSCALL (riscv_flush_icache, 3, start, end, flags);
> +#else
> +	/* FIXME: This should go away, as it's actually not quite correct. */
> +	__asm__ volatile ("fence.i");
> +	return 0;
> +#endif

Shouldn't have this #ifdef.  Just assume that the __NR_riscv_flush_icache 
macro is present (I take it that it's in Linux 4.15) - 
architecture-specific code should only ever need #ifdefs on __NR_* if the 
syscall is newer than the oldest Linux kernel headers version supported by 
glibc for that architecture.

> diff --git a/sysdeps/unix/sysv/linux/riscv/readelflib.c b/sysdeps/unix/sysv/linux/riscv/readelflib.c
> new file mode 100644
> index 000000000000..c8475241e31e
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/riscv/readelflib.c
> @@ -0,0 +1,92 @@
> +/* Copyright (C) 1999-2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Andreas Jaeger <aj@suse.de>, 1999 and
> +		  Jakub Jelinek <jakub@redhat.com>, 1999.

Remove "Contributed by".  Descriptive comment needed before copyright 
notice.

> +  /* RISC-V linkers encode the floating point ABI as part of the ELF headers.  */
> +  switch (flags & EF_RISCV_FLOAT_ABI)
> +    {
> +      case EF_RISCV_FLOAT_ABI_SOFT:
> +        *flag |= FLAG_RISCV_FLOAT_ABI_SOFT;
> +	break;
> +      case EF_RISCV_FLOAT_ABI_DOUBLE:
> +        *flag |= FLAG_RISCV_FLOAT_ABI_DOUBLE;
> +	break;
> +      case EF_RISCV_FLOAT_ABI_QUAD:
> +        return 1;
> +    }

You have an error here for the quad ABI, I'd expect one for the single ABI 
(or just have "default:" for the error case, which amounts to the same 
thing).

> +  /* RISC-V Linux ABIs mandate the presence of the C extension.  */
> +  if (flags & EF_RISCV_RVC)
> +    return 1;

Given that C support is required in the processor, surely you *shouldn't* 
return 1 (error) for it?

> +  /* The remainder of the header bits are reserved, so just be on the safe side
> +     and don't support them at all.  */
> +  if (flags & SUPPORTED_ELF_FLAGS)
> +    return 1;

I read this as returning 1 (failure) if EF_RISCV_FLOAT_ABI_DOUBLE is 
specified, simply because that happens to have a nonzero value.  Did you 
mean to use ~SUPPORTED_ELF_FLAGS in this test?  (And it's not actually 
true all the other bits are reserved, the ABI document defines 
EF_RISCV_RVE and EF_RISCV_TSO.)

> 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 int flags);

As noted, still missing leading __ on parameter names.

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

> +    /* 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)];

Should use __glibc_reserved naming convention.

-- 
Joseph S. Myers
joseph@codesourcery.com