Segfault

[GoogleCodeExporter]

There is a NULL pointer dereference bug in the current SVN head of 
include-what-you-use. This triggers reliably on any source file in our tree. 
Unfortunately I cannot include the sources to reproduce this crash, but I will 
try to post a reduced testcase.

Here is the valgrind result (with the heap info trimmed out) showing the 
stacktrace of the crash:

==916== Memcheck, a memory error detector
==916== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==916== Using Valgrind-3.8.0.SVN and LibVEX; rerun with -h for copyright info
==916== Command: include-what-you-use -DFRANTIC_DEBUG=1 -std=c++11 -I 
/home/user/software/build/Frantic/Frantic/Sources/Library -I 
/home/user/software/build/Frantic/Frantic/Sources/Test 
Sources/Test/Frantic/FranticConvolve.cxx
==916== 
==916== Invalid read of size 8
==916==    at 0x5A7810: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseType(clang::QualType) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A837B: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseAutoT
ypeLoc(clang::AutoTypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A99D1: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseTypeLoc(clang::TypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A9BA9: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseLValu
eReferenceTypeLoc(clang::LValueReferenceTypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A99D1: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseTypeLoc(clang::TypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5AE1F6: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseDecla
ratorHelper(clang::DeclaratorDecl*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5AE410: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseVarHe
lper(clang::VarDecl*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5AE4B2: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseVarDe
cl(clang::VarDecl*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x562DFC: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseDecl(clang::Decl*) [clone .part.2825] (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A8C4F: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseDeclS
tmt(clang::DeclStmt*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5628AC: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseStmt(clang::Stmt*) [clone .part.2813] (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A1317: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseCompo
undStmt(clang::CompoundStmt*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==916== 
Stack dump:
0.  <eof> parser at end of file
==916== 
==916== Process terminating with default action of signal 11 (SIGSEGV)
==916==  Access not within mapped region at address 0x0
==916==    at 0x5A7810: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseType(clang::QualType) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A837B: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseAutoT
ypeLoc(clang::AutoTypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A99D1: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseTypeLoc(clang::TypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A9BA9: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseLValu
eReferenceTypeLoc(clang::LValueReferenceTypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A99D1: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseTypeLoc(clang::TypeLoc) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5AE1F6: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseDecla
ratorHelper(clang::DeclaratorDecl*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5AE410: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseVarHe
lper(clang::VarDecl*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5AE4B2: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseVarDe
cl(clang::VarDecl*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x562DFC: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseDecl(clang::Decl*) [clone .part.2825] (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A8C4F: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseDeclS
tmt(clang::DeclStmt*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5628AC: 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseStmt(clang::Stmt*) [clone .part.2813] (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==    by 0x5A1317: 
clang::RecursiveASTVisitor<include_what_you_use::IwyuAstConsumer>::TraverseCompo
undStmt(clang::CompoundStmt*) (in 
/home/user/install/VA-HEPHAESTUS/bin/include-what-you-use)
==916==  If you believe this happened as a result of a stack
==916==  overflow in your program's main thread (unlikely but
==916==  possible), you can try to increase the size of the
==916==  main thread stack using the --main-stacksize= flag.
==916==  The main thread stack size used in this run was 8388608.

Original issue reported on code.google.com by supercilious.dude@gmail.com on 22 Aug 2012 at 11:07

[GoogleCodeExporter]

Please, provide also LLVM/Clang revision.

Original comment by vsap...@gmail.com on 22 Aug 2012 at 11:45

[GoogleCodeExporter]

The current git master branch of llvm + clang (built without polly or lld).

Just updated everything since posting and it is broken on llvm master branch 
revision fdeb9fe5e08146d9cb953000cb893eda80329a08 + clang master branch 
revision 54c2f88540994726032bfbf1d087bb67767432af.

A quick gdb session reveals that the line "return 
Base::TraverseTypeLoc(typeloc);" in iwyu.cc at line 395 is the source of the 
crash. Stepping into the clang code shows that the 
RecursiveASTVisitor::TraverseAutoTypeLoc call results in the call to 
include_what_you_use::BaseAstVisitor<include_what_you_use::IwyuAstConsumer>::Tra
verseType.

That call resolves to QualType::getTypePtr() from clang/AST/Type.h and there, 
getCommonPtr() returns NULL, and is unconditionally dereferenced: "return 
getCommonPtr()->BaseType"

I know nothing about clang internals, so I am unqualified to comment further, 
but I hope that helps. I will try to reproduce this crash without requiring 
40MB of our headers so there is a test case, but for now that's as far as I can 
go.

Original comment by supercilious.dude@gmail.com on 22 Aug 2012 at 12:30

[GoogleCodeExporter]

Just to add to my previous comment, a debug build of clang, with assertions 
enabled confirms the findings and does indeed abort with:

include-what-you-use: 
/home/user/software/repositories/llvm/tools/clang/include/clang/AST/Type.h:503: 
const clang::ExtQualsTypeCommonBase* clang::QualType::getCommonPtr() const: 
Assertion `!isNull() && "Cannot retrieve a NULL type pointer"' failed.
Stack dump:
0.      <eof> parser at end of file

Original comment by supercilious.dude@gmail.com on 22 Aug 2012 at 12:46

[GoogleCodeExporter]

AutoTypeLoc seems to be used for the C++11 'auto' keyword.

As far as I know, IWYU only does C++03 (and barely that), I expect C++11 adds 
new semantics for IWYU, especially around what constitutes a 'use'.

That said, it would be nice if it didn't segfault until we get as far as C++11 
support.

Anyway, using 'auto' and --std=c++11 might make it easier to come up with a 
narrow repro case.

Original comment by kim.gras...@gmail.com on 22 Aug 2012 at 7:11

[GoogleCodeExporter]

Can you, please, run IWYU with -Xiwyu --verbose=6? The entire command line 
looks like
$ ../../../../../build/Debug+Asserts/bin/include-what-you-use -Xiwyu 
--verbose=6 -std=c++11 -I . tests/t.cc

And post here the piece of log prior to segfault. I am looking for something 
like

tests/t.cc:11:7: (4) [ VarDecl ] Foo i = bar()
tests/t.cc:11:2: (5) [ AutoType ] class Foo
tests/t.cc:11:2: (5) [ AutoTypeLoc ] class Foo
tests/t.cc:11:2: (6) [ RecordType ] class Foo

I think it will point at a piece of source code causing segfault and will help 
to create a repro case.

Original comment by vsap...@gmail.com on 26 Aug 2012 at 7:08

[GoogleCodeExporter]

I posted this on issue 57, but it looks like it's more relevant to this one 
actually.

I just ran into this problem, and spent a while debugging it. I don't know what 
the exact cause is, but here's an extremely simple test case that demonstrates 
the issue.

Tested with Clang 3.1, and IWYU r371 with the addCommentHandler change reverted 
so that it compiles against 3.1.

Run with the following command line:
./iwyu -std=gnu++0x bad.cpp

Here's the contents of bad.cpp:

template<typename T>
class Foo {
};

template<typename T>
void g(Foo<T> x) {
  auto y = x;
}

int main() {
  return 0;
}

Original comment by christop...@gmail.com on 6 Nov 2012 at 6:28

[GoogleCodeExporter]

Original comment by vsap...@gmail.com on 25 Nov 2012 at 10:16

• Changed state: Accepted
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

We are hitting the assertion `!isNull() && "Cannot retrieve a NULL type 
pointer"' when we are trying to traverse deduced type of AutoType. But 
according to AutoType documentation [1]
> However, within templates and before the initializer is attached, there is no 
deduced type
> and an auto type is type-dependent and canonical.
So it is normal for deduced type to be null.

Resolved assertion failure by getting Type* only when traversed QualType is not 
null. Also in testing code added possibility to specify Clang flags for some 
files in order to use "-std=c++11". Patches are attached.

[1] http://clang.llvm.org/doxygen/classclang_1_1AutoType.html

Original comment by vsap...@gmail.com on 9 Dec 2012 at 4:00

Attachments:

• clang_flags.patch
• auto_type_within_template.patch

[GoogleCodeExporter]

Nice!

Two comments:

1) I think it would be better to delegate to the base RecursiveASTVisitor 
behavior if the QualType is null, i.e.

    if (!qualtype.isNull()) {
      const Type* type = qualtype.getTypePtr();
      if (current_ast_node_->StackContainsContent(type))
        return true;               // avoid recursion
      ASTNode node(type, *GlobalSourceManager());
      CurrentASTNodeUpdater canu(¤t_ast_node_, &node);
    }

    return Base::TraverseType(qualtype);

It currently only returns true as well, but if that ever changes, we'll benefit 
from the new behavior.

2) '-I .' in the flags between run_iwyu_tests.py and iwyu_test_util.py could 
probably be simplified by always starting clang_flags with '-I .'. In 
iwyu_test_util.py:

  clang_flags = clang_flags or []
  clang_flags = ['-I .'] + clang_flags

and then get rid of -I from run_iwyu_tests.py. I think we'll be hard pressed to 
find a test that doesn't use '-I .'.

Otherwise, looks good!

Original comment by kim.gras...@gmail.com on 10 Dec 2012 at 11:30

[GoogleCodeExporter]

I've attached the modified patch. I haven't put CurrentASTNodeUpdater inside if 
body because it will be destroyed too early (before Base::TraverseType).

Original comment by vsap...@gmail.com on 2 Jan 2013 at 9:48

Attachments:

• auto_type_within_template_v2.patch

[GoogleCodeExporter]

> I haven't put CurrentASTNodeUpdater inside if body
> because it will be destroyed too early (before Base::TraverseType).

Oh, silly of me!

This looks good to me and the new test passes on my Windows setup. Please 
commit!

Original comment by kim.gras...@gmail.com on 2 Jan 2013 at 10:46

[GoogleCodeExporter]

Oh, one thing I noticed;

+    clang_flags: Extra command-line flags to pass to clang, for example "-I .".

"-I ." seems like a bad example now that it's always added.

Original comment by kim.gras...@gmail.com on 2 Jan 2013 at 10:07

[GoogleCodeExporter]

This issue was closed by revision r421.

Original comment by vsap...@gmail.com on 3 Jan 2013 at 9:07

• Changed state: Fixed

[GoogleCodeExporter]

Changed the example to "-std=c++11", committed r421.

Original comment by vsap...@gmail.com on 3 Jan 2013 at 9:18