[clang][RISCV] Fix ABI handling of empty structs with hard FP calling conventions in C++

[asb]

Summary

As reported in https://github.com/llvm/llvm-project/issues/58929,
Clang's handling of empty structs in the case of small structs that may
be eligible to be passed using the hard FP calling convention doesn't
match g++. In general, C++ record fields are never empty unless
[[no_unique_address]] is used, but the RISC-V FP ABI overrides this.

After this patch, fields of structs that contain empty records will be
ignored, even in C++, when considering eligibility for the FP calling
convention ('flattening'). It isn't explicitly noted in the RISC-V
psABI, but arrays of empty records will disqualify a struct for
consideration of using the FP calling convention in g++. This patch
matches that behaviour. The psABI issue
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/358 seeks
to clarify this.

[asb]

Removed FIXME missed in initial version.

[luismarques]

LGMT.

clang/lib/CodeGen/TargetInfo.cpp

591

Nit: the !AsIfNoUniqueAddr condition could be tested before the !FD->hasAttr<NoUniqueAddressAttr>() condition, and I imagine that would be slightly cheaper for the AsIfNoUniqueAddr = true case?

[asb]

• Address review comments
• Refresh after adding zero-length array tests (and fix a bug this unearthed).
• Add to release notes

[asb]

Thanks Luis for the quick review. As an important part of this is trying to align with gcc/g++ I'd really appreciate a review from @kito-cheng too if you have the time (thanks in advance!).

[asb]

In D142327#4080044, @asb wrote:

Thanks Luis for the quick review. As an important part of this is trying to align with gcc/g++ I'd really appreciate a review from @kito-cheng too if you have the time (thanks in advance!).

Friendly ping to Kito as I spotted you mentioned you're back from vacation!

[asb]

Friendly ping on this (I think mostly directed at @kito-cheng who was hoping to find time to review the linked abi issue).

[kito-cheng]

LGTM, Verified with GCC and two clang w/ and w/o this patch, clang with this patch is matching GCC's behavior.

Also created an PR on psABI to clarify that: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/365

[luismarques]

clang/test/CodeGen/RISCV/abi-empty-structs.c

21–22

Shouldn't you mention the array exception here? (which you now cover in test_s7 and test_s8).

[rogfer01]

clang/lib/CodeGen/Targets/RISCV.cpp

178 ↗

(On Diff #543432)

I've observed (based on manually added tracing) that we may return true here with Field1Ty == nullptr and Field2Ty == nullptr.

Then RISCVABIInfo::coerceAndExpandFPCCEligibleStruct receives these two types and appends Field1Ty it into UnpaddedCoerceElts and then if Field2Ty == nullptr it calls ABIArgInfo::getCoerceAndExpand (around line 280 of clang/lib/CodeGen/Targets/RISCV.cpp) which then tries to do a dyn_cast on a null value and then an assertion fires (around line 256 of clang/include/clang/CodeGen/CGFunctionInfo.h)

I can reproduce this with the llvm-testsuite. Apologies that I have not managed to create a small reproducer yet.

[asb]

Reopening as I've reverted following Roger's bug report.

clang/lib/CodeGen/Targets/RISCV.cpp

178 ↗

(On Diff #543432)

Thank you, I've reverted until this can be investigated and fixed.

[SixWeining]

clang/lib/CodeGen/Targets/RISCV.cpp

178 ↗

(On Diff #543432)

LoongArch has the same issue and I write a reproducer:

$ cat test.cpp
#include <algorithm>
#include <vector>

static bool check(int i) {
  return i == 2;
}

int main()
{
  std::vector<int> v{1, 2, 3, 2, 2};
  //return std::count_if(v.begin(), v.end(), check); // ok
  return std::count_if(v.begin(), v.end(), [](int i) { return i == 2; }); // error
}

Let's investigate how to fix it.

[asb]

I've updated the patch to fix the reported issue (adding an additional check to detectFPCCEligibleStruct). What I haven't been able to do so far is to extract a test case that shows the issue indicated by @SixWeining without relying on libcxx headers. I may well be missing something obvious here.

[rogfer01]

This comment was removed by rogfer01.

[rogfer01]

I didn't have libcxx handy but @SixWeining testcase also fails with libstdcxx so I manually expanded what libstdcxx does and removed the templates.

With the new change this does not fail anymore.

typedef decltype((int *)2 - (int *)1) my_ptrdiff_t;

struct my_lambda {
  bool operator()(int i) { return i == 2; }
};

struct my_iter_pred {
  my_lambda _M_pred;

  explicit my_iter_pred(my_lambda __pred)
      : _M_pred(/* move */ static_cast<my_lambda &&>(__pred)) {}

  bool operator()(int *__it) { return bool(_M_pred(*__it)); }
};

my_ptrdiff_t __my_count_if(int *__first, int *__last, my_iter_pred __pred) {
  my_ptrdiff_t __n = 0;
  for (; __first != __last; ++__first)
    if (__pred(__first))
      ++__n;
  return __n;
}

inline my_iter_pred my_pred_iter(my_lambda __my_lambda) {
  return my_iter_pred(/* move */ static_cast<my_lambda &&>(__my_lambda));
}

inline my_ptrdiff_t my_count_if(int *__first, int *__last,
                                my_lambda __my_lambda) {
  return __my_count_if(__first, __last, my_pred_iter(__my_lambda));
}

int main() {
  int v[] = {1, 2, 3, 2, 2};
  return my_count_if(v, v + sizeof(v) / sizeof(*v), my_lambda());
}

I tried to remove the my_pred_iter / my_iter_pred wrapper but then the previous change would not fail anymore.

Hope this helps.

[asb]

This update includes test coverage for the bug that was fixed in the reverted version of the patch.

Thanks @rogfer01 for the smaller test case. To my surprise, cvise stripped out almost everything and test_s9 in abi-empty-structs.c reliably caused the assert for me.

Re-review appreciated in order to get this landed and ideally cherry-picked for the 17.x.

[rogfer01]

Thanks for the update @asb. LGTM.

clang/lib/CodeGen/TargetInfo.cpp

 llvm::SyncScope::ID
 TargetCodeGenInfo::getLLVMSyncScopeID(const LangOptions &LangOpts,
                                       SyncScope Scope,
                                       llvm::AtomicOrdering Ordering,
                                       llvm::LLVMContext &Ctx) const {
   return Ctx.getOrInsertSyncScopeID(""); /* default sync scope */
 }
 
-static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays);
+static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
+                          bool AsIfNoUniqueAddr = false);
 
 /// isEmptyField - Return true iff a the field is "empty", that is it
 /// is an unnamed bit-field or an (array of) empty record(s).
 static bool isEmptyField(ASTContext &Context, const FieldDecl *FD,
-                         bool AllowArrays) {
+                         bool AllowArrays, bool AsIfNoUniqueAddr = false) {
   if (FD->isUnnamedBitfield())
     return true;
 
   QualType FT = FD->getType();
 
   // Constant arrays of empty records count as empty, strip them off.
   // Constant arrays of zero length always count as empty.
   bool WasArray = false;
static bool isEmptyField(ASTContext &Context, const FieldDecl *FD,
   // current ABI.
   //
   // The exception to the above rule are fields marked with the
   // [[no_unique_address]] attribute (since C++20).  Those do count as empty
   // according to the Itanium ABI.  The exception applies only to records,
   // not arrays of records, so we must also check whether we stripped off an
   // array type above.
   if (isa<CXXRecordDecl>(RT->getDecl()) &&
-      (WasArray || !FD->hasAttr<NoUniqueAddressAttr>()))
+      (WasArray || (!FD->hasAttr<NoUniqueAddressAttr>() && !AsIfNoUniqueAddr)))
     return false;
 
-  return isEmptyRecord(Context, FT, AllowArrays);
+  return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
 }
 
 /// isEmptyRecord - Return true iff a structure contains only empty
 /// fields. Note that a structure with a flexible array member is not
-/// considered empty.
-static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays) {
+/// considered empty. If AsIfNoUniqueAddr is true, then C++ record fields are
+/// considered empty if the [[no_unique_address]] attribute would have made
+/// them empty.
+static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
+                          bool AsIfNoUniqueAddr) {
   const RecordType *RT = T->getAs<RecordType>();
   if (!RT)
     return false;
   const RecordDecl *RD = RT->getDecl();
   if (RD->hasFlexibleArrayMember())
     return false;
 
   // If this is a C++ record, check the bases first.
   if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD))
     for (const auto &I : CXXRD->bases())
-      if (!isEmptyRecord(Context, I.getType(), true))
+      if (!isEmptyRecord(Context, I.getType(), true, AsIfNoUniqueAddr))
         return false;
 
   for (const auto *I : RD->fields())
-    if (!isEmptyField(Context, I, AllowArrays))
+    if (!isEmptyField(Context, I, AllowArrays, AsIfNoUniqueAddr))
       return false;
   return true;
 }
 
 /// isSingleElementStruct - Determine if a structure is a "single
 /// element struct", i.e. it has exactly one non-empty field or
 /// exactly one field which is itself a single element
 /// struct. Structures with flexible array members are never
if (auto CTy = Ty->getAs<ComplexType>()) {
     Field2Ty = Field1Ty;
     Field2Off = Field1Off + getContext().getTypeSizeInChars(EltTy);
     return true;
   }
 
   if (const ConstantArrayType *ATy = getContext().getAsConstantArrayType(Ty)) {
     uint64_t ArraySize = ATy->getSize().getZExtValue();
     QualType EltTy = ATy->getElementType();
+    // Arrays of empty records make the struct ineligible for the FP calling
+    // convention in C++.
+    if (const auto *RTy = EltTy->getAs<RecordType>()) {
+      if (isa<CXXRecordDecl>(RTy->getDecl()) &&
+          isEmptyRecord(getContext(), EltTy, true, true))
+        return false;
+    }
     CharUnits EltSize = getContext().getTypeSizeInChars(EltTy);
     for (uint64_t i = 0; i < ArraySize; ++i) {
       bool Ret = detectFPCCEligibleStructHelper(EltTy, CurOff, Field1Ty,
                                                 Field1Off, Field2Ty, Field2Off);
       if (!Ret)
         return false;
       CurOff += EltSize;
     }
     return true;
   }
 
   if (const auto *RTy = Ty->getAs<RecordType>()) {
     // Structures with either a non-trivial destructor or a non-trivial
     // copy constructor are not eligible for the FP calling convention.
     if (getRecordArgABI(Ty, CGT.getCXXABI()))
       return false;
-    if (isEmptyRecord(getContext(), Ty, true))
+    if (isEmptyRecord(getContext(), Ty, true, true))
       return true;
     const RecordDecl *RD = RTy->getDecl();
     // Unions aren't eligible unless they're empty (which is caught above).
     if (RD->isUnion())
       return false;
     const ASTRecordLayout &Layout = getContext().getASTRecordLayout(RD);
     // If this is a C++ record, check the bases first.
     if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {

clang/test/CodeGen/RISCV/abi-empty-structs.c

 // RUN:   | FileCheck -check-prefixes=CHECK-CXX,CHECK32-CXX
 // RUN: %clang_cc1 -x c++ -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-CXX,CHECK64-CXX %s
 // RUN: %clang_cc1 -x c++ -triple riscv64 -target-feature +f -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-CXX,CHECK64-CXX
 
 #include <stdint.h>
 
 // Fields containing empty structs or unions are ignored when flattening
 // structs for the hard FP ABIs, even in C++.
-// FIXME: This isn't currently respected.
 
 struct empty { struct { struct { } e; }; };
 struct s1 { struct empty e; float f; };
 
 // CHECK-C-LABEL: define dso_local float @test_s1
 // CHECK-C-SAME: (float [[TMP0:%.*]]) #[[ATTR0:[0-9]+]] {
 // CHECK-C:  entry:
 //
-// CHECK32-CXX-LABEL: define dso_local [2 x i32] @_Z7test_s12s1
-// CHECK32-CXX-SAME: ([2 x i32] [[A_COERCE:%.*]]) #[[ATTR0:[0-9]+]] {
-// CHECK32-CXX:  entry:
-//
-// CHECK64-CXX-LABEL: define dso_local i64 @_Z7test_s12s1
-// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0:[0-9]+]] {
-// CHECK64-CXX:  entry:
+// CHECK-CXX-LABEL: define dso_local float @_Z7test_s12s1
+// CHECK-CXX-SAME: (float [[TMP0:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-CXX:  entry:
 //
 struct s1 test_s1(struct s1 a) {
   return a;
 }
 
 struct s2 { struct empty e; int32_t i; float f; };
 
 // CHECK-C-LABEL: define dso_local { i32, float } @test_s2
 // CHECK-C-SAME: (i32 [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] {
 // CHECK-C:  entry:
 //
-// CHECK32-CXX-LABEL: define dso_local void @_Z7test_s22s2
-// CHECK32-CXX-SAME: (ptr noalias sret([[STRUCT_S2:%.*]]) align 4 [[AGG_RESULT:%.*]], ptr noundef [[A:%.*]]) #[[ATTR0]] {
-// CHECK32-CXX:  entry:
-//
-// CHECK64-CXX-LABEL: define dso_local [2 x i64] @_Z7test_s22s2
-// CHECK64-CXX-SAME: ([2 x i64] [[A_COERCE:%.*]]) #[[ATTR0]] {
-// CHECK64-CXX:  entry:
+// CHECK-CXX-LABEL: define dso_local { i32, float } @_Z7test_s22s2
+// CHECK-CXX-SAME: (i32 [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] {
+// CHECK-CXX:  entry:
 //
 struct s2 test_s2(struct s2 a) {
   return a;
 }
 
 struct s3 { struct empty e; float f; float g; };
 
 // CHECK-C-LABEL: define dso_local { float, float } @test_s3
 // CHECK-C-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] {
 // CHECK-C:  entry:
 //
-// CHECK32-CXX-LABEL: define dso_local void @_Z7test_s32s3
-// CHECK32-CXX-SAME: (ptr noalias sret([[STRUCT_S3:%.*]]) align 4 [[AGG_RESULT:%.*]], ptr noundef [[A:%.*]]) #[[ATTR0]] {
-// CHECK32-CXX:  entry:
-//
-// CHECK64-CXX-LABEL: define dso_local [2 x i64] @_Z7test_s32s3
-// CHECK64-CXX-SAME: ([2 x i64] [[A_COERCE:%.*]]) #[[ATTR0]] {
-// CHECK64-CXX:  entry:
+// CHECK-CXX-LABEL: define dso_local { float, float } @_Z7test_s32s3
+// CHECK-CXX-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] {
+// CHECK-CXX:  entry:
 //
 struct s3 test_s3(struct s3 a) {
   return a;
 }
 
 struct s4 { struct empty e; float __complex__ c; };
 
 // CHECK-C-LABEL: define dso_local { float, float } @test_s4
 // CHECK-C-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] {
 // CHECK-C:  entry:
 //
-// CHECK32-CXX-LABEL: define dso_local void @_Z7test_s42s4
-// CHECK32-CXX-SAME: (ptr noalias sret([[STRUCT_S4:%.*]]) align 4 [[AGG_RESULT:%.*]], ptr noundef [[A:%.*]]) #[[ATTR0]] {
-// CHECK32-CXX:  entry:
-//
-// CHECK64-CXX-LABEL: define dso_local [2 x i64] @_Z7test_s42s4
-// CHECK64-CXX-SAME: ([2 x i64] [[A_COERCE:%.*]]) #[[ATTR0]] {
-// CHECK64-CXX:  entry:
+// CHECK-CXX-LABEL: define dso_local { float, float } @_Z7test_s42s4
+// CHECK-CXX-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] {
+// CHECK-CXX:  entry:
 //
 struct s4 test_s4(struct s4 a) {
   return a;
 }
 
 // An array of empty fields isn't ignored in C++ (this isn't explicit in the
 // psABI, but matches observed g++ behaviour).
 

 // CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0]] {
 // CHECK64-CXX:  entry:
 //
 struct s6 test_s6(struct s6 a) {
   return a;
 }
 
 //// NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
-// CHECK-CXX: {{.*}}
 // CHECK32-C: {{.*}}
 // CHECK64-C: {{.*}}