This is the mail archive of the 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 v2 01/15] RISC-V: Build Infastructure

Sorry it took a while to get back to you, I've been working through the test suite failures.

On Wed, 20 Dec 2017 08:21:55 PST (-0800), wrote:
On Tue, 19 Dec 2017, Palmer Dabbelt wrote:

diff --git a/sysdeps/riscv/Makefile b/sysdeps/riscv/Makefile
new file mode 100644
index 000000000000..64060531a1d5
--- /dev/null
+++ b/sysdeps/riscv/Makefile
@@ -0,0 +1,49 @@
+ifneq ($(all-rtld-routines),)
+CFLAGS-rtld.c += -mno-plt

My previous comments from
<> still apply:
this is too fragile (people won't think when making an
architecture-independent change about whether it needs a RISC-V-specific
change here) and completely lacking the required detailed explanatory
comment of what the issue is and the logic for determining what files need
this option (but really the logic should be implemented to avoid an
architecture-specific list of architecture-independent files being needed
at all).

Ah, sorry, I missed that from last time. I thought we were doing this because we couldn't call via the PLT in because they hadn't been resolved yet, but as far as I can tell there's something else handling this. I just ran into it when looking at the check-localplt stuff (I'm currently going through the test suites).

Andrew would know for sure what's going on here -- this has been in our port since ~2011 (when he added PLT support) and has never had an explanation. I feel like it's defunct: when I remove it we end up with only the PLT entries listed in check-localplt, I'd expect a lot more if everything was really emitting PLT calls. I can't figure out what's actually preventing from using PLT calls on the _dl_fixup codepath.

I assume I'm missing something here?

diff --git a/sysdeps/riscv/Versions b/sysdeps/riscv/Versions
new file mode 100644
index 000000000000..aafa348a2395
--- /dev/null
+++ b/sysdeps/riscv/Versions
@@ -0,0 +1,4 @@
+libc {
+  GLIBC_2.27 {
+  }

You shouldn't need this file at all when there aren't any versions to
include in it.

IIRC some test was depending on this. I've filled all the machines with builds right now, but I'll give it another look later tonight. I've got two small patches that touch shared build system stuff already.

diff --git a/sysdeps/riscv/preconfigure b/sysdeps/riscv/preconfigure

Does your soft-float lack support for exceptions and rounding modes?  If
so, I'd expect a setting of with_fp_cond, and a corresponding nofpu
directory or directories with Implies files pointing to ieee754/soft-fp
(arranged so that always comes before the other ieee754/ directories in
the sysdeps directory ordering).  That's needed to get the soft-fp
versions of fmaf/fma/fmal used instead of the default ones relying on
exceptions and rounding modes, so its absence should show up as test
failures for soft-float configurations.  In future it will also similarly
be needed to get soft-fp versions of TS 18661-1 narrowing functions used.

If I understand what's going on correctly here, we support exceptions in the soft float ABI when built with hardware floating point instructions, but otherwise we don't (we're always round nearest). I believe that means we need to set "with_fp_cond=1" when building with hardware floating point (regardless of ABI), and "with_fp_cond=0" otherwise.

Does this look sane?

diff --git a/sysdeps/riscv/preconfigure b/sysdeps/riscv/preconfigure
index e118a5ed28f6..6f2bc20ecd5b 100644
--- a/sysdeps/riscv/preconfigure
+++ b/sysdeps/riscv/preconfigure
@@ -16,12 +16,15 @@ riscv*)
    case "$flen" in
+       with_fp_cond=1
+       with_fp_cond=1
+       with_fp_cond=0
       echo "Unable to determine FLEN" >&2

Remark: it makes sense for ieee754/dbl-64/wordsize-64 to be used (before
ieee754/dbl-64 in the sysdeps directory ordering) in configurations with
64-bit registers (but that's just an optimization so you may wish to defer

+abi-ilp32-options     := -D__SIZEOF_INT__=4

abi-* options variables are no longer used with the current bits/syscall.h
generation mechanism, so you should remove all settings of such variables
from the patch.

OK, we'll remove them for the v3.

diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/Makefile b/sysdeps/unix/sysv/linux/riscv/rv64/Makefile
new file mode 100644
index 000000000000..cb60d740476d
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/riscv/rv64/Makefile
@@ -0,0 +1,4 @@
+ifeq ($(subdir),socket)
+CFLAGS-recv.c += -fexceptions
+CFLAGS-send.c += -fexceptions

I wonder if this should be made generic for all Linux ports (and removed
from the mips64 Makefile as well as here).

I think we got it from MIPS, but it doesn't seem like it should be ISA dependent. I'm afraid this is another thing that's been there since before I was around, but I feel like since it's not in anyone else's port it's safe to drop it from ours.

Thanks for reviewing our code, and sorry it's a bit of a mess. I'll try and get something a bit more sane out for a v3, but I'm afraid it might take a few more days -- luckily with the weekend and Christmas I should have some time to actually work on this :).

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