This is an archive of the discontinued LLVM Phabricator instance.

[coroutines][PR41909] Don't build dependent coroutine statements for generic lambda
ClosedPublic

Authored by modocache on May 28 2019, 1:56 PM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=41909 describes an issue in which
a generic lambda that takes a dependent argument auto set causes the
template instantiation machinery for coroutine body statements to crash
with an ICE. The issue is two-fold:

  1. The paths taken by the template instantiator contain several asserts that the coroutine promise must not have a dependent type.
  2. The template instantiator unconditionally builds corotuine statements that depend on the promise type, which cannot be dependent.

To work around the issue, prevent the template instantiator from building
dependent coroutine statements if the coroutine promise type is dependent.
Since we only expect this to occur in the case of a generic lambda, limit
the workaround behavior to just that case.

Diff Detail

Event Timeline

modocache created this revision.May 28 2019, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 1:56 PM
GorNishanov accepted this revision.May 30 2019, 11:03 AM

LGTM! Thank you for the fix

This revision is now accepted and ready to land.May 30 2019, 11:03 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2019, 5:47 PM

Great, thanks for the review!

rsmith added a subscriber: rsmith.Jun 2 2019, 11:40 PM
rsmith added inline comments.
cfe/trunk/lib/Sema/TreeTransform.h
7173 ↗(On Diff #202627)

This assert doesn't seem correct to me; there's no reason to assume we have a generic lambda here. (A non-generic lambda inside a generic lambda, whose parameter types are dependent on the generic lambda's parameters, would hit exactly the same issue, for instance.)

Generally we should write the template instantiation code so it could be used to substitute into the definition of a function template to produce another function template, even if we happen to never call it that way right now (though deduction guide processing gets pretty close). The right thing to do here is to either substitute into the representation we already formed (building a dependent representation if it's still dependent and a non-dependent representation otherwise) or to rebuild the coroutine body from scratch (again, creating a dependent representation if the result is still dependent).

modocache added inline comments.Jun 15 2019, 1:10 PM
cfe/trunk/lib/Sema/TreeTransform.h
7173 ↗(On Diff #202627)

Sorry I missed this! Indeed you're right, the check here is weak, and even with this patch the following coroutines code crashes in the same way: https://godbolt.org/z/ixLpCq

I'll send a follow-up patch that attempts to implement one of the approaches you describe. Thanks for the guidance!