Re: [PATCH v2 01/15] RISC-V: Build Infastructure

[Palmer Dabbelt <palmer at dabbelt dot com>]

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), joseph@codesourcery.com 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
> <https://sourceware.org/ml/libc-alpha/2017-06/msg00645.html> 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 ld.so 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 ld.so 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
    64)
       float_machine=rvd
+       with_fp_cond=1
       ;;
    32)
       float_machine=rvf
+       with_fp_cond=1
       ;;
    "")
       float_machine=
+       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
> it).
>
> > +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
> > +endif
>
> 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 :).