Page MenuHomePhabricator

[Sema] Fix assertion when `auto` parameter in lambda has an attribute.
ClosedPublic

Authored by vsapsai on Feb 25 2019, 5:40 PM.

Details

Summary

Fixes the assertion

no Attr* for AttributedType*
UNREACHABLE executed at llvm-project/clang/lib/Sema/SemaType.cpp:298!

In TypeProcessingState::getAttributedType we put into AttrsForTypes
types with auto but later in
TypeProcessingState::takeAttrForAttributedType we use transformed
types and that's why cannot find Attr corresponding to
AttributedType.

Fix by keeping AttrsForTypes up to date after replacing AutoType.

rdar://problem/47689465

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Feb 25 2019, 5:40 PM

Have doubts that checking only the outermost type for being AttributedType after Sema::ReplaceAutoType is sufficient but haven't found a counterexample yet. Decided to post the proposed fix to see if others have any ideas.

jkorous added inline comments.Feb 26 2019, 3:42 PM
clang/lib/Sema/SemaType.cpp
266 ↗(On Diff #188283)

Do you think it would make sense to terminate the loop early if AttrsForTypesSorted == true?

270 ↗(On Diff #188283)

Do you think it would be reasonable to conditionally assign (or rather keep) AttrsForTypesSorted = true in case they were sorted previously and new key doesn't break the order?

vsapsai added inline comments.Mar 4 2019, 7:38 PM
clang/lib/Sema/SemaType.cpp
266 ↗(On Diff #188283)

I agree with you it would be nice to reuse AttrsForTypesSorted property but in practice it doesn't matter. We sort AttrsForTypes only in takeAttrForAttributedType and we should do that after we've populated AttrsForTypes and after we've replaced auto types. It isn't strictly required but that's how it was working in debugger. Also I've tried to add assert(! AttrsForTypesSorted); and it wasn't triggered during ninja check-clang

Having AttrsForTypes sorted can be useful in the future but I don't think we should add code for that now.

Despite the linear iteration, we are not pessimizing the most common use case with no attributes for lambda parameters (expression "most common use case" is based on my experience and not substantiated by any data). So I think the performance shouldn't degrade noticeably.

aaron.ballman added inline comments.Mar 6 2019, 9:36 AM
clang/lib/Sema/SemaType.cpp
261 ↗(On Diff #188283)

I think this work should be done within SubstituteDeducedTypeTransform rather than on the side. Any caller to Sema::ReplaceAutoType() should get this same behavior.

vsapsai added inline comments.Mar 11 2019, 1:04 PM
clang/lib/Sema/SemaType.cpp
261 ↗(On Diff #188283)

Doing this work in SubstituteDeducedTypeTransform involves teaching SubstituteDeducedTypeTransform about TypeProcessingState and probably adding TypeProcessingState to Sema::ReplaceAutoType(). As for me, it exposes TypeProcessingState in more places than necessary. And it feels somewhat awkward that SubstituteDeducedTypeTransform is used in multiple places but TypeProcessingState is required only here.

I've modelled my approach after TypeProcessingState::getAttributedType where it forwards the call to Sema and keeps AttrsForTypes up to date. Do you still think SubstituteDeducedTypeTransform would be a better place for this code?

aaron.ballman accepted this revision.Mar 26 2019, 11:29 AM

LGTM!

clang/lib/Sema/SemaType.cpp
261 ↗(On Diff #188283)

Hmm, yeah, it doesn't seem like it would make sense there. This is keeping the TypeProcessingState up to date which is really only of interest during some stages of processing.

This revision is now accepted and ready to land.Mar 26 2019, 11:29 AM

Thanks for the review, Aaron.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2019, 11:45 AM