[Sema] Attempt to perform call-size-specific `__attribute__((alloc_align(param_idx)))` validation

[lebedev.ri]

Summary

alloc_align attribute takes parameter number, not the alignment itself,
so given just the attribute/function declaration we can't do any
sanity checking for said alignment.

However, at call site, given the actual Expr that is passed
into that parameter, we might be able to evaluate said Expr
as Integer Constant Expression, and perform the sanity checks.
But since there is no requirement for that argument to be an immediate,
we may fail, and that's okay.

However if we did evaluate, we should enforce the same constraints
as with __builtin_assume_aligned()/__attribute__((assume_aligned(imm))):
said alignment is a power of two, and is not greater than our magic threshold

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

3659

Does this need to be isInstantionDependent?

3669

I thought we had this stored somewhere else? We probably should have this be a constant somewhere in the frontend. I THINK I remember doing a review where I pulled this value into clang somewhere...

[jdoerfert]

clang/lib/Sema/SemaChecking.cpp

3669

That was D72998, and I don't think Clang is the right place for this constant. It is a property of the llvm alignment attribute and it should live there. Thus, llvm/include/Attributes.h or some similar place. Can't we "fix" the linker error by making it a constexpr global or are the errors because of other file content? If the latter, we could go with a llvm/include/magic_constants.h ;)

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

3669

The one I was thinking of was this one: https://reviews.llvm.org/D68824

I don't remember what we came up with on the linking issue. It would be really nice if it was just something included from LLVM, but I think SEMA typically doesn't include stuff from LLVM either.

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

3659

I'm not sure, should it be?
It seems there is no such check in Sema::SemaBuiltinAssumeAligned().

[jdoerfert]

clang/lib/Sema/SemaChecking.cpp

3669

I'm not too happy with the duplication of the constant but defining it once in clang is certainly better than having it in N places. For OpenMP we look into LLVM during SEMA and here there is an argument to be made that we should as well. I imagine more cases would pop up over time.

FWIW, if we allow to include LLVM headers, e.g., from IR or Frontend, we could still have a wrapper in SEMA to get the information so it doesn't expose the llvm:: namespace at the use sides (if that helps).

[hfinkel]

clang/lib/Sema/SemaChecking.cpp

3669

For OpenMP we look into LLVM during SEMA

How do we do that?

There's certainly an interesting philosophical issue around whether changes in LLVM should directly manifest as Clang behavioral changes, especially in -fsyntax-only. The answer to this question might be different for extensions vs. core language features (although alignment restrictions might implicate both). AFAIKT, historically , our answer has been to insist on separation.

[jdoerfert]

clang/lib/Sema/SemaChecking.cpp

3669

For OpenMP we look into LLVM during SEMA

How do we do that?

I was referring to code like this https://reviews.llvm.org/D71830#C1739755NL11085
which is in CodeGen right now but has to move to SemaOverload. The code is completely reusable between Clang and Flang so I put it in lib/Frontend/OpenMP and I think that is the right place for it.

There's certainly an interesting philosophical issue around whether changes in LLVM should directly manifest as Clang behavioral changes, especially in -fsyntax-only. The answer to this question might be different for extensions vs. core language features (although alignment restrictions might implicate both). AFAIKT, historically , our answer has been to insist on separation.

I get that in a general sense. For the problem at hand, and as far as I known, the restriction stems only from the LLVM-IR restriction, correct? If so, what is the argument for separation? I mean, a change of the value in LLVM might directly impact Clang behavior.

I could also see us clamping the alignment during codegen. While that might have other problems they seem less practical to me.

[hfinkel]

clang/lib/Sema/SemaChecking.cpp

3669

I was referring to code like this https://reviews.llvm.org/D71830#C1739755NL11085
which is in CodeGen right now but has to move to SemaOverload. The code is completely reusable between Clang and Flang so I put it in lib/Frontend/OpenMP and I think that is the right place for it.

Fair, but that's a library designed to be a home for cross-language frontend components. The variant-selection logic to which you're referring, itself, does not actually need to link to LLVM's IR library, correct?

I get that in a general sense. For the problem at hand, and as far as I known, the restriction stems only from the LLVM-IR restriction, correct? If so, what is the argument for separation? I mean, a change of the value in LLVM might directly impact Clang behavior.

Yes, I believe that the restriction is necessary because of an underlying LLVM IR restriction. From my perspective, your argument is perfectly rational. Clang only supports code generation using LLVM IR, and a restriction that comes from LLVM should be directly tied to the underlying LLVM threshold regardless of where it is surfaced. We have, however, avoided a linking dependence (I believe, primarily, to help the load times and file sizes of tools based on Clang which don't otherwise need to link to the LLVM IR libraries).

[jdoerfert]

clang/lib/Sema/SemaChecking.cpp

3669

The variant-selection logic to which you're referring, itself, does not actually need to link to LLVM's IR library, correct?

Correct.

We have, however, avoided a linking dependence (I believe, primarily, to help the load times and file sizes of tools based on Clang which don't otherwise need to link to the LLVM IR libraries).

That makes total sense to me. The idea of a separate header for magic constants, e.g., llvm/include/llvm/IR/IRConstants.h or llvm/include/llvm/Frontend/Constants.h, was to prevent any linking issues. Anyway, if people feel having the constant defined twice is better for now I won't object.

[lebedev.ri]

isValueDependent() already implies isTypeDependent(), don't recheck it.

clang/lib/Sema/SemaChecking.cpp

3659

As disscussed in IRC, that's https://clang.llvm.org/doxygen/classclang_1_1Expr.html#a954398acd24dedcfc181659f0b906228 clang::Expr::isInstantiationDependent(), i think this is consistent with other places,
and *might* be correct. Or other places are incorrect too.
It isn't obvious to me how to produce a testcase, so this might be better left as-is.

[lebedev.ri]

Thank you for the review!

[lebedev.ri]

Rebased, NFC.

[lebedev.ri]

Seeing some bot failures, reverted this one.

clang/lib/Sema/SemaChecking.cpp

if (FDecl || Proto) {
 
     // Type safety checking.
     if (FDecl) {
       for (const auto *I : FDecl->specific_attrs<ArgumentWithTypeTagAttr>())
         CheckArgumentWithTypeTag(I, Args, Loc);
     }
   }
 
+  if (FDecl && FDecl->hasAttr<AllocAlignAttr>()) {
+    auto *AA = FDecl->getAttr<AllocAlignAttr>();
+    const Expr *Arg = Args[AA->getParamIndex().getASTIndex()];
+    if (!Arg->isValueDependent()) {
+      llvm::APSInt I(64);
+      if (Arg->isIntegerConstantExpr(I, Context)) {
+        if (!I.isPowerOf2()) {
+          Diag(Arg->getExprLoc(), diag::err_alignment_not_power_of_two)
+              << Arg->getSourceRange();
+          return;
+        }
+
+        // Alignment calculations can wrap around if it's greater than 2**29.
+        unsigned MaximumAlignment = 536870912;
+        if (I > MaximumAlignment)
+          Diag(Arg->getExprLoc(), diag::warn_assume_aligned_too_great)
+              << Arg->getSourceRange() << MaximumAlignment;
+      }
+    }
+  }
+
   if (FD)
     diagnoseArgDependentDiagnoseIfAttrs(FD, ThisArg, Args, Loc);
 }
 
 /// CheckConstructorCall - Check a constructor call for correctness and safety
 /// properties not enforced by the C type system.
 void Sema::CheckConstructorCall(FunctionDecl *FDecl,
                                 ArrayRef<const Expr *> Args,

clang/test/Sema/alloc-align-attr.c

 // argument type
 void *test_bad_param_type(void) __attribute((alloc_align(1.1))); // expected-error {{'alloc_align' attribute requires parameter 1 to be an integer constant}}
 
 // argument count
 void *test_no_fn_proto(int x, int y) __attribute__((alloc_align)); // expected-error {{'alloc_align' attribute takes one argument}}
 void *test_no_fn_proto(int x, int y) __attribute__((alloc_align())); // expected-error {{'alloc_align' attribute takes one argument}}
 void *test_no_fn_proto(int x, int y) __attribute__((alloc_align(32, 45, 37))); // expected-error {{'alloc_align' attribute takes one argument}}
 
+void *passthrought(int a) {
+  return test_ptr_alloc_align(a);
+}
+void *align16() {
+  return test_ptr_alloc_align(16);
+}
+void *align15() {
+  return test_ptr_alloc_align(15); // expected-error {{requested alignment is not a power of 2}}
+}
+void *align536870912() {
+  return test_ptr_alloc_align(1073741824); // expected-warning {{requested alignment must be 536870912 bytes or smaller; maximum alignment assumed}}
+}

clang/test/SemaCXX/alloc-align-attr.cpp

 };
 
 template <typename T>
 void* dependent_param_func(T param) __attribute__((alloc_align(1)));// expected-error {{'alloc_align' attribute argument may only refer to a function parameter of integer type}}
 
 template <int T>
 void* illegal_align_param(int p) __attribute__((alloc_align(T))); // expected-error {{'alloc_align' attribute requires parameter 1 to be an integer constant}}
 
-void dependent_impl() {
+void dependent_impl(int align) {
   dependent_ret<int> a; // expected-note {{in instantiation of template class 'dependent_ret<int>' requested here}}
   a.Foo(1);
   a.Foo2(1);
-  dependent_ret<int*> b; 
-  a.Foo(1);
-  a.Foo2(1);
+  dependent_ret<int *> b;
+  b.Foo(1);
+  b.Foo2(1);
+  b.Foo(3);           // expected-error {{requested alignment is not a power of 2}}
+  b.Foo2(3);          // expected-error {{requested alignment is not a power of 2}}
+  b.Foo(1073741824);  // expected-warning {{requested alignment must be 536870912 bytes or smaller; maximum alignment assumed}}
+  b.Foo2(1073741824); // expected-warning {{requested alignment must be 536870912 bytes or smaller; maximum alignment assumed}}
+  b.Foo(align);
+  b.Foo2(align);
 
   dependent_param_struct<int> c; 
   c.Foo(1);
   dependent_param_struct<float> d; // expected-note {{in instantiation of template class 'dependent_param_struct<float>' requested here}}
   d.Foo(1.0);
   dependent_param_func<int>(1);
   dependent_param_func<float>(1); // expected-note {{in instantiation of function template specialization 'dependent_param_func<float>' requested here}}
 }