Page MenuHomeFreeBSD

Make mips_postboot_fixup work when building the kernel with clang+lld
ClosedPublic

Authored by arichardson on Jan 22 2018, 6:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 12 2024, 1:19 AM
Unknown Object (File)
Dec 20 2023, 1:38 AM
Unknown Object (File)
Nov 17 2023, 7:28 AM
Unknown Object (File)
Oct 29 2023, 2:54 PM
Unknown Object (File)
Aug 28 2023, 5:01 AM
Unknown Object (File)
Aug 10 2023, 5:30 PM
Unknown Object (File)
Jul 30 2023, 8:09 AM
Unknown Object (File)
Jul 30 2023, 7:32 AM
Subscribers
None

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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

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.

sys/mips/mips/machdep.c
395 ↗(On Diff #38380)

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

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.

sys/mips/mips/machdep.c
390 ↗(On Diff #38864)

I'd still remove the todo here.

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.

This revision is now accepted and ready to land.Feb 4 2018, 8:54 PM
This revision was automatically updated to reflect the committed changes.
arichardson marked an inline comment as done.