This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]