LoongArch: Ensure consistency with kernel by using union for struct members in mcontext_t and ucontext_t.
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
During the construction of the LoongArch Alpine system,
we found that there is an inconsistency in the member
names of mcontext_t and ucontext_t between musl and glibc,
which can cause compilation errors. After testing, we decided
to use union to keep these member names consistency.
Reference: 4fa9b3bfe6759c82beb4b043a54a3598ca467289
Co-authored-by: Xi Ruoyao <xry111@xry111.site>
---
.../unix/sysv/linux/loongarch/makecontext.c | 26 ++++++++--------
.../sysv/linux/loongarch/sigcontextinfo.h | 2 +-
.../unix/sysv/linux/loongarch/sys/ucontext.h | 31 ++++++++++++++++---
.../unix/sysv/linux/loongarch/ucontext_i.sym | 6 ++--
4 files changed, 43 insertions(+), 22 deletions(-)
Comments
The 04/03/2023 20:01, caiyinyu wrote:
> During the construction of the LoongArch Alpine system,
> we found that there is an inconsistency in the member
> names of mcontext_t and ucontext_t between musl and glibc,
> which can cause compilation errors. After testing, we decided
> to use union to keep these member names consistency.
there is no musl api compat requirement at the moment (the musl
loongarch port is not committed yet so surely that can be
changed to be consistent).
there is an api difference between glibc and linux uapi headers.
i personally don't think this is an issue. (the c abi must match
but struct sigcontext and mcontext_t are not used interchangably
in code. the c++ mangling abi is different even after your patch)
there is also a difference in ucontext compared to other targets:
they have uc_flags field while loongarch has __uc_flags. i don't
know if this may cause a portability issue.
in any case the commit message does not do a good job describing
the reasoning.
在 2023/4/5 上午1:38, Szabolcs Nagy 写道:
> The 04/03/2023 20:01, caiyinyu wrote:
>> During the construction of the LoongArch Alpine system,
>> we found that there is an inconsistency in the member
>> names of mcontext_t and ucontext_t between musl and glibc,
>> which can cause compilation errors. After testing, we decided
>> to use union to keep these member names consistency.
> there is no musl api compat requirement at the moment (the musl
> loongarch port is not committed yet so surely that can be
> changed to be consistent).
We want to ensure that the structure member names in mcontext_t are
consistent with the kernel (like other architectures)
before submitting LoongArch musl (Currently under development) to the
upstream community.
This will allow for consistency in the structure member names
across musl, glibc, and the kernel for related structures.
> there is an api difference between glibc and linux uapi headers.
> i personally don't think this is an issue. (the c abi must match
> but struct sigcontext and mcontext_t are not used interchangably
> in code. the c++ mangling abi is different even after your patch)
I am currently facing some confusion.
As far as I know, in C++, the member names of a struct or union are not
subject to mangling.
Mangling is only applied to the names of C++ entities such as
functions, variables, classes, and namespaces.
The member names of a struct or union are simply a part of
the struct or union, and their names are not encoded or modified.
Therefore, even if the member names of a struct or union are duplicated
in different translation units,
they will not be subject to mangling since they are different symbols.
could you please provide a more detailed explanation about this issue?
BTW, I have written a test case[1] and the result is as expected:
The symbol name of function cyyprint which takes mcontext_t as args
after C++ mangling are all _Z8cyyprint10mcontext_t.
[1]
compiled with g++ tst1/2.cpp -shared -o tst1/2.so
and nm tst1/2.so | grep cyy
➜ cat tst1.cpp
#include <iostream>
typedef struct mcontext_t {
unsigned long long __pc;
unsigned long long __gregs[32];
unsigned int __flags;
unsigned long long __extcontext[0] __attribute__((__aligned__(16)));
} mcontext_t;
void cyyprint(mcontext_t mx) { std::cout << mx.__pc << std::endl; }
➜ cat tst2.cpp
#include <iostream>
#include <features.h>
#ifdef __USE_MISC
#define __ctx(fld) fld
#else
#define __ctx(fld) __##fld
#endif
typedef struct mcontext_t {
union {
struct {
unsigned long long __ctx(sc_pc);
unsigned long long __ctx(sc_regs)[32];
unsigned int __ctx(sc_flags);
unsigned long long __ctx(sc_extcontext)[0]
__attribute__((__aligned__(16)));
};
struct {
unsigned long long __pc;
unsigned long long __gregs[32];
unsigned int __flags;
unsigned long long __extcontext[0] __attribute__((__aligned__(16)));
};
};
} mcontext_t;
void cyyprint(mcontext_t mx) { std::cout << mx.__pc << std::endl; }
> there is also a difference in ucontext compared to other targets:
> they have uc_flags field while loongarch has __uc_flags. i don't
> know if this may cause a portability issue.
>
> in any case the commit message does not do a good job describing
> the reasoning.
The 04/06/2023 17:59, caiyinyu wrote:
> 在 2023/4/5 上午1:38, Szabolcs Nagy 写道:
> > The 04/03/2023 20:01, caiyinyu wrote:
> > > During the construction of the LoongArch Alpine system,
> > > we found that there is an inconsistency in the member
> > > names of mcontext_t and ucontext_t between musl and glibc,
> > > which can cause compilation errors. After testing, we decided
> > > to use union to keep these member names consistency.
> > there is no musl api compat requirement at the moment (the musl
> > loongarch port is not committed yet so surely that can be
> > changed to be consistent).
>
>
> We want to ensure that the structure member names in mcontext_t are
>
> consistent with the kernel (like other architectures)
>
> before submitting LoongArch musl (Currently under development) to the
> upstream community.
>
> This will allow for consistency in the structure member names
>
> across musl, glibc, and the kernel for related structures.
>
>
> > there is an api difference between glibc and linux uapi headers.
> > i personally don't think this is an issue. (the c abi must match
> > but struct sigcontext and mcontext_t are not used interchangably
> > in code. the c++ mangling abi is different even after your patch)
>
>
> I am currently facing some confusion.
>
> As far as I know, in C++, the member names of a struct or union are not
> subject to mangling.
sorry i did not mean to say you are breaking c++ abi compared to
the current glibc code, but that the linux sigcontext struct has
different c++ abi even if the field names are the same (because
sigcontext vs mcontext_t struct tag).
this tells me that you cannot use those two types interchangably
in c++ library apis anyway and i would expect that the two types
don't get confused in source code, so the field names don't need
to be consistent (of course it's nicer if they are the same).
one minor issue is that in principle this is a posix api and the
header is supposed to work with any c99 compiler, but unnamed
union and struct members are not c99. in practice i don't think
this is an issue: all loongarch c compilers will support it
and glibc already relies on extensions (like attribute aligned).
i'm not against the patch, but to me the commit message implied
the change was because of musl, while it is for being more
consistent with linux uapi and other targets.
@@ -41,19 +41,19 @@ __makecontext (ucontext_t *ucp, void (*func) (void), int argc, long int a0,
ra = s0 = 0, terminating the stack for backtracing purposes.
s1 = the function we must call.
s2 = the subsequent context to run. */
- ucp->uc_mcontext.__gregs[LARCH_REG_RA] = (uintptr_t) 0;
- ucp->uc_mcontext.__gregs[LARCH_REG_S0] = (uintptr_t) 0;
- ucp->uc_mcontext.__gregs[LARCH_REG_S1] = (uintptr_t) func;
- ucp->uc_mcontext.__gregs[LARCH_REG_S2] = (uintptr_t) ucp->uc_link;
- ucp->uc_mcontext.__gregs[LARCH_REG_SP] = (uintptr_t) sp;
- ucp->uc_mcontext.__pc = (uintptr_t) &__start_context;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_RA] = (uintptr_t) 0;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_S0] = (uintptr_t) 0;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_S1] = (uintptr_t) func;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_S2] = (uintptr_t) ucp->uc_link;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_SP] = (uintptr_t) sp;
+ ucp->uc_mcontext.sc_pc = (uintptr_t) &__start_context;
/* Put args in a0-a7, then put any remaining args on the stack. */
- ucp->uc_mcontext.__gregs[LARCH_REG_A0 + 0] = (uintptr_t) a0;
- ucp->uc_mcontext.__gregs[LARCH_REG_A0 + 1] = (uintptr_t) a1;
- ucp->uc_mcontext.__gregs[LARCH_REG_A0 + 2] = (uintptr_t) a2;
- ucp->uc_mcontext.__gregs[LARCH_REG_A0 + 3] = (uintptr_t) a3;
- ucp->uc_mcontext.__gregs[LARCH_REG_A0 + 4] = (uintptr_t) a4;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_A0 + 0] = (uintptr_t) a0;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_A0 + 1] = (uintptr_t) a1;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_A0 + 2] = (uintptr_t) a2;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_A0 + 3] = (uintptr_t) a3;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_A0 + 4] = (uintptr_t) a4;
if (__glibc_unlikely (argc > 5))
{
@@ -62,14 +62,14 @@ __makecontext (ucontext_t *ucp, void (*func) (void), int argc, long int a0,
long int reg_args = argc < LARCH_REG_NARGS ? argc : LARCH_REG_NARGS;
for (long int i = 5; i < reg_args; i++)
- ucp->uc_mcontext.__gregs[LARCH_REG_A0 + i] = va_arg (vl, unsigned long int);
+ ucp->uc_mcontext.sc_regs[LARCH_REG_A0 + i] = va_arg (vl, unsigned long int);
long int stack_args = argc - reg_args;
if (stack_args > 0)
{
sp = (unsigned long int *)
(((uintptr_t) sp - stack_args * sizeof (long int)) & ALMASK);
- ucp->uc_mcontext.__gregs[LARCH_REG_SP] = (uintptr_t) sp;
+ ucp->uc_mcontext.sc_regs[LARCH_REG_SP] = (uintptr_t) sp;
for (long int i = 0; i < stack_args; i++)
sp[i] = va_arg (vl, unsigned long int);
}
@@ -26,7 +26,7 @@
static inline uintptr_t
sigcontext_get_pc (const ucontext_t *ctx)
{
- return ctx->uc_mcontext.__pc;
+ return ctx->uc_mcontext.sc_pc;
}
#endif
@@ -43,22 +43,43 @@ typedef unsigned long int greg_t;
typedef greg_t gregset_t[32];
#endif
+#ifdef __USE_MISC
+# define __ctx(fld) fld
+#else
+# define __ctx(fld) __ ## fld
+#endif
+
typedef struct mcontext_t
{
- unsigned long long __pc;
- unsigned long long __gregs[32];
- unsigned int __flags;
- unsigned long long __extcontext[0] __attribute__((__aligned__(16)));
+ union
+ {
+ struct
+ {
+ unsigned long long __ctx(sc_pc);
+ unsigned long long __ctx(sc_regs)[32];
+ unsigned int __ctx(sc_flags);
+ unsigned long long __ctx(sc_extcontext)[0] __attribute__((__aligned__(16)));
+ };
+ struct
+ {
+ unsigned long long __pc;
+ unsigned long long __gregs[32];
+ unsigned int __flags;
+ unsigned long long __extcontext[0] __attribute__((__aligned__(16)));
+ };
+ };
} mcontext_t;
/* Userlevel context. */
typedef struct ucontext_t
{
- unsigned long int __uc_flags;
+ unsigned long int __ctx(uc_flags);
struct ucontext_t *uc_link;
stack_t uc_stack;
sigset_t uc_sigmask;
mcontext_t uc_mcontext;
} ucontext_t;
+#undef __ctx
+
#endif /* sys/ucontext.h */
@@ -15,7 +15,7 @@ _NSIG8 (_NSIG / 8)
#define stack(member) ucontext (uc_stack.member)
#define mcontext(member) ucontext (uc_mcontext.member)
-UCONTEXT_FLAGS ucontext (__uc_flags)
+UCONTEXT_FLAGS ucontext (uc_flags)
UCONTEXT_LINK ucontext (uc_link)
UCONTEXT_STACK ucontext (uc_stack)
UCONTEXT_MCONTEXT ucontext (uc_mcontext)
@@ -25,7 +25,7 @@ STACK_SP stack (ss_sp)
STACK_SIZE stack (ss_size)
STACK_FLAGS stack (ss_flags)
-MCONTEXT_PC mcontext (__pc)
-MCONTEXT_GREGS mcontext (__gregs)
+MCONTEXT_PC mcontext (sc_pc)
+MCONTEXT_GREGS mcontext (sc_regs)
UCONTEXT_SIZE sizeof (ucontext_t)