On Tue, Jun 09, 2020 at 10:29:57AM +0000, Will Springer wrote: > On Saturday, May 30, 2020 3:56:47 PM PDT you wrote: > > On Friday, May 29, 2020 12:24:27 PM PDT Rich Felker wrote: > > > The argument passing for pread/pwrite is historically a mess and > > > differs between archs. musl has a dedicated macro that archs can > > > define to override it. But it looks like it should match regardless of > > > BE vs LE, and musl already defines it for powerpc with the default > > > definition, adding a zero arg to start on an even arg-slot index, > > > which is an odd register (since ppc32 args start with an odd one, r3). > > > > > > > [6]: > > > > https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c > > > > > > I don't think this is correct, but I'm confused about where it's > > > getting messed up because it looks like it should already be right. > > > > Hmm, interesting. Will have to go back to it I guess... > > > > > > This was enough to fix up the `file` bug. I'm no seasoned kernel > > > > hacker, though, and there is still concern over the right way to > > > > approach this, whether it should live in the kernel or libc, etc. > > > > Frankly, I don't know the ABI structure enough to understand why the > > > > register padding has to be different in this case, or what > > > > lower-level component is responsible for it.. For comparison, I had > > > > a > > > > look at the mips tree, since it's bi-endian and has a similar 32/64 > > > > situation. There is a macro conditional upon endianness that is > > > > responsible for munging long longs; it uses __MIPSEB__ and > > > > __MIPSEL__ > > > > instead of an if/else on the generic __LITTLE_ENDIAN__. Not sure > > > > what > > > > to make of that. (It also simply swaps registers for LE, unlike what > > > > I did for ppc.) > > > > > > Indeed the problem is probably that you need to swap registers for LE, > > > not remove the padding slot. Did you check what happens if you pass a > > > value larger than 32 bits? > > > > > > If so, the right way to fix this on the kernel side would be to > > > construct the value as a union rather than by bitwise ops so it's > > > > > > endian-agnostic: > > > (union { u32 parts[2]; u64 val; }){{ arg1, arg2 }}.val > > > > > > But the kernel folks might prefer endian ifdefs for some odd reason... > > > > You are right, this does seem odd considering what the other archs do. > > It's quite possible I made a silly mistake, of course... > > > > I haven't tested with values outside the 32-bit range yet; again, this > > is new territory for me, so I haven't exactly done exhaustive tests on > > everything. I'll give it a closer look. > > I took some cues from the mips linux32 syscall setup, and drafted a new > patch defining a macro to compose the hi/lo parts within the function, > instead of swapping the args at the function definition. `file /bin/bash` > and `truncate -s 5G test` both work correctly now. This appears to be the > correct solution, so I'm not sure what silly mistake I made before, but > apologies for the confusion. I've updated my gist with the new patch [1]. > [...] > > [1]: https://gist.github.com/Skirmisher/02891c1a8cafa0ff18b2460933ef4f3c
This patch looks correct. I prefer the union approach with no #ifdef but I'm fine with either. Rich
