I believe the right thing to do is just set LC_ALL=C in the places where it matters. Seconds since epoch is a horrible format to put in human-readable strings. Most of the existing uses of date already set LC_ALL=C, either explicitly for that command or implicitly from earlier in the script (such as both newvers.sh).

There was nothing for me to check. My concern was how human-unfriendly the format is.

My existing comments still stand. AFAICT the latest revision just rebased it?

In D54018#1234972, @markj wrote:

In D54018#1234971, @jrtc27 wrote:

Why not just replace the entirety of barrier.[ch] with pthread_barrier(3)?

I had the same thought, but apparently macOS doesn't implement it.

Why not just replace the entirety of barrier.[ch] with pthread_barrier(3)?

_ALIGNBYTES seems a bit underspecified and/or fraught. max_align_t is often higher than void * thanks to long double and/or int128. It would probably be more prudent to adapt to being explicit about what alignment each use case needs; many probably should be using max_align_t, or alignof(some struct being allocated). Then _ALIGNBYTES can remain for legacy use cases like cmsghdr where it's part of the ABI.

s/extern char/&[]/ in the subject, also &end is not char * in that case, it's char (*)[], which is an incomplete type so you can't perform pointer arithmetic on it (as sizeof(char[]) is not known). char itself decays to a char * and so pointer arithmetic on *that* works.

In D53817#1229572, @markj wrote:

In D53817#1229564, @brooks wrote:

D53818 actually does redefine vm_offset_t to ptraddr_t which isn't quite right, but will be once I extract the vm_pointer_t patches.

There's a bigger long term question if vm_foo_t should exist at all. I don't have a strong opinion there.

Me neither, but it's a lot of churn to remove at least vm_offset_t. vm_paddr_t and vm_pindex_t have no obvious substitute. vm_ooffset_t would have to be uint64ptr_t I think.

In D53819#1229986, @brooks wrote:

In D53819#1229955, @kib wrote:

And where is __intcap_t defined? Is it provided by some MD include, like machine/_types.h?

I do not remember seeing this MD define in the review series. If true, then should __intcap_t added before the use?

__intcap_t is a new primitive type provided by the compiler. It cannot be defined by a header except on systems that don't support it.

In D52526#1228284, @arichardson wrote:

In D52526#1228266, @jrtc27 wrote:

I forgot about this revision and ended up reinventing it in 4dd2b869cd078ed6f40c42d1ef429222da16a58f and 34e7a57673c9730ee5d1f7ebb07e152567bd8e0b so it should be fixed in-tree. I did however retain the knowledge about Clang's bugginess wrt __gnu_linux__ (and wondered where I'd got that from...) so did not use it myself... I did also check musl matched us rather than glibc.

Thanks for committing this. Do we still need the && defined(__USE_GNU)? Technically it's needed but I guess this file is only every compiled with -D_GNU_SOURCE so probably doesn't matter?

I forgot about this revision and ended up reinventing it in 4dd2b869cd078ed6f40c42d1ef429222da16a58f and 34e7a57673c9730ee5d1f7ebb07e152567bd8e0b so it should be fixed in-tree. I did however retain the knowledge about Clang's bugginess wrt __gnu_linux__ (and wondered where I'd got that from...) so did not use it myself... I did also check musl matched us rather than glibc.

New scripts need to be added to OptionalObsoleteFiles

You were exactly a month late for October 3rd... :)

Maybe a description along the lines of:

• Ensure all operands are output in fpe_store() and input in fpe_restore().

You need to actually edit the description in Phabricator; arc diff won't do it for you if you just amend the commit locally.

Thanks, I'd forgotten about this, and would have guessed it was a change I'd made in 15 not 14 but no, that much time has indeed passed.

This will not fix that bug. fence is fence iorw, iorw, so all you are doing is relaxing some ordering requirements.

Fixes: 8c9c3144ccfa ("Impleent COMPAT_FREEBSD32 for arm64. This is based on early work by andrew@.")

Do we actually have a proper definition of what *our* memory barrier APIs are meant to mean? What the ISA manual says is a sensible mapping isn't necessarily what we want; note that the table you're referencing is for "Linux memory primitives", and we are not Linux, nor do we follow its memory model.

In D53119#1213395, @mhorne wrote:

In D53119#1213368, @jrtc27 wrote:

The logic for removing this in the FDT is that, on Linux, the clock framework comes up early enough (or at least some early part of it) in order to be able to query the UART clock's frequency from the clock driver, whereas we don't do anything like that with clocks prior to newbus passes, so way too late for cninit.

Yes. I can note this in the commit message.

What do you think of the patch?

The logic for removing this in the FDT is that, on Linux, the clock framework comes up early enough (or at least some early part of it) in order to be able to query the UART clock's frequency from the clock driver, whereas we don't do anything like that with clocks prior to newbus passes, so way too late for cninit.

Is the expectation that we ensure all release media have all supported package sets present in an offline package repo? Because disc1/memstick won't have the real src package, for example, and bootonly/mini-memstick won't have any real packages at all. So I think for this to work even bootonly/mini-memstick needs to have a package repo of the package sets (as a pkgbase analogue to how it still has the MANIFEST file that lists all distribution sets, just the sets themselves aren't present)?

I think they should all be vital, in the sense that if you've installed that metapackage then you don't want your system to ever remove it without very explicit confirmation, no?

Please make base the default. bsdinstall defaults should be such that you can mash enter without thinking and get a sensible system. I suspect minimal is not what most want.

Is there a reason to limit this at all? If we’re restricting the set of kernels built already then should we not just pick up any and all kernel packages that release/Makefile got Makefile.inc1 to package?

I'm a bit uneasy about (a) removing the (Experimental) from pkgbase (b) adding the (Legacy) to Dist Sets when pkgbase is currently not fit for release whilst distribution sets are tried and tested. So this feels more aspirational than truthful, but I also understand the objective of the release is to do pkgbase... I just think we need to be very careful not to push pkgbase on our users too hard until we are truly willing to support it in the form they will be installing. I think doing so before we have metapackages integrated into bsdinstall, in particular, is premature.

I guess my view is that OLD_LIBS is really there so that soname bumps don't break existing binaries; you need the new library to be able to build the new binary, but don't want a window between installing the new library and being able to build the new binary where it doesn't work. That isn't really important for this case, because you can just not set WITHOUT_LIBFOO until you've updated all your binaries to not use that ABI. But at the same time, if we're using OLD_LIBS for all the libraries here, then yes, that's pointless if you're using OLD_FILES for rtld itself.

Does delete-old-libs even delete all of a given libcompat if you set WITHOUT_LIBFOO? Oh that's what this code is trying to do.

Just because sys/sysctl.h is self-contained doesn't mean certctl.c shouldn't also be including sys/types.h. Presumably it itself is using types from that header and so should explicitly include it, regardless of whether sys/sysctl.h is including it for its own needs (which is an implementation detail; what means it uses to give things it defines the right types is up to it). And given style(9) header ordering rules that would mean including sys/types.h first, which would render this obsolete.

It does appear both macOS and GNU date support -Iseconds. However, my concern with this change is that such timestamps are rather uglier and less human-readable, which is less of a problem for build system messages but more so for uname. I note that Linux uses LC_ALL=C date output for its uname, running uname -a on an Ubuntu system.

In D52387#1196252, @markj wrote:

In D52387#1196215, @jrtc27 wrote:

Are we sure all these scripts aren't set -u? Even if that's true, I don't love relying on that.

Yes, they are definitely not set -u-clean. I don't quite follow though: how does this patch rely on that? : ${FOO:=bar} is ok even with set -u configured.

Are we sure all these scripts aren't set -u? Even if that's true, I don't love relying on that.

Should this not be tied to SOURCE_DATE_EPOCH rather than some arbitrary and misleading timestamp (1st Jan 2024, which predates any release that will include this code)?

And to be clear, as far as I can tell, absolutely nothing has changed in Clang to make Clang 20 any less happy with this configuration than Clang 12; that is, I believe using Clang 12 with that configuration when this was committed would have been just as broken, and so adding any new version-specific behaviour is inappropriate (and so the subject and body of this review are misguided).

Ok so yeah there is the assumption that you're not doing that. Probably the correct fix is to drop the :S/^ld.//1W. But the other point to make is that your script is unnecessary, just pass --cross-toolchain=llvm@20 and it'll set all the X* variables for you to values that are known to work (because that's what CI is using), and you don't need to mess with PATH to make that work. You also do not need to set CC explicitly unless you really want to override the defaults, which I doubt.

In D52330#1195266, @wosch wrote:

In D52330#1195007, @jrtc27 wrote:

And what happens if you ls /opt/homebrew/opt/lld@20/bin?

$ ls -l /opt/homebrew/opt/lld@20/bin
total 128
lrwxrwxr-x 1 wosch admin 3 Jul 9 01:06 ld.lld -> lld
lrwxrwxr-x 1 wosch admin 3 Jul 9 01:06 ld64.lld -> lld
-r-xr-xr-x 1 wosch admin 62160 Sep 1 20:33 lld
lrwxrwxr-x 1 wosch admin 3 Jul 9 01:06 lld-link -> lld
lrwxrwxr-x 1 wosch admin 3 Jul 9 01:06 wasm-ld -> lld

$ /opt/homebrew/opt/lld@20/bin/ld.lld --version
Homebrew LLD 20.1.8 (compatible with GNU linkers)

calling “ldd” directly gives an error

$ /opt/homebrew/opt/lld@20/bin/lld --version
lld is a generic driver.
Invoke ld.lld (Unix), ld64.lld (macOS), lld-link (Windows), wasm-ld (WebAssembly) instead
echo $?
1

And what happens if you ls /opt/homebrew/opt/lld@20/bin?

Please commit this to main, it’s not appropriate to leave that broken

(By "this is odd" I mean "this change is definitely not right and will break other things even if it happens to end up producing a working build for you", so it's important to understand what exactly the problem you're seeing is rather than just deleting bits of code until your specific system is happy)

This looks odd. How were you trying to build? Have you seen https://github.com/freebsd/freebsd-src/commit/81fc74fd26da2f75c3010595db7a706c3e28d95b and its parent?

This does too many things in one change. Inferring whether it's pre- or post-install via looking at BSDINSTALL_CHROOT adds unnecessary complexity when the caller of the script already knows if it's pre-install or post-install. D52196 maintains that intentionality, and also better handles the time zone used during the install with a working /etc/localtime rather than needing to mess about with TZ. There may be ways you think the implementation there could be better, but I believe that the general design of it is better.