Problem with --prefix_header_includes

[GoogleCodeExporter]

Hello,

I wanted to give iwyu a try, since I think this kind of tool is essential when 
working for a long time on a big pipeline.

Our teams work on Windows with VS mainly. To be able to test quickly I just 
downloaded a pre-built binary (apparently version 3.4 from the 22nd of 
February) and started doing some tests with it.

It seems to be exactly what I need, but I seem to get false positives coming 
from the fact that it doesn't seem to take into account the precompiled header 
I use. I searched on the discussion group and I noticed that someone had the 
same issue and then the idea of supporting the option for 
prefix_header_includes came up, and was integrated on r500 (december last year).

For some reason, this doesn't seem to work in my test. Here is my case:

app (my main proj)
Main.cpp -> include FilePCH.h, declare and use object of type myclass.
FilePCH.h -> include mylib/myclass.h which resides in mylib

mylib (my library)
myclass.cpp, myclass.h -> declare and implement the object myclass

Even if this compiles fine in VS, iwyu has some unexpected behaviors. This is 
the command line I use:

include-what-you-use.exe iwyutest.cpp -I ..\ -include stdafx.h -Xiwyu 
--prefix_header_includes

And these are the results for prefix_header_includes:

- With --prefix_header_includes=add:

iwyutest.cpp:14:12: warning: format string is not a string liter
      (potentially insecure) [-Wformat-security]
    printf(buf);
           ^~~

iwyutest.cpp should add these lines:
#include <stdio.h>                      // for printf, sprintf_s
#include <tchar.h>                      // for _TCHAR, _tmain
#include "libtest/mytestclass.h"        // for ZMyTestClass

iwyutest.cpp should remove these lines:
- #include "stdafx.h"  // lines 4-4

The full include-list for iwyutest.cpp:
#include <stdio.h>                      // for printf, sprintf_s
#include <tchar.h>                      // for _TCHAR, _tmain
#include "libtest/mytestclass.h"        // for ZMyTestClass

---

It's as if he didn't get the precompiled header.

- With --prefix_header_includes=keep or --prefix_header_includes=remove:

iwyutest.cpp:14:12: warning: format string is not a string literal
      (potentially insecure) [-Wformat-security]
    printf(buf);
           ^~~
D:\dev\private\llvm-34\llvm\tools\clang\tools\include-what-you-use\iwyu_output.c
c:1484: Assertion failed: file_entry && "Valid file_entry is expected"

Both assert for some reason and they don't finish.

I was about to start setting up my machine to compile llvm, clang, etc to be 
able to debug it, but if anyone has an idea of what could be wrong I would like 
to hear it.

Thank you very much in advance.

PS: I attached the test sample

Original issue reported on code.google.com by dpun...@gmail.com on 7 Apr 2014 at 8:05

Attachments:

• iwyutest.rar

[GoogleCodeExporter]

I've setup the environment to be able to debug and I managed to get more 
information. The assert occurs in CleanupPrefixHeaderIncludes:

if (it->IsIncludeLine()) {
      file_entry = it->included_file();
      CHECK_(file_entry && "Valid file_entry is expected");
    } else {

The iterator values are these:

-       [ptr]   0x00850378 {line_="#include <tchar.h>" start_linenum_=-1 
end_linenum_=-1 ...}    include_what_you_use::OneIncludeOrForwardDeclareLine *
+       line_   "#include 
<tchar.h>"  std::basic_string<char,std::char_traits<char>,std::allocator<char> >
        start_linenum_  -1  int
        end_linenum_    -1  int
        is_desired_ true    bool
        is_present_ false   bool
+       symbol_counts_  [2](("_TCHAR", 1),("_tmain", 
1)) std::map<std::basic_string<char,std::char_traits<char>,std::allocator<char> 
>,int,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<cha
r> > 
>,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::al
locator<char> > const ,int> > >
+       quoted_include_ "<tchar.h>" std::basic_string<char,std::char_traits<char>,std
::allocator<char> >
+       included_file_  0x00000000 {Name=??? Size=??? ModTime=??? ...}  const 
clang::FileEntry *
+       fwd_decl_   0x00000000 {Name={...} }    const clang::NamedDecl *

Apparently it's not able to find tchar.h, even if it's located at the same 
place as stdio.h and it has no problem with stdio.h

I'll keep digging, but I will need to figure out how the files are collected.

Original comment by dpun...@gmail.com on 8 Apr 2014 at 3:11

[GoogleCodeExporter]

And the last info:

iwyutest.cpp contains two uses of tchar.h: _tmain and _TCHAR:

- _tmain is a macro defined as: [#define _tmain main] or [#define _tmain wmain] 
depending on the character settings.
- TCHAR has different declarations too, but it's a typedef.

When the reference of _tmain is inserted it is done through 
HandleMacroExpandedIdentifier->MacroExpands->ReportMacroUse->ReportFullSymbolUse
, which leaves decl to NULL.

When the reference of _TCHAR is inserted it is done through the ASTVisitor and 
ReportForwardDeclareUse, which sets decl to a typedefdecl.

When we build the list of lines for a IwyuFileInfo, in 
CalculateDesiredIncludesAndForwardDeclares, first we add a line for the tchar.h 
reference through the _tmain occurence (null decl), and when we reach the next 
one for _TCHAR (with valid decl) we skip it because tchar.h is already there:

if (!ContainsKey(include_map, use->suggested_header())) {  // must be added
    lines->push_back(OneIncludeOrForwardDeclareLine(
        GetFileEntry(use->decl()), use->suggested_header(), -1));
    include_map[use->suggested_header()] = lines->size() - 1;
}

If decl is NULL we get a NULL FileEntry when calling GetFileEntry(use->decl()), 
and when we get to CleanupPrefixHeaderIncludes we get the assert.

Can you tell me what is the expected behavior? Should it be supported that decl 
is NULL in this case? Is the assert valid? Or should we only add lines that 
contain decl inside the IwyuFileInfo?

Thank you in advance

Original comment by dpun...@gmail.com on 8 Apr 2014 at 5:49

[GoogleCodeExporter]

Great job, you've figured out everything right.  As for the ways to fix the 
issue, I think the following. 

We need file_entry in CleanupPrefixHeaderIncludes.  Without file_entry we 
cannot tell if used symbol is already present in prefix header.  And we'll 
recommend to #include prefix header.  That's why assert CHECK_(file_entry && 
"Valid file_entry is expected") in CleanupPrefixHeaderIncludes is needed.

On the other hand, macro isn't a declaration and it's fine that decl is NULL in 
OneUse created for a macro.

I think that the right fix is to use non-NULL FileEntry when create 
OneIncludeOrForwardDeclareLine in CalculateDesiredIncludesAndForwardDeclares.  
I.e. we need to use something else instead of GetFileEntry(use->decl()) for 
macro use.  If OneUse for macro doesn't contain necessary data to find out 
correct FileEntry, it's OK to add such data.

Original comment by vsap...@gmail.com on 8 Apr 2014 at 9:44

[GoogleCodeExporter]

Thank you! Does this looks good to you?

      if (use->is_full_use()) {
      CHECK_(use->has_suggested_header() && "Full uses should have #includes");

      const clang::FileEntry* fileEntry;

      if ( use->decl() != NULL )
      {
          fileEntry = GetFileEntry(use->decl());
      }
      else
      {
          fileEntry = GetFileEntry(use->use_loc());
      }

      if ( !ContainsKey(include_map, use->suggested_header())) {  // must be added
        lines->push_back(OneIncludeOrForwardDeclareLine(
            fileEntry, use->suggested_header(), -1));
        include_map[use->suggested_header()] = lines->size() - 1;
      }
      const int index = include_map[use->suggested_header()];
      (*lines)[index].set_desired();
      (*lines)[index].AddSymbolUse(use->short_symbol_name());

    // Forward-declare uses that are already satisfied by an #include
    // have that as their suggested_header.  For the rest, we need to
    // make sure there's a forward-declare in the current file.
    }

Original comment by dpun...@gmail.com on 9 Apr 2014 at 2:48

[GoogleCodeExporter]

Sorry I haven't gotten to this earlier (but I'm also glad, you seem to have 
figured it out nicely!).

This:

+      const clang::FileEntry* fileEntry;
+
+      if ( use->decl() != NULL )
+      {
+          fileEntry = GetFileEntry(use->decl());
+      }
+      else
+      {
+          fileEntry = GetFileEntry(use->use_loc());
+      }

looks like it would make a nice method on OneUse, e.g.

  const FileEntry* OneUse::GetUseFileEntry() const {
    if (decl_ != NULL)
      return GetFileEntry(decl_);
    else
      return GetFileEntry(use_loc_);
  }

I can try and cook up a test case from your example, and see if we can get it 
to run consistently across all platforms.

Original comment by kim.gras...@gmail.com on 9 Apr 2014 at 6:40

[GoogleCodeExporter]

Attached is a pair of patches:

- one with test modifications to reproduce the issue. Volodymyr, could you see 
if this causes assertion failures on HEAD in your non-Windows environment? It 
does here.
- one with dpunset's fix moved onto OneUse.

I'd love to hear what you think.

Original comment by kim.gras...@gmail.com on 9 Apr 2014 at 7:30

Attachments:

• issue125.patch
• issue125-test.patch

[GoogleCodeExporter]

I just integrated your patch, it works fine on my current test.

Thank you!

Original comment by dpun...@gmail.com on 9 Apr 2014 at 7:46

[GoogleCodeExporter]

The approach in general is correct, but we cannot use use_loc to get file 
entry. We need definition location, like dfn_location in ReportMacroUse. With 
GetFileEntry(use_loc_) we don't see a macro defined in prefix header as such 
and recommend to #include prefix header if 
--prefix_header_includes=keep|remove. I've attached a patch with corresponding 
test.

Original comment by vsap...@gmail.com on 9 Apr 2014 at 8:51

Attachments:

• issue125-test2.patch

[GoogleCodeExporter]

So something like this:

OneUse(const string& symbol_name,
       string& dfn_filepath,
       clang::SourceLocation use_loc,
       clang::SourceLocation def_loc);

void IwyuFileInfo::ReportFullSymbolUse(SourceLocation use_loc,
                                       SourceLocation def_loc,
                                       const string& dfn_filepath,
                                       const string& symbol) {
  symbol_uses_.push_back(OneUse(symbol, dfn_filepath, use_loc,def_loc));
  LogSymbolUse("Marked full-info use of symbol", symbol_uses_.back());
}

void IwyuPreprocessorInfo::ReportMacroUse(const string& name,
                                          SourceLocation usage_location,
                                          SourceLocation dfn_location) {
{
  ...
  ...

  GetFromFileInfoMap(used_in)->ReportFullSymbolUse(usage_location,dfn_location,
                                                   defined_path, name);

}

const clang::FileEntry* OneUse::GetUseFileEntry() const {
  if (decl_ != NULL)
    return GetFileEntry(decl_);
  else
    return GetFileEntry(def_loc_);
}

Original comment by dpun...@gmail.com on 9 Apr 2014 at 10:14

[GoogleCodeExporter]

Ah, I did wonder about that.

Thanks for improving the test case, but could you name the macro something 
other than FOO to hint at its role?

Would you like to integrate everything into a working patch, or should I? I can 
test on Windows either way.

Original comment by kim.gras...@gmail.com on 10 Apr 2014 at 8:14

[GoogleCodeExporter]

I've tried to prepare a patch with dfn_location added to OneUse. But turned out 
that we have other symbols without declaration and they don't have 
dfn_location. For example, in IwyuFileInfo::ReportIncludeFileUse there are no 
locations, even use location; in VisitCXXNewExpr we have 
ReportFullSymbolUse(CurrentLoc(), "<new>", "operator new"), i.e. no 
dfn_location. Currently I am thinking if we can obtain FileEntry from 
decl_filepath_. And maybe we'll need to replace the assert with error if 
not-existing filepath from IWYU pragma can end up in decl_filepath_.

I've renamed FOO to MACRO_IN_PREFIX_HEADER and added a comment, updated patch 
is attached.

Original comment by vsap...@gmail.com on 10 Apr 2014 at 2:31

Attachments:

• issue125-test3.patch

[GoogleCodeExporter]

Also, the result of iwyu is bad with the test I sent you and the fixes. Do you 
want to talk about this here or move it to another thread?

Original comment by dpun...@gmail.com on 10 Apr 2014 at 2:43

[GoogleCodeExporter]

Please, move to another thread, let's keep current issue assert-related.

Original comment by vsap...@gmail.com on 10 Apr 2014 at 2:48

[GoogleCodeExporter]

> For example, in IwyuFileInfo::ReportIncludeFileUse
> there are no locations, even use location;
> in VisitCXXNewExpr we have ReportFullSymbolUse(
> CurrentLoc(), "<new>", "operator new"), i.e. no
> dfn_location

Doesn't this seem like a hack, anyway? Maybe a more correct way would be to 
scan the system header search path for the header <new>, and then feed the 
actual path into ReportFullSymbolUse?

Original comment by kim.gras...@gmail.com on 10 Apr 2014 at 7:59

[GoogleCodeExporter]

I checked from where IwyuFileInfo::ReportIncludeFileUse is called, and it seems 
you could pass the information you need, right?

- In ProtectReexportIncludes: it's taken from a FileEntry
- In CalculateIwyuViolations: from IwyuFileInfo
- In MaybeProtectInclude: you have the includer and the includee

In VisitCXXNewExpr I just passed the same as currentloc, since in the comments 
it says it's just making up a path. It says also the contents don't matter.

Original comment by dpun...@gmail.com on 10 Apr 2014 at 8:18

[GoogleCodeExporter]

I think the comment is a little misleading. See the OneUse constructor that is 
called from ReportFullSymbolUse -- it checks if the dfn_filepath is a quoted 
include ("filename" or <filename>), and if so gives it special treatment, so 
that the suggested header becomes what you passed in.

I think the comment means that the contents of the _file_ are not important, 
i.e. that IWYU doesn't care if it defines any symbols related to operator new. 
The rule is that any use of placement new should include <new>.

Original comment by kim.gras...@gmail.com on 10 Apr 2014 at 8:28

[GoogleCodeExporter]

One more attempt to fix the issue is attached.

There is no test for ReportIncludeFileUse because actually we don't need 
FileEntry in ReportIncludeFileUse, it was added for consistency.  We don't need 
FileEntry there because the problem is when we try to create *new* 
OneIncludeOrForwardDeclareLine to satisfy OneUse.  And ReportIncludeFileUse is 
about *existing* includes, so there is no problem with *new* 
OneIncludeOrForwardDeclareLine.

If you are concerned with FileEntry's proliferation, it's the right approach.  
We shouldn't use quoted includes as unique file identifier, we should use 
FileEntry for this purpose, i.e. we need to use more FileEntry's.  For example, 
FileEntry can be useful in issue #5.

Original comment by vsap...@gmail.com on 18 Apr 2014 at 6:11

Attachments:

• issue125-r2.patch

[GoogleCodeExporter]

Looks good overall!

I'm finding it a little hard to follow the test cases, they seem to overlap, 
but maybe they test different aspects of <new>? I generally don't like tests 
phrased in terms of "doesn't crash", there's usually a more functional 
requirement buried in these tests (e.g. can combine placement new with prefix 
headers), but I think this is OK for now.

As for the FileEntry movement, I think that sounds good. The only reservation I 
have is the few places where we make up includes and add them; there will be no 
FileEntry available. I don't know exactly where that happens, though. I guess 
placement new is one case, but I think the symbol -> include mappings is 
another one. Hopefully this happens after we've made all the decisions 
involving FileEntries.

A few small nits:

+  symbol_uses_.push_back(OneUse(symbol, NULL, dfn_filepath,use_loc));

Space before use_loc.

-  // This will mostly be used for macro tokens.
>   void ReportFullSymbolUse(clang::SourceLocation use_loc,
>                            const string& dfn_filepath,
>                            const string& symbol);

It's a shame that we have to keep this one for the <new> special case... Could 
you change the comment to reflect that fact, instead of removing it? Just to 
make it clear that this overload should hardly ever be used.

+// IWYU doesn't crash when decides to add headers for these constructs but this

... doesn't crash when _it_ decides ...

Alternatively

... doesn't crash when _deciding_ ...

+// Tests that IWYU doesn't crash when checks if not encountered header <new> is
+// prefix header or not.

... doesn't crash when _it_ checks ...

Alternatively

... doesn't crash when _checking_ ...

Original comment by kim.gras...@gmail.com on 20 Apr 2014 at 9:01

[GoogleCodeExporter]

Ugh, hit Save too soon.

Both the new test cases fail on Windows. It looks like IWYU doesn't recognize 
that placement new should force inclusion of <new>.

--
>>> Running D:\dev\private\llvm-trunk\out\release\bin\include-what-you-use.exe 
-Xiwyu --verbose=3 -Xiwyu --prefix_header
_includes=remove -I . -include tests/cxx/prefix_header_attribution-d1.h 
tests/cxx/prefix_header_attribution.cc
Target triple: i686-pc-windows-msvc
tests/cxx/prefix_header_attribution.cc:16:1: warning: MACRO_IN_PREFIX_HEADER is 
defined in "tests/cxx/prefix_header_attr
ibution-i1.h", which isn't directly #included.

(tests/cxx/prefix_header_attribution.cc has correct #includes/fwd-decls)

F
======================================================================
FAIL: runTest (__main__.prefix_header_attribution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "run_iwyu_tests.py", line 155, in <lambda>
    {'runTest': lambda self, f=filename: self.RunOneTest(f),
  File "run_iwyu_tests.py", line 126, in RunOneTest
    iwyu_flags, clang_flags, verbose=True)
  File "D:\dev\private\llvm-trunk\llvm\tools\clang\tools\include-what-you-use\iwyu_test_util.py", line 424, in TestIwyuO
nRelativeFile
    test_case.assertTrue(not failures, ''.join(failures))
AssertionError:
tests/cxx/prefix_header_attribution.cc:21: Unmatched regex:
operator new is...*<new>
--

--
>>> Running D:\dev\private\llvm-trunk\out\release\bin\include-what-you-use.exe 
-Xiwyu --verbose=3 -Xiwyu --prefix_header
_includes=remove -I . tests/cxx/prefix_header_operator_new.cc
Target triple: i686-pc-windows-msvc

(tests/cxx/prefix_header_operator_new.cc has correct #includes/fwd-decls)

F
======================================================================
FAIL: runTest (__main__.prefix_header_operator_new)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "run_iwyu_tests.py", line 155, in <lambda>
    {'runTest': lambda self, f=filename: self.RunOneTest(f),
  File "run_iwyu_tests.py", line 126, in RunOneTest
    iwyu_flags, clang_flags, verbose=True)
  File "D:\dev\private\llvm-trunk\llvm\tools\clang\tools\include-what-you-use\iwyu_test_util.py", line 424, in TestIwyuO
nRelativeFile
    test_case.assertTrue(not failures, ''.join(failures))
AssertionError:
tests/cxx/prefix_header_operator_new.cc:15: Unmatched regex:
operator new is...*<new>
--

Original comment by kim.gras...@gmail.com on 20 Apr 2014 at 9:14

[GoogleCodeExporter]

The problem appears to be that IWYU doesn't see through the template.

If I replace:

  template<typename T> void CallPlacementNew(T *t) {
    // IWYU: operator new is...*<new>
    new (t) T();
  }

with:

  void foo() {
    int i = 0;
    // IWYU: operator new is...*<new>
    new (&i) int(123);
  }

the test produces the correct warnings. I'm not sure why the MSVC build behaves 
differently, could there be something related to it running in C++11 mode by 
default?

Original comment by kim.gras...@gmail.com on 21 Apr 2014 at 3:57

[GoogleCodeExporter]

One more detail: with the template, VisitCXXNewExpr is not even called.

Original comment by kim.gras...@gmail.com on 21 Apr 2014 at 4:02

[GoogleCodeExporter]

Sorry about the flood of comments, but I just wanted to add that I think we 
should dodge the template problem by making CallPlacementNew a normal function 
with an implementation similar to my foo above.

Then we can open a separate defect for the fact that VisitCXXNewExpr does not 
trigger in templates, there's probably a more systemic error that makes that 
happen.

Original comment by kim.gras...@gmail.com on 21 Apr 2014 at 4:16

[GoogleCodeExporter]

I've updated some comments, the entire updated patch is attached.

The problem with making CallPlacementNew a normal function is that it won't 
test anything then. Test requires to call operator new with dependent type, 
which is possible only in template. I think that the different behavior on 
Windows is caused by -fdelayed-template-parsing [1]. Seems like 
FunctionDecl::getBody() == NULL on Windows, while on Mac OS X 
FunctionDecl::getBody() == CompoundStmt. Can you please try to run IWYU with 
-fno-delayed-template-parsing to see if it helps?

[1] http://clang.llvm.org/docs/UsersManual.html#microsoft-extensions

Original comment by vsap...@gmail.com on 21 Apr 2014 at 9:49

Attachments:

• issue125-r3.patch

[GoogleCodeExporter]

Thanks, -fno-delayed-template-parsing did indeed help! It didn't occur to me 
that that was the root cause.

I went through VisitCXXNewExpr more carefully, and I understand now why the 
template is necessary. The new comments in the tests make it easier to grasp as 
well, thanks!

The patch as a whole now looks good to me. How do we handle the 
delayed-template-parsing problem on Windows? I tried adding 
"-fno-delayed-template-parsing" to the args in iwyu_driver.cc where we add 
"-fsyntax-only", and I don't see a difference in test results (4 still fail.)

Original comment by kim.gras...@gmail.com on 22 Apr 2014 at 6:29

[GoogleCodeExporter]

> I tried adding "-fno-delayed-template-parsing" to the args 
> in iwyu_driver.cc where we add "-fsyntax-only", and I don't
> see a difference in test results (4 still fail.)

OK, this is not at all clear. I mean I added it to a clean HEAD and all tests 
still pass that passed before.

So I think this is a reasonable way forward, unless we want to solve it in some 
cleaner way than -fsyntax-only.

I think that fix could/should go in as a separate commit.

Original comment by kim.gras...@gmail.com on 22 Apr 2014 at 6:33

[GoogleCodeExporter]

Before starting a new -fdelayed-template-parsing issue, do you need issue with 
templates to be resolved before the current issue or it doesn't matter? 
Spoiler: I think adding -fno-delayed-template-parsing might be not enough.

Original comment by vsap...@gmail.com on 23 Apr 2014 at 7:58

[GoogleCodeExporter]

Not necessarily, tests are already failing on Windows and a few more won't 
hurt, as long as we know they can be made to work. Feel free to commit.

Original comment by kim.gras...@gmail.com on 24 Apr 2014 at 9:24

[GoogleCodeExporter]

Committed changes in r537.

Original comment by vsap...@gmail.com on 24 Apr 2014 at 4:11

• Changed state: Fixed