[Clang] Fix instantiation of OpaqueValueExprs (Bug #45964)

[ricejasonf]

Summary

The structured bindings decomposition of a non-dependent array in a dependent context (a template) were, upon instantiation, creating nested OpaqueValueExprs that would trigger assertions in CodeGen. Additionally the OpaqueValuesExpr's contained SourceExpr is being emitted in CodeGen, but there was no code for its transform in template instantiation. This would trigger other assertions such as when emitting a DeclRefExpr that refers to a VarDecl that is not marked as ODR-used.

This is all based on cursory deduction, but with the way the code flows from SemaTemplateInstantiate back to SemaInit, it is apparent that the nesting of OpaqueValueExpr is unintentional.

This commit fixes https://bugs.llvm.org/show_bug.cgi?id=45964 and possible other issues involving OpaqueValueExprs in template instantiations might be resolved.

[ricejasonf]

Fixed the formatting with clang-format

[tambre]

Please:

• Add a test case based on the one in the bug. Hopefully you'll be able to reduce it further now that you understand the bug.
• Use imperative style for the commit message, i.e. "Fix instantiation of OpaqueValueExprs".

[ricejasonf]

I added a simple lit test. The case is the reduced case mentioned in the bug. I spent some time trying to reproduce this outside decomposition. The only other way (I know of) to produce an ArrayInitLoopExpr is an implicit copy constructor, but the AST for that is generated lazily so it never gets transformed. Other entities containing OpaqueValueExpr do not appear to have this problem either. Let me know if there are any more deficiencies or suggested changes. Thank you.

[aaron.ballman]

clang/lib/Sema/SemaInit.cpp

8646

clang/lib/Sema/TreeTransform.h

10515

10519–10521

clang/test/SemaCXX/pr45964-nested-ove.cpp

1

I think this test should live in CodeGenCXX rather than SemaCXX as it's testing codegen stuff (and rather than discarding the output, I'd recommend checking the output with FileCheck to verify that it looks valid).

[ricejasonf]

I moved the test to CodeGenCXX and actually test output with FileCheck. I used the code from a non-template function as the expected output. Some minor formatting changes are also addressed.

[aaron.ballman]

clang/lib/Sema/TreeTransform.h

10515–10525

These changes look correct to me, but I am not certain why they've been unnecessary for so long. This code is 11 years old and has never transformed the OpaqueValueExpr before (see https://github.com/llvm/llvm-project/commit/8d69a2160e48b73a4515c9401e67971fb8c14e07). @rjmccall, was this an oversight and we just got lucky for a long time, or is there a reason we didn't transform these expressions?

[rjmccall]

clang/lib/Sema/TreeTransform.h

10515–10525

OVE always has to appear within a containing construct that has to handle it specially to preserve OVE equality. It is likely that the structured bindings transform doesn't do that properly. That should probably be fixed rather than trying to handle it here; creating a new OVE every time we encounter one is very problematic. Perhaps this case should assert that it's unreachable.

For example, TreeTransform on PseudoObjectExpr recreates the syntactic form and then transforms that.

[ricejasonf]

I think I found a simple solution that bypasses transforming the OVE. The ArrayInitLoop always has an OVE in it so I strip it along with it in TransformInitializer.

I looked around in CodeGen and might have a better understanding of the purpose of OVEs now. The same pointer to the OVE appears twice inside the AIL. CodeGen keeps a mapping of the result of the source expression to the OVE so it emits the same result both times it encounters the OVE. Does that sound right?

[rjmccall]

Yes, that's right. OVE allows us to logically place a single expression in multiple positions in the expression hierarchy. This can be useful for either source or semantic fidelity.

[ricejasonf]

Fixed minor formatting issue. 😅 (Perhaps this should be in the tests included with check-clang?)

[ricejasonf]

I rebased onto main and made one small style fix. I don't think the rebase changes anything.

I do not understand why random things are failing in the CI. Even on my own build machine with a fresh build directory CMake fails with errors about unregistered files in ExecutionEngine/Orc.

All this stuff is completely unrelated to my very minor change. Is there something about the process that I am missing?

[tambre]

In D108482#3041127, @ricejasonf wrote:

I rebased onto main and made one small style fix. I don't think the rebase changes anything.

I do not understand why random things are failing in the CI. Even on my own build machine with a fresh build directory CMake fails with errors about unregistered files in ExecutionEngine/Orc.

All this stuff is completely unrelated to my very minor change. Is there something about the process that I am missing?

CI looks clean to me except for the Flang failure, which seems to have been reverted.

[tambre]

clang/test/CodeGenCXX/pr45964-decomp-transform.cpp

1 ↗

(On Diff #377033)

I'd prefer -std=c++17 since the standard and flag have been finalized.

[ricejasonf]

Fix up c++ version in test.

[aaron.ballman]

LGTM aside from a nit, but please give some time for @rjmccall to review as well in case he spots something I've missed.

clang/lib/Sema/TreeTransform.h

3845

No need for the cast, this function already returns that type.

[ricejasonf]

Remove unnecessary cast.

[rjmccall]

I assume we rebuild an AILE and OVE when we process the initializer and see it's a structured binding? In that case, this should be fine.

[ricejasonf]

In D108482#3047141, @rjmccall wrote:

I assume we rebuild an AILE and OVE when we process the initializer and see it's a structured binding? In that case, this should be fine.

Yes, the original problem was that the old OVE was being nested in the new one resulting in AILE->OVE->OVE. CodeGen was triggering an assert as it is apparently not equipped to handle multiple OVEs simultaneously or at least not when they are directly nested like that.

[ricejasonf]

Can this be accepted if there are no other issues?

[rjmccall]

LGTM, then.

[tambre]

I presume you lack the permissions to push this to the main repo yourself. Would you like me to do that for you?

[ricejasonf]

In D108482#3105889, @tambre wrote:

I presume you lack the permissions to push this to the main repo yourself. Would you like me to do that for you?

Yes, please.

[tambre]

In D108482#3108013, @ricejasonf wrote:

In D108482#3105889, @tambre wrote:

I presume you lack the permissions to push this to the main repo yourself. Would you like me to do that for you?

Yes, please.

Please provide the name and email you want it committed as. Unfortunately Phabricator diffs don't include this info.

[ricejasonf]

In D108482#3111114, @tambre wrote:

In D108482#3108013, @ricejasonf wrote:

In D108482#3105889, @tambre wrote:

I presume you lack the permissions to push this to the main repo yourself. Would you like me to do that for you?

Yes, please.

Please provide the name and email you want it committed as. Unfortunately Phabricator diffs don't include this info.

Jason Rice

[e-mail]

Thanks

clang/lib/Sema/SemaInit.cpp

for (step_iterator Step = step_begin(), StepEnd = step_end();
     case SK_ObjCObjectConversion:
       CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type,
                           CK_ObjCObjectLValueCast,
                           CurInit.get()->getValueKind());
       break;
 
     case SK_ArrayLoopIndex: {
       Expr *Cur = CurInit.get();
-      Expr *BaseExpr = new (S.Context)
+      // prevent nested OpaqueValueExprs
+      Expr *BaseExpr = dyn_cast<OpaqueValueExpr>(Cur);
+      if (!BaseExpr) {
+        BaseExpr = new (S.Context)
             OpaqueValueExpr(Cur->getExprLoc(), Cur->getType(),
                             Cur->getValueKind(), Cur->getObjectKind(), Cur);
+      }
       Expr *IndexExpr =
           new (S.Context) ArrayInitIndexExpr(S.Context.getSizeType());
       CurInit = S.CreateBuiltinArraySubscriptExpr(
           BaseExpr, Kind.getLocation(), IndexExpr, Kind.getLocation());
       ArrayLoopCommonExprs.push_back(BaseExpr);
       break;
     }
 

clang/lib/Sema/TreeTransform.h

ExprResult TreeTransform<Derived>::TransformInitializer(Expr *Init,
   if (!Init)
     return Init;
 
   if (auto *FE = dyn_cast<FullExpr>(Init))
     Init = FE->getSubExpr();
 
   if (auto *AIL = dyn_cast<ArrayInitLoopExpr>(Init))
     Init = AIL->getCommonExpr();
 
   if (MaterializeTemporaryExpr *MTE = dyn_cast<MaterializeTemporaryExpr>(Init))
     Init = MTE->getSubExpr();
 
   while (CXXBindTemporaryExpr *Binder = dyn_cast<CXXBindTemporaryExpr>(Init))
     Init = Binder->getSubExpr();
 
   if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(Init))
     Init = ICE->getSubExprAsWritten();
return getDerived().RebuildOffsetOfExpr(E->getOperatorLoc(), Type,
                                           Components, E->getRParenLoc());
 }
 
 template<typename Derived>
 ExprResult
 TreeTransform<Derived>::TransformOpaqueValueExpr(OpaqueValueExpr *E) {
   assert((!E->getSourceExpr() || getDerived().AlreadyTransformed(E->getType())) &&
          "opaque value expression requires transformation");
+
+  // Note that SourceExpr can be nullptr
+  ExprResult SourceExpr = TransformExpr(E->getSourceExpr());
+  if (SourceExpr.isInvalid())
+    return ExprError();
+  if (SourceExpr.get() == E->getSourceExpr() && !getDerived().AlwaysRebuild()) {
     return E;
   }
 
+  OpaqueValueExpr *New = new (SemaRef.Context)
+      OpaqueValueExpr(E->getExprLoc(), E->getType(), E->getValueKind(),
+                      E->getObjectKind(), SourceExpr.get());
+  return New;
+}
+
 template<typename Derived>
 ExprResult
 TreeTransform<Derived>::TransformTypoExpr(TypoExpr *E) {
   return E;
 }
 
 template <typename Derived>
 ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {

clang/test/SemaCXX/pr45964-nested-ove.cpp

+// RUN: %clang_cc1 -std=c++17 -emit-codegen-only -verify %s
+// Don't crash (Bugzilla - Bug 45964).
+
+// non-dependent ArrayInitLoop should not, upon instantiation,
+// contain an OpaqueValueExpr with a nested OpaqueValueExpr or an
+// uninstantiated source expr. (triggers asserts in CodeGen)
+
+// expected-no-diagnostics
+int a[1];
+template <int> void b() {
+  auto [c] = a;
+}
+void (*d)(){b<0>};