In D52528#1200432, @emaste wrote:

commit message disagrees with __nonstring?

In D52563#1200450, @sjg wrote:

No need for a comment, I committed the following upstream:

Index: var.c
===================================================================
RCS file: /cvsroot/src/usr.bin/make/var.c,v
retrieving revision 1.1171
diff -u -p -r1.1171 var.c
--- var.c       29 Jun 2025 11:02:17 -0000      1.1171
+++ var.c       16 Sep 2025 15:13:28 -0000
@@ -1850,7 +1850,7 @@ QuoteShell(const char *str, bool quoteDo
 static char *
 Hash(const char *str)
 {
-       static const char hexdigits[16] = "0123456789abcdef";
+       static const char hexdigits[] = "0123456789abcdef";
        const unsigned char *ustr = (const unsigned char *)str;
 
        uint32_t h = 0x971e137bU;

Okay it looks like musl does not have the weird char* strerror_r: https://git.musl-libc.org/cgit/musl/tree/include/string.h#n66, so it will need to be conditional on glibc.

rebase

rebase

rebase

rebase

In D52531#1199810, @emaste wrote:

Note: This patch should be a separate commit and not merged.
For testing purposes they are combined.

This seems confusing in the commit message

silence warning instead

avoid sys/types.h includes, and update test list

In D52041#1188826, @imp wrote:

Do you need sys/types.h, or sys/_types.h? The latter has less namespace pollution...

And if these are on the excluded list for check headers for buildworld, please remove them.

In D52041#1188826, @imp wrote:

Do you need sys/types.h, or sys/_types.h? The latter has less namespace pollution...

And if these are on the excluded list for check headers for buildworld, please remove them.

This looks sensible to me. One suggestion would be to avoid the manual caching.

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...