This is an archive of the discontinued LLVM Phabricator instance.

[clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.
Needs ReviewPublic

Authored by teemperor on May 30 2020, 1:56 PM.

Details

Summary

Decl::dump is primarily used for debugging to visualise the current state of a declaration. Usually
Decl::dump just displays the current state of the Decl and doesn't actually change any of its
state, however since commit 457226e02a6e8533eaaa864a3fd7c8eeccd2bf58 the method
actually started loading additional declarations from the ExternalASTSource. This causes that
calling Decl::dump during a debugging session now actually does permanent changes to the
AST and will cause the debugged program run to deviate from the original run.

The change that caused this behaviour is the addition of hasConstexprDestructor
(which is called from the TextNodeDumper) which performs a lookup into the current
CXXRecordDecl to find the destructor. All other similar methods just return their
respective bit in the DefinitionData (which obviously doesn't have such side effects).

This just changes the node printer to emit "unknown_constexpr" in case a CXXRecordDecl
is dumped that could potentially call into the ExternalASTSource instead of the
usually empty string/"constexpr". For CXXRecordDecls that can safely be dumped the
old behaviour is preserved

Diff Detail

Event Timeline

teemperor created this revision.May 30 2020, 1:56 PM
dexonsmith added a subscriber: dexonsmith.
bruno accepted this revision.Aug 18 2020, 11:00 AM

Neat unit test! LGTM (one minor suggestion below).

clang/lib/AST/TextNodeDumper.cpp
2033

How about maybe_constexpr?

This revision is now accepted and ready to land.Aug 18 2020, 11:00 AM
teemperor updated this revision to Diff 289716.Sep 3 2020, 7:15 AM
  • rename to maybe_constexpr
  • Update some dump output tests
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2020, 3:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

And what if deserialization is forced?

teemperor reopened this revision.Sep 7 2020, 5:49 AM

And what if deserialization is forced?

That's a good point. That should indeed continue to work with the ASTDumper and now (potentially) doesn't. I'll revert.

This revision is now accepted and ready to land.Sep 7 2020, 5:49 AM
teemperor updated this revision to Diff 301577.Oct 29 2020, 4:52 AM
  • Respect forced deserialisation.
teemperor requested review of this revision.Oct 29 2020, 4:52 AM
teemperor marked an inline comment as done.

@teemperor is there a reason why the fixed patch never landed? This looks a good improvement to have.

This is technically in the "Waiting on Review" queue, that's why I also added another reviewer :). The original revision got accepted and then I reverted and updated the patch/test to honor the Deserialize value (which was broken in the first version). I guess the fact that this got reviewed, landed, reverted and back into review caused some confusion

Also obligatory "friendly ping" on this :)

vsapsai added inline comments.Aug 4 2021, 12:57 PM
clang/lib/AST/ASTDumper.cpp
199 ↗(On Diff #301577)

It is possible it will complicate the implementation too much but what if P.setDeserialize(Deserialize); is responsible for propagating Deserialize to its NodeDelegate?

teemperor updated this revision to Diff 364385.Aug 5 2021, 2:01 AM
  • Make setter propagate 'deserialize' to the node dumper (thanks Volodymyr!)

As long as JSON dumper still compiles, it looks good to me. Just have a few small nits.

clang/include/clang/AST/TextNodeDumper.h
47

Would it be better to make the default value the same as in ASTNodeTraverser? I.e.,

/// Indicates whether we should trigger deserialization of nodes that had
/// not already been loaded.
bool Deserialize = false;
clang/lib/AST/TextNodeDumper.cpp
2031–2034

The inconsistency that visually single-statement "if" branch has curly braces but "else" branch doesn't is grating. I understand a macro can expand into multiple statements but visually it looks like a single statement. Probably I would add braces to "else" as well.