[v3,07/19] RISC-V: Add path of library directories for the 32-bit
Commit Message
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
and ilp32 directories for RV32. While refactoring this code let's split
out the confusing if statements into a loop to make it easier to
understand and extend.
---
sysdeps/unix/sysv/linux/riscv/dl-cache.h | 39 +++++++++++++++++-------
1 file changed, 28 insertions(+), 11 deletions(-)
Comments
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
@@ -30,10 +30,12 @@
((flags) == _DL_CACHE_DEFAULT_ID)
/* If given a path to one of our library directories, adds every library
- directory via add_dir (), otherwise just adds the giver directory. On
+ directory via add_dir (), otherwise just adds the given directory. On
RISC-V, libraries can be found in paths ending in:
- /lib64/lp64d
- /lib64/lp64
+ - /lib32/ilp32d
+ - /lib32/ilp32
- /lib (only ld.so)
so this will add all of those paths.
@@ -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; \
+ path[len] = '\0'; \
+ break; \
+ } \
+ i++; \
+ lib_dir = lib_dirs[i]; \
} \
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); \
memcpy (path + len, "64/lp64d", 9); \
add_dir (path); \
memcpy (path + len, "64/lp64", 8); \