Make mips_postboot_fixup work when building the kernel with clang+lld

[arichardson]

Summary

The compiler/linker can align fake_preload anyway it would like. When
building the kernel with gcc+bfd this always happened to be a multiple of 8.
When I built the kernel with clang and linked with lld fake_preload
happened to only be aligned to 4 bytes which caused a an ADDRS trap because
the compiler will emit sd instructions to store to this buffer.

[jhb]

I would perhaps use _Alignof(u_long) since that is what the later 'roundup()' statements assume. I would also just amend the comment to say something like: "Ensure the buffer is properly aligned to hold module metadata structures." If we had a proper "struct module_metadata" then we could use _Alignof with that type, but we don't have that type.

[arichardson]

Use _Alignof()

[imp]

In D14018#294255, @jhb wrote:

If we had a proper "struct module_metadata" then we could use _Alignof with that type, but we don't have that type.

Maybe we should create one and use it everywhere. Then we wouldn't need _Alignof at all... Then again, one could also have static u_long fake_repload[64]; too and not need the _AlignOf. Don't see why we need to add extra complex bits that honestly aren't needed.

I know all the other places in the tree do this construct as it was copied from there. This is a strong hint those are also wrong.

sys/mips/mips/machdep.c

395 ↗

(On Diff #38380)

/*

• We store u_long sized objects into the reload area, so the array must be so aligned. The standard allows any alignment for char data. */

I think is more than sufficient. The details aren't interesting about the specific compilers today. Also, we don't need the TODO. People can grep for that when they add _Alignas to sys/cdefs.h. :)

[imp]

sys/mips/mips/machdep.c

395 ↗

(On Diff #38380)

This is my old comment ignore it. My new comment is simply:

Use 'static u_long fake_preload[64];' instead. But then you'd need to change the #define to be:
*(type *)(preload_ptr + (size / sizeof(u_long)) = (value)

or use the less insane walking code that I used in devmatch.

[imp]

sys/mips/mips/machdep.c

395 ↗

(On Diff #38380)

Oh, the devmatch code is for walking, not creating....

[jhb]

sys/mips/mips/machdep.c

395 ↗

(On Diff #38380)

So I think it is overall simpler to just leave it as a char array with _Alignof(). Actually, I think your suggestion of using an array of u_long won't actually work when stuffing the uint32_t objects in at the start of each chunk of data as the 'size / sizeof' will round down (truncate) and store the uint32_t's on top of each other. I do think we should probably use the simpler comment you suggested. The TODO is still relevant as we already have _Alignas() in sys/cdefs.h, but for pre-C11 compilers it is crippled as it can only accept a constant, not a type.

[arichardson]

Shorten comment

[imp]

sys/mips/mips/machdep.c

390 ↗

(On Diff #38864)

I'd still remove the todo here.

[arichardson]

remove unecessary TODO

[imp]

I'm happy! Thanks for putting up with the nitpicks. The metadata is something that we need to go back and cleanup in a saner way on all the architectures, but that's well beyond the charter of this change.