[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 18 Jun 2020 11:37:05 -0500
From: <sidneym@...eaurora.org>
To: <musl@...ts.openwall.com>
Subject: RE: Hexagon DSP support
> -----Original Message-----
> From: Rich Felker <dalias@...c.org>
> Sent: Tuesday, May 5, 2020 7:59 PM
> To: sidneym@...eaurora.org
> Cc: musl@...ts.openwall.com
> Subject: Re: [musl] Hexagon DSP support
>
> On Tue, May 05, 2020 at 06:37:41PM -0500, sidneym@...eaurora.org wrote:
> > >- The definitions in bits/msg.h, bits/sem.h, and bits/shm.h are not
> > > time64-compatible. In fact bits/sem.h is a regression; you had it
> > > right before and broke it.
> > As you point out below the core of the problem was the missing time64
> > calls. I've updated the structures to use what I hope or the correct
> > types.
>
> I think they're still wrong and you need to do like the existing 32-bit
archs,
> defining IPC_STAT to 0x102 and putting separate unsigned long *_lo/*_hi
> members in the right places for the kernel ABI and adding the time_t
members
> at the ends. This is almost surely the case if _Alignof(long long) is 8 on
hexagon,
> since the kernel's include/uapi/asm-generic/msgbuf.h etc. have the lo/hi
pairs
> misaligned (struct ipc_perm is an odd number of 32-bit words). The fact
that
> you had to change qemu to make this work suggests that you still have it
> wrong too -- you should not have to change qemu or the kernel to make it
> work if you do it right.
The IPC_STAT setting was a key element missing from the port.
>
> > >- The definition in bits/stat.h is using the wrong types (you need to
> > > use libc type names for all the members; this is a policy
> > > requirement to ensure that none inadvertently mismatch). Note that
> > > there's not even really a need to use an arch-specific header here
> > > anymore; the kernel definition comes from arch/hexagon/kstat.h and
> > > the bits/stat.h can just be copied from aarch64 or something else
> > > with a clean layout.
> >
> > I used OpenRISC's stat.h.
>
> Is there a reason? It has old __st_*tim32 members but you don't need them
> since there's no old-musl-ABI involved.
I think you had originally suggested using aarch64 and that is what is there
now.
>
> > src/api/ftw.c:44:1: error: switch condition has boolean value
> > [-Werror,-Wswitch-bool]
> > C(S_ISBLK(0))
> > ^~~~~~~~~~~~~
>
> You're still building libc-test with -Werror which will give bogus
results.
>
> > src/api/sys_ipc.c:13:1: error: initializing 'uid_t *' (aka 'unsigned
> > int *') with an expression of type 'int *' converts between pointers
> > to integer types with different sign [-Werror,-Wpointer-sign]
> > F(uid_t,uid)
> > ^~~~~~~~~~~~
> > src/api/sys_ipc.c:3:20: note: expanded from macro 'F'
> > #define F(t,n) {t *y = &x.n;}
> > ^ ~~~~
>
> This is happening because struct ipc_perm's members have the wrong types
> (but I think you should just delete your bits/ipc.h because the generic
one is
> correct and matches the kernel's ABI for hexagon).
QEMU for Hexagon's target_ipc_perm was based on the deprecated one cited in
uapi/linux/ipc.h instead of the generic version in
./uapi/asm-generic/ipcbuf.h I removed the file, ipc.h from musl and
corrected QEMU.
>
> > src/functional/ipc_msg.c:63: qid_ds.msg_qnum == 0 failed: got 16384,
> > want 0
> > src/functional/ipc_msg.c:67: (long long)qid_ds.msg_rtime == 0 failed:
> > got 6823500725968961536, want 0
> > src/functional/ipc_msg.c:69: qid_ds.msg_ctime >= t failed: got 69713,
> > want >= 1588720066
> > src/functional/ipc_msg.c:73: qid_ds.msg_qbytes > 0 failed: got 0, want
> > > 0
> > src/functional/ipc_msg.c:78: qid_ds.msg_qnum == 1 failed: got 16384,
> > want 1
> > src/functional/ipc_msg.c:79: qid_ds.msg_lspid == getpid() failed: got
> > 0, want 240818
> > src/functional/ipc_msg.c:81: msg_stime is 0 want >= 1588720066
> > src/functional/ipc_msg.c:130: child exit status: 256 FAIL
> > src/functional/ipc_msg-static.exe [status 1]
> > src/functional/ipc_msg.c:63: qid_ds.msg_qnum == 0 failed: got 16384,
> > want 0
> > src/functional/ipc_msg.c:67: (long long)qid_ds.msg_rtime == 0 failed:
> > got 6823500725968961536, want 0
> > src/functional/ipc_msg.c:69: qid_ds.msg_ctime >= t failed: got 0, want
> > >= 1588720066
> > src/functional/ipc_msg.c:73: qid_ds.msg_qbytes > 0 failed: got 0, want
> > > 0
> > src/functional/ipc_msg.c:78: qid_ds.msg_qnum == 1 failed: got 16384,
> > want 1
> > src/functional/ipc_msg.c:79: qid_ds.msg_lspid == getpid() failed: got
> > 0, want 240786
> > src/functional/ipc_msg.c:81: msg_stime is 0 want >= 1588720066
> > src/functional/ipc_msg.c:130: child exit status: 256 FAIL
> > src/functional/ipc_msg.exe [status 1]
>
> These indicate your struct ipc_perm/msqid_ds mismatch kernel, as described
> above.
>
> Otherwise I didn't see anything obviously wrong in tests. Having less
noise
> from -Werror and the above issues would make it easier to review the
report
> though and ensure I'm not missing anything.
I attached the updated REPORT with warning output disabled, -w and
-fno-rounding-math (See https://bugs.llvm.org/show_bug.cgi?id=45329) along
with the patch. I've rebased a couple of times without any conflicts and
the git repo is here: https://github.com/quic/musl/tree/hexagon
My most recent rebase was yesterday and that is what I used for the patch
and the libc-test.
I'd like to get Hexagon added as a supported architecture and I am certain
there are preferred windows when new architectures are accepted.
Understanding when those are and what requirements for inclusion are would
be good to know.
Thanks
>
> Rich
Download attachment "REPORT" of type "application/octet-stream" (248152 bytes)
Download attachment "musl-add-hexagon.diff" of type "application/octet-stream" (40623 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
