Errors in libc++. precomputed_tpl_args test fails

[GoogleCodeExporter]

Steps to reproduce:
Prerequisites: system where libc++ is default stdlib, for example, Mac OS X 
10.9+.
1. Run precomputed_tpl_args.cc test.

Actual result:
Test fails.  In addition to unmatched regex errors, IWYU summary differs
    -#include "tests/cxx/precomputed_tpl_args-d1.h"  // for D1SpecializationClass, less
    -#include "tests/cxx/precomputed_tpl_args-i1.h"  // for IndirectClass, SpecializationClass, less
    +#include "tests/cxx/precomputed_tpl_args-d1.h"  // for D1SpecializationClass (ptr only), less
    +#include "tests/cxx/precomputed_tpl_args-i1.h"  // for IndirectClass, SpecializationClass (ptr only), less

Expected result:
Test should pass.

Original issue reported on code.google.com by vsap...@gmail.com on 18 May 2014 at 4:08

[GoogleCodeExporter]

In iwyu_cache.cc in FullUseCache::GetPrecomputedResugarMap we use 
tpl_decl->getQualifiedNameAsString() to check if template type is full-use 
type.  The problem is that for libc++ classes getQualifiedNameAsString will 
return something like "std::__1::set" which doesn't match against value 
"std::set" in kFullUseTypes array.  FWIW, __1 is inline namespace.

I've decided to add GetWrittenQualifiedNameAsString which returns qualified 
name suppressing anonymous and inline namespaces.  So for libstdc++ and libc++ 
name will be "std::set".  I've asked on cfe-dev mailing list about using 
PrintingPolicy::SuppressUnwrittenScope [1].  Meanwhile I've prepared proof of 
concept GetWrittenQualifiedNameAsString, which is copy-pasted 
NamedDecl::printQualifiedName with a few tweaks.  Attached patch is a work in 
progress, it's purpose is to inform everybody about current bug investigation 
state.

I've also attached a test which prevents harmful mapping <memory>, kPublic to 
<vector>, kPublic.  Not sure if it is really useful.

[1] http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-May/037029.html

Original comment by vsap...@gmail.com on 18 May 2014 at 4:37

Attachments:

• issue132-poc.patch
• stl_container_provides_allocator.cc

[GoogleCodeExporter]

It makes sense to me to get this fixed in Clang, which makes the IWYU fix much 
easier.

I'm not sure I understand the FileTransitivelyIncludes change... Does it fix 
something else?

The root of the problem seems to be that libc++ STL classes are not named 
"std::xyz" but "std::__1::xyz", and your custom decl name printer fixes that. 
How is the #include graph involved here?

Original comment by kim.gras...@gmail.com on 20 May 2014 at 7:13

[GoogleCodeExporter]

I've added FileTransitivelyIncludes because PublicHeaderIntendsToProvide works 
incorrectly for libc++.  For std::set we have tpl_file <set> and tpl_arg_file 
<memory>.  <set> doesn't include <memory> directly.  It includes <__tree>, 
which includes <memory>.  And PublicHeaderIntendsToProvide follows transitive 
includes only for files which have some files mapped to them (see 
IwyuPreprocessorInfo::PopulateIntendsToProvideMap and especially 
private_headers_behind).  Some private headers are mapped to <set>, for example 
<bits/stl_set.h>, <bits/stl_tree.h>.  But all these files are present only in 
libstdc++, so in libc++ <set> doesn't have any files mapped to it.  If I add 
mappings like "<__tree>, kPrivate, <set>, kPublic" or "<memory>, kPublic, 
<set>, kPublic", then PublicHeaderIntendsToProvide works fine and 
FileTransitivelyIncludes isn't required.

For the record, in libstdc++ tpl_file is <bits/stl_set.h> and tpl_arg_file is 
<bits/c++allocator.h>.  And <bits/stl_set.h> doesn't include transitively 
<bits/c++allocator.h>.  A lot of code in 
IwyuPreprocessorInfo::PopulateIntendsToProvideMap is devoted to populate 
intends_to_provide_map_ so that PublicHeaderIntendsToProvide works for 
libstdc++.

I think, in the final patch I just need to fix PublicHeaderIntendsToProvide.  
Current plan is to change it to something like 
FileTransitivelyIncludes(PublicHeaderFor(public_header), other_file).

Original comment by vsap...@gmail.com on 21 May 2014 at 8:21

[GoogleCodeExporter]

> But all these files are present only in libstdc++, so in libc++ <set> doesn't
> have any files mapped to it.

So shouldn't we add separate mappings for libc++? This is why I built the 
external mapping mechanism, to be able to add mappings for other standard 
libraries.

It would be ideal, of course, if IWYU worked without mappings, but I wonder if 
it's as simple as re-exporting the entire transitive graph...?

Original comment by kim.gras...@gmail.com on 28 May 2014 at 9:39

[GoogleCodeExporter]

As far as I can tell, private => public mappings aren't required for libc++ (at 
least not as many as for libstdc++).  For example, <set> includes <__tree>, but 
__tree usage is just implementation detail and doesn't leak into std::set 
public interface.  That's why IWYU doesn't suggest to include <__tree> and we 
don't need mappings to fight with that.

Adding mappings only to fix ReplayClassMemberUsesFromPrecomputedList looks 
obscure and fragile to me.  I want to investigate other approaches before going 
down this path.

Original comment by vsap...@gmail.com on 1 Jun 2014 at 5:46

[GoogleCodeExporter]

But we don't want <set> to provide <memory>, do we? At least not for anything 
but the allocator.

I just want to make sure we don't end up in a situation where this is OK:

  #include <set>

  int main() {
    std::unique_ptr<int> p(new 200);
    return *p;
  }

But I can't claim to see through PublicHeaderIntendsToProvide entirely...

Original comment by kim.gras...@gmail.com on 3 Jun 2014 at 8:03

[GoogleCodeExporter]

You are right, we don't want <set> to provide <memory>, 
stl_container_provides_allocator.cc tests such case.  
ReplayClassMemberUsesFromPrecomputedList calls PublicHeaderIntendsToProvide 
only for template arguments.  Overall idea is (at least how I understand it): 
template user shouldn't include headers for template default arguments if 
template includes them already.

Original comment by vsap...@gmail.com on 3 Jun 2014 at 9:48

[GoogleCodeExporter]

Thanks, now I understand better. And sorry for completely overlooking your test 
case that proved what I was worried about :-/

This, again, looks to me like a case of author intent, maybe? The STL is 
probably fine with this solution, but user types may have something like

  template<class T>
  class MyAllocator;

  template<class T, class A=MyAllocator<T> >
  class MyContainer {
  };

and I think the same rule applies here, that the forward decl should force 
users to include MyAllocator's defining header.

It'd be interesting to write a test for this, it might already work for some 
reason :-)

But I think it's safe to say that this is gold-plating at this point; if your 
original approach passes the current tests, it should be fine.

Original comment by kim.gras...@gmail.com on 4 Jun 2014 at 4:36

[GoogleCodeExporter]

Here are 2 patches:
- issue132-IsPublic.patch is a refinement of my described approach;
- issue132-mapping.patch is a fix by adding some straightforward libc++ 
mappings.

Both patches have the same outcome on Mac OS X. Personally I prefer 
issue132-IsPublic.patch because it is more general and more focused. Thoughts?

Original comment by vsap...@gmail.com on 1 Sep 2014 at 5:41

Attachments:

• issue132-IsPublic.patch
• issue132-mapping.patch

[GoogleCodeExporter]

Thanks, good to see development on this!

Now I think I see what you're doing -- instead of/in addition to working 
backwards from private_headers_behind, you also check if the header is marked 
as public. This happens to work with the current libstdc++ mappings, because 
they contain most public STL headers, right?

It's sort of sneaky, but it will most likely work, because the libstdc++ 
mappings will need all public headers listed as such.

+  const string quoted_file = ConvertToQuotedInclude(GetFilePath(file));

It'd be nice not to have to convert back to a quoted include here, but I guess 
that's the best we can do...

+    bool is_public_header = ContainsKey(private_headers_behind, file) ||
+                            GlobalIncludePicker().IsPublic(file);

Is the private_headers_behind check even necessary anymore? Are there cases 
where it will be true where IsPublic isn't?

Fun fact: with your IsPublic patch in place, I have two more tests succeeding 
on Windows, too.

I can't say for sure which one I prefer, the IsPublic patch is nice because it 
solves this problem more generally.

But the mapping patch is nice because it adds mappings for libc++ and documents 
how to generate them. Since this is exactly the kind of problem mappings were 
designed to solve, it seems a shame not to use them. Also, the mapping patch is 
mostly mechanical, which is a good sign.

You seem more inclined toward the IsPublic patch, though, so let's go with 
that, especially since it make Windows support better out-of-the-box.

I have a few new ideas for mappings that I'd like to explore separately.

Original comment by kim.gras...@gmail.com on 1 Sep 2014 at 7:54

[GoogleCodeExporter]

> +    bool is_public_header = ContainsKey(private_headers_behind, file) ||
> +                            GlobalIncludePicker().IsPublic(file);
>
> Is the private_headers_behind check even necessary anymore?
> Are there cases where it will be true where IsPublic isn't?

I removed the ContainsKey check here and re-ran the tests. We still have only 
two failures, but I haven't checked details of the failing ones to see if they 
still break in the same place.

Original comment by kim.gras...@gmail.com on 1 Sep 2014 at 8:11

[GoogleCodeExporter]

> Now I think I see what you're doing -- instead of/in addition to working
> backwards from private_headers_behind, you also check if the header is marked
> as public. This happens to work with the current libstdc++ mappings, because
> they contain most public STL headers, right?

Right.

> It's sort of sneaky, but it will most likely work, because the libstdc++
> mappings will need all public headers listed as such.

Yeah, we can store public headers specified by C++ standard and use them in 
addition to libstdc++ mappings.  And I don't think we need to expose these 
headers in .imp files, only hardcode them.  I've started doing it and think it 
deserves a separate commit.

> It'd be nice not to have to convert back to a quoted include here, but I guess
> that's the best we can do...

To simplify moving toward FileEntry we can use more and more FileEntry in 
interfaces.  And implementation can convert to a quoted include for time being.

> Is the private_headers_behind check even necessary anymore? Are there cases
> where it will be true where IsPublic isn't?

Good catch.  Replaced ContainsKey(private_headers_behind, ...) with 
picker.IsPublic() and everything works like before.

> Fun fact: with your IsPublic patch in place, I have two more tests succeeding 
on Windows, too.

I was hoping for that.  It's good to know that it works like intended.

Honestly speaking, suggested libc++ mappings are low-quality.  Their only 
purpose is to add public headers to private_headers_behind.  Which is sneaky in 
it's own way because connection between existing_file => public_header mapping 
and precomputed template arguments isn't obvious.

So I prefer to use IsPublic approach and a new patch is based on that.

Original comment by vsap...@gmail.com on 5 Sep 2014 at 4:04

Attachments:

• issue132.patch

[GoogleCodeExporter]

Your new testcase fails on MSVC's STL:

======================================================================
FAIL: runTest (__main__.stl_container_provides_allocator)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "run_iwyu_tests.py", line 158, in <lambda>
    {'runTest': lambda self, f=filename: self.RunOneTest(f),
  File "run_iwyu_tests.py", line 129, 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/stl_container_provides_allocator.cc:27: Unexpected diagnostic:
std::uninitialized_fill is defined in <xmemory>, which isn't directly #included.

Unexpected summary diffs for tests/cxx/stl_container_provides_allocator.cc:
+++
@@ -1,8 +1,10 @@
 tests/cxx/stl_container_provides_allocator.cc should add these lines:
+#include <xmemory>

 tests/cxx/stl_container_provides_allocator.cc should remove these lines:
 - #include <stdio.h>  // lines XX-XX
+- #include <memory>  // lines XX-XX

 The full include-list for tests/cxx/stl_container_provides_allocator.cc:
-#include <memory>  // for uninitialized_fill
 #include <vector>  // for vector
+#include <xmemory>  // for uninitialized_fill
---

I haven't looked at it in detail, but I'm surprised, I would have guessed the 
private <xmemory> should have been short-circuited by your patch...

I'll try and take a look at it tomorrow.

Original comment by kim.gras...@gmail.com on 5 Sep 2014 at 7:28

[GoogleCodeExporter]

Actually, it's correct. Test case checks that <vector> provides from <memory> 
only allocator and nothing else. As the result uninitialized_fill isn't 
re-exported and is taken from <xmemory>.

What do you suggest to do with the patch? I can leave it in my working copy 
uncommitted for now. Or maybe trading 2 existing failing test cases for 1 new 
isn't such a bad deal after all.

Original comment by vsap...@gmail.com on 6 Sep 2014 at 5:32

[GoogleCodeExporter]

Ah, I see. Now that I actually read the test case, I understand what's going on.

This is directly attributable to the missing MSVC mappings, and that's a 
separate problem altogether. Please commit!

Original comment by kim.gras...@gmail.com on 6 Sep 2014 at 6:51

• Added labels: Priorit-Medium
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Here is a patch that marks all STL headers specified in Standard as public. 
Also C -> C++ mappings are extracted to a separate file/array.

Original comment by vsap...@gmail.com on 24 Sep 2014 at 5:36

Attachments:

• issue132-stl_public_headers.patch

[GoogleCodeExporter]

Looks good.

One thing I liked about csilvers' mappings is he described how they were 
generated.

Did you just manually copy and paste the header names, or could you cook up a 
simple script to parse it out of the standard draft at GitHub? I'm terrible at 
shell scripting, but I get a sense it should be possible to look for 
"tab:cpp.library.headers" and then consume all \tcode rows until the next \end.

I've been meaning to build mapping generators for all sorts of libraries and I 
had my eyes on the standard draft, but I don't speak TeX, so I gave up.

If it's too much effort (for me it would be), please commit as-is.

Original comment by kim.gras...@gmail.com on 24 Sep 2014 at 8:01

[GoogleCodeExporter]

Thanks for review. I have just copied and pasted the header names. Creating a 
script to parse headers from the standard draft is an interesting exercise. 
I'll try to do something, but if I don't come up with a solution, I'll commit 
the change as-is on the weekend.

Original comment by vsap...@gmail.com on 25 Sep 2014 at 4:56

[GoogleCodeExporter]

Cool. Please don't block the commit on scriptability.

By the way, I forgot to ask, this isn't intended to solve any concrete problem 
(e.g. test failure), just build better metadata, right?

Original comment by kim.gras...@gmail.com on 28 Sep 2014 at 7:02

[GoogleCodeExporter]

Yes, the change doesn't fix anything, just makes STL headers independent of 
libstdc++ headers. Committed r583 with appropriate shell command. Actually, it 
wasn't very hard, thanks for pushing me in that direction.

Original comment by vsap...@gmail.com on 28 Sep 2014 at 6:04

• Changed state: Fixed
• Added labels: Priority-Medium
• Removed labels: Priorit-Medium