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

[Maciej W. Rozycki]

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