This is an archive of the discontinued LLVM Phabricator instance.

[modules] Delay calling DeclMustBeEmitted until it's safe
ClosedPublic

Authored by teemperor on Mar 9 2017, 2:31 PM.

Details

Reviewers
rsmith
Summary

This patch implements the suggestion in D29753 that calling DeclMustBeEmitted in the middle of deserialization should be avoided and that the actual check should be deferred until it's safe to do so.

This patch fixes the attached test case we encountered when compiling our code with modules: Here clang would crash when accessing the invalid redecl chains while trying to evaluate the value of a const VarDecl that contains a function call. I've also added a test case for the the assertion fail hat Richard mentioned in D29753 which is also fixed by this patch.

Diff Detail

Event Timeline

teemperor created this revision.Mar 9 2017, 2:31 PM
teemperor added a reviewer: rsmith.
teemperor added a subscriber: v.g.vassilev.
rsmith edited edge metadata.Mar 9 2017, 3:27 PM

Can you also add the test from PR32186?

include/clang/Serialization/ASTReader.h
974–983

Too much indentation here.

982

Please lowercase the first letter of this function.

lib/Serialization/ASTReaderDecl.cpp
3634

Too much indentation.

3648

I'm concerned about this call; loadDeclUpdateRecords is called when the AST invariants are not met, so it's not safe to call DeclMustBeEmitted from here in general.

There are two ways we can get here: either we just loaded a declaration and we're updating it (in which case we can entirely skip both this check and adding an entry to PotentiallyInterestingDecls, because there already is one), or it's a pre-existing declaration and we've just loaded a new module file (in which case I think it's safe to call DeclMustBeEmitted from here). We could track which case we're in on PendingUpdateRecords and conditionally skip this.

3668

This call looks problematic too. Can we just remove it? We're going to do the same check again later anyway.

teemperor updated this revision to Diff 91402.Mar 10 2017, 1:32 PM
  • Renamed InterestingDecls to PotentiallyInterestingDecls to better reflect its purpose.
  • Ran patch through clang-format (now even with the correct code-style) and did the other requested style fixes.
  • Added test from PR32186.
bruno added a comment.Mar 10 2017, 1:39 PM

@teemperor, can you rename PR32186 to something more meaningful?

teemperor marked 4 inline comments as done.Mar 10 2017, 1:40 PM
teemperor added inline comments.
lib/Serialization/ASTReaderDecl.cpp
3668

The purpose seems to be that we only send every declaration once to the consumer.

All initially interesting decls were already in the list from the start and we now add all decls that became interesting since then. Without this check we send some declarations multiple times to the consumer and it seems to me that this wasn't intended (even though no test case in clang actually seems to test against this behavior)

What we could do is make the final list of declarations value-unique before passing it o the consumer and then remove those two checks above?

teemperor updated this revision to Diff 91411.Mar 10 2017, 2:14 PM
teemperor marked an inline comment as done.
  • Prefixed PR31863.cpp with something meaningful.
  • Added comment to delaly-emit-check.cpp to explain what we test there.
rsmith added inline comments.Mar 22 2017, 3:41 PM
lib/Serialization/ASTReaderDecl.cpp
3649

Why is this safe to call now? If we just loaded D, this could trigger deserializations and generally have the same problems as other calls to this function.

teemperor updated this revision to Diff 92798.Mar 23 2017, 8:02 AM
  • Removed the work-in-progress changes to: loadDeclUpdateRecords
  • Added FIXME that this function should be patched too.

Our code is fixed with this patch and fixing the unsafe calls in loadDeclUpdateRecords seems like a more complicated problem. I would say this is out of scope for this patch. I'll make another patch that addresses loadDeclUpdateRecords and keep this patch just for fixing the single isConsumerInterestedIn call in ReadDeclRecord.

teemperor updated this revision to Diff 94655.Apr 10 2017, 2:14 AM
  • Fixed compilation error in the cleaned up version (removed two lines too much...)
rsmith accepted this revision.Apr 10 2017, 2:09 PM

This is not a complete fix, since the call within loadDeclUpdateRecords could still be problematic, but it's at least an improvement.

Also, phabricator is not showing any test changes with the latest version of the patch. I assume they're still there and unchanged from the previous version.

lib/Serialization/ASTReaderDecl.cpp
3676

Delete this call to isConsumerInterestedIn -- there's no point calling it here given that PassInterestingDeclsToConsumer is going to do the same check.

This revision is now accepted and ready to land.Apr 10 2017, 2:09 PM
teemperor updated this revision to Diff 94860.Apr 11 2017, 11:09 AM
  • Readded the missing tests to get the final diff.
v.g.vassilev closed this revision.Apr 12 2017, 3:09 PM

Landed in r300110.