[clang][modules] Over-align the `Module` class

[jansvoboda11]

Summary

This makes llvm::PointerIntPair<Module *, 3> from f35230ae work across platforms.

[srj]

How did the original change pass presubmit testing -- is LLVM no longer testing on 32-bit targets?

EDIT: I ask because (presumably) the original change was able to land without triggering any failures, so I'd like to know how we're going to vet this change to ensure it fixes the issue.

[jansvoboda11]

Pre-commit CI only runs on x64 Debian and x64 Windows. I don't have good way to reproduce the issue, so ideally the person who reported it (@ms178?) steps up.

[srj]

clang/include/clang/Basic/Module.h

96–98

Probably worth a comment here, it's not necessarily obvious to the reader that this is to fix a 32-bit pointer issue.

[srj]

In D134653#3815627, @jansvoboda11 wrote:

Pre-commit CI only runs on x64 Debian and x64 Windows. I don't have good way to reproduce the issue, so ideally the person who reported it (@ms178?) steps up.

I'm the one who reported the breakage.

Regarding presubmit testing, that's of great concern to me; if LLVM isn't testing on x86-32 then it's not really *supporting* x86-32 anymore (as a compilation host anyway), and that's a rather large change to make without warning. Do you know to whom I should escalate that concern?

[srj]

In D134653#3815627, @jansvoboda11 wrote:

Pre-commit CI only runs on x64 Debian and x64 Windows. I don't have good way to reproduce the issue, so ideally the person who reported it (@ms178?) steps up.

FYI, adding this patch to my local build does allow me to *compile* properly now. I haven't tested for correctness.

[jansvoboda11]

Document reason for alignas.

[jansvoboda11]

In D134653#3815672, @srj wrote:

FYI, adding this patch to my local build does allow me to *compile* properly now. I haven't tested for correctness.

Thanks for verifying the build. You can test for correctness by "building" the check-clang target.

In D134653#3815648, @srj wrote:

Regarding presubmit testing, that's of great concern to me; if LLVM isn't testing on x86-32 then it's not really *supporting* x86-32 anymore (as a compilation host anyway), and that's a rather large change to make without warning. Do you know to whom I should escalate that concern?

You can contact the infrastructure working group. Note that we run more configurations post-commit: https://lab.llvm.org/buildbot/#/console

[srj]

Thanks for verifying the build. You can test for correctness by "building" the check-clang target.

Thanks -- I'll also build and test Halide locally.

You can contact the infrastructure working group. Note that we run more configurations post-commit: https://lab.llvm.org/buildbot/#/console

I opened a question at https://discourse.llvm.org/t/x86-32-bit-testing/65480 on Friday but got no responses. Is there a better venue for asking the right people?

[srj]

Thanks for verifying the build. You can test for correctness by "building" the check-clang target.

Tests are passing locally, LGTM

[jansvoboda11]

In D134653#3815703, @srj wrote:

I opened a question at https://discourse.llvm.org/t/x86-32-bit-testing/65480 on Friday but got no responses. Is there a better venue for asking the right people?

That's what I would do, I'm not aware of better place to raise this.

In D134653#3815715, @srj wrote:

Tests are passing locally, LGTM

Thank you for confirming the fix!

clang/include/clang/Basic/Module.h

static ASTFileSignature create(InputIt First, InputIt Last) {
 
     ASTFileSignature Signature;
     std::copy(First, Last, Signature.begin());
     return Signature;
   }
 };
 
 /// Describes a module or submodule.
-class Module {
+///
+/// Aligned to 8 bytes to allow for llvm::PointerIntPair<Module *, 3>.
+class alignas(8) Module {
 public:
   /// The name of this module.
   std::string Name;
 
   /// The location of the module definition.
   SourceLocation DefinitionLoc;
 
   enum ModuleKind {