uweigand marked 6 inline comments as done.
uweigand added inline comments.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:521
+ // [[no_unique_address]] attribute (since C++20). Those do count
+ // as empty according to the Itanium ABI. This property is currently
+ // only respected if the AllowNoUniqueAddr parameter is true.
----------------
hubert.reinterpretcast wrote:
> This check is being done after removal of the array types by `AllowArrays`,
> so this code is also conferring the property of being empty to arrays. It
> seems GCC erroneously does the same for base class fields (but not for direct
> members).
>
> ```
> struct Empty {};
>
> struct A {
> Empty emp [[no_unique_address]][3];
> };
>
> struct B : A {
> float f;
> };
>
> struct C {
> Empty emp [[no_unique_address]][3];
> float f;
> };
>
> extern char szb[sizeof(B)];
> extern char szb[sizeof(float)]; // GCC likes this
> extern char szc[sizeof(C)];
> extern char szc[sizeof(float)]; // GCC does not like this
> ```
>
> Compiler Explorer link: https://godbolt.org/z/NFuca9
This version should fix clang; I agree that GCC still gets this wrong.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:524
+ if (isa<CXXRecordDecl>(RT->getDecl()) &&
+ !(AllowNoUniqueAddr && FD->hasAttr<NoUniqueAddressAttr>()))
return false;
----------------
efriedma wrote:
> Does this do the right thing with a field that's an array of empty classes?
You're right. As Hubert notes, arrays of empty classes do not count as empty.
This version should fix the problem.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7245
// do count. So do anonymous bitfields that aren't zero-sized.
- if (getContext().getLangOpts().CPlusPlus &&
- FD->isZeroLengthBitField(getContext()))
- continue;
+ if (getContext().getLangOpts().CPlusPlus) {
+ if (FD->isZeroLengthBitField(getContext()))
----------------
efriedma wrote:
> Only loosely relevant to this patch, but checking getLangOpts().CPlusPlus
> here seems weird; doesn't that break calling functions defined in C from C++
> code?
I agree that this difference between C and C++ is weird, but it does match the
behavior of GCC. (Which is itself weird, but a long-standing accident that we
probably cannot fix without breaking existing code at this point.)
Now, you bring up an interesting point: When C++ code calls a function defined
in C code, the C++ part would have to refer to an `extern "C"` declaration.
The correct thing to do would probably be to handle those according to the C
ABI rules, not the C++ rules, in this case where the two differ. But currently
GCC doesn't do that either. (But since that would be broken anyway, I think we
**can** fix that.) In any case, I agree that this is really a separate
problem, distinct from this patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81583/new/
https://reviews.llvm.org/D81583
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
