[PATCH] D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

2022-11-18 Thread Lin Yurong via Phabricator via cfe-commits 
yronglin added a comment.

Thank you so much for your review, @rjmccall


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138137/new/

https://reviews.llvm.org/D138137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

2022-11-18 Thread Gold Cat via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG80f444646c62: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg 
crash with empty record type variadic… (authored by yronglin, committed by 
objwyh).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138137/new/

https://reviews.llvm.org/D138137

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/arm-vaarg.c


Index: clang/test/CodeGen/arm-vaarg.c
===
--- /dev/null
+++ clang/test/CodeGen/arm-vaarg.c
@@ -0,0 +1,23 @@
+// RUN: %clang -Xclang -no-opaque-pointers -mfloat-abi=soft -target 
arm-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+// CHECK: [[ALLOCA_VA_LIST:%[a-zA-Z0-9._]+]] = alloca %struct.__va_list, align 
4
+// CHECK: call void @llvm.va_start
+// CHECK-NEXT: [[AP_ADDR:%[a-zA-Z0-9._]+]] = bitcast %struct.__va_list* 
[[ALLOCA_VA_LIST]] to i8**
+// CHECK-NEXT: [[LOAD_AP:%[a-zA-Z0-9._]+]] = load i8*, i8** [[AP_ADDR]], align 
4
+// CHECK-NEXT: [[EMPTY_PTR:%[a-zA-Z0-9._]+]] = bitcast i8* [[LOAD_AP]] to 
%struct.Empty*
+
+  // It's conceivable that EMPTY_PTR may not actually be a valid pointer
+  // (e.g. it's at the very bottom of the stack and the next page is
+  // invalid). This doesn't matter provided it's never loaded (there's no
+  // well-defined way to tell), but it becomes a problem if we do try to use 
it.
+// CHECK-NOT: load %struct.Empty, %struct.Empty* [[EMPTY_PTR]]
+  __builtin_va_list l;
+  __builtin_va_start(l, a);
+  emptyvar = __builtin_va_arg(l, struct Empty);
+  __builtin_va_end(l);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7050,10 +7050,10 @@
 
   // Empty records are ignored for parameter passing purposes.
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
+auto *Load = CGF.Builder.CreateLoad(VAListAddr);
+Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
+return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
   }
 
   CharUnits TySize = getContext().getTypeSizeInChars(Ty);


Index: clang/test/CodeGen/arm-vaarg.c
===
--- /dev/null
+++ clang/test/CodeGen/arm-vaarg.c
@@ -0,0 +1,23 @@
+// RUN: %clang -Xclang -no-opaque-pointers -mfloat-abi=soft -target arm-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+// CHECK: [[ALLOCA_VA_LIST:%[a-zA-Z0-9._]+]] = alloca %struct.__va_list, align 4
+// CHECK: call void @llvm.va_start
+// CHECK-NEXT: [[AP_ADDR:%[a-zA-Z0-9._]+]] = bitcast %struct.__va_list* [[ALLOCA_VA_LIST]] to i8**
+// CHECK-NEXT: [[LOAD_AP:%[a-zA-Z0-9._]+]] = load i8*, i8** [[AP_ADDR]], align 4
+// CHECK-NEXT: [[EMPTY_PTR:%[a-zA-Z0-9._]+]] = bitcast i8* [[LOAD_AP]] to %struct.Empty*
+
+  // It's conceivable that EMPTY_PTR may not actually be a valid pointer
+  // (e.g. it's at the very bottom of the stack and the next page is
+  // invalid). This doesn't matter provided it's never loaded (there's no
+  // well-defined way to tell), but it becomes a problem if we do try to use it.
+// CHECK-NOT: load %struct.Empty, %struct.Empty* [[EMPTY_PTR]]
+  __builtin_va_list l;
+  __builtin_va_start(l, a);
+  emptyvar = __builtin_va_arg(l, struct Empty);
+  __builtin_va_end(l);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7050,10 +7050,10 @@
 
   // Empty records are ignored for parameter passing purposes.
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
+auto *Load = CGF.Builder.CreateLoad(VAListAddr);
+Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
+return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
   }
 
   CharUnits TySize = getContext().getTypeSizeInChars(Ty);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

2022-11-18 Thread Lin Yurong via Phabricator via cfe-commits 
yronglin added a comment.

Thank you so much for your review, @rjmccall , can you land this patch for me? 
Please use 'yronglin ' to commit the change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138137/new/

https://reviews.llvm.org/D138137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

2022-11-17 Thread Lin Yurong via Phabricator via cfe-commits 
yronglin updated this revision to Diff 476093.
yronglin added a comment.

Fix test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138137/new/

https://reviews.llvm.org/D138137

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/arm-vaarg.c


Index: clang/test/CodeGen/arm-vaarg.c
===
--- /dev/null
+++ clang/test/CodeGen/arm-vaarg.c
@@ -0,0 +1,23 @@
+// RUN: %clang -Xclang -no-opaque-pointers -mfloat-abi=soft -target 
arm-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+// CHECK: [[ALLOCA_VA_LIST:%[a-zA-Z0-9._]+]] = alloca %struct.__va_list, align 
4
+// CHECK: call void @llvm.va_start
+// CHECK-NEXT: [[AP_ADDR:%[a-zA-Z0-9._]+]] = bitcast %struct.__va_list* 
[[ALLOCA_VA_LIST]] to i8**
+// CHECK-NEXT: [[LOAD_AP:%[a-zA-Z0-9._]+]] = load i8*, i8** [[AP_ADDR]], align 
4
+// CHECK-NEXT: [[EMPTY_PTR:%[a-zA-Z0-9._]+]] = bitcast i8* [[LOAD_AP]] to 
%struct.Empty*
+
+  // It's conceivable that EMPTY_PTR may not actually be a valid pointer
+  // (e.g. it's at the very bottom of the stack and the next page is
+  // invalid). This doesn't matter provided it's never loaded (there's no
+  // well-defined way to tell), but it becomes a problem if we do try to use 
it.
+// CHECK-NOT: load %struct.Empty, %struct.Empty* [[EMPTY_PTR]]
+  __builtin_va_list l;
+  __builtin_va_start(l, a);
+  emptyvar = __builtin_va_arg(l, struct Empty);
+  __builtin_va_end(l);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7050,10 +7050,10 @@
 
   // Empty records are ignored for parameter passing purposes.
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
+auto *Load = CGF.Builder.CreateLoad(VAListAddr);
+Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
+return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
   }
 
   CharUnits TySize = getContext().getTypeSizeInChars(Ty);


Index: clang/test/CodeGen/arm-vaarg.c
===
--- /dev/null
+++ clang/test/CodeGen/arm-vaarg.c
@@ -0,0 +1,23 @@
+// RUN: %clang -Xclang -no-opaque-pointers -mfloat-abi=soft -target arm-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+// CHECK: [[ALLOCA_VA_LIST:%[a-zA-Z0-9._]+]] = alloca %struct.__va_list, align 4
+// CHECK: call void @llvm.va_start
+// CHECK-NEXT: [[AP_ADDR:%[a-zA-Z0-9._]+]] = bitcast %struct.__va_list* [[ALLOCA_VA_LIST]] to i8**
+// CHECK-NEXT: [[LOAD_AP:%[a-zA-Z0-9._]+]] = load i8*, i8** [[AP_ADDR]], align 4
+// CHECK-NEXT: [[EMPTY_PTR:%[a-zA-Z0-9._]+]] = bitcast i8* [[LOAD_AP]] to %struct.Empty*
+
+  // It's conceivable that EMPTY_PTR may not actually be a valid pointer
+  // (e.g. it's at the very bottom of the stack and the next page is
+  // invalid). This doesn't matter provided it's never loaded (there's no
+  // well-defined way to tell), but it becomes a problem if we do try to use it.
+// CHECK-NOT: load %struct.Empty, %struct.Empty* [[EMPTY_PTR]]
+  __builtin_va_list l;
+  __builtin_va_start(l, a);
+  emptyvar = __builtin_va_arg(l, struct Empty);
+  __builtin_va_end(l);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7050,10 +7050,10 @@
 
   // Empty records are ignored for parameter passing purposes.
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
+auto *Load = CGF.Builder.CreateLoad(VAListAddr);
+Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
+return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
   }
 
   CharUnits TySize = getContext().getTypeSizeInChars(Ty);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

2022-11-16 Thread John McCall via Phabricator via cfe-commits 
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138137/new/

https://reviews.llvm.org/D138137

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

2022-11-16 Thread Lin Yurong via Phabricator via cfe-commits 
yronglin created this revision.
yronglin added reviewers: rjmccall, asl.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
yronglin requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

Open issue: https://github.com/llvm/llvm-project/issues/58794


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138137

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/arm-vaarg.c


Index: clang/test/CodeGen/arm-vaarg.c
===
--- /dev/null
+++ clang/test/CodeGen/arm-vaarg.c
@@ -0,0 +1,23 @@
+// RUN: %clang -Xclang -no-opaque-pointers -mfloat-abi=soft -target 
arm-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+// CHECK: [[ALLOCA_VA_LIST:%[0-9]]] = alloca %struct.__va_list, align 4
+// CHECK: call void @llvm.va_start
+// CHECK-NEXT: [[AP_ADDR:%[0-9]+]] = bitcast %struct.__va_list* 
[[ALLOCA_VA_LIST]] to i8**
+// CHECK-NEXT: [[LOAD_AP:%[0-9]+]] = load i8*, i8** [[AP_ADDR]], align 4
+// CHECK-NEXT: [[EMPTY_PTR:%[0-9]+]] = bitcast i8* [[LOAD_AP]] to 
%struct.Empty*
+
+  // It's conceivable that EMPTY_PTR may not actually be a valid pointer
+  // (e.g. it's at the very bottom of the stack and the next page is
+  // invalid). This doesn't matter provided it's never loaded (there's no
+  // well-defined way to tell), but it becomes a problem if we do try to use 
it.
+// CHECK-NOT: load %struct.Empty, %struct.Empty* [[EMPTY_PTR]]
+  __builtin_va_list l;
+  __builtin_va_start(l, a);
+  emptyvar = __builtin_va_arg(l, struct Empty);
+  __builtin_va_end(l);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7050,10 +7050,10 @@
 
   // Empty records are ignored for parameter passing purposes.
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
+auto *Load = CGF.Builder.CreateLoad(VAListAddr);
+Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
+return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
   }
 
   CharUnits TySize = getContext().getTypeSizeInChars(Ty);


Index: clang/test/CodeGen/arm-vaarg.c
===
--- /dev/null
+++ clang/test/CodeGen/arm-vaarg.c
@@ -0,0 +1,23 @@
+// RUN: %clang -Xclang -no-opaque-pointers -mfloat-abi=soft -target arm-linux-gnu -emit-llvm -S -o - %s | FileCheck %s
+
+struct Empty {};
+
+struct Empty emptyvar;
+
+void take_args(int a, ...) {
+// CHECK: [[ALLOCA_VA_LIST:%[0-9]]] = alloca %struct.__va_list, align 4
+// CHECK: call void @llvm.va_start
+// CHECK-NEXT: [[AP_ADDR:%[0-9]+]] = bitcast %struct.__va_list* [[ALLOCA_VA_LIST]] to i8**
+// CHECK-NEXT: [[LOAD_AP:%[0-9]+]] = load i8*, i8** [[AP_ADDR]], align 4
+// CHECK-NEXT: [[EMPTY_PTR:%[0-9]+]] = bitcast i8* [[LOAD_AP]] to %struct.Empty*
+
+  // It's conceivable that EMPTY_PTR may not actually be a valid pointer
+  // (e.g. it's at the very bottom of the stack and the next page is
+  // invalid). This doesn't matter provided it's never loaded (there's no
+  // well-defined way to tell), but it becomes a problem if we do try to use it.
+// CHECK-NOT: load %struct.Empty, %struct.Empty* [[EMPTY_PTR]]
+  __builtin_va_list l;
+  __builtin_va_start(l, a);
+  emptyvar = __builtin_va_arg(l, struct Empty);
+  __builtin_va_end(l);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7050,10 +7050,10 @@
 
   // Empty records are ignored for parameter passing purposes.
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
+auto *Load = CGF.Builder.CreateLoad(VAListAddr);
+Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
+return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
   }
 
   CharUnits TySize = getContext().getTypeSizeInChars(Ty);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits