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

Thu Oct 10 12:47:02 PDT 2019

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<IgnoredAttributes>;

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