[clang] Fix TemplateInstantiator crash transforming loop hint argument

[brunodf-snps]

In cases where multiple template instations occur (e.g. with a generic lambda), the loop hint argument expression may still be value dependent. (It is also not needed to do the constant evaluation when the loop hint is not an unroll count anyway.)

[brunodf-snps]

Godbolt demo of the crash: https://godbolt.org/z/n4EsEdGs9

[llvmbot]

@llvm/pr-subscribers-clang

Author: Bruno De Fraine (brunodf-snps)

Changes

In cases where multiple template instations occur (e.g. with a generic lambda), the loop hint argument expression may still be value dependent. (It is also not needed to do the constant evaluation when the loop hint is not an unroll count anyway.)

---

Full diff: https://github.com/llvm/llvm-project/pull/172289.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+11-7)
• (modified) clang/test/Sema/unroll-template-value-crash.cpp (+25)

diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 35205f40cbcef..dcbedb2c2ee22 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2197,19 +2197,23 @@ TemplateInstantiator::TransformLoopHintAttr(const LoopHintAttr *LH) {
 
   // Generate error if there is a problem with the value.
   if (getSema().CheckLoopHintExpr(TransformedExpr, LH->getLocation(),
-                                  LH->getSemanticSpelling() ==
+                                  /*AllowZero=*/LH->getSemanticSpelling() ==
                                       LoopHintAttr::Pragma_unroll))
     return LH;
 
   LoopHintAttr::OptionType Option = LH->getOption();
   LoopHintAttr::LoopHintState State = LH->getState();
 
-  llvm::APSInt ValueAPS =
-      TransformedExpr->EvaluateKnownConstInt(getSema().getASTContext());
-  // The values of 0 and 1 block any unrolling of the loop.
-  if (ValueAPS.isZero() || ValueAPS.isOne()) {
-    Option = LoopHintAttr::Unroll;
-    State = LoopHintAttr::Disable;
+  if (Option == LoopHintAttr::UnrollCount &&
+      !TransformedExpr->isValueDependent()) {
+    llvm::APSInt ValueAPS =
+        TransformedExpr->EvaluateKnownConstInt(getSema().getASTContext());
+    // The values of 0 and 1 block any unrolling of the loop (also see
+    // handleLoopHintAttr in SemaStmtAttr).
+    if (ValueAPS.isZero() || ValueAPS.isOne()) {
+      Option = LoopHintAttr::Unroll;
+      State = LoopHintAttr::Disable;
+    }
   }
 
   // Create new LoopHintValueAttr with integral expression in place of the
diff --git a/clang/test/Sema/unroll-template-value-crash.cpp b/clang/test/Sema/unroll-template-value-crash.cpp
index d8953c4845c26..cda4b897509b9 100644
--- a/clang/test/Sema/unroll-template-value-crash.cpp
+++ b/clang/test/Sema/unroll-template-value-crash.cpp
@@ -8,3 +8,28 @@ template <int Unroll> void foo() {
   #pragma GCC unroll Unroll
   for (int i = 0; i < Unroll; ++i);
 }
+
+struct val {
+  constexpr operator int() const { return 1; }
+};
+
+// generic lambda (using double template instantiation)
+
+template<typename T>
+void use(T t) {
+  auto lam = [](auto N) {
+    #pragma clang loop unroll_count(N+1)
+    for (int i = 0; i < 10; ++i);
+
+    #pragma unroll N+1
+    for (int i = 0; i < 10; ++i);
+
+    #pragma GCC unroll N+1
+    for (int i = 0; i < 10; ++i);
+  };
+  lam(t);
+}
+
+void test() {
+  use(val());
+}

[erichkeane]

Patch needs a release note

[erichkeane]

sigh This lambda is actually a really bad example here, since the standard doesnt' allow us to do ANY instantiation here until it is non dependent. ONE day we will fix that, but we should come up with an additional test here where this is problematic (perhaps partial specializations? or template member function in a class tempalte?).

[brunodf-snps]

Since your comment, I have tried quite a bit to trigger the crash without using a generic lambda: using partial/full specializations, template members inside template classes, template template parameters..., all without success. Perhaps I lack the imagination to come up with sufficiently complex template instantiation scenarios, but since you mention that the instantiation of lambdas is a current design flaw, I'm starting to suspect that the problem can only arise in the context of this flawed design.

If that is the case, then I think we would still need the modification as a workaround, but we should document in the code that the extra check should no longer be needed if the instantiation of lambdas is ever redesigned?

[erichkeane]

Hmm, im shocked at that, but I guess it is possible. Yes, I think this is valuable to have in anyway then, I was just hoping that we could repro this elsewise.

[erichkeane]

Why does the 'UnrollCount' matter here at all? It seems like the previous code didn't really need to check that?

[brunodf-snps]

I actually think that was a bug, this code would disable unrolling in cases where it's not appropriate. Compare the loop metadata in these two functions: https://godbolt.org/z/cWTrYMThb

[erichkeane]

Hmm, im shocked at that, but I guess it is possible. Yes, I think this is valuable to have in anyway then, I was just hoping that we could repro this elsewise.