Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LoongArch] Fix ABI mismatch with gcc/g++ about empty structs passing #70320

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

SixWeining
Copy link
Contributor

How empty structs (not as fields of container struct) are passed in C++ is not explicitly documented in psABI. However, this patch fixes the mismatch with g++.

Note that the unnamed bitfield case struct { int : 1; } in C is also fixed. Previously clang regards it as an empty struct and then ignores it when passing. Now size of the struct is counted; since it's size is not 0, clang will not ignore it even in C.

While https://reviews.llvm.org/D156116 fixed the handling of empty struct when considering eligibility of the container struct for the FP calling convention ('flattening'), this patch fixes the handling of passing the empty struct itself.

Fix #70319

How empty structs (not as fields of container struct) are passed in C++
is not explicitly documented in psABI. However, this patch fixes the
mismatch with g++.

Note that the unnamed bitfield case `struct { int : 1; }` in C is also
fixed. Previously clang regards it as an empty struct and then ignores
it when passing. Now size of the struct is counted; since it's size is
not 0, clang will not ignore it even in C.

While https://reviews.llvm.org/D156116 fixed the handling of empty
struct when considering eligibility of the container struct for the FP
calling convention ('flattening'), this patch fixes the handling of
passing the empty struct itself.

Fix llvm#70319
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen backend:loongarch labels Oct 26, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 26, 2023

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

@llvm/pr-subscribers-clang-codegen

Author: Lu Weining (SixWeining)

Changes

How empty structs (not as fields of container struct) are passed in C++ is not explicitly documented in psABI. However, this patch fixes the mismatch with g++.

Note that the unnamed bitfield case struct { int : 1; } in C is also fixed. Previously clang regards it as an empty struct and then ignores it when passing. Now size of the struct is counted; since it's size is not 0, clang will not ignore it even in C.

While https://reviews.llvm.org/D156116 fixed the handling of empty struct when considering eligibility of the container struct for the FP calling convention ('flattening'), this patch fixes the handling of passing the empty struct itself.

Fix #70319


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/Targets/LoongArch.cpp (+6-4)
  • (modified) clang/test/CodeGen/LoongArch/abi-lp64d-empty-structs.c (+4-4)
diff --git a/clang/lib/CodeGen/Targets/LoongArch.cpp b/clang/lib/CodeGen/Targets/LoongArch.cpp
index 7483bf6d6d1e8e2..bc508a99da9cedf 100644
--- a/clang/lib/CodeGen/Targets/LoongArch.cpp
+++ b/clang/lib/CodeGen/Targets/LoongArch.cpp
@@ -308,12 +308,14 @@ ABIArgInfo LoongArchABIInfo::classifyArgumentType(QualType Ty, bool IsFixed,
                                            CGCXXABI::RAA_DirectInMemory);
   }
 
-  // Ignore empty structs/unions.
-  if (isEmptyRecord(getContext(), Ty, true))
-    return ABIArgInfo::getIgnore();
-
   uint64_t Size = getContext().getTypeSize(Ty);
 
+  // Ignore empty struct or union whose size is zero, e.g. `struct { }` in C or
+  // `struct { int a[0]; }` in C++. In C++, `struct { }` is empty but it's size
+  // is 1 byte and g++ doesn't ignore it; clang++ matches this behaviour.
+  if (isEmptyRecord(getContext(), Ty, true) && Size == 0)
+    return ABIArgInfo::getIgnore();
+
   // Pass floating point values via FARs if possible.
   if (IsFixed && Ty->isFloatingType() && !Ty->isComplexType() &&
       FRLen >= Size && FARsLeft) {
diff --git a/clang/test/CodeGen/LoongArch/abi-lp64d-empty-structs.c b/clang/test/CodeGen/LoongArch/abi-lp64d-empty-structs.c
index d0daafac336ec0c..281b7b15841a999 100644
--- a/clang/test/CodeGen/LoongArch/abi-lp64d-empty-structs.c
+++ b/clang/test/CodeGen/LoongArch/abi-lp64d-empty-structs.c
@@ -93,7 +93,7 @@ struct s9 test_s9(struct s9 a) {
 }
 
 // CHECK-C: define{{.*}} void @test_s10()
-// CHECK-CXX: define{{.*}} void @_Z8test_s103s10()
+// CHECK-CXX: define{{.*}} i64 @_Z8test_s103s10(i64 {{.*}})
 struct s10 { };
 struct s10 test_s10(struct s10 a) {
   return a;
@@ -128,14 +128,14 @@ struct s14 test_s14(struct s14 a) {
 }
 
 // CHECK-C: define{{.*}} void @test_s15()
-// CHECK-CXX: define{{.*}} void @_Z8test_s153s15()
+// CHECK-CXX: define{{.*}} i64 @_Z8test_s153s15(i64 {{.*}})
 struct s15 { int : 0; };
 struct s15 test_s15(struct s15 a) {
   return a;
 }
 
-// CHECK-C: define{{.*}} void @test_s16()
-// CHECK-CXX: define{{.*}} void @_Z8test_s163s16()
+// CHECK-C: define{{.*}} i64 @test_s16(i64 {{.*}})
+// CHECK-CXX: define{{.*}} i64 @_Z8test_s163s16(i64 {{.*}})
 struct s16 { int : 1; };
 struct s16 test_s16(struct s16 a) {
   return a;

@SixWeining
Copy link
Contributor Author

Hi @xen0n @xry111, what do you think about this change? If it is ok, I'd like to checrry-pick to release/17.x (hope it's not too late...).

@xry111
Copy link
Contributor

xry111 commented Oct 30, 2023

LGTM. I've no permission to make a formal ("GitHub style") approval.

@SixWeining
Copy link
Contributor Author

LGTM. I've no permission to make a formal ("GitHub style") approval.

Thanks. You can request to join the llvm organization via https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access firstly. Then you can make formal ("Github style") approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:loongarch clang:codegen clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LoongArch] An error when cross using clang++ and g++
4 participants