Errors in libc++

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Run IWYU test suite with libc++, not with libstdc++
2. Or run IWYU on attached 'template.cpp'. It is a repro case based on 
'overloaded_class.cc' failing with libc++.

What is the expected output?
No errors because clang has no errors in 'template.cpp'.

What do you see instead?
The following error:

./tests/libcxx_vector.h:14:12: error: constructor for 
'test::compressed_pair<int *, test::allocator<int> &>' must explicitly 
initialize the reference member 'second_'
  explicit compressed_pair(T1 t1) : first_(t1) {}
           ^
./tests/libcxx_vector.h:29:20: note: in instantiation of member function 
'test::compressed_pair<int *, test::allocator<int> &>::compressed_pair' 
requested here
  split_buffer() : end_cap_(0) {}
                   ^
./tests/libcxx_vector.h:49:47: note: in instantiation of member function 
'test::split_buffer<int, test::allocator<int> &>::split_buffer' requested here
    split_buffer<value_type, allocator_type&> v(n, a);
                                              ^
./tests/libcxx_vector.h:12:6: note: declared here
  T2 second_;
     ^

Original issue reported on code.google.com by vsap...@gmail.com on 10 Jan 2014 at 7:33

• Blocking: #136

Attachments:

• libcxx_vector.h
• template.cpp

[GoogleCodeExporter]

Looks like the problem is that IWYU forces instantiation of 'split_buffer' 
constructor without arguments, but this constructor isn't used. And it cannot 
be used because it itself uses 'compressed_pair<int, allocator<int>&>' without 
initializing second member, which has reference type.

Original comment by vsap...@gmail.com on 10 Jan 2014 at 7:41

[GoogleCodeExporter]

So, bug in libc++? Do you want to post a patch to cfe-commits? I have one 
outstanding for a similar LLVM bug.

Original comment by kim.gras...@gmail.com on 10 Jan 2014 at 8:30

[GoogleCodeExporter]

I need to check 'libcxx_vector.h' with another compiler and read about template 
instantiation. But I am afraid that templates are instantiated lazily and parts 
of template code can be semantically incorrect for some template arguments, as 
long as these parts aren't used. That's why libc++ is correct and IWYU hits 
problems due to eager template instantiation.

Original comment by vsap...@gmail.com on 10 Jan 2014 at 9:10

[GoogleCodeExporter]

I see what you mean now -- as long as compressed_pair is not instantiated with 
a reference as its second argument, everything is fine.

The test suite for boost::compressed_pair indicates that _maybe_ this should be 
OK:
https://gitorious.org/boost/utility/source/91407e0a5de9ad867d59f7b1d00d4b90a8921
792:compressed_pair_test.cpp#L200

I don't think the test suite covers a non-empty, reference T2 like there is in 
libc++, but there's a lot of other cases that I don't believe libc++ handles, 
so it could be that libc++'s implementation is just less elaborate. I can't 
think of a way to implement it off-hand, but my template skills degrade 
significantly after 10AM. :-)

Original comment by kim.gras...@gmail.com on 10 Jan 2014 at 10:08

[GoogleCodeExporter]

Original comment by kim.gras...@gmail.com on 16 Jan 2014 at 6:07

• Now blocking: #119

[GoogleCodeExporter]

Some more digging has revealed, that we encounter an error when 
InstantiateImplicitMethods, namely when instantiate unused 

split_buffer<int, allocator<int>&>() : end_cap_(0) {}

which in turn instantiates 

compressed_pair<int, allocator<int>&>(int)

So when I've commented out instantiating constructors (patch is attached), it 
fixed the problem.  What is more interesting, it didn't break anything else.  
But I think it's a dubious fix and need to investigate more if not 
instantiating all constructors doesn't cause other issues.

Original comment by vsap...@gmail.com on 19 Jan 2014 at 8:13

Attachments:

• not_instantiate_ctors.patch

[GoogleCodeExporter]

That's curious, that all tests pass without it...

I've always been uncomfortable with modifying the AST during traversal, but the 
comments around it make good case: we can't risk removing headers used by one 
constructor that happens to be unused in the context we're evaluating.

We should probably cook up a test case for that.

I've spent a little time trying to understand how Sema's instantiation logic 
works, to see if instantiations that won't work can be trimmed, but I haven't 
gotten very far.

Original comment by kim.gras...@gmail.com on 19 Jan 2014 at 8:58

[GoogleCodeExporter]

Original comment by vsap...@gmail.com on 21 Jan 2014 at 6:16

[GoogleCodeExporter]

TL;DR summary: patch isn't very interesting, it is mostly explanation why 
dubios fix in not_instantiate_ctors.patch might be the right fix.

The problem is that instantiating all constructors triggers an error.  So, I've 
decided to investigate if we can capture uses in instantiated constructor 
without its instantiation (by examining template pattern, for example).  And 
the question is: what uses are provided by instantiated methods?

As far as I can tell, instantiated methods provide dependent uses.  For 
example, SomeClass isn't used in

    template <typename T> struct Foo {
      Foo() { T::a(); }
    };

SomeClass is used when Foo is instantiated like

    Foo<SomeClass> f;

which differs from not dependent use

    Foo() { SomeClass::a(); }

In this case SomeClass use will be reported without instantiation.  But 
experimenting with IWYU has shown that in

    template <typename T> struct Foo {
      Foo() {}
      Foo(int unused) { T::a(); }
    };
    void bar() {
      Foo<SomeClass> f;
    }

for SomeClass forward declaration is enough, full use isn't required.  I.e. 
T::a() doesn't trigger full use.  It is so because uses are reported during 
recursive visiting and unused constructors aren't visited (see 
BaseAstVisitor::TraverseCXXConstructExpr for handling constructors, 
destructors).  So instantiating unused constructors has no value.  Destructor 
instantiation is explained in patch explanation.

Digging through commits' history has convinced me that instantiating unused 
constructors wasn't a goal.  r236 shows that only implicit 
constructors/destructors were defined.  And method name 
InstantiateImplicitMethods indicates that it's about implicit methods, not 
about unused constructors.

I've checked that GCC and MSVC don't instantiate unused methods and have found 
a relevant explanation in C++ standard:
[temp.inst] 14.7.1
> Unless a member of a class template or a member template has been explicitly
> instantiated or explicitly specialized,
> the specialization of the member is implicitly instantiated when the
> specialization is referenced in a context that requires the member definition
> to exist;

The attached patch is intermediate result, some asserts probably will be 
removed.  getTemplateInstantiationPattern asserts aren't needed, I've added 
them only to prove my hypothesis that con-, destructors cannot simultaneously 
be implicit and have template instantiation pattern.  It proves that removing 
direct manipulations with sema.PendingInstantiations doesn't affect defining 
implicit con-, destructor.  But I don't think that 
getTemplateInstantiationPattern asserts provide much value, so I am going to 
remove them in a final patch.

CHECK_(dtor->isDefined) is needed because it checks "destructor is defined" 
postcondition and shows that adding destructor to PendingInstantiations is 
useless (see Sema::InstantiateFunctionDefinition [1], defined functions aren't 
instantiated).  And CHECK_(ctor->isDefined) isn't really needed, I've added it 
for similarity with dtor->isDefined, to see what will happen.

[1] 
http://clang.llvm.org/doxygen/SemaTemplateInstantiateDecl_8cpp_source.html#l0329
8

Original comment by vsap...@gmail.com on 3 May 2014 at 10:23

Attachments:

• issue118r1.patch

[GoogleCodeExporter]

Thanks for doing all the hard detective work!

I think I understand what you're saying... I misinterpreted you earlier, I 
thought the entire InstantiateImplicitMethods was unnecessary.

If we remove the sema.PendingInstantiations.push_back, I would expect 
sema.PerformPendingInstantiations to be superfluous as well, but it turns out 
it's necessary for this part of badinc.cc:

  template<class T> struct NeverCalledTemplateStruct {
    NeverCalledTemplateStruct() { T t; }
  };
  // IWYU: I1_Class is...*badinc-i1.h
  struct NeverCalledStruct {
    // IWYU: I1_Class needs a declaration
    NeverCalledTemplateStruct<I1_Class> c;
  };

The reason, as I understand it, is that DefineImplicitDefaultConstructor will 
cause instantiation of NeverCalledTemplateStruct<I1_Class>, which in turn will 
require that we PerformPendingInstantiations afterwards.

But this touches on something you haven't mentioned -- the implicit ctors/dtors 
themselves won't be templated, but they might cause templates to be 
instantiated. Have you factored that into your analysis?

There were comments in the commit history mentioning crashes when attempting to 
instantiate non-templates, but I think the getTemplateInstantiationPattern 
check was misguided.

There are no differences in test outputs between HEAD and your patch, so in 
that sense it seems right.

Original comment by kim.gras...@gmail.com on 4 May 2014 at 8:09

[GoogleCodeExporter]

I haven't really considered that implicit ctors/dtors might instantiate 
templates.  But looks like IWYU behavior is still correct.  We need to visit 
class data members and if these members are built by defining constructor, that 
seems like a reasonable approach.

Final patch is delayed because additional testing has revealed assertion 
failure for CHECK_(dtor->isDefined).  As far as I can tell, Clang itself tries 
to instantiate all destructors.  But if instantiation is suppressed, destructor 
will stay undefined.  And instantiation can be suppressed with "extern 
template".  I'll start a new thread on the mailing list about "extern template".

Original comment by vsap...@gmail.com on 5 May 2014 at 11:34

[GoogleCodeExporter]

The updated patch is attached.

Original comment by vsap...@gmail.com on 6 May 2014 at 10:39

Attachments:

• issue118r2.patch

[GoogleCodeExporter]

Review request ping.

Original comment by vsap...@gmail.com on 29 May 2014 at 7:26

[GoogleCodeExporter]

I think the patch looks good, and the tests all pass!

But I wonder if the test case could be further reduced? Isn't it just a case of 
having a class with two constructors, one of which is illegal? So why not stop 
at compressed_pair, instead of going through the trouble of implementing 
split_buffer and vector?

Something like:

  template< class T1, class T2 >
  class PartiallyInvalid {
    T1& v1;
    T2& v2;

  public:
    PartiallyInvalid(T1& v)
      : v1(v) {
      // This ctor is invalid, v2 cannot be left uninitialized.
    }

    PartiallyInvalid(T1& v1, T2& v2)
      : v1(v1)
      , v2(v2) {
      // This ctor is fine, both references are initialized.
    }
  };

  // Now call the valid ctor. This program should compile,
  // but IWYU without the fix should break.
  int i = 100, j = 200;
  PartiallyInvalid<int, int> pi(i, j);

Isn't that clearer? Or am I missing some detail?

Original comment by kim.gras...@gmail.com on 29 May 2014 at 8:16

[GoogleCodeExporter]

Original comment by kim.gras...@gmail.com on 30 May 2014 at 6:20

• Now blocking: #136

[GoogleCodeExporter]

You are completely right, the test case can be and should be shortened.  I 
prefer to have non-reference variables, just types T1 and T2.  This way it 
cannot be detected prior to instantiation that some constructors cannot be 
used.  In your case PartiallyInvalid(T1& v1) is always invalid regardless of T1 
and T2.  Updated patch is attached.

Original comment by vsap...@gmail.com on 30 May 2014 at 10:58

Attachments:

• issue118r3.patch

[GoogleCodeExporter]

Nicely done! Looks good, please commit.

Hopefully this means we can close #136 as well immediately.

Original comment by kim.gras...@gmail.com on 30 May 2014 at 11:58

[GoogleCodeExporter]

This issue was closed by revision r549.

Original comment by vsap...@gmail.com on 30 May 2014 at 12:36

• Changed state: Fixed