[Clang] Verify data layout consistency #144720
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen Author: Nikita Popov (nikic) ChangesVerify that the alignments specified by clang TargetInfo match the alignments specified by LLVM data layout, which will hopefully prevent accidental mismatches in the future. This currently contains opt-outs for a lot of existing mismatches. I'm also skipping the verification if options like The verification happens in CodeGen, as we can't have an IR dependency in Basic. Full diff: https://github.com/llvm/llvm-project/pull/144720.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index c27168e4c4bfe..aabc872e22df1 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -332,6 +332,76 @@ const TargetCodeGenInfo &CodeGenModule::getTargetCodeGenInfo() {
return *TheTargetCodeGenInfo;
}
+static void checkDataLayoutConsistency(const TargetInfo &Target,
+ llvm::LLVMContext &Context,
+ const LangOptions &Opts) {
+#ifndef NDEBUG
+ // Don't verify non-standard ABI configurations.
+ if (Opts.AlignDouble || Opts.OpenCL || Opts.HLSL)
+ return;
+
+ llvm::Triple Triple = Target.getTriple();
+ llvm::DataLayout DL(Target.getDataLayoutString());
+ auto Check = [&](const char *Name, llvm::Type *Ty, unsigned Alignment) {
+ llvm::Align DLAlign = DL.getABITypeAlign(Ty);
+ llvm::Align ClangAlign(Alignment / 8);
+ if (DLAlign != ClangAlign) {
+ llvm::errs() << "For target " << Triple.str() << " type " << Name
+ << " mapping to " << *Ty << " has data layout alignment "
+ << DLAlign.value() << " while clang specifies "
+ << ClangAlign.value() << "\n";
+ abort();
+ }
+ };
+
+ Check("bool", llvm::Type::getIntNTy(Context, Target.BoolWidth),
+ Target.BoolAlign);
+ Check("short", llvm::Type::getIntNTy(Context, Target.ShortWidth),
+ Target.ShortAlign);
+ // FIXME: M68k specifies incorrect wrong int and long alignments in Clang
+ // and incorrect long long alignment in both LLVM and Clang.
+ if (Triple.getArch() != llvm::Triple::m68k) {
+ Check("int", llvm::Type::getIntNTy(Context, Target.IntWidth),
+ Target.IntAlign);
+ Check("long", llvm::Type::getIntNTy(Context, Target.LongWidth),
+ Target.LongAlign);
+ Check("long long", llvm::Type::getIntNTy(Context, Target.LongLongWidth),
+ Target.LongLongAlign);
+ }
+ // FIXME: There are int128 alignment mismatches on multiple targets.
+ if (Target.hasInt128Type() && !Target.getTargetOpts().ForceEnableInt128 &&
+ !Triple.isAMDGPU() && !Triple.isSPIRV() &&
+ Triple.getArch() != llvm::Triple::ve)
+ Check("__int128", llvm::Type::getIntNTy(Context, 128), Target.Int128Align);
+
+ if (Target.hasFloat16Type())
+ Check("half", llvm::Type::getFloatingPointTy(Context, *Target.HalfFormat),
+ Target.HalfAlign);
+ if (Target.hasBFloat16Type())
+ Check("bfloat", llvm::Type::getBFloatTy(Context), Target.BFloat16Align);
+ Check("float", llvm::Type::getFloatingPointTy(Context, *Target.FloatFormat),
+ Target.FloatAlign);
+ // FIXME: AIX specifies wrong double alignment in DataLayout
+ if (!Triple.isOSAIX()) {
+ Check("double",
+ llvm::Type::getFloatingPointTy(Context, *Target.DoubleFormat),
+ Target.DoubleAlign);
+ Check("long double",
+ llvm::Type::getFloatingPointTy(Context, *Target.LongDoubleFormat),
+ Target.LongDoubleAlign);
+ }
+ // FIXME: Wasm has a mismatch in f128 alignment between Clang and LLVM.
+ if (Target.hasFloat128Type() && !Triple.isWasm())
+ Check("__float128", llvm::Type::getFP128Ty(Context), Target.Float128Align);
+ if (Target.hasIbm128Type())
+ Check("__ibm128", llvm::Type::getPPC_FP128Ty(Context), Target.Ibm128Align);
+
+ // FIXME: Clang specifies incorrect pointer alignment for m68k.
+ if (Triple.getArch() != llvm::Triple::m68k)
+ Check("void*", llvm::PointerType::getUnqual(Context), Target.PointerAlign);
+#endif
+}
+
CodeGenModule::CodeGenModule(ASTContext &C,
IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
const HeaderSearchOptions &HSO,
@@ -458,6 +528,9 @@ CodeGenModule::CodeGenModule(ASTContext &C,
if (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
getModule().addModuleFlag(llvm::Module::Error, "NumRegisterParameters",
CodeGenOpts.NumRegisterParameters);
+
+ if (!Context.getAuxTargetInfo())
+ checkDataLayoutConsistency(Context.getTargetInfo(), LLVMContext, LangOpts);
}
CodeGenModule::~CodeGenModule() {}
|
|
@llvm/pr-subscribers-clang Author: Nikita Popov (nikic) ChangesVerify that the alignments specified by clang TargetInfo match the alignments specified by LLVM data layout, which will hopefully prevent accidental mismatches in the future. This currently contains opt-outs for a lot of existing mismatches. I'm also skipping the verification if options like The verification happens in CodeGen, as we can't have an IR dependency in Basic. Full diff: https://github.com/llvm/llvm-project/pull/144720.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index c27168e4c4bfe..aabc872e22df1 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -332,6 +332,76 @@ const TargetCodeGenInfo &CodeGenModule::getTargetCodeGenInfo() {
return *TheTargetCodeGenInfo;
}
+static void checkDataLayoutConsistency(const TargetInfo &Target,
+ llvm::LLVMContext &Context,
+ const LangOptions &Opts) {
+#ifndef NDEBUG
+ // Don't verify non-standard ABI configurations.
+ if (Opts.AlignDouble || Opts.OpenCL || Opts.HLSL)
+ return;
+
+ llvm::Triple Triple = Target.getTriple();
+ llvm::DataLayout DL(Target.getDataLayoutString());
+ auto Check = [&](const char *Name, llvm::Type *Ty, unsigned Alignment) {
+ llvm::Align DLAlign = DL.getABITypeAlign(Ty);
+ llvm::Align ClangAlign(Alignment / 8);
+ if (DLAlign != ClangAlign) {
+ llvm::errs() << "For target " << Triple.str() << " type " << Name
+ << " mapping to " << *Ty << " has data layout alignment "
+ << DLAlign.value() << " while clang specifies "
+ << ClangAlign.value() << "\n";
+ abort();
+ }
+ };
+
+ Check("bool", llvm::Type::getIntNTy(Context, Target.BoolWidth),
+ Target.BoolAlign);
+ Check("short", llvm::Type::getIntNTy(Context, Target.ShortWidth),
+ Target.ShortAlign);
+ // FIXME: M68k specifies incorrect wrong int and long alignments in Clang
+ // and incorrect long long alignment in both LLVM and Clang.
+ if (Triple.getArch() != llvm::Triple::m68k) {
+ Check("int", llvm::Type::getIntNTy(Context, Target.IntWidth),
+ Target.IntAlign);
+ Check("long", llvm::Type::getIntNTy(Context, Target.LongWidth),
+ Target.LongAlign);
+ Check("long long", llvm::Type::getIntNTy(Context, Target.LongLongWidth),
+ Target.LongLongAlign);
+ }
+ // FIXME: There are int128 alignment mismatches on multiple targets.
+ if (Target.hasInt128Type() && !Target.getTargetOpts().ForceEnableInt128 &&
+ !Triple.isAMDGPU() && !Triple.isSPIRV() &&
+ Triple.getArch() != llvm::Triple::ve)
+ Check("__int128", llvm::Type::getIntNTy(Context, 128), Target.Int128Align);
+
+ if (Target.hasFloat16Type())
+ Check("half", llvm::Type::getFloatingPointTy(Context, *Target.HalfFormat),
+ Target.HalfAlign);
+ if (Target.hasBFloat16Type())
+ Check("bfloat", llvm::Type::getBFloatTy(Context), Target.BFloat16Align);
+ Check("float", llvm::Type::getFloatingPointTy(Context, *Target.FloatFormat),
+ Target.FloatAlign);
+ // FIXME: AIX specifies wrong double alignment in DataLayout
+ if (!Triple.isOSAIX()) {
+ Check("double",
+ llvm::Type::getFloatingPointTy(Context, *Target.DoubleFormat),
+ Target.DoubleAlign);
+ Check("long double",
+ llvm::Type::getFloatingPointTy(Context, *Target.LongDoubleFormat),
+ Target.LongDoubleAlign);
+ }
+ // FIXME: Wasm has a mismatch in f128 alignment between Clang and LLVM.
+ if (Target.hasFloat128Type() && !Triple.isWasm())
+ Check("__float128", llvm::Type::getFP128Ty(Context), Target.Float128Align);
+ if (Target.hasIbm128Type())
+ Check("__ibm128", llvm::Type::getPPC_FP128Ty(Context), Target.Ibm128Align);
+
+ // FIXME: Clang specifies incorrect pointer alignment for m68k.
+ if (Triple.getArch() != llvm::Triple::m68k)
+ Check("void*", llvm::PointerType::getUnqual(Context), Target.PointerAlign);
+#endif
+}
+
CodeGenModule::CodeGenModule(ASTContext &C,
IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
const HeaderSearchOptions &HSO,
@@ -458,6 +528,9 @@ CodeGenModule::CodeGenModule(ASTContext &C,
if (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
getModule().addModuleFlag(llvm::Module::Error, "NumRegisterParameters",
CodeGenOpts.NumRegisterParameters);
+
+ if (!Context.getAuxTargetInfo())
+ checkDataLayoutConsistency(Context.getTargetInfo(), LLVMContext, LangOpts);
}
CodeGenModule::~CodeGenModule() {}
|
This was referenced Jun 18, 2025
|
So, devil's advocate: do we actually care about IR data layout alignments? Clang should be putting explicit alignments on everything in the IR. And, I mean, I can certainly imagine that a target might end up in an impossible situation for terrible historical reasons, because e.g. it has an |
|
@rjmccall From a Clang perspective, largely no -- but not entirely. There are cases when LLVM materializes allocations based on the data layout in ways that are observable and not controllable by Clang. One of these is certain inline asm constraints (which can end up passing constant pool addresses that don't satisfy alignment requirements) and another is certain edge cases around libcall lowering (libcalls that result in sret demotion, which is why clang used to miscompile f128 libcalls). From an LLVM perspective, we do care because non-Clang frontends use LLVM's data layout as their source of truth. For most targets, the information there is sufficient -- as long as it's actually correct. It seems that sometimes Clang developers also expect that the data layout is the source of truth, which is how we end up with situations like m68k forgetting to specify Clang-side alignments entirely (#144740). |
|
Okay. I'm not really opposed to trying to make this accurate, even if it were purely on general principle. I have to say that things like inline asm constraints and libcall lowering sound like gaps that we should be fixing to allow an explicit alignment, though. |
nikic
force-pushed
the
clang-data-layout-verify
branch
from
c4082c4 to
4a70541
Compare
|
Ping :) |
efriedma-quic
approved these changes
Jun 27, 2025
nikic
added a commit
that referenced
this pull request
Jul 1, 2025
As the data layout a few lines further up specifies, the int, long and pointer alignment should be 16 instead of the default of 32. The long long alignment is also incorrect, but that would require a change to the data layout as well. Comparison with GCC, which consistently uses 2 byte alignment: https://gcc.godbolt.org/z/K3x6a7dEf At least based on some spot checks, the changes to bit field layout also make use match GCC now. This was found by #144720.
Verify that the alignments specified by clang TargetInfo match the alignments specified by LLVM data layout, which will hopefully prevent accidental mismatches in the future. This currently contains opt-outs for a lot of existing mismatches. I'm also skipping the verification if options like `-malign-double` are used, or a language that mandates sizes/alignments that differ from C.
nikic
force-pushed
the
clang-data-layout-verify
branch
from
4a70541 to
e0b1d80
Compare
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 1, 2025
As the data layout a few lines further up specifies, the int, long and pointer alignment should be 16 instead of the default of 32. The long long alignment is also incorrect, but that would require a change to the data layout as well. Comparison with GCC, which consistently uses 2 byte alignment: https://gcc.godbolt.org/z/K3x6a7dEf At least based on some spot checks, the changes to bit field layout also make use match GCC now. This was found by llvm/llvm-project#144720.
|
I've put up #146494 for the wasm f128 alignment. |
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
As the data layout a few lines further up specifies, the int, long and pointer alignment should be 16 instead of the default of 32. The long long alignment is also incorrect, but that would require a change to the data layout as well. Comparison with GCC, which consistently uses 2 byte alignment: https://gcc.godbolt.org/z/K3x6a7dEf At least based on some spot checks, the changes to bit field layout also make use match GCC now. This was found by llvm#144720.
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
Verify that the alignments specified by clang TargetInfo match the alignments specified by LLVM data layout, which will hopefully prevent accidental mismatches in the future. This currently contains opt-outs for a number of of existing mismatches. I'm also skipping the verification if options like `-malign-double` are used, or a language that mandates sizes/alignments that differ from C. The verification happens in CodeGen, as we can't have an IR dependency in Basic.
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
As the data layout a few lines further up specifies, the int, long and pointer alignment should be 16 instead of the default of 32. The long long alignment is also incorrect, but that would require a change to the data layout as well. Comparison with GCC, which consistently uses 2 byte alignment: https://gcc.godbolt.org/z/K3x6a7dEf At least based on some spot checks, the changes to bit field layout also make use match GCC now. This was found by llvm#144720.
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
Verify that the alignments specified by clang TargetInfo match the alignments specified by LLVM data layout, which will hopefully prevent accidental mismatches in the future. This currently contains opt-outs for a number of of existing mismatches. I'm also skipping the verification if options like `-malign-double` are used, or a language that mandates sizes/alignments that differ from C. The verification happens in CodeGen, as we can't have an IR dependency in Basic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Verify that the alignments specified by clang TargetInfo match the alignments specified by LLVM data layout, which will hopefully prevent accidental mismatches in the future.
This currently contains opt-outs for a lot of existing mismatches.
I'm also skipping the verification if options like
-malign-doubleare used, or a language that mandates sizes/alignments that differ from C.The verification happens in CodeGen, as we can't have an IR dependency in Basic.