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

[Alistair Francis]

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