Generally, this looks good to my eye. I think the masks are overkill, but that comes from my experience in pci land where they were over-used and under-needed.

I would posit that fsck is the wrong place to replicate newfs' calculations, unless there's some value that's added that I'm not seeing.

If newfs was used with default settings, and the partition was not resized, then alternate superblocks can be found by fsck using the same calculation as was done by newfs.
The problematic case is the e.g. resizing of the partition and the filesystem after initial filesystem creation, since alternate superblocks with stale content would be found.

This also works, and I like the larger area protect by _KERNEL.

Should have been done when the kernel side of this was committed.

In D11636#241109, @rlibby wrote:

kib/imp feedback: detect old compiler version rather than move decls.

I checked that this compiles with gcc 4.2 WITHOUT_MODULES=efirt, and with clang 4.0 (plain, including efirt). I am still testing gcc > 4.4.

I guess clang also defines __GNUC__ as the clang build failed without the clang check. I don't have a way to check __INTEL_COMPILER, I am just following sys/sys/cdefs.h on that one.

In D11636#241066, @rlibby wrote:

In D11636#241063, @imp wrote:

The proper solution then is what both kib and I are saying: add the proper ifdef so it isn't included when compiling with the old compiler. The declarations belong here as they aren't machine specific. The old compiler is going away, and it makes no sense to move something out of the way when we'll just move it back for arm64 EFIRT support later. Just ifdef it out for gcc < 4.4. We'll be dropping support for that in 12 most likely and it's a lot easier to delete a couple of lines than to move it around.

Oh, yeah, I'm happy to do that. I got the wrong impression from the previous comment I guess. I'll update this shortly with the compiler version detection.

In D11636#241062, @rlibby wrote:

In D11636#241051, @imp wrote:

IMHO, This is completely bogus.

We do not support and cannot support building anything using EFI with the old gcc compiler. Better to have a check to see if it is the old gcc compiler and #error, or like we do for the loader build, exclude those modules when built with old gcc.

There will be other architectures where this code is used, like arm64, once the VM stuff is done for them.

Do not commit this code.

What's the use case that this is even included with gcc 4.2.1?

This isn't to build efirt with gcc 4.2, it's to NOT build it!

The EFIABI_ATTR decls currently are visible even when not compiling efi code, which breaks the gcc 4.2 build even when specifying WITHOUT_MODULES=efirt. E.g.

cc [...] /usr/src/freebsd/sys/amd64/amd64/machdep.c
cc1: warnings being treated as errors
In file included from /usr/src/freebsd/sys/amd64/amd64/machdep.c:65:
/usr/src/freebsd/sys/sys/efi.h:146: warning: 'ms_abi' attribute directive ignored [-Wattributes]

And also some userland code (don't recall exactly what, some ioctl code). So either the decls need to be hidden from in-tree gcc somehow, or it will no longer work for amd64 buildkernel or buildworld.

IMHO, This is completely bogus.

In D11628#240890, @ngie wrote:

In D11628#240879, @imp wrote:

Would SUBDIR_UNSORTED make more sense? Of course SUBDIR_PARALLEL likely should wither and die for a similar reason...

I don't think SUBDIR_UNSORTED would make sense to be honest. That sounds like an opt-out feature that might break downstream consumers of bsd.subdir.mk. I wanted to make this opt-in so projects that used bsd.subdir.mk and didn't use SUBDIR_PARALLEL wouldn't break after this change. FreeBSD has several Makefiles in gnu/ for instance at some point in time that were also affected by this, IIRC, so I expect downstream consumers to be more likely to have Makefiles which might be affected by the converse behavior if it was globally enabled and available as an opt-out feature.

Would SUBDIR_UNSORTED make more sense? Of course SUBDIR_PARALLEL likely should wither and die for a similar reason...

In D11589#240652, @kib wrote:

In D11589#240165, @imp wrote:

We have had an issue, still not completely root caused, where the last alternate supreblock is bad (all zeros, either due to enemy action, or a newfs run gone awry). We needed some way to proceed with the fsck anyway to help track down the problem. In looking at that problem, the unused proto became obvious, so that was fixed as well. So the CHK changes are to give more data about how they don't match, and the prompt is to allow fsck to proceed in the face of this error to find out if it is localized to that one block, or more systemic. I have the two logical commits separated out in my git tree that I'll commit from.

In D11589#240154, @kib wrote:

Can you remind me the context where the previous discussion occur ? I do not remember a bit about it, sorry.

Updating D11589: Print more info when super blocks don't match, implement ability to

ignore mismatches.

Why not 50MB instead of 512kb? That's stupidly small and precludes any and all future use of UEFI programs as well as boot block bloat. bz2 compressed, the size difference is trivial.

While complete, this interface is a total PITA. End to end protection type 2 is what again? Oh, I have to go look in the standard to understand all these parameters?
Can we at least have a description of what's going on here? A table for each of the magic numbers?

In D11432#236754, @sbruno wrote:

This was fixed at r320522

Is this really format, or is it namespace provisioning? There's another set of name space provisioning patches that are also under review right now.

This looks good to me as well...

While not technically incorrect (except maybe the lib32 comment), this papers over a weakness in the new lazy depend generation that should actually be fixed in said generation. Or failing that, the check belongs in libc generically so we don't have to maintain these silly lists.

In D11185#233556, @imp wrote:

Rebase to latest tip of the spear...

Rebase to latest tip of the spear...

Remove unused includes, per kib.

I've updated the summary to be the commit message, modulo the content we put in the commit message that differential barfs on.

Latest nits from kib@

In D11185#233466, @emaste wrote:

I consider this compat work as optional and done by will of the submitter, not as a precedent enforcing new project policy on forward-incompatible ABI changes.

In my opinion I think we should neither mandate nor discourage forward-compatibility shims.

Add getfsstat. needed for df.

OK, with the latest update, I think I've addressed everything there.

Fix leak I overlooked.

Missed a spelling mistake kib@ found.

Implement kib's suggestions (hopefully I didn't miss any)

Updated with the various issues raised in the code review: weak
symbols, osreldate, missing fields.

again

Trying with magic in git.config file.

In D11115#230171, @emaste wrote:

In D11115#230152, @dim wrote:

I think this is a good idea, but the last time I tried this...

Yes important point - now I seem to recall we spoke about this in the past and you pointed this out but I can't find details. Perhaps it was on IRC. I have been running a similar patch in test trees for quite some time, but will do a more detailed comparison of the differences.

There's no problem with either memcpy or memmove in the kernel. They just work. For like at least decade....

I'm not entirely sure that switching interpreters is a good idea. I rather think it's a terrible idea, but am open to seeing how it progresses before rendering final judgement.

In D10642#226038, @rgrimes wrote:

This is starting to sound like a terminal emulation type vs term= miss match perhaps? My term size was quiet correct, that is what a standard Putty serial terminal size comes up as.

In D10489#226073, @danfe wrote:

OK, let's see if I understood you correctly this time Warner. make(1) is indeed apparently setting MACHINE and MACHINE_ARCH globally.

In D10642#225577, @rgrimes wrote:

Isnt this caused by a missing entry in /etc/ttys for the serial console? This should be coming from there, not from .dot files.

I don't know if there's folks still using glimpse from before... But it's been long enough that I think it can go.

In D10787#224240, @ngie wrote:

In D10787#223634, @imp wrote:

In D10787#223484, @ngie wrote:

Only arm64 exposes __ILP32__ and __LP64__ (depending on whether or not it's 32-bit or 64-bit), per a quick grep of contrib/gcc and contrib/llvm .

All the architectures do. We use it elsewhere in the tree and it works. Your grep has lead you astray.

contrib/gcc/config/i386/freebsd64.h: builtin_define ("LP64"); \
contrib/gcc/config/rs6000/freebsd.h: builtin_define ("LP64"); \
contrib/gcc/config/sparc/freebsd.h: builtin_define ("LP64"); \

are just three examples.

I meant to say "only arm64 exposes it, in addition to the other architectures currently handled".

What I'm wondering about thought is why aren't arm and mips in this list at all? And why does powerpc assume 32 bit?

Beats me why arm/mips were never considered, but there is an issue potentially with how __ILP32__, __LP32__, and __LP64__ are defined on those architectures.

This review specifically addresses the hardcoded have_<FOO> for LP64 and ILP32 that was incorrect (maybe not when this was first written, but wasn't correct after additional architectures were added).

In D10787#223484, @ngie wrote:

Only arm64 exposes __ILP32__ and __LP64__ (depending on whether or not it's 32-bit or 64-bit), per a quick grep of contrib/gcc and contrib/llvm .

Actually I think I take that back... mips and arm aren't listed at all. Are they affected?

Looks good to my eye as well...

Looks good to me.

In D10489#222483, @danfe wrote:

Strictly speaking, you are correct, but man pages often have references elsewhere, and having it here wouldn't be terrible.

Understood, fair enough. Would .Xr build 7 reference suffice (it already establishes a connection between variables and uname -[mp] output there), or you'd rather duplicate this information explicitly here as well?