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

[Joseph Myers <joseph at codesourcery dot com>]

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>.

> 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.

> 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).

> 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.

> 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.

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.

> 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).

> 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.

> 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.

-- 
Joseph S. Myers
joseph@codesourcery.com