[v2,05/18] RISC-V: Add path of library directories for the 32-bit

  From: Zong Li <zongbox@gmail.com>

For the recommand of 64 bit version, we add the libraries path of 32 bit
in this patch.

The status of RV32 binaries under RV64 kernels is that the ISA
optionally supports having different XLEN for user and supervisor modes,
but AFAIK there's no silicon that implements this feature, and the Linux
kernel doesn't support it yet.

For the recommand of 64 bit version, we add the libraries path of 32 bit
in this patch. This includes a fix to avoid an out of bound array check
when building with GCC 8.2.
---
 sysdeps/unix/sysv/linux/riscv/dl-cache.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
  

On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:

> From: Zong Li <zongbox@gmail.com>
> 
> For the recommand of 64 bit version, we add the libraries path of 32 bit
> in this patch.

 I have troubles comprehending this sentence; also it's repeated literally 
below.  Please rewrite an remove the duplication.

> The status of RV32 binaries under RV64 kernels is that the ISA
> optionally supports having different XLEN for user and supervisor modes,
> but AFAIK there's no silicon that implements this feature, and the Linux
> kernel doesn't support it yet.

 I'm not sure if this note is relevant here.

> For the recommand of 64 bit version, we add the libraries path of 32 bit
> in this patch. This includes a fix to avoid an out of bound array check
> when building with GCC 8.2.

 Where exactly is that fix?  Shouldn't it be applied as a separate 
preparatory change?

> diff --git a/sysdeps/unix/sysv/linux/riscv/dl-cache.h b/sysdeps/unix/sysv/linux/riscv/dl-cache.h
> index c297dfe84f..60fc172edb 100644
> --- a/sysdeps/unix/sysv/linux/riscv/dl-cache.h
> +++ b/sysdeps/unix/sysv/linux/riscv/dl-cache.h
[...]
> @@ -49,9 +51,16 @@
>    do							    		\
>      {									\
>        size_t len = strlen (dir);					\
> -      char path[len + 9];						\
> +      char path[len + 10];						\
>        memcpy (path, dir, len + 1);					\
> -      if (len >= 12 && ! memcmp(path + len - 12, "/lib64/lp64d", 12))	\
> +      if (len >= 13 && ! memcmp(path + len - 13, "/lib32/ilp32d", 13))	\
> +        {								\
> +          len -= 9;							\
> +	  path[len] = '\0';						\
> +        }								\
> +      if (len >= 12							\
> +          && (! memcmp(path + len - 12, "/lib32/ilp32", 12)		\
> +              || ! memcmp(path + len - 12, "/lib64/lp64d", 12)))	\

 Please correct indentation above and use tabs throughout.

 I think this code should use `else' clauses.  While the truncation of the 
string will cause subsequent `memcmp' calls to fail, they'll still be 
executed, `len' permitting, making this ugly.

 Though frankly with the growing number of entries I would rewrite this 
sequence of conditionals entirely, as a loop over a (static) array of the 
strings processed, cutting the insane number of error-prone hardcoded 
string lengths while at it too.  It is only ever used in `ldconfig', and 
then invoked there at most twice, so it is not that it is critical enough 
for performance to justify illegibility, and making additional `strlen' 
calls shouldn't be a big deal.

> @@ -64,6 +73,10 @@
>        add_dir (path);							\
>        if (len >= 4 && ! memcmp(path + len - 4, "/lib", 4))		\
>  	{								\
> +	  memcpy (path + len, "32/ilp32d", 10);				\
> +	  add_dir (path);						\
> +	  memcpy (path + len, "32/ilp32", 9);				\
> +	  add_dir (path);						\

 Then the same array of strings could be used here (skipping over the 
"/lib" prefix throughout).

  Maciej
  

On Wed, Jul 8, 2020 at 11:43 AM Maciej W. Rozycki via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:
>
> > From: Zong Li <zongbox@gmail.com>
> >
> > For the recommand of 64 bit version, we add the libraries path of 32 bit
> > in this patch.
>
>  I have troubles comprehending this sentence; also it's repeated literally
> below.  Please rewrite an remove the duplication.
>
> > The status of RV32 binaries under RV64 kernels is that the ISA
> > optionally supports having different XLEN for user and supervisor modes,
> > but AFAIK there's no silicon that implements this feature, and the Linux
> > kernel doesn't support it yet.
>
>  I'm not sure if this note is relevant here.
>
> > For the recommand of 64 bit version, we add the libraries path of 32 bit
> > in this patch. This includes a fix to avoid an out of bound array check
> > when building with GCC 8.2.
>
>  Where exactly is that fix?  Shouldn't it be applied as a separate
> preparatory change?

I have re-written the commit message to be clearer.

>
> > diff --git a/sysdeps/unix/sysv/linux/riscv/dl-cache.h b/sysdeps/unix/sysv/linux/riscv/dl-cache.h
> > index c297dfe84f..60fc172edb 100644
> > --- a/sysdeps/unix/sysv/linux/riscv/dl-cache.h
> > +++ b/sysdeps/unix/sysv/linux/riscv/dl-cache.h
> [...]
> > @@ -49,9 +51,16 @@
> >    do                                                                 \
> >      {                                                                        \
> >        size_t len = strlen (dir);                                     \
> > -      char path[len + 9];                                            \
> > +      char path[len + 10];                                           \
> >        memcpy (path, dir, len + 1);                                   \
> > -      if (len >= 12 && ! memcmp(path + len - 12, "/lib64/lp64d", 12))        \
> > +      if (len >= 13 && ! memcmp(path + len - 13, "/lib32/ilp32d", 13))       \
> > +        {                                                            \
> > +          len -= 9;                                                  \
> > +       path[len] = '\0';                                             \
> > +        }                                                            \
> > +      if (len >= 12                                                  \
> > +          && (! memcmp(path + len - 12, "/lib32/ilp32", 12)          \
> > +              || ! memcmp(path + len - 12, "/lib64/lp64d", 12)))     \
>
>  Please correct indentation above and use tabs throughout.

Fixed.

>
>  I think this code should use `else' clauses.  While the truncation of the
> string will cause subsequent `memcmp' calls to fail, they'll still be
> executed, `len' permitting, making this ugly.
>
>  Though frankly with the growing number of entries I would rewrite this
> sequence of conditionals entirely, as a loop over a (static) array of the
> strings processed, cutting the insane number of error-prone hardcoded
> string lengths while at it too.  It is only ever used in `ldconfig', and
> then invoked there at most twice, so it is not that it is critical enough
> for performance to justify illegibility, and making additional `strlen'
> calls shouldn't be a big deal.

I have converted this to a loop

Alistair

>
> > @@ -64,6 +73,10 @@
> >        add_dir (path);                                                        \
> >        if (len >= 4 && ! memcmp(path + len - 4, "/lib", 4))           \
> >       {                                                               \
> > +       memcpy (path + len, "32/ilp32d", 10);                         \
> > +       add_dir (path);                                               \
> > +       memcpy (path + len, "32/ilp32", 9);                           \
> > +       add_dir (path);                                               \
>
>  Then the same array of strings could be used here (skipping over the
> "/lib" prefix throughout).
>
>   Maciej