[Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

[shafik]

Summary

If we provide too large a value for the alignment attribute APInt::getZExtValue() will assert. This PR adds a active bits check and folds it into the MaximumAlignment check.

This fixes: https://github.com/llvm/llvm-project/issues/50534

[erichkeane]

clang/lib/Sema/SemaDeclAttr.cpp

4498–4499

What is the purpose of using 'ActiveBits' here? Why is this not a sub-set of the operator > from below? That is, 'ActiveBits' allows for 0xFF when the 'MaxValue' is 0x80 (top bit set), but the operator > would have caught that anyway, right?

Also, how does 'ActiveBits' work with negative numbers?

[erichkeane]

clang/lib/Sema/SemaDeclAttr.cpp

4502

So looking more closely, THIS is the problem right here. I think we probably should just be storing MaximumAlignment and AlignVal as an llvm::APInt and just do the comparison on APInt, rather than try to be tricky with the bits here.

Basically, don't extract the 'Alignment' from the APInt until after the test for >.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

4502

You're correct that this is the cause of the assertion, which happens when Alignment can't fit in a uint64_t.

It took me a minute to see what you meant, but I agree, we can test whether Alignment < MaximumAlignment and rely on the fact that MaximumAlignment will be representable in a 64-bit integer. I think I led @shafik astray with a comment I made on the issue.

[shafik]

• Switched to using APSInt::operator> as discussed offline w/ Erich and Aaron

[shafik]

clang/test/Sema/attr-aligned.c

1

The previous triple did not support __int128_t

[erichkeane]

Needs a release note. Also, the changing of order is POSSIBLY a change in behavior (in that we're now diagnosing 'out of range' before 'power of 2'), but I don't think we care.

[aaron.ballman]

LGTM with a release note.

In D152335#4404114, @erichkeane wrote:

Needs a release note. Also, the changing of order is POSSIBLY a change in behavior (in that we're now diagnosing 'out of range' before 'power of 2'), but I don't think we care.

I don't consider that a behavioral change to be worried about. Telling the user "this value is too big" before telling them "it's not a power of two" seems equivalent or perhaps slightly better than "this is not a power of two" and then "oh, and it's too big."

[erichkeane]

In D152335#4404118, @aaron.ballman wrote:

LGTM with a release note.

In D152335#4404114, @erichkeane wrote:

Needs a release note. Also, the changing of order is POSSIBLY a change in behavior (in that we're now diagnosing 'out of range' before 'power of 2'), but I don't think we care.

I don't consider that a behavioral change to be worried about. Telling the user "this value is too big" before telling them "it's not a power of two" seems equivalent or perhaps slightly better than "this is not a power of two" and then "oh, and it's too big."

Well, its telling them "this value is too big" instead of "its not a power of two" rather than the other way around. But yeah, i don't think I care? I'm a little frightened there is no test that fails because of this, but not surprised.

[shafik]

• Add release note

clang/lib/Sema/SemaDeclAttr.cpp

void Sema::AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
 
   // FIXME: Cache the number on the AL object?
   llvm::APSInt Alignment;
   ExprResult ICE = VerifyIntegerConstantExpression(
       E, &Alignment, diag::err_aligned_attribute_argument_not_int);
   if (ICE.isInvalid())
     return;
 
+  uint64_t MaximumAlignment = Sema::MaximumAlignment;
+  if (Context.getTargetInfo().getTriple().isOSBinFormatCOFF())
+    MaximumAlignment = std::min(MaximumAlignment, uint64_t(8192));
+  if (Alignment > MaximumAlignment) {
+    Diag(AttrLoc, diag::err_attribute_aligned_too_great)
+        << MaximumAlignment << E->getSourceRange();
+    return;
+  }
+
   uint64_t AlignVal = Alignment.getZExtValue();
   // C++11 [dcl.align]p2:
   //   -- if the constant expression evaluates to zero, the alignment
   //      specifier shall have no effect
   // C11 6.7.5p6:
   //   An alignment specification of zero has no effect.
   if (!(TmpAttr.isAlignas() && !Alignment)) {
     if (!llvm::isPowerOf2_64(AlignVal)) {
       Diag(AttrLoc, diag::err_alignment_not_power_of_two)
         << E->getSourceRange();
       return;
     }
   }
 
-  uint64_t MaximumAlignment = Sema::MaximumAlignment;
-  if (Context.getTargetInfo().getTriple().isOSBinFormatCOFF())
-    MaximumAlignment = std::min(MaximumAlignment, uint64_t(8192));
-  if (AlignVal > MaximumAlignment) {
-    Diag(AttrLoc, diag::err_attribute_aligned_too_great)
-        << MaximumAlignment << E->getSourceRange();
-    return;
-  }
-
   const auto *VD = dyn_cast<VarDecl>(D);
   if (VD) {
     unsigned MaxTLSAlign =
         Context.toCharUnitsFromBits(Context.getTargetInfo().getMaxTLSAlign())
             .getQuantity();
     if (MaxTLSAlign && AlignVal > MaxTLSAlign &&
         VD->getTLSKind() != VarDecl::TLS_None) {
       Diag(VD->getLocation(), diag::err_tls_var_aligned_over_maximum)
           << (unsigned)AlignVal << VD << MaxTLSAlign;
       return;
     }
   }

clang/test/Sema/attr-aligned.c

-// RUN: %clang_cc1 -triple i386-apple-darwin9 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fsyntax-only -verify %s
 
 int x __attribute__((aligned(3))); // expected-error {{requested alignment is not a power of 2}}
 int y __attribute__((aligned(1ull << 33))); // expected-error {{requested alignment must be 4294967296 bytes or smaller}}
 int y __attribute__((aligned(1ull << 32)));
+// GH50534
+int z __attribute__((aligned((__int128_t)0x1234567890abcde0ULL << 64))); // expected-error {{requested alignment must be 4294967296 bytes or smaller}}
 
 // PR26444
 int y __attribute__((aligned(1 << 29)));
 int y __attribute__((aligned(1 << 28)));
 
 // PR3254
 short g0[3] __attribute__((aligned));
 short g0_chk[__alignof__(g0) == 16 ? 1 : -1];