fix: C++ empty record with align lead to va_list out of sync

[hstk30-hw]

About #69872 , just for compatible C++ empty record with align UB with gcc

https://godbolt.org/z/qsze8fqra

rel commit 1711cc9

[llvmbot]

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (hstk30-hw)

Changes

About #69872 , just for compatible C++ empty record with align UB with gcc

https://godbolt.org/z/qsze8fqra

---

Full diff: https://github.com/llvm/llvm-project/pull/72197.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/ABIInfoImpl.cpp (+7-1)
• (modified) clang/test/CodeGen/aarch64-args.cpp (+6)

diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp b/clang/lib/CodeGen/ABIInfoImpl.cpp index 2b20d5a13346d34..bc8ac937be37399 100644 --- a/clang/lib/CodeGen/ABIInfoImpl.cpp +++ b/clang/lib/CodeGen/ABIInfoImpl.cpp @@ -296,10 +296,16 @@ bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, return false; // If this is a C++ record, check the bases first. - if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) + if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) { for (const auto &I : CXXRD->bases()) if (!isEmptyRecord(Context, I.getType(), true, AsIfNoUniqueAddr)) return false; + // C++ object size >= 1 byte, empty struct is 1 byte. + // FIXME: an alignment on a empty record is a UB, may just warning it， + // this code just want to compatible gcc. + if (Context.getTypeSize(T) > 8) + return false; + } for (const auto *I : RD->fields()) if (!isEmptyField(Context, I, AllowArrays, AsIfNoUniqueAddr)) diff --git a/clang/test/CodeGen/aarch64-args.cpp b/clang/test/CodeGen/aarch64-args.cpp index fe1298cc683a404..4794f0ae7c903dc 100644 --- a/clang/test/CodeGen/aarch64-args.cpp +++ b/clang/test/CodeGen/aarch64-args.cpp @@ -65,3 +65,9 @@ EXTERNC struct SortOfEmpty sort_of_empty_ret(void) { struct SortOfEmpty e; return e; } + +// CHECK-GNU-CXX: define{{.*}} i32 @empty_align_arg(i128 %a.coerce, i32 noundef %b) +struct EmptyAlign { long long int __attribute__((aligned)) : 0; }; +EXTERNC int empty_align_arg(struct EmptyAlign a, int b) { + return b; +} \ No newline at end of file 

[hstk30-hw]

@AaronBallman @erichkeane Check it plz.

[AaronBallman]

Thank you for this! Please be sure to add a release note to clang/docs/ReleaseNotes.rst as well so users know about the change in behavior.

[rjmccall]

First off, the change here actually applies to all over-aligned empty structs, not just to those with aligned bit-fields. Maybe we can say that aligned zero-width bit-fields are ill-formed, but I don't think we can say that all aligned empty classes are ill-formed, among other things because I'm pretty sure they're explicitly allowed in C++. So let's just set the aligned bit-field question aside for a moment.

Second, I don't understand why the fix is to isEmptyRecord. This is a very general routine that affects a lot of code in a lot of targets. And for most of those purposes, I think an aligned empty class should still be considered an empty record. I think we need to understand why this is fixing the bug.

Third, it does look like we have a real ABI bug here. I haven't looked at other targets, but at least on AArch64, a 16-byte-aligned empty class should definitely be taking up two registers. For some reason, we are generating IR to pass this class as a single byte rather than as a number of bytes equal to its size.

Finally, as usual, some platforms may want to opt out of this ABI break; I'll ask around at Apple.

[rjmccall]

Oh, the Apple AArch64 calling convention diverges from AAPCS and ignores empty classes as parameters. We appear to consider this an empty class regardless of alignment, which as discussed I believe is correct. So Apple does not want this change on either level: we do not want our ABI to change, and we should not be considering this an empty class.

[rjmccall]

Marking as changes requested so that this doesn't get committed.

[hstk30-hw]

This is the code I debug located. Seem the comment is out of date?

https://github.com/llvm/llvm-project/blob/1b82cc1186b8bf716205c9b86b851db667792a64/clang/lib/CodeGen/Targets/AArch64.cpp#L298C1-L311C4

This issue 1711cc9 point that when pass the empty class the va_list will get out of sync.

we should not be considering this an empty class.

It confuse me whether an empty class with alignment (size > 1) is an empty class in C++?

[efriedma-quic]

The proper fix here is probably to just delete the return ABIArgInfo::getDirect(llvm::Type::getInt8Ty(getVMContext())); from the empty struct codepath on aarch64.

Alignment shouldn't affect whether a class is empty. The issue here is just that according to aarch64 AAPCS rules, there isn't supposed to be a special case for empty classes; they're supposed to be passed exactly the same way as non-empty classes. If there's no alignment involved, that's the same as an i8; if there's alignment, though, that increases the size of the struct, and therefore the calling convention. It looks like whoever wrote it wasn't considering that an empty struct can have size greater than one byte if alignment is involved.

The code you noted is supposed to handle two cases, neither of which are relevant to your testcase:

• Darwin-specific calling convention rules.
• GNU extensions for zero-size structs (which aren't allowed according to either C or C++ standards, but GNU invented a bunch of non-standard rules for them)

[rjmccall]

This is the code I debug located. Seem the comment is out of date?

That seems like the right place to fix the issue, specifically the place below where it always passes a single i8. The right rule (edit: as Eli points out) is to just fall through to the normal conventions for passing the argument if we don't decide to ignore it.

The comment isn't wrong, but it could be clearer. Let me suggest this:

AAPCS64 does not say that empty records are ignored as arguments,
but other compilers do so in certain situations, and we copy that behavior.
Those situations are in fact language-mode-specific, which seems really
unfortunate, but it's something we just have to accept. If this doesn't
apply, just fall through to the standard argument-handling path.
Darwin overrides the psABI here to ignore all empty records in all modes.

This issue 1711cc9 point that when pass the empty class the va_list will get out of sync.

That seems like something we should fix, yeah. Generally va_list handling needs to do the same kind of classification that argument-passing does.

Edit: sorry about the close/reopen below, I mis-clicked

[efriedma-quic]

Good to see this works without any unexpected side-effects.

Please also add a test for 32-byte alignment (since that generates different code)

[hstk30-hw]

For now,
empty record with size <= 64, still use i8 in IR (before empty record without align always use i8), and lowering to a general purpose register.
empty record with size <= 128, use i128 in IR, and lowering to two gr-registers (C.10).
empty record with size > 128, pass ptr and the val is stored on stack.

Before,
empty record always hold by i8 ， and lowering to a gr-register, so va_arg parse arg error with bad gr_offs or stack offs.

[hstk30-hw]

OK, just delete the

return ABIArgInfo::getDirect(llvm::Type::getInt8Ty(getVMContext()));

from the empty struct codepath, let it fall through to the main path.
Check it again, plz.

[hstk30-hw]

I guess the i8 is from https://github.com/ARM-software/abi-aa/blob/2a70c42d62e9c3eb5887fa50b71257f20daca6f9/cppabi64/cppabi64.rst#41summary-of-differences-from-and-additions-to-the-generic-c-abi

The GC++ABI defines the way in which empty class types are laid out. For the purposes of parameter passing in [AAPCS64], a parameter whose type is an empty class shall be treated as if its type were an aggregate with a single member of type unsigned byte.

and in gcppabi

Arguments of empty class types that are not non-trivial for the purposes of calls are passed no differently from ordinary classes.

I guess I should fix in va_arg instead of classifyArgumentType , right? @rjmccall
(In va_arg， handle empty record consistently like classifyArgumentType)

[efriedma-quic]

LGTM

In terms of va_list etc., we first need to make sure calls are correct (compare against gcc etc.), then we need to make sure va_list is consistent with that.

[efriedma-quic]

Is this patch ready to merge?

[efriedma-quic]

Ping

[erichkeane]

I have no problem with this either, is this ready to merge?

[hstk30-hw]

Sorry for delay. I reread the historical comment and will fix it in the next few days :)

[hstk30-hw]

I think it's ready to merge. Please review it again all.

[hstk30-hw]

ping...
@rjmccall Check it, and remove the requested changes plz.

[hstk30-hw]

Ping

[AaronBallman]

The changes look reasonable to me, but I'd appreciate if @rjmccall or @efriedma-quic could make the final sign-off.

[efriedma-quic]

I'm still fine with this. And I think @rjmccall's concerns were addressed by the latest version.

[efriedma-quic]

Maybe tweak the release note a little; the reference to i8 doesn't really make sense from the perspective of a clang user.

Suggested change

	- Let C++ empty record fall through to the standard argument-handling path instead of
	always pass a single ``i8`` according to aarch64 AAPCS rules.
	- Fix AArch64 argument passing for C++ empty classes with large explicitly specified
	alignment.

[hstk30-hw]

I think this patch had meet @rjmccall 's suggestion. Let us merge it :)