Re: Re: [musl] add loongarch64 port v7.

[Rich Felker <dalias@libc.org>]

On Sat, Aug 05, 2023 at 11:43:08AM -0400, Rich Felker wrote:
> On Sat, Aug 05, 2023 at 02:18:35PM +0800, 翟小娟 wrote:
> > We are currently working on bringing loongarch support to the alpine
> > community. Alpine depends on musl, and Alpine and its users are
> > waiting for musl to support loongarch. This patch seems to be a long
> > time ago, we hope to know the review status of this patch.
> 
> Last time I set out to look at it, I hit that the exact patch I had
> previously reviewed and tentatively okayed wasn't stored anywhere
> permanent. I realize this has taken quite a while, and now there is a
> patch on the list, so this coming week I will go back and review the
> diff of that against the previous posted version and mailing list
> comments. As long as everything matches up, I expect to commit it.
> 
> Thanks for checking back in on this, and sorry it's taken so long.

OK, I'm looking at this now as the diffs between v5 and v6 (which I
theoretically reviewed before, but the exact v6 I looked at did not
exist in permanent form) and between v6 and v7.

v5->v6:

> -+#define __BYTE_ORDER 1234
> ++#define __BYTE_ORDER  __LITTLE_ENDIAN

This is gratuitous, mismatches what is done on other archs, and is
less safe.

> -+TYPEDEF unsigned nlink_t;
> ++TYPEDEF unsigned int nlink_t;

Gratuitous and in opposite direction of coding style used elsewhere in
musl. There are a few other instances of this too.

> -+      register uintptr_t tp __asm__("tp");
> -+      __asm__ ("" : "=r" (tp) );
> ++      uintptr_t tp;
> ++      __asm__ __volatile__("move %0, $tp" : "=r"(tp));
>  +      return tp;

Not clear what the motivation is here. Is it working around a compiler
bug? The original form was more optimal.

>  +#define CRTJMP(pc,sp) __asm__ __volatile__( \
> -+      "move $sp,%1 ; jr %0" : : "r"(pc), "r"(sp) : "memory" )
> -+
> -+#define GETFUNCSYM(fp, sym, got) __asm__ ( \
> -+      ".hidden " #sym "\n" \
> -+      ".align 8 \n" \
> -+      "       la.local $t1, "#sym" \n" \
> -+      "       move %0, $t1 \n" \
> -+      : "=r"(*(fp)) : : "memory" )
> ++      "move $sp, %1 ; jr %0" : : "r"(pc), "r"(sp) : "memory" )

Not clear why this was changed. It was never discussed afaik. It looks
somewhat dubious removing GETFUNCSYM and using the maybe-unreliable
default definition.

It also looks like v5->v6 rewrote sigsetjmp.

v6->v7:

sigcontext/mcontext_t change mostly makes sense, but the
namespace-safe and full mcontext_t differ in their use of the aligned
attribute and it's not clear that the attribute is needed (since the
offset is naturally aligned and any instance of the object is produced
by the kernel, not by userspace).

> -+      long               __uc_pad;

This change to ucontext_t actually makes the struct ABI-incompatible
in the namespace-safe version without the alignment attribute. IIRC I
explicitly requested the explicit padding field to avoid this kind of
footgun. Removing it does not seem like a good idea.

In stat.h:

> -+      unsigned long __pad;
> ++      unsigned long __pad1;

This is gratuitous and makes the definition gratuitously mismatch what
will become the "generic" version of bits/stat.h. There is no contract
for applications to be able to access these padding fields by name, so
no motivation to make their names match glibc's names or the kernel's.

In fenv.S:

> ++#ifdef __clang__
> ++#define FCSR $fcsr0
> ++#else
> ++#define FCSR $r0
> ++#endif

It's not clear to me what's going on here. Is there a clang
incompatibility in the assembler language you're working around? Or
what? If so that seems like a tooling bug that should be fixed.

Otherwise, everything looks as expected, I think. I'm okay with making
any fixups for merging rather than throwing this back on your team for
more revisions, but can you please follow up and clarify the above?

Rich