This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4 13/17] RISC-V: Linux ABI
- From: Joseph Myers <joseph at codesourcery dot com>
- To: Palmer Dabbelt <palmer at dabbelt dot com>
- Cc: <libc-alpha at sourceware dot org>, <patches at groups dot riscv dot org>
- Date: Mon, 15 Jan 2018 17:37:46 +0000
- Subject: Re: [PATCH v4 13/17] RISC-V: Linux ABI
- Authentication-results: sourceware.org; auth=none
- References: <20180113103816.4861-1-palmer@dabbelt.com> <20180113103816.4861-14-palmer@dabbelt.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