Page MenuHomePhabricator

Check TUScope is valid before use
Needs ReviewPublic

Authored by trixirt on Nov 2 2018, 2:04 PM.



Include-what-you-use crashes when run against llvm because of late use of TUScope.
The testcase is

Enabling incremental processing, fixes this problem but causes 3 regression in iwyu.

So the fix is moving to clang.

Diff Detail

rC Clang

Event Timeline

trixirt created this revision.Nov 2 2018, 2:04 PM

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

Since the crash happens with the iwyu tool, a bit ago i added the testcase at the iwyu project here.

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

kimgr added subscribers: zturner, kimgr.EditedNov 16 2018, 10:42 PM

Here's some related suggestions/questions for context:

Kim, IWYU maintainer

jkorous added subscribers: rjmccall, doug.gregor.

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 added a comment.Jun 7 2021, 1:34 PM

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, we explicitly re-point Sema::TUScope to Sema::getCurScope(), which seems to have fixed the crash.

This revision can be closed.