Check TUScope is valid before use

[trixirt]

Summary

Include-what-you-use crashes when run against llvm because of late use of TUScope.
The testcase is
https://github.com/trixirt/include-what-you-use/commit/dfec8cf07015fb5fe2db10df2f0a005250953131

Enabling incremental processing, fixes this problem but causes 3 regression in iwyu.
https://github.com/include-what-you-use/include-what-you-use/pull/586

So the fix is moving to clang.

[jkorous]

Could you please add a test? I'd suggest minimizing the testcase you linked and placing it to clang/test.

[trixirt]

Since the crash happens with the iwyu tool, a bit ago i added the testcase at the iwyu project here.
https://github.com/include-what-you-use/include-what-you-use/pull/601

I don't know if it makes sense to run iwyu from clang/test.

[kimgr]

Here's some related suggestions/questions for context:

• Earlier patch from @zturner with a minimal repro case: https://reviews.llvm.org/D31697. I don't think this is reproducible with Clang proper.
• Open question for alternative solutions: https://lists.llvm.org/pipermail/cfe-dev/2018-March/057147.html

FWIW,
Kim, IWYU maintainer

[jkorous]

Adding @rjmccall and @doug.gregor who might have some insight.

Honestly, I don't know how IWYU works and my familiarity with Sema is limited so bear with me.

From what I see TUScope is set only in one place in clang and that is void Sema::ActOnTranslationUnitScope(Scope *S).

> grep -nr --exclude="*/test/*" "TUScope\s\+=" .
./lib/Sema/Sema.cpp:70:  TUScope = S;
./lib/Sema/Sema.cpp:149:  TUScope = nullptr;
./lib/Sema/Sema.cpp:946:      TUScope = nullptr;
./lib/Sema/Sema.cpp:1168:    TUScope = nullptr;

(TUScope seems not to be copied anywhere.)

> grep -nr --exclude="*/test/*" "=\s\+TUScope" .

ActOnTranslationUnitScope itself is called only from void Parser::Initialize().

Just an idea - can't your code mimic behavior of Parser::Initialize()?

Other than that it seems that:

1. The code obviously assumes TUScope != nullptr.
2. We aren't aware of hitting this case in clang.

So even with my limited insight I would be inclined to make the assumption explicit. The whole block of code after if (isValidVarArgType(E->getType()) == VAK_Undefined) seems to be mostly checks for errors so ExprError() seems like the right error reporting mechanism here as it's part of this method's interface and randomly picked caller's code in SemaOverload.cpp seems to handle such case.

Re: tests Since you are using Sema from your code I assume you should be able to create a regression test (with a suitable code input) as a minimal reproducer. You might take a look how is Sema set up in unittests.

[kimgr]

This was eventually fixed in IWYU based on @jkorous' suggestion above. I believe the problem is/was:

• After code is parsed and the AST is built, Sema resets its TUScope member to null
• We use Sema to lookup and define default constructors before traversing the AST using a RAV
• Some yet-not-fully-identified constructs cause Sema to need a TUScope for this (something to do with LazilyCreateBuiltin, but I haven't been able to construct a reduced testcase)

So in https://github.com/include-what-you-use/include-what-you-use/commit/5e7843434169a8af333305ebd6e1434cc3cffb66, we explicitly re-point Sema::TUScope to Sema::getCurScope(), which seems to have fixed the crash.

This revision can be closed.

lib/Sema/SemaExpr.cpp

ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT,
   // emitted along with format string checking in Sema::CheckFunctionCall().
   if (isValidVarArgType(E->getType()) == VAK_Undefined) {
     // Turn this into a trap.
     CXXScopeSpec SS;
     SourceLocation TemplateKWLoc;
     UnqualifiedId Name;
     Name.setIdentifier(PP.getIdentifierInfo("__builtin_trap"),
                        E->getBeginLoc());
+
+    if (TUScope == nullptr)
+      return ExprError();
+
     ExprResult TrapFn = ActOnIdExpression(TUScope, SS, TemplateKWLoc,
                                           Name, true, false);
     if (TrapFn.isInvalid())
       return ExprError();
 
     ExprResult Call = ActOnCallExpr(TUScope, TrapFn.get(), E->getBeginLoc(),
                                     None, E->getEndLoc());
     if (Call.isInvalid())