[X86_64] fix empty structure vaarg in c++ #77907

hstk30-hw · 2024-01-12T11:06:27Z

SizeInBytes of empty structure is 0 in C, while 1 in C++. And empty structure argument of the function is ignored in X86_64 backend.As a result, the value of variable arguments in C++ is incorrect. fix #77036

llvmbot · 2024-01-12T11:06:57Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-backend-x86

Author: None (hstk30-hw)

Changes

SizeInBytes of empty structure is 0 in C, while 1 in C++. And empty structure argument of the function is ignored in X86_64 backend.As a result, the value of variable arguments in C++ is incorrect. fix #77036

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

2 Files Affected:

(modified) clang/lib/CodeGen/Targets/X86.cpp (+5)
(added) clang/test/CodeGen/X86/x86_64-vaarg.c (+15)

diff --git a/clang/lib/CodeGen/Targets/X86.cpp b/clang/lib/CodeGen/Targets/X86.cpp
index d053f41ab168f5..c2c11280838b12 100644
--- a/clang/lib/CodeGen/Targets/X86.cpp
+++ b/clang/lib/CodeGen/Targets/X86.cpp
@@ -2989,6 +2989,11 @@ static Address EmitX86_64VAArgFromMemory(CodeGenFunction &CGF,
   // an 8 byte boundary.
 
   uint64_t SizeInBytes = (CGF.getContext().getTypeSize(Ty) + 7) / 8;
+
+  if (isEmptyRecord(CGF.getContext(), Ty, true)) {
+    SizeInBytes = 0;  
+  }
+
   llvm::Value *Offset =
       llvm::ConstantInt::get(CGF.Int32Ty, (SizeInBytes + 7)  & ~7);
   overflow_arg_area = CGF.Builder.CreateGEP(CGF.Int8Ty, overflow_arg_area,
diff --git a/clang/test/CodeGen/X86/x86_64-vaarg.c b/clang/test/CodeGen/X86/x86_64-vaarg.c
new file mode 100644
index 00000000000000..ae201a2d3f3017
--- /dev/null
+++ b/clang/test/CodeGen/X86/x86_64-vaarg.c
@@ -0,0 +1,15 @@
+// RUN: %clang -xc++ -target x86_64-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+    // CHECK:      %overflow_arg_area = load ptr, ptr %overflow_arg_area_p, align 8
+    // CHECK-NEXT: %overflow_arg_area.next = getelementptr i8, ptr %overflow_arg_area, i32 0
+    // CHECK-NEXT: store ptr %overflow_arg_area.next, ptr %overflow_arg_area_p, align 8
+    __builtin_va_list l;
+    __builtin_va_start(l, a);
+    emptyvar = __builtin_va_arg(l, struct Empty);
+    __builtin_va_end(l);
+}

llvmbot · 2024-01-12T11:06:57Z

@llvm/pr-subscribers-clang

Author: None (hstk30-hw)

Changes

SizeInBytes of empty structure is 0 in C, while 1 in C++. And empty structure argument of the function is ignored in X86_64 backend.As a result, the value of variable arguments in C++ is incorrect. fix #77036

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

2 Files Affected:

(modified) clang/lib/CodeGen/Targets/X86.cpp (+5)
(added) clang/test/CodeGen/X86/x86_64-vaarg.c (+15)

diff --git a/clang/lib/CodeGen/Targets/X86.cpp b/clang/lib/CodeGen/Targets/X86.cpp
index d053f41ab168f5..c2c11280838b12 100644
--- a/clang/lib/CodeGen/Targets/X86.cpp
+++ b/clang/lib/CodeGen/Targets/X86.cpp
@@ -2989,6 +2989,11 @@ static Address EmitX86_64VAArgFromMemory(CodeGenFunction &CGF,
   // an 8 byte boundary.
 
   uint64_t SizeInBytes = (CGF.getContext().getTypeSize(Ty) + 7) / 8;
+
+  if (isEmptyRecord(CGF.getContext(), Ty, true)) {
+    SizeInBytes = 0;  
+  }
+
   llvm::Value *Offset =
       llvm::ConstantInt::get(CGF.Int32Ty, (SizeInBytes + 7)  & ~7);
   overflow_arg_area = CGF.Builder.CreateGEP(CGF.Int8Ty, overflow_arg_area,
diff --git a/clang/test/CodeGen/X86/x86_64-vaarg.c b/clang/test/CodeGen/X86/x86_64-vaarg.c
new file mode 100644
index 00000000000000..ae201a2d3f3017
--- /dev/null
+++ b/clang/test/CodeGen/X86/x86_64-vaarg.c
@@ -0,0 +1,15 @@
+// RUN: %clang -xc++ -target x86_64-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+    // CHECK:      %overflow_arg_area = load ptr, ptr %overflow_arg_area_p, align 8
+    // CHECK-NEXT: %overflow_arg_area.next = getelementptr i8, ptr %overflow_arg_area, i32 0
+    // CHECK-NEXT: store ptr %overflow_arg_area.next, ptr %overflow_arg_area_p, align 8
+    __builtin_va_list l;
+    __builtin_va_start(l, a);
+    emptyvar = __builtin_va_arg(l, struct Empty);
+    __builtin_va_end(l);
+}

github-actions · 2024-01-12T11:08:44Z

✅ With the latest revision this PR passed the C/C++ code formatter.

phoebewang · 2024-01-13T08:32:30Z

clang/lib/CodeGen/Targets/X86.cpp

  uint64_t SizeInBytes = (CGF.getContext().getTypeSize(Ty) + 7) / 8;
+
+  if (isEmptyRecord(CGF.getContext(), Ty, true)) {
+    SizeInBytes = 0;
+  }
+


Suggestion:

uint64_t SizeInBytes = 0; if (!isEmptyRecord(CGF.getContext(), Ty, true)) SizeInBytes = (CGF.getContext().getTypeSize(Ty) + 7) / 8;

phoebewang · 2024-01-13T10:49:25Z

I checked it locally, the patch doesn't fix the reported problem:

$ clang pr77036.cpp && ./a.out
-nan
Fail

CoTinker · 2024-01-15T02:21:38Z

I checked it locally, the patch doesn't fix the reported problem:
$ clang pr77036.cpp && ./a.out
-nan
Fail

It seems that struct S14{} is empty record, but struct S14 { union{}a;} is not.

CoTinker · 2024-01-17T04:09:00Z

I've revised it. Can you check it again? @phoebewang

phoebewang · 2024-01-17T05:50:38Z

I've revised it. Can you check it again? @phoebewang

I'm not sure the usage of the isEmptyRecord. Tagging @asb who modified the interface recently.

efriedma-quic · 2024-01-17T22:52:17Z

clang/lib/CodeGen/Targets/X86.cpp

-  uint64_t SizeInBytes = (CGF.getContext().getTypeSize(Ty) + 7) / 8;
+  uint64_t SizeInBytes = 0;
+  if (!isEmptyRecord(CGF.getContext(), Ty, true, true))
+    SizeInBytes = (CGF.getContext().getTypeSize(Ty) + 7) / 8;


Using isEmptyRecord() like this doesn't seem right. I mean, it's probably pretty close to being correct in most cases, but I suspect there are edge cases where it does something slightly different.

Also, as far as I can tell, we aren't supposed to realign the va_list if an argument is empty (which could matter for an empty struct with an alignment attribute).

I think we should be using the result of classifyArgumentType() here to check what to do here. If the ABIArgInfo is "ignore", the argument isn't passed at all, so we shouldn't access the va_list. We should just make a temporary and return its address.

CoTinker · 2024-03-05T07:01:33Z

ping~

efriedma-quic · 2024-03-05T08:29:55Z

clang/lib/CodeGen/Targets/X86.cpp

+    llvm::Value *Load = CGF.Builder.CreateLoad(VAListAddr);
+    llvm::Value *Offset = llvm::ConstantInt::get(CGF.Int32Ty, 0);
+    Load = CGF.Builder.CreateGEP(CGF.Int8Ty, Load, Offset);
+    return Address(Load, CGF.ConvertTypeForMem(Ty), Align);


I don't think the address here is guaranteed to be appropriately aligned. It's probably better to create a temporary like I suggested before.

Also, a GEP with offset 0 does nothing.

how to create a temporary?

CGF.CreateMemTemp()

CoTinker · 2024-03-19T06:28:46Z

ping~

phoebewang · 2024-03-19T07:10:49Z

clang/lib/CodeGen/Targets/X86.cpp

+  if (AI.isIgnore()) {
+    return CGF.CreateMemTemp(Ty);
+  }


Does it behave differently between C and C++? Maybe adding a C test for it?

Besides, do not use parentheses for single line code.

phoebewang · 2024-03-19T08:01:50Z

Checked both pr77036.cpp and pr77036.c get the same result, so looks like a right fix. But I want to wait @efriedma-quic to sign off.

efriedma-quic

Code change looks fine; the regression test needs a few tweaks.

efriedma-quic · 2024-03-20T01:23:22Z

clang/test/CodeGenCXX/x86_64-vaarg.cpp

+
+// CHECK-LABEL: define{{.*}} void @{{.*}}empty_record_test{{.*}}()
+empty empty_record_test(void) {
+// CHECK: [[RET:%[a-z]+]] = alloca %struct.empty, align 1


This test doesn't really make it obvious that this is working the way you want; I mean, the existence of TMP sort of hints at it, but it's not comprehensive.

I'd suggest:

Make the va_list an argument to the function, instead of declaring it as a local.

Consider autogenerating the CHECK lines using update_cc_test_checks.py

SizeInBytes of empty structure is 0 in C, while 1 in C++. And empty structure argument of the function is ignored in X86_64 backend.As a result, the value of variable arguments in C++ is incorrect.So we should just make a temporary and return its address.

efriedma-quic

LGTM

SizeInBytes of empty structure is 0 in C, while 1 in C++. And empty structure argument of the function is ignored in X86_64 backend.As a result, the value of variable arguments in C++ is incorrect. fix llvm#77036 Co-authored-by: Longsheng Mou <[email protected]>

hstk30-hw requested review from AaronBallman and phoebewang January 12, 2024 11:06

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:codegen IR generation bugs: mangling, exceptions, etc. labels Jan 12, 2024

hstk30-hw force-pushed the va_arg branch 2 times, most recently from fa84978 to 63ed15b Compare January 12, 2024 11:49

hstk30-hw added the ABI Application Binary Interface label Jan 12, 2024

hstk30-hw force-pushed the va_arg branch 2 times, most recently from 0cb5f9a to 8e1d61d Compare January 13, 2024 03:36

phoebewang reviewed Jan 13, 2024

View reviewed changes

hstk30-hw force-pushed the va_arg branch from 8e1d61d to e4cd3bd Compare January 13, 2024 09:54

hstk30-hw force-pushed the va_arg branch from e4cd3bd to 2f42907 Compare January 15, 2024 12:26

phoebewang requested a review from asb January 17, 2024 05:51

efriedma-quic reviewed Jan 17, 2024

View reviewed changes

hstk30-hw force-pushed the va_arg branch from 2f42907 to aa02306 Compare March 5, 2024 06:37

efriedma-quic reviewed Mar 5, 2024

View reviewed changes

hstk30-hw force-pushed the va_arg branch from aa02306 to 75eb445 Compare March 15, 2024 11:41

phoebewang reviewed Mar 19, 2024

View reviewed changes

hstk30-hw force-pushed the va_arg branch from 75eb445 to 95e4455 Compare March 19, 2024 07:42

efriedma-quic reviewed Mar 20, 2024

View reviewed changes

hstk30-hw force-pushed the va_arg branch from 95e4455 to 9f8515c Compare March 20, 2024 05:05

efriedma-quic approved these changes Mar 20, 2024

View reviewed changes

hstk30-hw merged commit 631248d into llvm:main Mar 21, 2024

[X86_64] fix empty structure vaarg in c++ #77907

[X86_64] fix empty structure vaarg in c++ #77907

Uh oh!

Conversation

hstk30-hw commented Jan 12, 2024

Uh oh!

llvmbot commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 12, 2024

Uh oh!

github-actions bot commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoebewang commented Jan 13, 2024

Uh oh!

CoTinker commented Jan 15, 2024

Uh oh!

CoTinker commented Jan 17, 2024

Uh oh!

phoebewang commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoTinker commented Mar 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoTinker commented Mar 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoebewang commented Mar 19, 2024

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Jan 12, 2024 •

edited

Loading

github-actions bot commented Jan 12, 2024 •

edited

Loading