This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ricejasonf on Aug 20 2021, 1:25 PM.

Details

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.

Diff Detail

Event Timeline

ricejasonf requested review of this revision.Aug 20 2021, 1:25 PM
ricejasonf created this revision.

Fixed the formatting with clang-format

tambre requested changes to this revision.Aug 21 2021, 1:24 AM
tambre added a subscriber: 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".
This revision now requires changes to proceed.Aug 21 2021, 1:24 AM
ricejasonf retitled this revision from [Clang] Fixes instantiation of OpaqueValueExprs (Bug #45964) to [Clang] Fix instantiation of OpaqueValueExprs (Bug #45964).Aug 24 2021, 3:48 PM

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 added inline comments.
clang/lib/Sema/SemaInit.cpp
8646 ↗(On Diff #368485)
clang/lib/Sema/TreeTransform.h
10560
10564–10566
clang/test/SemaCXX/pr45964-nested-ove.cpp
1 ↗(On Diff #368485)

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

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 added a subscriber: rjmccall.
aaron.ballman added inline comments.
clang/lib/Sema/TreeTransform.h
10560–10570

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 added inline comments.Aug 30 2021, 11:21 AM
clang/lib/Sema/TreeTransform.h
10560–10570

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.

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?

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 marked 2 inline comments as done.

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

ricejasonf updated this revision to Diff 377033.Oct 4 2021, 2:16 PM

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 added a comment.Oct 5 2021, 1:47 AM

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 added inline comments.Oct 5 2021, 1:48 AM
clang/test/CodeGenCXX/pr45964-decomp-transform.cpp
2

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

ricejasonf marked 2 inline comments as done.

Fix up c++ version in test.

aaron.ballman accepted this revision.Oct 6 2021, 5:10 AM

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
3857

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

ricejasonf updated this revision to Diff 377566.Oct 6 2021, 9:07 AM
ricejasonf marked an inline comment as done.

Remove unnecessary cast.

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 marked an inline comment as done.Oct 7 2021, 9:44 AM

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.

Can this be accepted if there are no other issues?

rjmccall accepted this revision.Oct 22 2021, 11:38 AM

LGTM, then.

tambre added a comment.Nov 3 2021, 6:46 AM

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

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 added a comment.Nov 5 2021, 3:32 AM

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.

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

ricejasonf@gmail.com

Thanks

This revision was not accepted when it landed; it landed in state Needs Review.Nov 6 2021, 1:09 AM
This revision was automatically updated to reflect the committed changes.