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.

Note that I don't think the idea of setting the time early is inherently bad; in fact it makes sense to have the correct time for as much of the install as possible. But there's more work needed to be able to actually do it properly; for example, you likely need to have some kind of /etc/localtime -> /tmp/bsdinstall_etc/localtime or whatever like we do for resolv.conf, and then use *that* to populate /etc/localtime for the installed system. If that can be made to work, then I think it should be done unconditionally, otherwise it shouldn't be done at all.

In D51935#1188136, @ngie wrote:

In D51935#1188118, @kevans wrote:

Indeed, that run has also failed because we don't have ssl headers available on the host

Yes, I see the pickle you're dealing with now.

I'll get out of the way. This is not super ideal, but no less ideal than the current implementation I suppose.

In D51935#1188113, @ngie wrote:

In D51935#1188110, @ngie wrote:

I wouldn't say building libcrypto is small necessarily... I need to understand why certctl is needed during the build process instead of later on (as part of the install process).

certctl is used as part of distributeworld/installworld. Seems like a prime candidate to build as part of build-tools or cross-tools instead of prebuilding another copy/set of libraries.

I need to understand why certctl is needed during the build process instead of later on (as part of the install process).

What about release/scripts/pkgbase-stage.lua, which has a bunch of duplicated logic to decide which kernels to put on install media?

Fixes: 81d8827ad875 ("certctl: Reimplement in C")

Please land this with this one bug fixed (you can add my Reviewed by: for that modified version of this patch), assuming you've verified it does fix the build still

In D51924#1186691, @kib wrote:

In the description: s/eto/to/

Don't round up

In D51882#1185733, @lwhsu wrote:

Maybe make it optional and can be set in the config so we have "develop" and "release" modes?

Fix metalog output to be relative to DESTDIR.