[PATCH] D45812: Fix __attribute__((force_align_arg_pointer)) misalignment bug

[Henrik Gramner via Phabricator via cfe-commits]

Gramner created this revision.
Gramner added a reviewer: erichkeane.
Gramner added a project: clang.
Herald added a subscriber: javed.absar.

The force_align_arg_pointer attribute was using a hardcoded 16-byte
alignment value which in combination with -mstack-alignment=32 (or
larger) would produce a misaligned stack which could result in crashes
when accessing stack buffers using aligned AVX load/store instructions.

Fix the issue by using the "stackrealign" function attribute instead
of using a hardcoded 16-byte alignment.


Repository:
  rC Clang

https://reviews.llvm.org/D45812

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/function-attributes.c


Index: test/CodeGen/function-attributes.c
===
--- test/CodeGen/function-attributes.c
+++ test/CodeGen/function-attributes.c
@@ -71,7 +71,7 @@
 
 // PR5254
 // CHECK-LABEL: define void @f16
-// CHECK: [[ALIGN:#[0-9]+]]
+// CHECK: [[SR:#[0-9]+]]
 // CHECK: {
 void __attribute__((force_align_arg_pointer)) f16(void) {
 }
@@ -112,7 +112,7 @@
 // CHECK: attributes [[NUW]] = { nounwind optsize{{.*}} }
 // CHECK: attributes [[AI]] = { alwaysinline nounwind optsize{{.*}} }
 // CHECK: attributes [[NUW_OS_RN]] = { nounwind optsize readnone{{.*}} }
-// CHECK: attributes [[ALIGN]] = { nounwind optsize alignstack=16{{.*}} }
+// CHECK: attributes [[SR]] = { nounwind optsize{{.*}} "stackrealign"{{.*}} }
 // CHECK: attributes [[RT]] = { nounwind optsize returns_twice{{.*}} }
 // CHECK: attributes [[NR]] = { noreturn optsize }
 // CHECK: attributes [[NUW_RN]] = { nounwind optsize readnone }
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1941,13 +1941,8 @@
 return;
   if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
 if (FD->hasAttr()) {
-  // Get the LLVM function.
   llvm::Function *Fn = cast(GV);
-
-  // Now add the 'alignstack' attribute with a value of 16.
-  llvm::AttrBuilder B;
-  B.addStackAlignmentAttr(16);
-  Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+  Fn->addFnAttr("stackrealign");
 }
 if (FD->hasAttr()) {
   llvm::Function *Fn = cast(GV);
@@ -2299,13 +2294,8 @@
   return;
 if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
   if (FD->hasAttr()) {
-// Get the LLVM function.
-auto *Fn = cast(GV);
-
-// Now add the 'alignstack' attribute with a value of 16.
-llvm::AttrBuilder B;
-B.addStackAlignmentAttr(16);
-Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+llvm::Function *Fn = cast(GV);
+Fn->addFnAttr("stackrealign");
   }
   if (FD->hasAttr()) {
 llvm::Function *Fn = cast(GV);
@@ -2431,13 +2421,8 @@
 return;
   if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
 if (FD->hasAttr()) {
-  // Get the LLVM function.
-  auto *Fn = cast(GV);
-
-  // Now add the 'alignstack' attribute with a value of 16.
-  llvm::AttrBuilder B;
-  B.addStackAlignmentAttr(16);
-  Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+  llvm::Function *Fn = cast(GV);
+  Fn->addFnAttr("stackrealign");
 }
 if (FD->hasAttr()) {
   llvm::Function *Fn = cast(GV);


Index: test/CodeGen/function-attributes.c
===
--- test/CodeGen/function-attributes.c
+++ test/CodeGen/function-attributes.c
@@ -71,7 +71,7 @@
 
 // PR5254
 // CHECK-LABEL: define void @f16
-// CHECK: [[ALIGN:#[0-9]+]]
+// CHECK: [[SR:#[0-9]+]]
 // CHECK: {
 void __attribute__((force_align_arg_pointer)) f16(void) {
 }
@@ -112,7 +112,7 @@
 // CHECK: attributes [[NUW]] = { nounwind optsize{{.*}} }
 // CHECK: attributes [[AI]] = { alwaysinline nounwind optsize{{.*}} }
 // CHECK: attributes [[NUW_OS_RN]] = { nounwind optsize readnone{{.*}} }
-// CHECK: attributes [[ALIGN]] = { nounwind optsize alignstack=16{{.*}} }
+// CHECK: attributes [[SR]] = { nounwind optsize{{.*}} "stackrealign"{{.*}} }
 // CHECK: attributes [[RT]] = { nounwind optsize returns_twice{{.*}} }
 // CHECK: attributes [[NR]] = { noreturn optsize }
 // CHECK: attributes [[NUW_RN]] = { nounwind optsize readnone }
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1941,13 +1941,8 @@
 return;
   if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
 if (FD->hasAttr()) {
-  // Get the LLVM function.
   llvm::Function *Fn = cast(GV);
-
-  // Now add the 'alignstack' attribute with a value of 16.
-  llvm::AttrBuilder B;
-  B.addStackAlignmentAttr(16);
-  Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+  Fn->addFnAttr("stackrealign");
 }
 if (FD->hasAttr()) {
   llvm::Function *Fn = cast(GV);
@@ -2299,13 +2294,8 @@
   return;
 if (co

[PATCH] D45812: Fix __attribute__((force_align_arg_pointer)) misalignment bug

[Henrik Gramner via Phabricator via cfe-commits]

Gramner updated this revision to Diff 143083.
Gramner added a comment.

Full context.


https://reviews.llvm.org/D45812

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/function-attributes.c


Index: test/CodeGen/function-attributes.c
===
--- test/CodeGen/function-attributes.c
+++ test/CodeGen/function-attributes.c
@@ -71,7 +71,7 @@
 
 // PR5254
 // CHECK-LABEL: define void @f16
-// CHECK: [[ALIGN:#[0-9]+]]
+// CHECK: [[SR:#[0-9]+]]
 // CHECK: {
 void __attribute__((force_align_arg_pointer)) f16(void) {
 }
@@ -112,7 +112,7 @@
 // CHECK: attributes [[NUW]] = { nounwind optsize{{.*}} }
 // CHECK: attributes [[AI]] = { alwaysinline nounwind optsize{{.*}} }
 // CHECK: attributes [[NUW_OS_RN]] = { nounwind optsize readnone{{.*}} }
-// CHECK: attributes [[ALIGN]] = { nounwind optsize alignstack=16{{.*}} }
+// CHECK: attributes [[SR]] = { nounwind optsize{{.*}} "stackrealign"{{.*}} }
 // CHECK: attributes [[RT]] = { nounwind optsize returns_twice{{.*}} }
 // CHECK: attributes [[NR]] = { noreturn optsize }
 // CHECK: attributes [[NUW_RN]] = { nounwind optsize readnone }
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1941,13 +1941,8 @@
 return;
   if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
 if (FD->hasAttr()) {
-  // Get the LLVM function.
   llvm::Function *Fn = cast(GV);
-
-  // Now add the 'alignstack' attribute with a value of 16.
-  llvm::AttrBuilder B;
-  B.addStackAlignmentAttr(16);
-  Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+  Fn->addFnAttr("stackrealign");
 }
 if (FD->hasAttr()) {
   llvm::Function *Fn = cast(GV);
@@ -2299,13 +2294,8 @@
   return;
 if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
   if (FD->hasAttr()) {
-// Get the LLVM function.
-auto *Fn = cast(GV);
-
-// Now add the 'alignstack' attribute with a value of 16.
-llvm::AttrBuilder B;
-B.addStackAlignmentAttr(16);
-Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+llvm::Function *Fn = cast(GV);
+Fn->addFnAttr("stackrealign");
   }
   if (FD->hasAttr()) {
 llvm::Function *Fn = cast(GV);
@@ -2431,13 +2421,8 @@
 return;
   if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
 if (FD->hasAttr()) {
-  // Get the LLVM function.
-  auto *Fn = cast(GV);
-
-  // Now add the 'alignstack' attribute with a value of 16.
-  llvm::AttrBuilder B;
-  B.addStackAlignmentAttr(16);
-  Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+  llvm::Function *Fn = cast(GV);
+  Fn->addFnAttr("stackrealign");
 }
 if (FD->hasAttr()) {
   llvm::Function *Fn = cast(GV);


Index: test/CodeGen/function-attributes.c
===
--- test/CodeGen/function-attributes.c
+++ test/CodeGen/function-attributes.c
@@ -71,7 +71,7 @@
 
 // PR5254
 // CHECK-LABEL: define void @f16
-// CHECK: [[ALIGN:#[0-9]+]]
+// CHECK: [[SR:#[0-9]+]]
 // CHECK: {
 void __attribute__((force_align_arg_pointer)) f16(void) {
 }
@@ -112,7 +112,7 @@
 // CHECK: attributes [[NUW]] = { nounwind optsize{{.*}} }
 // CHECK: attributes [[AI]] = { alwaysinline nounwind optsize{{.*}} }
 // CHECK: attributes [[NUW_OS_RN]] = { nounwind optsize readnone{{.*}} }
-// CHECK: attributes [[ALIGN]] = { nounwind optsize alignstack=16{{.*}} }
+// CHECK: attributes [[SR]] = { nounwind optsize{{.*}} "stackrealign"{{.*}} }
 // CHECK: attributes [[RT]] = { nounwind optsize returns_twice{{.*}} }
 // CHECK: attributes [[NR]] = { noreturn optsize }
 // CHECK: attributes [[NUW_RN]] = { nounwind optsize readnone }
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1941,13 +1941,8 @@
 return;
   if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
 if (FD->hasAttr()) {
-  // Get the LLVM function.
   llvm::Function *Fn = cast(GV);
-
-  // Now add the 'alignstack' attribute with a value of 16.
-  llvm::AttrBuilder B;
-  B.addStackAlignmentAttr(16);
-  Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+  Fn->addFnAttr("stackrealign");
 }
 if (FD->hasAttr()) {
   llvm::Function *Fn = cast(GV);
@@ -2299,13 +2294,8 @@
   return;
 if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
   if (FD->hasAttr()) {
-// Get the LLVM function.
-auto *Fn = cast(GV);
-
-// Now add the 'alignstack' attribute with a value of 16.
-llvm::AttrBuilder B;
-B.addStackAlignmentAttr(16);
-Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+llvm::Function *Fn = cast(GV);
+Fn->addFnAttr("stackrealign");
   }
   if (FD->hasAttr()) {

[PATCH] D45812: Fix __attribute__((force_align_arg_pointer)) misalignment bug

[Henrik Gramner via Phabricator via cfe-commits]

Gramner added a comment.

In https://reviews.llvm.org/D45812#1072066, @erichkeane wrote:

> Can you make sure that this doesn't cause stackrealign to happen 2x if using 
> -mstackrealign?  Additionally, please provide the diff with full-context (see 
> https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).
>   What is the commit before this?  The line numbers don't look right to me.
>
> Barring any surprises from those two, this looks alright to me.


The commit before this one is git hash 4889058f23607ff086470afe013082ca4a278ddb

It does "the right thing" as far as I can see, e.g. no double stack realignment 
or anything weird, when tested on 64-bit Linux with various combinations (and 
lack of thereof) of -mstack-alignment=32, -mstack-alignment=64, -mstackrealign, 
-m32. Unable to test on other systems though.


https://reviews.llvm.org/D45812



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

[PATCH] D45812: Fix __attribute__((force_align_arg_pointer)) misalignment bug

[Henrik Gramner via Phabricator via cfe-commits]

Gramner added a comment.

In https://reviews.llvm.org/D45812#1072128, @erichkeane wrote:

> LGTM, Do you have commit access, or do you want me to commit it for you?


I do not have commit access, so please do. Thanks for the review.


https://reviews.llvm.org/D45812



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