Messages in this thread | | | Date | Wed, 20 Jan 2021 14:54:47 +0000 | From | Mark Rutland <> | Subject | Re: rcutorture initrd/nolibc build on ARMv8? |
| |
On Wed, Jan 20, 2021 at 03:25:00PM +0100, Willy Tarreau wrote: > On Wed, Jan 20, 2021 at 01:45:11PM +0000, Mark Rutland wrote: > > > Ah that's very interesting indeed because actually these ones should > > > only be used when __NR_dup3 or __NR_clone are not defined. Thus I wanted > > > to check the definitions that were reported in your error output but > > > actually what was needed was to figure whether the correct ones were > > > present, and they are, here on my machine (and yes I agree that in this > > > case the dup2/fork above are bofus): > > > > The issue is that even if a function is unused, the compiler still has > > to parse and compile the code, so where __NR_dup2 is not defined, we'll > > get a build error for: > > > > static __attribute__((unused)) > > int sys_dup2(int old, int new) > > { > > return my_syscall2(__NR_dup2, old, new); > > } > > For sure but this is supposed to be used only when __NR_dup3 is not > defined. Ah now I understand where my mistake is: after it built > successfully for me I inspected the most recent tree which has these > in place. Sorry for being stupid! > > In my local tree it's defined like this: > > static __attribute__((unused)) > int sys_dup2(int old, int new) > { > #ifdef __NR_dup3 > return my_syscall3(__NR_dup3, old, new, 0); > #else > return my_syscall2(__NR_dup2, old, new); > #endif > }
Ah, much better!
For robustness, I think it would be worth doing:
static __attribute__((unused)) int sys_dup2(int old, int new) { #if defined(__NR_dup3) return my_syscall3(__NR_dup3, old, new, 0); #elif defined(__NR_dup2) return my_syscall2(__NR_dup2, old, new); #else #error Cannot implement dup2 #endif }
... and getting rid of the ARCH_WANT_* definitions (which are never legitimate for userspace to set). That way, there's no risk that we accidentally use a bogus syscall number in future. Where the kernel does implement a syscall, it will have done whatever is necessary to expose the corresponding __NR_<syscall> to userspace without userspace needing to define anything.
> I didn't want to do that because that would break userland which needs > dup2(), hence the mapping to dup3 instead: > > static __attribute__((unused)) > int sys_dup2(int old, int new) > { > #ifdef __NR_dup3 > return my_syscall3(__NR_dup3, old, new, 0); > #else > return my_syscall2(__NR_dup2, old, new); > #endif > }
Sure, makes sense, though as above it might be worth adding an explicit check for the fallback syscall number.
> I shouldn't need since these are already fixed in my tree. At first glance > the equivalent of the following commits is missing from the kernel version: > > https://github.com/wtarreau/nolibc/commit/2379f25073f906d0bad22c48566fcffee8fc9cde > https://github.com/wtarreau/nolibc/commit/fd5272ec2c66e6f5b195d41c9c8978f58eb74668 > https://github.com/wtarreau/nolibc/commit/47cc42a79c92305f4f8bc02fb28628a4fdd63eaa > https://github.com/wtarreau/nolibc/commit/d2dc42fd614991c741dfdc8b984864fa3cf64c8e > https://github.com/wtarreau/nolibc/commit/800f75c13ede49097325f82a4cca3515c44a7939 > > However I'm interested in knowing if the latest version fixes everything > for you or not : > > https://raw.githubusercontent.com/wtarreau/nolibc/master/nolibc.h
I replaced my in-tree copy with that, and it built without issues, and the tests ran correctly.
> OK thanks! I will retry here without setting those. I'm pretty sure I > needed these ones to find the __NR_* values but it's possible that it > was before I had the alternate ones and that these are finally not > nedeed at all (which I would prefer as these are ugly).
Great! I reckon they're not needed at all so long as usage of each __NR_* is suitably guarded (such as above).
If you do spot issues when removing ARCH_WANT_*, I'm happy to take a look, and/or to test patches handling any fallout.
Thanks, Mark.
| |