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

[Palmer Dabbelt <palmer at dabbelt dot com>]

On Wed, 14 Jun 2017 13:29:17 PDT (-0700), joseph@codesourcery.com wrote:
> On Wed, 14 Jun 2017, Palmer Dabbelt wrote:
>
>> diff --git a/sysdeps/riscv/Implies b/sysdeps/riscv/Implies
>> new file mode 100644
>> index 0000000000..15b9e02448
>> --- /dev/null
>> +++ b/sysdeps/riscv/Implies
>> @@ -0,0 +1,6 @@
>> +init_array
>> +
>> +ieee754/ldbl-128
>> +ieee754/dbl-64
>> +ieee754/flt-32
>> +riscv/soft-fp
>
> You should never need to have <arch>/Implies referencing <arch>/<subdir>;
> in such a case, remove the unnecessary soft-fp directory level and put
> sfp-machine.h directly in <arch>.  See
> <https://sourceware.org/ml/libc-alpha/2014-10/msg00369.html>.

Makes sense, I've moved everything from the subdirectory up a level

  https://github.com/riscv/riscv-glibc/commit/7d9fb3b962e2749b54ddd906b4df91ad854ca45e

>> diff --git a/sysdeps/riscv/Makefile b/sysdeps/riscv/Makefile
>> new file mode 100644
>> index 0000000000..5a699a2e64
>> --- /dev/null
>> +++ b/sysdeps/riscv/Makefile
>> @@ -0,0 +1,45 @@
>> +ifneq ($(all-rtld-routines),)
>> +CFLAGS-rtld.c += -mno-plt
>> +CFLAGS-dl-load.c += -mno-plt
>> +CFLAGS-dl-cache.c += -mno-plt
>> +CFLAGS-dl-lookup.c += -mno-plt
>> +CFLAGS-dl-object.c += -mno-plt
>> +CFLAGS-dl-reloc.c += -mno-plt
>> +CFLAGS-dl-deps.c += -mno-plt
>> +CFLAGS-dl-runtime.c += -mno-plt
>> +CFLAGS-dl-error.c += -mno-plt
>> +CFLAGS-dl-init.c += -mno-plt
>> +CFLAGS-dl-fini.c += -mno-plt
>> +CFLAGS-dl-debug.c += -mno-plt
>> +CFLAGS-dl-misc.c += -mno-plt
>> +CFLAGS-dl-version.c += -mno-plt
>> +CFLAGS-dl-profile.c += -mno-plt
>> +CFLAGS-dl-conflict.c += -mno-plt
>> +CFLAGS-dl-tls.c += -mno-plt
>> +CFLAGS-dl-origin.c += -mno-plt
>> +CFLAGS-dl-scope.c += -mno-plt
>> +CFLAGS-dl-execstack.c += -mno-plt
>> +CFLAGS-dl-caller.c += -mno-plt
>> +CFLAGS-dl-open.c += -mno-plt
>> +CFLAGS-dl-close.c += -mno-plt
>> +CFLAGS-dl-sysdep.c += -mno-plt
>> +CFLAGS-dl-environ.c += -mno-plt
>> +CFLAGS-dl-minimal.c += -mno-plt
>> +CFLAGS-dl-static.c += -mno-plt
>> +CFLAGS-dl-brk.c += -mno-plt
>> +CFLAGS-dl-sbrk.c += -mno-plt
>> +CFLAGS-dl-getcwd.c += -mno-plt
>> +CFLAGS-dl-openat64.c += -mno-plt
>> +CFLAGS-dl-opendir.c += -mno-plt
>> +CFLAGS-dl-fxstatat64.c += -mno-plt
>> +endif
>> +
>> +CFLAGS-closedir.c += -mno-plt
>> +CFLAGS-exit.c += -mno-plt
>> +CFLAGS-cxa_atexit.c += -mno-plt
>
> I think this at least needs some explanatory comment.  It feels very
> fragile to require lots of architecture-independent files to be built with
> an architecture-specific option - it's the sort of thing where people
> making architecture-independent changes could easily break the build
> accidentally without realising that their change requires the option to be
> added for a new file.  So if you can find a way to avoid needing such an
> architecture-specific list of files, that would be a good idea.

Andrew and I don't actually remember what that was for, we're hoping it's just
cruft leftover from working around a bug in binutils.  He's going to take a
look at it.

>> diff --git a/sysdeps/riscv/Versions b/sysdeps/riscv/Versions
>> new file mode 100644
>> index 0000000000..5a0c2d23f8
>> --- /dev/null
>> +++ b/sysdeps/riscv/Versions
>> @@ -0,0 +1,5 @@
>> +libc {
>> +  GLIBC_2.14 {
>> +    __memcpy_g;
>> +  }
>> +}
>
> No, the minimum version should be 2.26, and you shouldn't need any such
> special string variants (given that we've killed bits/string2.h, are about
> to kill bits/string.h, and such exports as there are from old string
> function inlines are or should soon be compat symbols).

OK, makes sense.

  https://github.com/riscv/riscv-glibc/commit/bbab2031f32970763a0327925fabde969fcd9a77

>> diff --git a/sysdeps/riscv/configure b/sysdeps/riscv/configure
>> new file mode 100644
>> index 0000000000..9f790ec527
>> --- /dev/null
>> +++ b/sysdeps/riscv/configure
>> @@ -0,0 +1,86 @@
>> +
>> +# as_fn_set_status STATUS
>> +# -----------------------
>> +# Set $? to STATUS, without forking.
>> +as_fn_set_status ()
>> +{
>> +  return $1
>> +} # as_fn_set_status
>
> Please make sure this file is regenerated.  Any sysdeps configure script
> generated with current aclocal.m4 should not contain such functions,
> assuming the GLIBC_PROVIDES in aclocal.m4 is correct.

I think we missed the -I when regenerating this, it's been fixed

  https://github.com/riscv/riscv-glibc/commit/bcb054441d6fe79c231a8420c67da907fa1ce10f

>> diff --git a/sysdeps/riscv/configure.in b/sysdeps/riscv/configure.in
>> new file mode 100644
>> index 0000000000..34f62d4b4b
>> --- /dev/null
>> +++ b/sysdeps/riscv/configure.in
>
> We use configure.ac naming now, not configure.in.

OK, fixed

  https://github.com/riscv/riscv-glibc/commit/357e0312945c6746e52e5c58ade3182f29b4eaaf
  https://github.com/riscv/riscv-glibc/commit/8b1b9a4f2f1db655b0d107d72b01e60210dd6329

> Since you have multiple ABI variants, you should be setting default-abi
> here via LIBC_CONFIG_VAR.  Then sysdeps/unix/sysv/linux/riscv/Makefile
> should set abi-variants and appropriate options for each ABI variant (see
> other such Makefiles for examples) - this is used in building
> bits/syscall.h.

OK, that makes sense.  I've patterned this after aarch64 and smoke tested it.

  https://github.com/riscv/riscv-glibc/commit/e3944da6dfd861f4e40bb34c107d479568ba523d

>> diff --git a/sysdeps/unix/sysv/linux/riscv/configure.in b/sysdeps/unix/sysv/linux/riscv/configure.in
>> new file mode 100644
>> index 0000000000..5b48e1aed6
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/riscv/configure.in
>> @@ -0,0 +1,8 @@
>> +sinclude(./aclocal.m4)dnl Autoconf lossage
>> +GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
>> +# Local configure fragment for sysdeps/unix/sysv/linux/riscv.
>> +
>> +if test -z "$arch_minimum_kernel"; then
>> +  arch_minimum_kernel=3.0.0
>> +  libc_cv_gcc_unwind_find_fde=no
>> +fi
>
> Should use .ac naming.  No need for the 'if test -z
> "$arch_minimum_kernel"; then'.  No need to set
> libc_cv_gcc_unwind_find_fde=no.  Should set arch_minimum_kernel=10.0.0
> until the kernel port is actually in Linus's tree (of course you then need
> to change that locally for testing purposes).

OK, makes sense

  https://github.com/riscv/riscv-glibc/commit/f70efbda1a01c87fb4e6e0d57138a49aa0100e88

>> diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/Makefile b/sysdeps/unix/sysv/linux/riscv/rv64/Makefile
>> new file mode 100644
>> index 0000000000..0a37c5b9b4
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/riscv/rv64/Makefile
>> @@ -0,0 +1,9 @@
>> +ifeq ($(subdir),socket)
>> +CFLAGS-recv.c += -fexceptions
>> +CFLAGS-send.c += -fexceptions
>> +endif
>> +
>> +ifeq ($(subdir),nptl)
>> +CFLAGS-recv.c += -fexceptions
>> +CFLAGS-send.c += -fexceptions
>> +endif
>
> In the nptl directory this is the default and should not be needed.

https://github.com/riscv/riscv-glibc/commit/142092fc6b6a738e067e4ce22cf420aa502b06af

>> diff --git a/sysdeps/unix/sysv/linux/riscv/shlib-versions b/sysdeps/unix/sysv/linux/riscv/shlib-versions
>> new file mode 100644
>> index 0000000000..66214b953e
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/riscv/shlib-versions
>> @@ -0,0 +1,17 @@
>> +DEFAULT		GLIBC_2.25
>
> Should be GLIBC_2.26.

https://github.com/riscv/riscv-glibc/commit/d94d34ef8c20b4c7554bbe25e758642492ee1c85