This is an archive of the discontinued LLVM Phabricator instance.

[clang][AST] Handle overload callee type in CallExpr::getCallReturnType.
ClosedPublic

Authored by balazske on Jan 22 2021, 9:09 AM.

Details

Summary

The function did not handle every case. In some cases this
caused assertion failure.
After the fix the function returns DependentTy if the exact
return type can not be determined.

It seems that clang itself does not call the function in the
affected cases but some checker or other code may call it.

Diff Detail

Event Timeline

balazske created this revision.Jan 22 2021, 9:09 AM
balazske requested review of this revision.Jan 22 2021, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2021, 9:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Should add some responsible (for this code) reviewers.

balazske added inline comments.Jan 25 2021, 7:04 AM
clang/lib/AST/Expr.cpp
1405

Here occurs the problem: If CalleeType is of type BuiltinType::Overload it does not cast to FunctionType.

Hi Balázs,

Could you please create a lit test file which ignites the related crash?

balazske updated this revision to Diff 319261.Jan 26 2021, 3:28 AM

Adding a test, fixing other problems revealed by the test.

I could add a test in ToolingTests, lit test is much more difficult to add because the function is normally not called on "bad" code.

clang/lib/AST/Expr.cpp
1400

I am not sure if dependent type is here (and later) the correct value to return. But only a null type is another possibility.

martong added subscribers: majnemer, rsmith.

Adding reviewers.
@rsmith as code owner,
@majnemer based on git blame of CallExpr::getCallReturnType.

This causes a crash if the function is called in such case.

Is the crash caused by the below assertion?

QualType Expr::findBoundMemberType(const Expr *expr) {
  assert(expr->hasPlaceholderType(BuiltinType::BoundMember));
clang/lib/AST/Expr.cpp
1400

Perhaps we should handle this similarly as the type of UnresolvedMemberExpr or OverloadExpr? Which I assume is a null type.

clang/unittests/Tooling/SourceCodeTest.cpp
642–644

Do we have the crash with both f_overload and with a.f(p) calls?
If yes, then please create two separate test cases.

martong added inline comments.Mar 9 2021, 2:34 AM
clang/lib/AST/Expr.cpp
1403

Can't we create a built in place holder type here? Or could we simply just return CalleeType?

balazske updated this revision to Diff 329647.Mar 10 2021, 6:29 AM

rebase, split the test case

martong added subscribers: aaron.ballman, dblaikie.

Hi @aaron.ballman @riccibruno @dblaikie, I am adding you as a reviewer because it seems you have great experience with Clang's AST.
Could you please take a look on this patch? The problem is that we currently do not handle a CallExpr's return type properly in case of a dependent context. This seems to be a flaw in the implementation of CallExpr.

Added a couple of other folks who might have more context on the tooling front...

This looks like a reasonable solution to me -- if we don't know the function call result type (e.g. dependent-type callExpr in template context), modeling it as a dependent type.

clang/lib/AST/Expr.cpp
1387

this can be removed, dependent type is a builtin type, so you could just use Ctx.DependentTy.

1399

The only case is UnresolvedMemberExpr, I think we'd keep the previous comment and special-case the UnresolvedMemberExpr (like the CXXPseudoDestructorExpr above).

if (isa<UnresolvedMemberExpr>(...))
  return Ctx.DependentTy;

 // This should never be overloaded and so should never return null.
 CalleeType = Expr::findBoundMemberType(Callee);
 CHECK(!CalleeType.isNull());
clang/unittests/Tooling/SourceCodeTest.cpp
630

CalleeType is not a specific term in getCallReturnType, just CalleeType is overload and dependent, I think we can also verify this in the Visitor.OnCall callback.

IIUC, the patch fixes three different crashes

  • calling getCallReturntype on the f(t) expr
  • calling getCallReturntype on the f_overload() expr
  • calling getCallReturntype on the a.f_overload(p); expr

I think it would be much clear to express them in the OnCall callback (rather than calling getCallReturnType on every Expr). One option is to use the annotation, and identify the expression by location like below. An other benefit is that we can unify the test code (instead of duplicating them) without hurting the code readability.

template<class T, class F>
void templ(const T& t, F f) {
  $crash1[[f]](t);
  // CalleeType in getCallReturntype is Overload and dependent
}

struct A {
  void f_overload(int);
  void f_overload(double);
};

void f() {
  int i = 0;
  templ(i, [](const auto &p) {
    $crash2[[f_overload]](p); // UnresolvedLookupExpr
    // CalleeType in getCallReturntype is Overload and not dependent
  });

  templ(i, [](const auto &p) {
    A a;
    a.$crash3[[f_overload]](p); // UnresolvedMemberExpr
    // CalleeType in getCallReturntype is BoundMember and has overloads
  });
  
}
balazske added inline comments.Mar 16 2021, 12:32 AM
clang/lib/AST/Expr.cpp
1387

useful, I did not observe that

clang/unittests/Tooling/SourceCodeTest.cpp
630

The current test finds every CallExpr and calls the getCallReturnType on it. The test should verify that this function works, so at least calling it is needed. An additional check could be to verify that the "Callee" is really of the specific kind (UnresolvedLookupExpr and the others), this can be added. I do not get it how the annotations can be used here, it is possible only to get the position of the code in the string but how to use this value?

balazske updated this revision to Diff 332686.Mar 23 2021, 8:42 AM

Fixed according to the comments.

balazske marked 3 inline comments as done.Mar 23 2021, 8:50 AM

Ping.
I am not sure how to the test should be changed.

hokein added inline comments.Apr 7 2021, 3:57 AM
clang/unittests/Tooling/SourceCodeTest.cpp
630

yes, I'd like to avoid the current test pattern where we invoke getCallReturnType on every CallExpr, and don't verify the call-expr kind.

you can find the corresponding expr by matching the annotation locations, mostly can be done in the OnCall callback, like

Visitor.OnCall = [](..Expr) {
   // check if location of Expr is one of the annotation locations
   //  - no: return
   //  - yes: find out the which one -- let's say, crash2, then we know the Expr should be UnresolvedLookupExpr, and the CalleeType is Overload and not dependent, and verify them.
};
balazske updated this revision to Diff 336333.Apr 9 2021, 12:59 AM

Rebase, changed test according to review comments.

balazske marked an inline comment as done.Apr 9 2021, 1:01 AM
balazske added inline comments.
clang/unittests/Tooling/SourceCodeTest.cpp
630

I am not familiar with this kind of tests but updated the code to check CallExpr at the affected places only.

balazske edited the summary of this revision. (Show Details)Apr 9 2021, 1:04 AM
hokein accepted this revision.Apr 9 2021, 3:49 AM

thanks, looks good!

clang/lib/AST/Expr.cpp
1386

nit: looks like an accident change, remove it.

This revision is now accepted and ready to land.Apr 9 2021, 3:49 AM
This revision was automatically updated to reflect the committed changes.
balazske marked an inline comment as done.
balazske marked an inline comment as done.Apr 12 2021, 12:34 AM
balazske added inline comments.
clang/lib/AST/Expr.cpp
1386

Fixed in the committed code.