[PATCH v3 07/19] RISC-V: Add path of library directories for the 32-bit

[Maciej W. Rozycki]

On Sun, 12 Jul 2020, Alistair Francis via Libc-alpha wrote:

> With RV32 support the list of possible RISC-V system directories
> increases to:
>      - /lib64/lp64d
>      - /lib64/lp64
>      - /lib32/ilp32d
>      - /lib32/ilp32
>      - /lib (only ld.so)
> 
> This patch changes the add_system_di () macro to support the new ilp32d

 add_system_dir

> and ilp32 directories for RV32. While refactoring this code let's split
                                 ^
 Missing space.

> out the confusing if statements into a loop to make it easier to
> understand and extend.

 This change doesn't appear to do what's intended; the list of directories 
printed without it is:

/lib: (from <builtin>:0)
/lib64/lp64d: (from <builtin>:0)
/lib64/lp64: (from <builtin>:0)
/usr/lib: (from <builtin>:0)
/usr/lib64/lp64d: (from <builtin>:0)
/usr/lib64/lp64: (from <builtin>:0)

while with the change applied I only get:

/lib64/lp64d: (from <builtin>:0)

> diff --git a/sysdeps/unix/sysv/linux/riscv/dl-cache.h b/sysdeps/unix/sysv/linux/riscv/dl-cache.h
> index b3cda4ef9f..7317406036 100644
> --- a/sysdeps/unix/sysv/linux/riscv/dl-cache.h
> +++ b/sysdeps/unix/sysv/linux/riscv/dl-cache.h
[...]
> @@ -45,25 +47,40 @@
>  	 architectures and have that automatically imply /usr/local/lib64/lp64d
>  	 etc. so that libraries can be found that come from software that does
>  	 use the ABI-specific directories.  */
> +
>  #define add_system_dir(dir) 									\
>    do							    					\
>      {												\
> +	static const char* lib_dirs[] = {							\
> +		"/lib64/lp64d",									\
> +		"/lib64/lp64",									\
> +		"/lib32/ilp32d",								\
> +		"/lib32/ilp32",									\
> +		NULL,										\
> +	};											\
>  	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))				\
> -	{											\
> -	  len -= 8;										\
> -	  path[len] = '\0';									\
> -	}											\
> -	if (len >= 11 && ! memcmp(path + len - 11, "/lib64/lp64", 11))				\
> -	{											\
> -	  len -= 7;										\
> -	  path[len] = '\0';									\
> +	int i = 0;										\
> +	const char* lib_dir = lib_dirs[0];								\
> +												\
> +	while (lib_dir != NULL) {								\
> +		size_t dir_len = strlen (lib_dir);						\
> +		if (len >= dir_len && ! memcmp(path + len - dir_len, lib_dir, dir_len)) {	\
> +			len -= dir_len + 4;							\

 Thinko here, and the reason for the breakage noted above; it should be:

			len -= dir_len - 4;

> +			path[len] = '\0';							\
> +			break;									\
> +		}										\
> +		i++;										\
> +		lib_dir = lib_dirs[i];								\

 Please place the block opening brace on its own on a separate line and 
indent by two spaces per level, replacing every group of 8 with a tab, and 
stay within 79 columns (preferably 74).  Also the use of a space after the 
negation operator is deprecated.

 Please have a look at:
<https://www.gnu.org/prep/standards/standards.html#Formatting>
<https://sourceware.org/glibc/wiki/Style_and_Conventions#Code_Formatting>
for full details.

 This might be slightly cleaner if rewritten as a `for' loop:

      const size_t lib_len = sizeof ("/lib") - 1;			\
      size_t len = strlen (dir);					\
      char path[len + 10];						\
      const char **ptr;							\
									\
      memcpy (path, dir, len + 1);					\
									\
      for (ptr = lib_dirs; *ptr != NULL; ptr++)				\
 	{								\
	  const char *lib_dir = *ptr;					\
	  size_t dir_len = strlen (lib_dir);				\
									\
	  if (len >= dir_len						\
	      && !memcmp (path + len - dir_len, lib_dir, dir_len))	\
	    {								\
	      len -= dir_len - lib_len;					\
	      path[len] = '\0';						\
	      break;							\
	    }								\
	}								\
      add_dir (path);							\

And then for the second part I previously requested:

      if (len >= lib_len						\
	  && !memcmp (path + len - lib_len, "/lib", lib_len))		\
	for (ptr = lib_dirs; *ptr != NULL; ptr++)			\
	  {								\
	    const char *lib_dir = *ptr;					\
	    size_t dir_len = strlen (lib_dir);				\
									\
	    assert (dir_len >= lib_len);				\
	    memcpy (path + len, lib_dir + lib_len,			\
		    dir_len - lib_len + 1);				\
	    add_dir (path);						\
	  }								\

replacing the current handcrafted chain of `memcpy' calls.  (We could 
benefit from the use of the ARRAY_SIZE macro if we had it, but we don't, 
so let's not complicate things further; this is not a performance-critical 
piece of code).

 NB I'd keep any broken formatting as it is in otherwise unchanged lines, 
as this will make the change more readable and backporting easier.  Any 
clean-up can be done with a follow-up change, preferably across the whole 
file (i.e. including the breakage I observed in the review of 06/19 and 
possibly other places).

  Maciej