This is an archive of the discontinued LLVM Phabricator instance.

[AST] Store "UsesADL" information in CallExpr.
ClosedPublic

Authored by EricWF on Dec 10 2018, 4:44 PM.

Details

Summary

Currently the Clang AST doesn't store information about how the callee of a CallExpr was found. Specifically if it was found using ADL.

However, this information is invaluable to tooling. Consider a tool which renames usages of a function. If the originally CallExpr was formed using ADL, then the tooling may need to additionally qualify the replacement.
Without information about how the callee was found, the tooling is left scratching it's head. Additionally, we want to be able to match ADL calls as quickly as possible, which means avoiding computing the answer on the fly.

This patch changes CallExpr to store whether it's callee was found using ADL. It does not change the size of any AST nodes.

Diff Detail

Event Timeline

EricWF created this revision.Dec 10 2018, 4:44 PM
fowles added inline comments.Dec 10 2018, 9:56 PM
include/clang/AST/ExprCXX.h
218

Can CUDAKernelCalls ever use ADL?

499

Same question, I assume user defined literals can only be called as string suffixes and thus never get ADL

include/clang/ASTMatchers/ASTMatchers.h
1275

y(42); // Doesn't match

include/clang/Sema/Overload.h
775

maybe bit pack these? I think overload sets for operator<< actually get pretty big

lib/Sema/SemaOverload.cpp
8946

should this fix be in a separate change?

12024

/*IsExecConfig=*/

12077

here too

lib/Serialization/ASTReaderStmt.cpp
735

readInt into a bool?

unittests/ASTMatchers/ASTMatchersNodeTest.cpp
204

if you use cc or cpp as your delimiter, I think clang-format will recurse correctly

EricWF marked 14 inline comments as done.Dec 10 2018, 10:49 PM

Address review comments. Fixes incoming.

include/clang/AST/ExprCXX.h
218

Yeah, after further investigation, I don't think so. I'll revert this change.

499

Right. Their argument is a literal of some sort.

lib/Sema/SemaOverload.cpp
8946

I'll commit the fix as a separate patch.

lib/Serialization/ASTReaderStmt.cpp
735

Yeah. You serialize and serialize all integer types using readInt()

unittests/ASTMatchers/ASTMatchersNodeTest.cpp
204

That's super cool. Thanks!

riccibruno added inline comments.Dec 11 2018, 5:30 AM
include/clang/AST/Expr.h
2425

There is no need to pass this flag to the empty constructor
since it is going to be deserialized in ASTReaderStmt.
Only what is strictly needed to create the empty CallExpr
is passed here. In fact if you wanted to pass the flag when creating
the empty CallExpr you would have to update what is
under case EXPR_CALL: in ASTReader::ReadStmtFromStream.

include/clang/AST/ExprCXX.h
106

same

178

same

224

same

505

same

lib/Serialization/ASTReaderStmt.cpp
741

E->setUsesADL(Record.readInt()) with the
bool UsesADL = Record.readInt(); removed ?

EricWF marked 12 inline comments as done.Dec 11 2018, 7:33 AM

Address more review comments. Update incoming.

include/clang/AST/Expr.h
2425

Ack. Thanks for the explanation.

EricWF updated this revision to Diff 177705.Dec 11 2018, 7:37 AM
EricWF marked an inline comment as done.

Update with fixes for review comments.

riccibruno added inline comments.Dec 11 2018, 7:44 AM
lib/AST/Expr.cpp
1267

It do not really matter but there is not point initializing this bit here.

aaron.ballman added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
1260

Don't forget to regenerate the documentation and update Registry.cpp to add clang-query support.

EricWF updated this revision to Diff 177716.Dec 11 2018, 8:06 AM

More tests.

EricWF updated this revision to Diff 177720.Dec 11 2018, 8:30 AM
EricWF marked 2 inline comments as done.

Register matcher and regenerate docs.

aaron.ballman marked an inline comment as done.Dec 11 2018, 8:55 AM

A few more minor nits.

docs/LibASTMatchersReference.html
2579–2581 ↗(On Diff #177720)

This is not your bug to fix, but it seems the documentation generator is stripping the comment markers. This impacts other matchers as well, such as throughUsingDecl().

include/clang/Sema/Overload.h
831

OverloadCandidate set -> OverloadCandidateSet ? (I'd also be fine with removing the comment -- seems somewhat obvious from context.)

lib/Sema/SemaExpr.cpp
5672

No need to add the braces here (alternatively, add them to the else clause).

unittests/ASTMatchers/ASTMatchersNodeTest.cpp
204

Wow, that is really neat!

EricWF marked 3 inline comments as done.Dec 11 2018, 9:09 AM

Address more inline comments.

docs/LibASTMatchersReference.html
2579–2581 ↗(On Diff #177720)

I wonder if it strips /**/ comments. I don't want to get blocked on this.

lib/AST/Expr.cpp
1267

I'm a but nervous MSAN won't catch an uninitialized read because it's a bitfield, so I would rather be safe than sorry?

aaron.ballman added inline comments.Dec 11 2018, 9:13 AM
docs/LibASTMatchersReference.html
2579–2581 ↗(On Diff #177720)

Don't let it block you -- I'm poking at a fix currently, but this is not your bug.

riccibruno added inline comments.Dec 11 2018, 9:35 AM
lib/AST/Expr.cpp
1267

I believe that msan can cope with bit level operations just fine.
What I am afraid is that initializing it here will hide the
fact that it is not initialized during deserialization if the
E->setUsesADL(Record.readInt()); is removed by mistake.

At least not initializing it here will leave a chance for msan
to catch this.

rsmith added inline comments.Dec 11 2018, 9:40 AM
include/clang/Sema/Sema.h
2758

Long lists of bool arguments make me nervous, especially with default arguments. Can you introduce an enum for the new param at least?

lib/AST/ASTImporter.cpp
7387

Do we need to pass through the usesADL flag here too?

lib/AST/Expr.cpp
1267

IIRC all the ExprBits get initialized by the Expr(EmptyShell) ctor.

lib/Sema/SemaExpr.cpp
5673

If you believe that CUDA kernel calls can't find functions with ADL, please add an assert here.

riccibruno added inline comments.Dec 11 2018, 9:47 AM
lib/AST/Expr.cpp
1267

I don't think they do.

EricWF marked 13 inline comments as done.Dec 11 2018, 11:40 AM
EricWF added inline comments.
include/clang/Sema/Sema.h
2758

Ack. I was thinking the same thing.

lib/AST/ASTImporter.cpp
7387

Seems like it. I'll add tests as well.

lib/AST/Expr.cpp
1267

Regardless. I'll remove the initialization.

I think potentially getting a random value will make it easier to spot bugs.

EricWF updated this revision to Diff 177752.Dec 11 2018, 11:51 AM
EricWF marked 3 inline comments as done.

Address review comments.

EricWF marked an inline comment as done.Dec 11 2018, 11:55 AM

I think this is good to go. Any more comments?

include/clang/Sema/Sema.h
2758

And you were right to be nervous. The change found bugs.

EricWF marked an inline comment as done.Dec 11 2018, 8:11 PM
EricWF added inline comments.
docs/LibASTMatchersReference.html
2579–2581 ↗(On Diff #177720)

Thanks for the fix!

rsmith accepted this revision.Dec 12 2018, 12:18 PM
rsmith added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
1276

I think it would be useful to add to the example:

using NS::y;
y(x); // Found by both unqualified lookup and ADL, doesn't match
This revision is now accepted and ready to land.Dec 12 2018, 12:18 PM
EricWF updated this revision to Diff 177907.Dec 12 2018, 1:50 PM
EricWF marked an inline comment as done.

Address final review comments.

This revision was automatically updated to reflect the committed changes.