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: Palmer Dabbelt <palmer at dabbelt dot com>
- To: joseph at codesourcery dot com
- Cc: libc-alpha at sourceware dot org, patches at groups dot riscv dot org
- Date: Mon, 22 Jan 2018 16:40:28 -0800 (PST)
- Subject: Re: [PATCH v4 13/17] RISC-V: Linux ABI
- Authentication-results: sourceware.org; auth=none
On Mon, 15 Jan 2018 09:37:46 PST (-0800), joseph@codesourcery.com wrote:
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.
Sorry about that -- this was added late to Linux so I put the FIXME version in
there and forgot to remove it.
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.
Thanks, I fixed it. I copied this from elsewhere in glibc and I guess I forgot
the rules.
+ /* 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).
Thanks for catching that.
+ /* 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?
Yes, sorry about that -- I had the test reversed. After talking to some other
people, we actually don't want to even check this here: it's legal to link C
and non-C objects together, so we should support loading them together as well.
+ /* 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.)
Yep, sorry, I had that backwards -- I guess I was being an idiot when writing
this code, as multiple people have pointed out how poorly it went. RVE and
RVTSO aren't supported in glibc yet. I've added a comment explaining this
better
/* The ELF flags supported by our current glibc port:
- EF_RISCV_FLOAT_ABI: We support the soft and double ABIs.
- EF_RISCV_RVC: While the Linux ABI mandates the presence of the C
extension, we can still support libraries compiled without that extension
so we just ignore this flag.
- EF_RISCV_RVE: glibc (and Linux) don't support RV32E based systems.
- EF_RISCV_TSO: The TSO extension isn't supported, as doing so would require
some mechanism to ensure that the TSO extension is enabled which doesn't
currently exist. */
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.
Thanks.
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.
I've called it "__glibc_reserved", which from my reading is what I'm supposed
to do?