Fix __builtin_assume_aligned with too large values.

[erichkeane]

Summary

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.

[lebedev.ri]

Thanks, i like it.

clang/lib/CodeGen/CodeGenFunction.cpp

2058–2060

Just remove.

clang/lib/CodeGen/CodeGenFunction.h

2831–2832

Just remove.

clang/lib/Sema/SemaChecking.cpp

6066

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

[erichkeane]

Remove unused code

[hfinkel]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2854

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'.

clang/lib/Sema/SemaChecking.cpp

6066

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.

[erichkeane]

Hal's comments

[lebedev.ri]

LG, thanks.

[erichkeane]

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

[thakis]

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

[thakis]

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

[rsmith]

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.)

clang/include/clang/Basic/DiagnosticSemaKinds.td

2857–2859

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?

clang/lib/CodeGen/CGStmtOpenMP.cpp

1522

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.

clang/lib/Sema/SemaChecking.cpp

6067–6070

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.)

[erichkeane]

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

2857–2859

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.

clang/lib/Sema/SemaChecking.cpp

6067–6070

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.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

6067–6070

It's been a while, but I seem to recall this matching a behavior of MSVC at the time.

[hans]

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?

[erichkeane]

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_alignment_too_small : Error<
   "requested alignment must be %0 or greater">;
 def err_alignment_too_big : Error<
   "requested alignment must be %0 or smaller">;
 def err_alignment_not_power_of_two : Error<
   "requested alignment is not a power of 2">;
 def err_alignment_dependent_typedef_name : Error<
   "requested alignment is dependent but declaration is not dependent">;
 
 def err_attribute_aligned_too_great : Error<
   "requested alignment must be %0 bytes or smaller">;
+def warn_assume_aligned_too_great
+    : Warning<"requested alignment must be %0 bytes or smaller; assumption "
+              "ignored">,
+      InGroup<DiagGroup<"builtin-assume-aligned-alignment">>;
 def warn_redeclaration_without_attribute_prev_attribute_ignored : Warning<
   "%q0 redeclared without %1 attribute: previous %1 ignored">,
   InGroup<MicrosoftInconsistentDllImport>;
 def warn_redeclaration_without_import_attribute : Warning<
   "%q0 redeclared without 'dllimport' attribute: 'dllexport' attribute added">,
   InGroup<MicrosoftInconsistentDllImport>;
 def warn_dllimport_dropped_from_inline_function : Warning<
   "%q0 redeclared inline; %1 attribute ignored">,

clang/lib/CodeGen/CGBuiltin.cpp

RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
   case Builtin::BI__builtin_assume_aligned: {
     const Expr *Ptr = E->getArg(0);
     Value *PtrValue = EmitScalarExpr(Ptr);
     Value *OffsetValue =
       (E->getNumArgs() > 2) ? EmitScalarExpr(E->getArg(2)) : nullptr;
 
     Value *AlignmentValue = EmitScalarExpr(E->getArg(1));
     ConstantInt *AlignmentCI = cast<ConstantInt>(AlignmentValue);
-    unsigned Alignment = (unsigned)AlignmentCI->getZExtValue();
 
     EmitAlignmentAssumption(PtrValue, Ptr,
                             /*The expr loc is sufficient.*/ SourceLocation(),
-                            Alignment, OffsetValue);
+                            AlignmentCI, OffsetValue);
     return RValue::get(PtrValue);
   }
   case Builtin::BI__assume:
   case Builtin::BI__builtin_assume: {
     if (E->getArg(0)->HasSideEffects(getContext()))
       return RValue::get(nullptr);
 
     Value *ArgValue = EmitScalarExpr(E->getArg(0));

clang/lib/CodeGen/CGCall.cpp

if (Ret.isScalar() && TargetDecl) {
     if (const auto *AA = TargetDecl->getAttr<AssumeAlignedAttr>()) {
       llvm::Value *OffsetValue = nullptr;
       if (const auto *Offset = AA->getOffset())
         OffsetValue = EmitScalarExpr(Offset);
 
       llvm::Value *Alignment = EmitScalarExpr(AA->getAlignment());
       llvm::ConstantInt *AlignmentCI = cast<llvm::ConstantInt>(Alignment);
       EmitAlignmentAssumption(Ret.getScalarVal(), RetTy, Loc, AA->getLocation(),
-                              AlignmentCI->getZExtValue(), OffsetValue);
+                              AlignmentCI, OffsetValue);
     } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
       llvm::Value *AlignmentVal = CallArgs[AA->getParamIndex().getLLVMIndex()]
                                       .getRValue(*this)
                                       .getScalarVal();
       EmitAlignmentAssumption(Ret.getScalarVal(), RetTy, Loc, AA->getLocation(),
                               AlignmentVal);
     }
   }

clang/lib/CodeGen/CGExprScalar.cpp

if (!AVAttr)
           dyn_cast<TypedefType>(E->getType()))
         AVAttr = TTy->getDecl()->getAttr<AlignValueAttr>();
 
     if (!AVAttr)
       return;
 
     Value *AlignmentValue = CGF.EmitScalarExpr(AVAttr->getAlignment());
     llvm::ConstantInt *AlignmentCI = cast<llvm::ConstantInt>(AlignmentValue);
-    CGF.EmitAlignmentAssumption(V, E, AVAttr->getLocation(),
-                                AlignmentCI->getZExtValue());
+    CGF.EmitAlignmentAssumption(V, E, AVAttr->getLocation(), AlignmentCI);
   }
 
   /// EmitLoadOfLValue - Given an expression with complex type that represents a
   /// value l-value, this method emits the address of the l-value, then loads
   /// and returns the result.
   Value *EmitLoadOfLValue(const Expr *E) {
     Value *V = EmitLoadOfLValue(EmitCheckedLValue(E, CodeGenFunction::TCK_Load),
                                 E->getExprLoc());

clang/lib/CodeGen/CGStmtOpenMP.cpp

if (DoneBB)
     EmitBlock(DoneBB, /*IsFinished=*/true);
 }
 
 static void emitAlignedClause(CodeGenFunction &CGF,
                               const OMPExecutableDirective &D) {
   if (!CGF.HaveInsertPoint())
     return;
   for (const auto *Clause : D.getClausesOfKind<OMPAlignedClause>()) {
-    unsigned ClauseAlignment = 0;
+    size_t ClauseAlignment = 0;
     if (const Expr *AlignmentExpr = Clause->getAlignment()) {
       auto *AlignmentCI =
           cast<llvm::ConstantInt>(CGF.EmitScalarExpr(AlignmentExpr));
-      ClauseAlignment = static_cast<unsigned>(AlignmentCI->getZExtValue());
+      ClauseAlignment = AlignmentCI->getZExtValue();
     }
     for (const Expr *E : Clause->varlists()) {
-      unsigned Alignment = ClauseAlignment;
+      size_t Alignment = ClauseAlignment;
       if (Alignment == 0) {
         // OpenMP [2.8.1, Description]
         // If no optional parameter is specified, implementation-defined default
         // alignments for SIMD instructions on the target platforms are assumed.
         Alignment =
             CGF.getContext()
                 .toCharUnitsFromBits(CGF.getContext().getOpenMPDefaultSimdAlign(
                     E->getType()->getPointeeType()))
                 .getQuantity();
       }
       assert((Alignment == 0 || llvm::isPowerOf2_32(Alignment)) &&
              "alignment is not power of 2");
       if (Alignment != 0) {
         llvm::Value *PtrValue = CGF.EmitScalarExpr(E);
         CGF.EmitAlignmentAssumption(
-            PtrValue, E, /*No second loc needed*/ SourceLocation(), Alignment);
+            PtrValue, E, /*No second loc needed*/ SourceLocation(),
+            llvm::ConstantInt::get(CGF.SizeTy, Alignment));
       }
     }
   }
 }
 
 void CodeGenFunction::EmitOMPPrivateLoopCounters(
     const OMPLoopDirective &S, CodeGenFunction::OMPPrivateScope &LoopScope) {
   if (!HaveInsertPoint())

clang/lib/CodeGen/CodeGenFunction.h

void EmitAlignmentAssumptionCheck(llvm::Value *Ptr, QualType Ty,
                                     llvm::Value *OffsetValue,
                                     llvm::Value *TheCheck,
                                     llvm::Instruction *Assumption);
 
   void EmitAlignmentAssumption(llvm::Value *PtrValue, QualType Ty,
                                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);
 
   //===--------------------------------------------------------------------===//
   //                             Statement Emission
   //===--------------------------------------------------------------------===//
 
   /// EmitStopPoint - Emit a debug stoppoint if we are emitting debug info.
   void EmitStopPoint(const Stmt *S);

clang/lib/CodeGen/CodeGenFunction.cpp

void CodeGenFunction::EmitAlignmentAssumption(llvm::Value *PtrValue,
   llvm::Value *TheCheck;
   llvm::Instruction *Assumption = Builder.CreateAlignmentAssumption(
       CGM.getDataLayout(), PtrValue, Alignment, OffsetValue, &TheCheck);
   if (SanOpts.has(SanitizerKind::Alignment)) {
     EmitAlignmentAssumptionCheck(PtrValue, Ty, Loc, AssumptionLoc, Alignment,
                                  OffsetValue, TheCheck, Assumption);
   }
 }
 
 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, &TheCheck);
-  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<CastExpr>(E))
     E = CE->getSubExprAsWritten();
   QualType Ty = E->getType();
   SourceLocation Loc = E->getExprLoc();
 
   EmitAlignmentAssumption(PtrValue, Ty, Loc, AssumptionLoc, Alignment,
                           OffsetValue);

clang/lib/Sema/SemaChecking.cpp

bool Sema::SemaBuiltinAssumeAligned(CallExpr *TheCall) {
   if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
     llvm::APSInt Result;
     if (SemaBuiltinConstantArg(TheCall, 1, Result))
       return true;
 
     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) {
     ExprResult Arg(TheCall->getArg(2));
     InitializedEntity Entity = InitializedEntity::InitializeParameter(Context,
       Context.getSizeType(), false);
     Arg = PerformCopyInitialization(Entity, SourceLocation(), Arg);
     if (Arg.isInvalid()) return true;

clang/test/Sema/builtin-assume-aligned.c

 
 void *test_no_fn_proto() __attribute__((assume_aligned(31))); // expected-error {{requested alignment is not a power of 2}}
 void *test_no_fn_proto() __attribute__((assume_aligned(32, 73))); // no-warning
 
 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())); // 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];
+}