[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane added a comment.

In D68824#1733621 , @hans wrote:

> We hit this in V8 which is annotating some pointers as being 4GB-aligned. 
> Does anyone know what it would take to raise Clang's limit here?


The issue is LLVM's limit here. I'm unaware of the reasoning for this limit in 
LLVM, however the clang limit is based on LLVMs.  I'd suggest starting a 
conversation on llvm-dev mailing list to have the discussion of why this limit 
exists, and whether it is modifiable.


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

We hit this in V8 which is annotating some pointers as being 4GB-aligned. Does 
anyone know what it would take to raise Clang's limit here?


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:6067-6070
+// Alignment calculations can wrap around if it's greater than 2**28.
+unsigned MaximumAlignment =
+Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192
+: 268435456;

erichkeane wrote:
> rsmith wrote:
> > Why is there a different limit depending on bin format? We can support this 
> > at the IR level regardless, can't we? (I don't see how the binary format is 
> > relevant.)
> I'd copied it from the Sema::AddAlignedAttr implementation, but I cannot seem 
> to figure out the origin of that.  @majnemer added the 2**28 business back in 
> 2015, but @aaron.ballman put the limit of 8192 in here: 
> https://reviews.llvm.org/rL158717#change-N0HH8qtBJv7d
> (note it was reverted and relanded).
> 
> I don't see sufficient justification in that history now that I've looked 
> back to keep that log in here, so I'll keep us at 2**28.
> 
It's been a while, but I seem to recall this matching a behavior of MSVC at the 
time. 


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 5 inline comments as done.
erichkeane added subscribers: majnemer, aaron.ballman.
erichkeane added a comment.

In D68824#1704942 , @rsmith wrote:

> This seems to be missing a CodeGen test for what IR we generate on an 
> overly-large alignment. (The warning says the alignment is ignored, but I 
> don't see where you're actually doing anything different in that case when 
> generating IR.)


My intent was to just let the value through, and let LLVM ignore it (while 
alerting the developer).  I'll add a CodeGen test as well when relanding, it 
seems to have been lost in the process of developing the patch.  Thanks for 
your attention here.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2857-2859
+: Warning<"requested alignment must be %0 bytes or smaller; assumption "
+  "ignored">,
+  InGroup>;

rsmith wrote:
> Ignoring the assumption in this case seems unnecessarily user-hostile (and 
> would require hard-coding an arbitrary LLVM limit in the source code to work 
> around). Couldn't we just assume the highest alignment that we do support 
> instead?
This more closely fit GCCs behavior (of ignoring any invalid value, though 
their top limit is int64-max).  We can definitely just assume highest 
alignment, I'll do that in my re-land.



Comment at: clang/lib/Sema/SemaChecking.cpp:6067-6070
+// Alignment calculations can wrap around if it's greater than 2**28.
+unsigned MaximumAlignment =
+Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192
+: 268435456;

rsmith wrote:
> Why is there a different limit depending on bin format? We can support this 
> at the IR level regardless, can't we? (I don't see how the binary format is 
> relevant.)
I'd copied it from the Sema::AddAlignedAttr implementation, but I cannot seem 
to figure out the origin of that.  @majnemer added the 2**28 business back in 
2015, but @aaron.ballman put the limit of 8192 in here: 
https://reviews.llvm.org/rL158717#change-N0HH8qtBJv7d
(note it was reverted and relanded).

I don't see sufficient justification in that history now that I've looked back 
to keep that log in here, so I'll keep us at 2**28.



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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This seems to be missing a CodeGen test for what IR we generate on an 
overly-large alignment. (The warning says the alignment is ignored, but I don't 
see where you're actually doing anything different in that case when generating 
IR.)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2857-2859
+: Warning<"requested alignment must be %0 bytes or smaller; assumption "
+  "ignored">,
+  InGroup>;

Ignoring the assumption in this case seems unnecessarily user-hostile (and 
would require hard-coding an arbitrary LLVM limit in the source code to work 
around). Couldn't we just assume the highest alignment that we do support 
instead?



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1522
   for (const auto *Clause : D.getClausesOfKind()) {
-unsigned ClauseAlignment = 0;
+size_t ClauseAlignment = 0;
 if (const Expr *AlignmentExpr = Clause->getAlignment()) {

It's not appropriate to assume that `size_t` is any bigger than `unsigned` or 
generally that it's meaningful on the target. If you want 64 bits here, use 
`uint64_t`, but the right choice is probably `llvm::APInt`.



Comment at: clang/lib/Sema/SemaChecking.cpp:6067-6070
+// Alignment calculations can wrap around if it's greater than 2**28.
+unsigned MaximumAlignment =
+Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192
+: 268435456;

Why is there a different limit depending on bin format? We can support this at 
the IR level regardless, can't we? (I don't see how the binary format is 
relevant.)


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

https://reviews.llvm.org/D68824



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

Re: [PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Keane, Erich via cfe-commits]

Thanks! On the way home for the evening, so I'll fix it up tomorrow.

On Oct 10, 2019 2:34 PM, Nico Weber via Phabricator  
wrote:
thakis added a comment.

Since you just left IRC, I reverted this in 374456 for now.


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Since you just left IRC, I reverted this in 374456 for now.


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

The new test fails on Windows:

  -- Testing: 16032 tests, 32 workers --
  Testing:  0.. 10.. 20.. 30.. 40.. 50
  FAIL: Clang :: Sema/builtin-assume-aligned.c (8862 of 16032)
   TEST 'Clang :: Sema/builtin-assume-aligned.c' FAILED 

  Script:
  --
  : 'RUN: at line 1';   c:\src\llvm-project\out\gn\bin\clang.exe -cc1 
-internal-isystem c:\src\llvm-project\out\gn\lib\clang\10.0.0\include 
-nostdsysteminc -fsyntax-only -verify 
C:\src\llvm-project\clang\test\Sema\builtin-assume-aligned.c
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  $ ":" "RUN: at line 1"
  $ "c:\src\llvm-project\out\gn\bin\clang.exe" "-cc1" "-internal-isystem" 
"c:\src\llvm-project\out\gn\lib\clang\10.0.0\include" "-nostdsysteminc" 
"-fsyntax-only" "-verify" 
"C:\src\llvm-project\clang\test\Sema\builtin-assume-aligned.c"
  # command stderr:
  error: 'warning' diagnostics expected but not seen: 
File C:\src\llvm-project\clang\test\Sema\builtin-assume-aligned.c Line 62: 
requested alignment must be 268435456 bytes or smaller; assumption ignored
  error: 'warning' diagnostics seen but not expected: 
File C:\src\llvm-project\clang\test\Sema\builtin-assume-aligned.c Line 62: 
requested alignment must be 8192 bytes or smaller; assumption ignored
  2 errors generated.
  
  error: command failed with exit status: 1
  
  --
  
  
  Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
  Testing Time: 152.23s
  
  Failing Tests (1):
  Clang :: Sema/builtin-assume-aligned.c


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 224456.
erichkeane added a comment.

cant use llvm::Value in Sema without some linker issues.


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

https://reviews.llvm.org/D68824

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/builtin-assume-aligned.c

Index: clang/test/Sema/builtin-assume-aligned.c
===
--- clang/test/Sema/builtin-assume-aligned.c
+++ clang/test/Sema/builtin-assume-aligned.c
@@ -58,3 +58,7 @@
 void *test_no_fn_proto() __attribute__((assume_aligned())); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32, 45, 37))); // expected-error {{'assume_aligned' attribute takes no more than 2 arguments}}
 
+int pr43638(int *a) {
+  a = __builtin_assume_aligned(a, 4294967296); // expected-warning {{requested alignment must be %0 bytes or smaller; assumption ignored}}
+return a[0];
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -6063,6 +6063,14 @@
 if (!Result.isPowerOf2())
   return Diag(TheCall->getBeginLoc(), diag::err_alignment_not_power_of_two)
  << Arg->getSourceRange();
+
+// Alignment calculations can wrap around if it's greater than 2**28.
+unsigned MaximumAlignment =
+Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192
+: 268435456;
+if (Result > MaximumAlignment)
+  Diag(TheCall->getBeginLoc(), diag::warn_assume_aligned_too_great)
+  << Arg->getSourceRange() << MaximumAlignment;
   }
 
   if (NumArgs > 2) {
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -2829,13 +2829,8 @@
llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
 
-  void EmitAlignmentAssumption(llvm::Value *PtrValue, QualType Ty,
-   SourceLocation Loc, SourceLocation AssumptionLoc,
-   unsigned Alignment,
-   llvm::Value *OffsetValue = nullptr);
-
   void EmitAlignmentAssumption(llvm::Value *PtrValue, const Expr *E,
-   SourceLocation AssumptionLoc, unsigned Alignment,
+   SourceLocation AssumptionLoc, llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
 
   //======//
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2057,24 +2057,9 @@
 }
 
 void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
-  QualType Ty, SourceLocation Loc,
-  SourceLocation AssumptionLoc,
-  unsigned Alignment,
-  llvm::Value *OffsetValue) {
-  llvm::Value *TheCheck;
-  llvm::Instruction *Assumption = Builder.CreateAlignmentAssumption(
-  CGM.getDataLayout(), PtrValue, Alignment, OffsetValue, );
-  if (SanOpts.has(SanitizerKind::Alignment)) {
-llvm::Value *AlignmentVal = llvm::ConstantInt::get(IntPtrTy, Alignment);
-EmitAlignmentAssumptionCheck(PtrValue, Ty, Loc, AssumptionLoc, AlignmentVal,
- OffsetValue, TheCheck, Assumption);
-  }
-}
-
-void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
   const Expr *E,
   SourceLocation AssumptionLoc,
-  unsigned Alignment,
+  llvm::Value *Alignment,
   llvm::Value *OffsetValue) {
   if (auto *CE = dyn_cast(E))
 E = CE->getSubExprAsWritten();
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1519,14 +1519,14 @@
   if (!CGF.HaveInsertPoint())
 return;
   for (const auto *Clause : D.getClausesOfKind()) {
-unsigned ClauseAlignment = 0;
+size_t 

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

LG, thanks.


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 224455.
erichkeane marked 5 inline comments as done.
erichkeane added a comment.

Hal's comments


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

https://reviews.llvm.org/D68824

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/builtin-assume-aligned.c

Index: clang/test/Sema/builtin-assume-aligned.c
===
--- clang/test/Sema/builtin-assume-aligned.c
+++ clang/test/Sema/builtin-assume-aligned.c
@@ -58,3 +58,7 @@
 void *test_no_fn_proto() __attribute__((assume_aligned())); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32, 45, 37))); // expected-error {{'assume_aligned' attribute takes no more than 2 arguments}}
 
+int pr43638(int *a) {
+  a = __builtin_assume_aligned(a, 4294967296); // expected-warning {{requested alignment must be %0 bytes or smaller; assumption ignored}}
+return a[0];
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -76,6 +76,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/IR/Value.h"
 #include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
@@ -6063,6 +6064,9 @@
 if (!Result.isPowerOf2())
   return Diag(TheCall->getBeginLoc(), diag::err_alignment_not_power_of_two)
  << Arg->getSourceRange();
+if (Result > llvm::Value::MaximumAlignment)
+  Diag(TheCall->getBeginLoc(), diag::warn_assume_aligned_too_great)
+  << Arg->getSourceRange() << llvm::Value::MaximumAlignment;
   }
 
   if (NumArgs > 2) {
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -2829,13 +2829,8 @@
llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
 
-  void EmitAlignmentAssumption(llvm::Value *PtrValue, QualType Ty,
-   SourceLocation Loc, SourceLocation AssumptionLoc,
-   unsigned Alignment,
-   llvm::Value *OffsetValue = nullptr);
-
   void EmitAlignmentAssumption(llvm::Value *PtrValue, const Expr *E,
-   SourceLocation AssumptionLoc, unsigned Alignment,
+   SourceLocation AssumptionLoc, llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
 
   //======//
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2057,24 +2057,9 @@
 }
 
 void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
-  QualType Ty, SourceLocation Loc,
-  SourceLocation AssumptionLoc,
-  unsigned Alignment,
-  llvm::Value *OffsetValue) {
-  llvm::Value *TheCheck;
-  llvm::Instruction *Assumption = Builder.CreateAlignmentAssumption(
-  CGM.getDataLayout(), PtrValue, Alignment, OffsetValue, );
-  if (SanOpts.has(SanitizerKind::Alignment)) {
-llvm::Value *AlignmentVal = llvm::ConstantInt::get(IntPtrTy, Alignment);
-EmitAlignmentAssumptionCheck(PtrValue, Ty, Loc, AssumptionLoc, AlignmentVal,
- OffsetValue, TheCheck, Assumption);
-  }
-}
-
-void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
   const Expr *E,
   SourceLocation AssumptionLoc,
-  unsigned Alignment,
+  llvm::Value *Alignment,
   llvm::Value *OffsetValue) {
   if (auto *CE = dyn_cast(E))
 E = CE->getSubExprAsWritten();
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1519,14 +1519,14 @@
   if (!CGF.HaveInsertPoint())
 return;
   for (const auto *Clause : D.getClausesOfKind()) {
-unsigned ClauseAlignment = 0;

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2854
+def warn_assume_align_too_big : Warning<"alignments greater than 1 << 29 are "
+  "unsupported by LLVM">, InGroup;
 

Should this be in the IgnoredAttributes group? The builtin is not an attribute.

Also, I'd rather that the wording were similar to that used by 
err_attribute_aligned_too_great and other errors, and use 268435456 (which is 
what we get for MaxValidAlignment based on LLVM restrictions) instead of '1 << 
29'.





Comment at: clang/lib/Sema/SemaChecking.cpp:6066
  << Arg->getSourceRange();
+if (Result > (1 << 29ULL))
+  Diag(TheCall->getBeginLoc(), diag::warn_assume_align_too_big)

lebedev.ri wrote:
> `(1ULL << 29ULL)`.
> Is there some define for this somwhere, don't like magic numbers.
Yep, there's Value::MaximumAlignment.

We have:


  // Alignment calculations can wrap around if it's greater than 2**28.
  unsigned MaxValidAlignment =
  Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192
  : 268435456;

in Sema::AddAlignedAttr. which also has the magic-number problem.


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

https://reviews.llvm.org/D68824



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 224449.
erichkeane added a comment.

Remove unused code


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

https://reviews.llvm.org/D68824

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/builtin-assume-aligned.c

Index: clang/test/Sema/builtin-assume-aligned.c
===
--- clang/test/Sema/builtin-assume-aligned.c
+++ clang/test/Sema/builtin-assume-aligned.c
@@ -58,3 +58,7 @@
 void *test_no_fn_proto() __attribute__((assume_aligned())); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32, 45, 37))); // expected-error {{'assume_aligned' attribute takes no more than 2 arguments}}
 
+int pr43638(int *a) {
+  a = __builtin_assume_aligned(a, 4294967296); // expected-warning {{alignments greater than 1 << 29 are unsupported by LLVM}}
+  return a[0];
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -6063,6 +6063,9 @@
 if (!Result.isPowerOf2())
   return Diag(TheCall->getBeginLoc(), diag::err_alignment_not_power_of_two)
  << Arg->getSourceRange();
+if (Result > (1 << 29ULL))
+  Diag(TheCall->getBeginLoc(), diag::warn_assume_align_too_big)
+ << Arg->getSourceRange();
   }
 
   if (NumArgs > 2) {
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -2829,13 +2829,8 @@
llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
 
-  void EmitAlignmentAssumption(llvm::Value *PtrValue, QualType Ty,
-   SourceLocation Loc, SourceLocation AssumptionLoc,
-   unsigned Alignment,
-   llvm::Value *OffsetValue = nullptr);
-
   void EmitAlignmentAssumption(llvm::Value *PtrValue, const Expr *E,
-   SourceLocation AssumptionLoc, unsigned Alignment,
+   SourceLocation AssumptionLoc, llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
 
   //======//
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2057,24 +2057,9 @@
 }
 
 void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
-  QualType Ty, SourceLocation Loc,
-  SourceLocation AssumptionLoc,
-  unsigned Alignment,
-  llvm::Value *OffsetValue) {
-  llvm::Value *TheCheck;
-  llvm::Instruction *Assumption = Builder.CreateAlignmentAssumption(
-  CGM.getDataLayout(), PtrValue, Alignment, OffsetValue, );
-  if (SanOpts.has(SanitizerKind::Alignment)) {
-llvm::Value *AlignmentVal = llvm::ConstantInt::get(IntPtrTy, Alignment);
-EmitAlignmentAssumptionCheck(PtrValue, Ty, Loc, AssumptionLoc, AlignmentVal,
- OffsetValue, TheCheck, Assumption);
-  }
-}
-
-void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
   const Expr *E,
   SourceLocation AssumptionLoc,
-  unsigned Alignment,
+  llvm::Value *Alignment,
   llvm::Value *OffsetValue) {
   if (auto *CE = dyn_cast(E))
 E = CE->getSubExprAsWritten();
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1519,14 +1519,14 @@
   if (!CGF.HaveInsertPoint())
 return;
   for (const auto *Clause : D.getClausesOfKind()) {
-unsigned ClauseAlignment = 0;
+size_t ClauseAlignment = 0;
 if (const Expr *AlignmentExpr = Clause->getAlignment()) {
   auto *AlignmentCI =
   cast(CGF.EmitScalarExpr(AlignmentExpr));
-  ClauseAlignment = static_cast(AlignmentCI->getZExtValue());
+  ClauseAlignment = AlignmentCI->getZExtValue();
 }
 for (const Expr *E : Clause->varlists()) {
-  

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added reviewers: lebedev.ri, hfinkel, joey, jdoerfert.
lebedev.ri added a comment.

Thanks, i like it.




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2058-2073
+/*
 void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
   QualType Ty, SourceLocation Loc,
   SourceLocation AssumptionLoc,
   unsigned Alignment,
   llvm::Value *OffsetValue) {
   llvm::Value *TheCheck;

Just remove.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2831-2836
+/*
   void EmitAlignmentAssumption(llvm::Value *PtrValue, QualType Ty,
SourceLocation Loc, SourceLocation 
AssumptionLoc,
unsigned Alignment,
llvm::Value *OffsetValue = nullptr);
+*/

Just remove.



Comment at: clang/lib/Sema/SemaChecking.cpp:6066
  << Arg->getSourceRange();
+if (Result > (1 << 29ULL))
+  Diag(TheCall->getBeginLoc(), diag::warn_assume_align_too_big)

`(1ULL << 29ULL)`.
Is there some define for this somwhere, don't like magic numbers.


Code to handle __builtin_assume_aligned was allowing larger values, but
would convert this to unsigned along the way. This patch removes the
EmitAssumeAligned overloads that take unsigned to do away with this
problem.

Additionally, it adds a warning that values greater than 1 <<29 are
ignored by LLVM.


https://reviews.llvm.org/D68824

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/builtin-assume-aligned.c

Index: clang/test/Sema/builtin-assume-aligned.c
===
--- clang/test/Sema/builtin-assume-aligned.c
+++ clang/test/Sema/builtin-assume-aligned.c
@@ -58,3 +58,7 @@
 void *test_no_fn_proto() __attribute__((assume_aligned())); // expected-error {{'assume_aligned' attribute takes at least 1 argument}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32, 45, 37))); // expected-error {{'assume_aligned' attribute takes no more than 2 arguments}}
 
+int pr43638(int *a) {
+  a = __builtin_assume_aligned(a, 4294967296); // expected-warning {{alignments greater than 1 << 29 are unsupported by LLVM}}
+  return a[0];
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -6063,6 +6063,9 @@
 if (!Result.isPowerOf2())
   return Diag(TheCall->getBeginLoc(), diag::err_alignment_not_power_of_two)
  << Arg->getSourceRange();
+if (Result > (1 << 29ULL))
+  Diag(TheCall->getBeginLoc(), diag::warn_assume_align_too_big)
+ << Arg->getSourceRange();
   }
 
   if (NumArgs > 2) {
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -2828,14 +2828,14 @@
SourceLocation Loc, SourceLocation AssumptionLoc,
llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
-
+/*
   void EmitAlignmentAssumption(llvm::Value *PtrValue, QualType Ty,
SourceLocation Loc, SourceLocation AssumptionLoc,
unsigned Alignment,
llvm::Value *OffsetValue = nullptr);
-
+*/
   void EmitAlignmentAssumption(llvm::Value *PtrValue, const Expr *E,
-   SourceLocation AssumptionLoc, unsigned Alignment,
+   SourceLocation AssumptionLoc, llvm::Value *Alignment,
llvm::Value *OffsetValue = nullptr);
 
   //======//
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2055,7 +2055,7 @@
  OffsetValue, TheCheck, Assumption);
   }
 }
-
+/*
 void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
   QualType Ty, SourceLocation Loc,
   SourceLocation AssumptionLoc,
@@ -2070,11 +2070,11 @@
  OffsetValue, TheCheck, Assumption);
   }
 }
-
+*/
 void 

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thanks, i like it.




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2058-2073
+/*
 void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
   QualType Ty, SourceLocation Loc,
   SourceLocation AssumptionLoc,
   unsigned Alignment,
   llvm::Value *OffsetValue) {
   llvm::Value *TheCheck;

Just remove.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2831-2836
+/*
   void EmitAlignmentAssumption(llvm::Value *PtrValue, QualType Ty,
SourceLocation Loc, SourceLocation 
AssumptionLoc,
unsigned Alignment,
llvm::Value *OffsetValue = nullptr);
+*/

Just remove.



Comment at: clang/lib/Sema/SemaChecking.cpp:6066
  << Arg->getSourceRange();
+if (Result > (1 << 29ULL))
+  Diag(TheCall->getBeginLoc(), diag::warn_assume_align_too_big)

`(1ULL << 29ULL)`.
Is there some define for this somwhere, don't like magic numbers.


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

https://reviews.llvm.org/D68824



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