Page MenuHomePhabricator

Check TUScope is valid before use
Needs ReviewPublic

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

Details

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.

Diff Detail

Repository
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.
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 added subscribers: zturner, kimgr.EditedNov 16 2018, 10:42 PM

Here's some related suggestions/questions for context:

FWIW,
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.