[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[Ulrich Weigand via Phabricator via cfe-commits]

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