In D46403#1057696, @jrtc27 wrote:

In D46403#1057695, @arichardson wrote:

In D46403#1057654, @kib wrote:

In D46403#1057647, @arichardson wrote:

In D46403#1057619, @kib wrote:

Could it be simplified by providing an fcntl variant that takes the third arg unconditionally, instead?

I would also be happy with that approach, just assumed this variant would be more flexible. Would the unconditional one look like this: fcntl2(int, int, __intptr_t)?

The actual syscall takes three args unconditionally, so there is no point in adding more complications IMO. By existing conventions, the function perhaps should be called fcntl3.

Actually, I just remembered this would not help with the interposing of fcntl since the interposer would still need to extract the variadic argument.

Well, fcntl would call fcntl3, and you can then interpose the latter, just as with vfcntl.

In D46403#1057654, @kib wrote:

In D46403#1057647, @arichardson wrote:

In D46403#1057619, @kib wrote:

Could it be simplified by providing an fcntl variant that takes the third arg unconditionally, instead?

I would also be happy with that approach, just assumed this variant would be more flexible. Would the unconditional one look like this: fcntl2(int, int, __intptr_t)?

The actual syscall takes three args unconditionally, so there is no point in adding more complications IMO. By existing conventions, the function perhaps should be called fcntl3.

review comments

In D46403#1057607, @jrtc27 wrote:

there
is currently no portable way to interpose a variadic function without
relying on undefined behaviour

That's not entirely true, but it requires you to know how many arguments were passed in every possible case, what types they are, va_arg them in turn and call the interposed function with those arguments (or more). Whereas interposing a function that takes the va_list itself means you only need to interpret the arguments in the cases where you actually want to inspect at them.

In D46403#1057619, @kib wrote:

Could it be simplified by providing an fcntl variant that takes the third arg unconditionally, instead?

Also zero fp

In D46400#1057540, @jrtc27 wrote:

Should we also do fp/s0 to match, like arm64 does?

No sure how much the builtins optimization matters here but if it does the other workaround would be:

It looks like removing this call to memcpy might actually also fix the TSan issue I tried to work around in https://reviews.freebsd.org/D28536. Might be time to try run the TSan testsuite again.

They should build for x86, that was the whole point of the upstream changes (supporting ieee 128-bit float in addition to ld80)

Thanks for working on this. This reminds me of my efforts a few years ago to avoid the build-tools awkwardness: https://reviews.freebsd.org/D28253.

In D44030#1004970, @kevans wrote:

In D44030#1004924, @arichardson wrote:

Have you tried running the lsan test suite with this change?

I'm working on it, but just trying to get the llvm test suite running has been... rough. I chose to redirect from your question to the tangentially related and still valuable: "Have you tried running the asan test suite with this change?" since we can know how much of the asan test suite has passed already.

After fumbling around and finally figuring out that I needed to configure llvm/ with -DLLVM_ENABLE_PROJECTS="clang;compiler-rt" because some of the test prereqs are in llvm or clang, most of the tests that actually run fail because they're trying to do something like:

22:56 <@kevans91_>  70252 103012 Asan-aarch64-inline CALL  execve(0,0x81278f78,0x81278f98)
22:56 <@kevans91_>  70252 103012 Asan-aarch64-inline RET   execve -1 errno 9 Bad file descriptor

... and this is the realization that I just burned a not-insignificant chunk of time because this hasn't been upstreamed yet, three months later: https://cgit.freebsd.org/src/commit/?id=4c9a0adad18263ec8725d9bfc5f560c6ad1da8bd

Have you tried running the lsan test suite with this change?

For example the linux joystick.h defines a bunch of ioctls that I don't see implemented on FreeBSD, so we really shouldn't claim support for it? Maybe the header should only includes the constants?

In D43498#996756, @adridg wrote:

input-event-codes.h hasn't changedu pstream in 4 years (perhaps "of course" because it's a tagged release) but if wulf@ says that that one should be obtained upstream, then that seems reasonable. There's no meaningful difference between the base-system-provided version and upstream that I can see -- it's all defines of constants. One concern I have is that we stopped using base-system headers at some point -- at least that's how I remember it -- because the dev/evdev/ headers were not meant for user-space consumption. I can't find that in ports commit history, though, so maybe I'm dreaming it.

Commit message should probably be qualified with "for kernel compilation"?

This LGTM but the commit message should also include the minimum Clang+GCC versions

ping?

Based on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c56 we can use [[nodiscard]] with GCC 11 and higher which has the expected semantics. But for now this change seems fine

Both of these features are required by POSIX, %% also seems to be required:
https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_06_02

Looking at the Clang change that added -std=c17 (https://github.com/llvm/llvm-project/commit/5b6c0f75e01571851b767dc63a3229c962f464f1), the only difference is the value of __STDC_VERSION__, so unless code uses this in preprocessor macros it's identical to -std=c11

I had a look on godbolt to see which compiler versions support c17: https://godbolt.org/z/EGd4TqP3j

Overall I'd be very happy to see the standard bumped to c17 but I'm not sure what the current minimum compiler versions are. But then again maybe it's time to say you need a c17 compiler for current...

I see you have a reduced test case, would it be possible to turn it into a regression test for in the ld-elf tests?

In D41983#957349, @yuripv wrote:

In D41983#957328, @arichardson wrote:

You could push to a GitHub fork, then it will run the bootstrap steps as part of the GitHub action

Sorry for being slow (that's not the first I'm told about this), but I don't know where exactly should I push? To a branch in my fork of freebsd-src on github (and create a pull request?)?

You could push to a GitHub fork, then it will run the bootstrap steps as part of the GitHub action

This will almost certainly break the cross build. The typedefs in the local _ctype.h are needed. Can you modify that file to include the new generated header?

In D40623#955061, @arichardson wrote:

Is there a reason you didn't add __asm__ volatile(".cfi_undefined x30"); to the start of the C function? In my testing with CheriBSD this appears to work in the same way (libunwind tests fail before, succeed after).

Is there a reason you didn't add __asm__ volatile(".cfi_undefined x30"); to the start of the C function? In my testing with CheriBSD this appears to work in the same way (libunwind tests fail before, succeed after).

Thanks for working on this - I also tried to get it to work a few years ago but didn't get too far. It looks like this includes the fix to the uninitialized variable that I ran into (https://github.com/arichardson/cheribsd/commits/virtfs-cheribsd) so I think all of my fixes from back then are included.

Thanks for fixing this - I wonder if it's time to try turning on strict tmppath by default again upstream. I initially tried that but gave up after noticing that it broke various cases like etcupdate that I didn't have time to debug.

This breaks the macos bootstrap jobs, could you add an ifdef around the new code?

IIRC this is also built during bootstrap. Could you check that the GitHub actions CI is still happy before committing?

I'm surprised this is needed, you'd hope the compiler could do this transformation. Maybe worth filling a LLVM bug report?

This will sort by name not priority. Ld.lld will sort by priority automatically if you omit init_array from the linker script but for bfd you need SORT_BY_INIT_PRIORITY

Thanks for adding this - I was recently trying to debug RISC-V issues using qemu tracing and the constant SBI calls meant that it made essentially zero progress since tracing slowed down qemu so much that the next timer interrupt fired once it returned from the sbi call.

In D39916#914668, @hselasky wrote:

In D39916#914571, @arichardson wrote:

In D39916#914569, @hselasky wrote:

Sorting 1125 sysinit items
117378 instructions with qsort()
5509157 instructions with bubble

And if the list is already sorted and you use insertion sort there instead of bubble sort, what is the time then? Can you get that into the list too?

I guess it will be something like 500x instead of your 50x.

--HPS

Yes that would obviously be faster but I don't have time to work on that. If you are planning to work on this and it's going to be ready for review in the near future I'd be happy to drop this patch.

However, using a pre-existing sorting function to obtain a noticeable speedup seems like a valid approach to me. Just because it could be done in a more optimal way at some hypothetical point in the future shouldn't mean we can't make incremental improvements on the way to an ideal solution. Otherwise things would never improve...

Could we make a deal on that? I writeup a patch for the 500x case today, and you can test it and compare against the current results?

In D39916#914569, @hselasky wrote:

Sorting 1125 sysinit items
117378 instructions with qsort()
5509157 instructions with bubble

And if the list is already sorted and you use insertion sort there instead of bubble sort, what is the time then? Can you get that into the list too?

I guess it will be something like 500x instead of your 50x.

--HPS

fix tslog

Rebased on latest main.
@cperciva it would be great if you could test how much this helps your usecase.

In D39916#914437, @kevans wrote:

In D39916#908771, @arichardson wrote:

In D39916#908757, @mjg wrote:

this is a massively dodgy change, one has to expect some of the real orderings work by accident. i think it would be better to take a look at trimming the list instead.

Trimming the list definitely makes sense but I went for the simpler change here. The list is currently somewhat arbitrarily sorted by the linker and I don't think this makes it any worse. We could add a stable sort to libkern but I don't think this should be needed.

Arbitrary ordering of the linker has bitten us in the past, sometimes it seems like it'd be nice to have the option to introduce some intentional chaos so that we can find potential issues by randomly reordering (within a {subsystem, order}, naturally), but you'd really want to be able to do that *and* find a way to make it reproducible (e.g., feed it this seed via tunable and it'll re-order in such a way that GENERIC dies in a horrible fire)

Can this be committed, so this header is standalone even if you don't use a patched LLVM?

In D39916#908757, @mjg wrote:

this is a massively dodgy change, one has to expect some of the real orderings work by accident. i think it would be better to take a look at trimming the list instead.

In D39916#908603, @imp wrote:

does qsort's unstable sort actually change the order of anything?

Looks good to me now but please wait for @jrtc27 and/or @imp before committing.

Thanks for the update, looks like it's much closer to the build world flags now. Would be good to know if the __unused bits can be removed but otherwise looks good to me (well the parts that I understand).

I'd prefer if the __enum_uint8(vtype) changes were only inside structs and not in the function signatures but I don't feel strongly about it.

I think the second macro is unnecessary based on my experiments with godbolt. Otherwise LGTM once we've decided where to place it (and I'd suggest with the _KERNEL guard).

LGTM

Not tested it but changes LGTM. Looking forward to dropping the workaround from cheribuild

Using the C23 underlying enum type extension is IMO better and safer. Support with clang (since version 8) and GCC 13+: https://godbolt.org/z/n4EshMY1j