Clang emits unreachable `internal linkage` constructor

Hi,

This fairly simple (and valid) code does not link (on MacOS):

#include <iostream>
#include <string>
namespace {
  struct VA {
  };
  struct A : virtual public VA {
    A() {
      static bool b = false;
      std::string str;
    }
  };
}
int main(int argc, char* argv) {
}

The issue is that clang emits two constructors (the complete one and the base one) for struct A because it has a virtual base class.

Because struct A is declared in an anonymous namespace, these constructors are internal linkage. Only one of them is actually called, the other is unreachable.

The “always-inliner” does not visit unreachable internal function. However this constructor is calling into a function from libc++ that are marked “available_externally” and “always inline”: the basic_string constructor.

The problem is that the linkage “available_externally" is a “lie”: there is an external template instantiation in the header, but it is marked “visibility hidden”. So the function cannot be linked to in the libc++ dylib. (I think it’d work with a static libc++).

I see multiple ways to address such cases, possibly many of them can be implemented:

- Don’t emit unreferenced internal_linkage function. (We should try to do this anyway right?)
- Have the CGSCC run on the unreachable part of the call-graph.
- Run global-DCE at O0 (might be a good idea anyway if it speeds-up the build)
- We should have the “available_externally” compatible with “hidden visibility”. Maybe we need a way to have to mark some method excluded from one `external template` instantiation so that it would be ODR.
- We could make the basic_string constructor and other method in the same situation internal linkage instead.

Thoughts?

Hi,

This fairly simple (and valid) code does not link (on MacOS):

#include <iostream>
#include <string>
namespace {
  struct VA {
  };
  struct A : virtual public VA {
    A() {
      static bool b = false;
      std::string str;
    }
  };
}
int main(int argc, char* argv) {
  A::A a;
}

The issue is that clang emits two constructors (the complete one and the base one) for struct A because it has a virtual base class.

Because struct A is declared in an anonymous namespace, these constructors are internal linkage. Only one of them is actually called, the other is unreachable.

The “always-inliner” does not visit unreachable internal function. However this constructor is calling into a function from libc++ that are marked “available_externally” and “always inline”: the basic_string constructor.

The problem is that the linkage “available_externally" is a “lie”: there is an external template instantiation in the header, but it is marked “visibility hidden”. So the function cannot be linked to in the libc++ dylib. (I think it’d work with a static libc++).

I see multiple ways to address such cases, possibly many of them can be implemented:

- Don’t emit unreferenced internal_linkage function. (We should try to do this anyway right?)

This makes sense to me. It seems to me like a bug that it's emitted.

- Have the CGSCC run on the unreachable part of the call-graph.

This seems to pessimize the normal case.

- Run global-DCE at O0 (might be a good idea anyway if it speeds-up the build)

Would we be surprised by anything it deletes? If it's only going to delete unreferenced static globals, it seems (1) harmless and (2) unnecessary unless there are frontend bugs. But maybe it's

- We should have the “available_externally” compatible with “hidden visibility”. Maybe we need a way to have to mark some method excluded from one `external template` instantiation so that it would be ODR.

This seems useful. I think individual functions should be able to opt-out of a class-wide `extern template` instantiation.

- We could make the basic_string constructor and other method in the same situation internal linkage instead.

Could pointer equality fail on the function pointers of basic_string::basic_string?

IMO this is the real problem. If basic_string isn't actually available
externally, why have explicit template instaniation declarations in
<string>? IIRC John put these back for some reason, but if all of
basic_string is always_inline, what is the point?

I also think libc++ should stop abusing always_inline for linkage purposes.
We should use __attribute__((internal_linkage)) instead, and allow the
optimizer to make informed inlining decisions.

It seems a general issue though: how do you mix visibility hidden and external template? This is fine if you link statically, but can’t be exported outside of a DSO.
Should we design a warning for when we mix external template with visibility hidden?

Also the intent for libc++ seems that we’d like most of the class to be external template but for some method. Maybe it could be achieved with a base class that would be external template, and the derived wouldn’t? (The derived would contain the current “hidden” method).

We have another thread for this: http://lists.llvm.org/pipermail/cfe-dev/2016-October/051151.html ; feel free to chime in there!

It seems a general issue though: how do you mix visibility hidden and
external template? This is fine if you link statically, but can’t be
exported outside of a DSO.
Should we design a warning for when we mix external template with
visibility hidden?

I don't think we should warn on that. I think if someone uses hidden
visibility and extern template decls, it means they know they will
statically link in a definition of that template. That seems like a
legitimate use case.

Also the intent for libc++ seems that we’d like most of the class to be
external template but for some method. Maybe it could be achieved with a
base class that would be external template, and the derived wouldn’t? (The
derived would contain the current “hidden” method).

I don't think I fully understand the way the _LIBCPP_INLINE_VISIBILITY is
applied to basic_string, so I couldn't say for sure. I can't tell if
libc++.dylib is supposed to provide basic_string symbols or not.

We have another thread for this: http://lists.llvm.org/
pipermail/cfe-dev/2016-October/051151.html ; feel free to chime in there!

Neat. These things get lost during post-vacation triage. =P

>
> Hi,
>
> This fairly simple (and valid) code does not link (on MacOS):
>
>
> #include <iostream>
> #include <string>
> namespace {
> struct VA {
> };
> struct A : virtual public VA {
> A() {
> static bool b = false;
> std::string str;
> }
> };
> }
> int main(int argc, char* argv) {
> A::A a;
> }
>
>
>
> The issue is that clang emits two constructors (the complete one and the
base one) for struct A because it has a virtual base class.
>
> Because struct A is declared in an anonymous namespace, these
constructors are internal linkage. Only one of them is actually called, the
other is unreachable.
>
> The “always-inliner” does not visit unreachable internal function.
However this constructor is calling into a function from libc++ that are
marked “available_externally” and “always inline”: the basic_string
constructor.
>
> The problem is that the linkage “available_externally" is a “lie”: there
is an external template instantiation in the header, but it is marked
“visibility hidden”. So the function cannot be linked to in the libc++
dylib. (I think it’d work with a static libc++).
>
> I see multiple ways to address such cases, possibly many of them can be
implemented:
>
> - Don’t emit unreferenced internal_linkage function. (We should try to
do this anyway right?)

This makes sense to me. It seems to me like a bug that it's emitted.

Regardless, resolving it does not fix the problem here. For instance, the
same issue would arise under -femit-all-decls, or if uses exist at the AST
level but are optimized away after IR generation.

- Have the CGSCC run on the unreachable part of the call-graph.

This seems to pessimize the normal case.

Nonetheless, it's necessary if we want to provide the guarantee that the
always_inline attribute is supposed to provide. (It's not a hint; per the
GCC documentation it is an error if a function is annotated always_inline
and is not inlined.)

If we don't want to provide that guarantee, so be it, but then libc++'s use
of this attribute is simply wrong, since it's relying on that guarantee.

If uses are optimized away after IR generation, this is not a problem from an LLVM point of view: clang didn’t emit an unreachable internal function in the first place, LLVM will track that the uses were removed and delete the newly unreachable internal function.

Your point stands for -femit-all-decls though.

Hi,

This fairly simple (and valid) code does not link (on MacOS):

#include <iostream>
#include <string>
namespace {
  struct VA {
  };
  struct A : virtual public VA {
    A() {
      static bool b = false;
      std::string str;
    }
  };
}
int main(int argc, char* argv) {
  A::A a;
}

The issue is that clang emits two constructors (the complete one and the base one) for struct A because it has a virtual base class.

Because struct A is declared in an anonymous namespace, these constructors are internal linkage. Only one of them is actually called, the other is unreachable.

The “always-inliner” does not visit unreachable internal function. However this constructor is calling into a function from libc++ that are marked “available_externally” and “always inline”: the basic_string constructor.

The problem is that the linkage “available_externally" is a “lie”: there is an external template instantiation in the header, but it is marked “visibility hidden”. So the function cannot be linked to in the libc++ dylib. (I think it’d work with a static libc++).

Is it actually hidden in the library, or is it in practice exported? That is, is the link failure specifically because the external definition is not defined within the current linkage unit (and thus we emit a code sequence that cannot be used to call an external symbol), or is because the symbol isn't available at all?

Assuming the latter, then maybe the right answer is that libc++ should only be declaring an external instantiation of the members it actually exports. That would be the most explicit thing, because arguably what libc++ is currently doing is a lie. Trying to work around that problem in the compiler by recognizing certain members as not "really" being externally available seems doomed to failure.

John.

Right: that’s why I suggested going through a base class that would be externally instantiated, and a derived one for the specific member intended to be hidden.
That seems like the most “clear” (and maintainable) way to achieve it, but there might be drawbacks (or alternative) that I missed.

Hi,

This fairly simple (and valid) code does not link (on MacOS):

#include <iostream>
#include <string>
namespace {
  struct VA {
  };
  struct A : virtual public VA {
    A() {
      static bool b = false;
      std::string str;
    }
  };
}
int main(int argc, char* argv) {
  A::A a;
}

The issue is that clang emits two constructors (the complete one and the base one) for struct A because it has a virtual base class.

Because struct A is declared in an anonymous namespace, these constructors are internal linkage. Only one of them is actually called, the other is unreachable.

The “always-inliner” does not visit unreachable internal function. However this constructor is calling into a function from libc++ that are marked “available_externally” and “always inline”: the basic_string constructor.

The problem is that the linkage “available_externally" is a “lie”: there is an external template instantiation in the header, but it is marked “visibility hidden”. So the function cannot be linked to in the libc++ dylib. (I think it’d work with a static libc++).

Is it actually hidden in the library, or is it in practice exported? That is, is the link failure specifically because the external definition is not defined within the current linkage unit (and thus we emit a code sequence that cannot be used to call an external symbol), or is because the symbol isn't available at all?

Assuming the latter, then maybe the right answer is that libc++ should only be declaring an external instantiation of the members it actually exports. That would be the most explicit thing, because arguably what libc++ is currently doing is a lie. Trying to work around that problem in the compiler by recognizing certain members as not "really" being externally available seems doomed to failure.

Right: that’s why I suggested going through a base class that would be externally instantiated, and a derived one for the specific member intended to be hidden.
That seems like the most “clear” (and maintainable) way to achieve it, but there might be drawbacks (or alternative) that I missed.

Well, I don't know about "maintainable". The set of members that are guaranteed to be explicitly instantiated is fixed. If a future version of the standard adds members to basic_string, those members will not be explicitly instantiated in existing versions of the library, and that's something the compiler needs to know about. If you want to expand the set of explicit instantiations, you'll need to gate those explicit instantiation declarations on the target deployment version.

Enumerating all the methods that are instantiated is probably *obnoxious*, but it's the most correct thing to do.

John.

OK I see, makes sense.
I was finding the base-class trick a little less syntax-heavy, but as you mention this is a “fixed” list anyway.