Is it really valid to discard externally instantiated functions from a TU when marked inline?

Hi,

While investigating the situation of visibility annotations and linkage in libc++ with the goal of removing uses of `__always_inline__`, Eric Fiselier and I stumbled upon the attached test case, which I don't think Clang compiles properly. Here's the gist of the test case, reduced to the important parts (see the attachment if you want to repro):
    
    // RUN: %cxx -shared -o %T/libtest.so %flags %compile_flags -fPIC %s
    // RUN: %cxx -c -o %T/main.o %flags %compile_flags %s -O2 -DBUILDING_MAIN
    // RUN: %cxx -o %T/test.exe -L%T/ -Wl,-rpath,%T/ -ltest %T/main.o
    // RUN: %T/test.exe

    template <class T>
    struct Foo {
        inline __attribute__((visibility("hidden")))
        int __init(int x) { /* LOTS OF CODE */ }

        inline __attribute__((visibility("default")))
        int bar(int x) { return __init(x); }
    };

    extern template struct Foo<int>;

    #ifdef BUILDING_MAIN
        int main() {
            Foo<int> f;
            f.bar(101);
        }
    #else
        template struct Foo<int>;
    #endif

When running the attached file in `lit`, we get:

    Undefined symbols for architecture x86_64:
      "Foo<int>::__init(int)", referenced from:
          _main in main.o
    ld: symbol(s) not found for architecture x86_64

The idea here is that `__init` is a pretty big function, and we're promising that an external definition of it is available through the use of the extern template declaration. With the appropriate optimization level (O2 and above), LLVM decides not to include the definition of `__init` in the executable and to use the one available externally. Unfortunately, `__init` has hidden visibility, and so the definition in the .so is not visible to the executable, and the link fails.

Where I think Clang/LLVM goes wrong is when it decides to remove the instantiation in the executable in favor of the one that is hypothetically provided externally. Indeed, the Standard says in [temp.explicit]/12 ([temp.explicit]):

    Except for inline functions and variables, declarations with types deduced from their initializer or return value ([dcl.spec.auto]), const variables of literal types, variables of reference types, and class template specializations, explicit instantiation declarations have the effect of suppressing the implicit instantiation of the definition of the entity to which they refer. [ Note: The intent is that an inline function that is the subject of an explicit instantiation declaration will still be implicitly instantiated when odr-used so that the body can be considered for inlining, but that no out-of-line copy of the inline function would be generated in the translation unit. — end note ]

Only reading the normative wording, it seems like LLVM should leave the instantiation there because it can't actually assume that there will be a definition provided elsewhere (yes, despite the extern template declaration, because the function is inline). Then, the non-normative note seems to be approving of what LLVM is doing, but I'm wondering whether that's really the intended behavior.

Questions:
1. Is what LLVM's doing there legal?
2. If it is legal, then libc++ needs a way to express that a function should either be inlined in the caller, or emitted in the TU and de-duplicated later (I think that’s linkonce_odr), despite there being an extern template declaration promising a definition elsewhere. I think the current situation is that the function gets available_externally linkage instead. Is there a way to express this at the C++ source code level?

Thank you,
Louis Dionne

linkage.sh.cpp (1.03 KB)

Hi,

While investigating the situation of visibility annotations and linkage in
libc++ with the goal of removing uses of `__always_inline__`, Eric Fiselier
and I stumbled upon the attached test case, which I don't think Clang
compiles properly. Here's the gist of the test case, reduced to the
important parts (see the attachment if you want to repro):

    // RUN: %cxx -shared -o %T/libtest.so %flags %compile_flags -fPIC %s
    // RUN: %cxx -c -o %T/main.o %flags %compile_flags %s -O2
-DBUILDING_MAIN
    // RUN: %cxx -o %T/test.exe -L%T/ -Wl,-rpath,%T/ -ltest %T/main.o
    // RUN: %T/test.exe

    template <class T>
    struct Foo {
        inline __attribute__((visibility("hidden")))
        int __init(int x) { /* LOTS OF CODE */ }

        inline __attribute__((visibility("default")))
        int bar(int x) { return __init(x); }
    };

    extern template struct Foo<int>;

    #ifdef BUILDING_MAIN
        int main() {
            Foo<int> f;
            f.bar(101);
        }
    #else
        template struct Foo<int>;
    #endif

When running the attached file in `lit`, we get:

    Undefined symbols for architecture x86_64:
      "Foo<int>::__init(int)", referenced from:
          _main in main.o
    ld: symbol(s) not found for architecture x86_64

The idea here is that `__init` is a pretty big function, and we're
promising that an external definition of it is available through the use of
the extern template declaration. With the appropriate optimization level
(O2 and above), LLVM decides not to include the definition of `__init` in
the executable and to use the one available externally. Unfortunately,
`__init` has hidden visibility, and so the definition in the .so is not
visible to the executable, and the link fails.

Where I think Clang/LLVM goes wrong is when it decides to remove the
instantiation in the executable in favor of the one that is hypothetically
provided externally. Indeed, the Standard says in [temp.explicit]/12 (
[temp.explicit]):

    Except for inline functions and variables, declarations with types
deduced from their initializer or return value ([dcl.spec.auto]), const
variables of literal types, variables of reference types, and class
template specializations, explicit instantiation declarations have the
effect of suppressing the implicit instantiation of the definition of the
entity to which they refer. [ Note: The intent is that an inline function
that is the subject of an explicit instantiation declaration will still be
implicitly instantiated when odr-used so that the body can be considered
for inlining, but that no out-of-line copy of the inline function would be
generated in the translation unit. — end note ]

Only reading the normative wording, it seems like LLVM should leave the
instantiation there because it can't actually assume that there will be a
definition provided elsewhere (yes, despite the extern template
declaration, because the function is inline). Then, the non-normative note
seems to be approving of what LLVM is doing, but I'm wondering whether
that's really the intended behavior.

Questions:
1. Is what LLVM's doing there legal?

The Standard does not say anything about the instantiation producing a
definition associated with object file that results from translating the
current translation unit. The program is only valid if there is indeed an
explicit instantiation provided elsewhere. That said, the as-if rule that
allows the suppression of the definition assumes that a provided explicit
instantiation can be linked against. It is up the the designers of the
extension (the visibility attribute) to say whether or not the rule with
templates having hidden visibility is that the explicit instantiation needs
to be provided in the same "module".

This is probably much more of a question for the Clang list…

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

In the meantime, this is blocking our ability to use neither __attribute__((Internal_linkage)) nor __attribute__((__always_inline__)) on any declaration that may have an extern template declaration (which is basically all declarations, because users can write extern template declarations for std:: components in their own code).

Louis

- llvm-dev (sorry Chandler, I’m not accustomed to which topics should be
discussed on which lists yet)

This is probably much more of a question for the Clang list....

Hi,

While investigating the situation of visibility annotations and linkage
in libc++ with the goal of removing uses of `__always_inline__`, Eric
Fiselier and I stumbled upon the attached test case, which I don't think
Clang compiles properly. Here's the gist of the test case, reduced to the
important parts (see the attachment if you want to repro):

    // RUN: %cxx -shared -o %T/libtest.so %flags %compile_flags -fPIC %s
    // RUN: %cxx -c -o %T/main.o %flags %compile_flags %s -O2
-DBUILDING_MAIN
    // RUN: %cxx -o %T/test.exe -L%T/ -Wl,-rpath,%T/ -ltest %T/main.o
    // RUN: %T/test.exe

    template <class T>
    struct Foo {
        inline __attribute__((visibility("hidden")))
        int __init(int x) { /* LOTS OF CODE */ }

        inline __attribute__((visibility("default")))
        int bar(int x) { return __init(x); }
    };

    extern template struct Foo<int>;

    #ifdef BUILDING_MAIN
        int main() {
            Foo<int> f;
            f.bar(101);
        }
    #else
        template struct Foo<int>;
    #endif

When running the attached file in `lit`, we get:

    Undefined symbols for architecture x86_64:
      "Foo<int>::__init(int)", referenced from:
          _main in main.o
    ld: symbol(s) not found for architecture x86_64

The idea here is that `__init` is a pretty big function, and we're
promising that an external definition of it is available through the use of
the extern template declaration. With the appropriate optimization level
(O2 and above), LLVM decides not to include the definition of `__init` in
the executable and to use the one available externally. Unfortunately,
`__init` has hidden visibility, and so the definition in the .so is not
visible to the executable, and the link fails.

Where I think Clang/LLVM goes wrong is when it decides to remove the
instantiation in the executable in favor of the one that is hypothetically
provided externally. Indeed, the Standard says in [temp.explicit]/12 (
[temp.explicit]):

    Except for inline functions and variables, declarations with types
deduced from their initializer or return value ([dcl.spec.auto]), const
variables of literal types, variables of reference types, and class
template specializations, explicit instantiation declarations have the
effect of suppressing the implicit instantiation of the definition of the
entity to which they refer. [ Note: The intent is that an inline function
that is the subject of an explicit instantiation declaration will still be
implicitly instantiated when odr-used so that the body can be considered
for inlining, but that no out-of-line copy of the inline function would be
generated in the translation unit. — end note ]

Only reading the normative wording, it seems like LLVM should leave the
instantiation there because it can't actually assume that there will be a
definition provided elsewhere (yes, despite the extern template
declaration, because the function is inline). Then, the non-normative note
seems to be approving of what LLVM is doing, but I'm wondering whether
that's really the intended behavior.

Questions:
1. Is what LLVM's doing there legal?

The Standard does not say anything about the instantiation producing a
definition associated with object file that results from translating the
current translation unit. The program is only valid if there is indeed an
explicit instantiation provided elsewhere. That said, the as-if rule that
allows the suppression of the definition assumes that a provided explicit
instantiation can be linked against. It is up the the designers of the
extension (the visibility attribute) to say whether or not the rule with
templates having hidden visibility is that the explicit instantiation needs
to be provided in the same “module".

Thanks Hubert. That makes some amount of sense. So in that case, it would
either be
1. A bug that Clang is not emitting `__init` in the object file since it
has hidden visibility, and it can’t tell whether we’re going to try to link
dynamically or statically with the other module that provides it.
2. The intended design that visibility attributes interact very poorly
with extern template declarations. This would be sad, since libc++ is a
pretty big user of both, and it really isn’t working that well for us.

Let's map visibility attributes as affecting what constitutes the sets of
eligible uses and definitions where the ODR and the similar rules around
templates apply. Which is to say, it partitions your program.
For example, it is okay to have multiple definitions of non-inline void
::foo(); having external linkage and hidden or internal visibility as long
as each definition was in a separate module.
Similarly, it is not okay have the only definition of void ::bar() be
hidden or internal in one module when uses occur in another.
This model leads to the current result that is observed with extern
template declarations.

Technically, a possible solution is to provide explicit instantiation
definitions with inline visibility in an object file to be included on the
link step.

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

libc++ should declare explicit instantiations of just the functions it actually intends to provide in the library instead of declaring an instantiation of the entire class-template and then trying to retroactively patch over the problem with a mess of attributes on every declaration. You should be able to then just give the templates type_visibility(“default”) and visibility(“hidden”). You can put visibility(“default”) directly on the instantiation and it should override any attributes from the template.

John.

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

libc++ should declare explicit instantiations of just the functions it actually intends to provide in the library instead of declaring an instantiation of the entire class-template and then trying to retroactively patch over the problem with a mess of attributes on every declaration. You should be able to then just give the templates type_visibility(“default”) and visibility(“hidden”). You can put visibility(“default”) directly on the instantiation and it should override any attributes from the template.

That’s a very good point, somehow I hadn’t thought of it before.

One problem is that attributes like internal_linkage need to appear on the first declaration, and can’t be added later when declaring an instantiation.
Though I’m not sure if that restriction is artificial, at least in this particular case.

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

libc++ should declare explicit instantiations of just the functions it actually intends to provide in the library instead of declaring an instantiation of the entire class-template and then trying to retroactively patch over the problem with a mess of attributes on every declaration. You should be able to then just give the templates type_visibility(“default”) and visibility(“hidden”). You can put visibility(“default”) directly on the instantiation and it should override any attributes from the template.

That’s a very good point, somehow I hadn’t thought of it before.

One problem is that attributes like internal_linkage need to appear on the first declaration, and can’t be added later when declaring an instantiation.
Though I’m not sure if that restriction is artificial, at least in this particular case.

Well, I can’t imagine why you would declare an internal-linkage explicit instantiation, but if you have a need to, I don’t see any reason we couldn’t allow that.

The visibility attributes, for better or worse, are more flexible about some of these things already.

John.

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

libc++ should declare explicit instantiations of just the functions it actually intends to provide in the library instead of declaring an instantiation of the entire class-template and then trying to retroactively patch over the problem with a mess of attributes on every declaration. You should be able to then just give the templates type_visibility(“default”) and visibility(“hidden”). You can put visibility(“default”) directly on the instantiation and it should override any attributes from the template.

That’s a very good point, somehow I hadn’t thought of it before.

One problem is that attributes like internal_linkage need to appear on the first declaration, and can’t be added later when declaring an instantiation.
Though I’m not sure if that restriction is artificial, at least in this particular case.

Well, I can’t imagine why you would declare an internal-linkage explicit instantiation, but if you have a need to, I don’t see any reason we couldn’t allow that.

The visibility attributes, for better or worse, are more flexible about some of these things already.

Is there a way to explicitly instantiate the vtable and the typeinfo of a type (and give them default visibility), other than to explicitly instantiate the type itself?

If so, we could do the following for templates that we wish to explicitly instantiate in the dylib:

// in the headers
#define HIDDEN attribute((visibility(“hidden”)))
#define VISIBLE attribute((visibility(“default”)))

template <class _CharT, class _Traits>
class HIDDEN basic_istream
: virtual public basic_ios<_CharT, _Traits>
{
/* no visibility macro here (like today) */
basic_istream& operator>>(bool& __n) { … }
basic_istream& operator>>(short& __n) { … }

};

extern template basic_istream::typeinfo; // obviously not valid, but you get the point
extern template basic_istream::vtable; // ditto
extern template basic_istream& basic_istream::operator>>(bool&);
extern template basic_istream& basic_istream::operator>>(short&);

// in the dylib
template VISIBLE basic_istream::typeinfo;
template VISIBLE basic_istream::vtable;
template VISIBLE basic_istream& basic_istream::operator>>(bool&);
template VISIBLE basic_istream& basic_istream::operator>>(short&);

This would be more straightforward than what we do today, and it would allow us to see what’s in the dylib and what’s not very easily.

John, is this roughly what you had in mind?

Louis

That’s a very good point, somehow I hadn’t thought of it before.

One problem is that attributes like internal_linkage need to appear on the first declaration, and can’t be added later when declaring an instantiation.
Though I’m not sure if that restriction is artificial, at least in this particular case.

Well, I can’t imagine why you would declare an internal-linkage explicit instantiation, but if you have a need to, I don’t see any reason we couldn’t allow that.

The visibility attributes, for better or worse, are more flexible about some of these things already.

Is there a way to explicitly instantiate the vtable and the typeinfo of a type (and give them default visibility), other than to explicitly instantiate the type itself?

For the v-table, unfortunately not. For RTTI, it’s the same except that explicitly instantiating a non-dynamic class doesn’t eagerly emit RTTI for it.

Does this matter, though? What explicitly-instantiated class templates in the stdlib actually have virtual me…

extern template basic_istream::typeinfo; // obviously not valid, but you get the point
extern template basic_istream::vtable; // ditto

…oh, right, the stream classes. Ugh.

extern template basic_istream& basic_istream::operator>>(bool&);
extern template basic_istream& basic_istream::operator>>(short&);

// in the dylib
template VISIBLE basic_istream::typeinfo;
template VISIBLE basic_istream::vtable;
template VISIBLE basic_istream& basic_istream::operator>>(bool&);
template VISIBLE basic_istream& basic_istream::operator>>(short&);

This would be more straightforward than what we do today, and it would allow us to see what’s in the dylib and what’s not very easily.

John, is this roughly what you had in mind?

Yeah, this is what I had in mind. Unfortunately, the standard provides no mechanism for instantiating just the v-table/VTT/RTTI without also instantiating all the members.

(A reasonable spelling for the standard way of doing this would be something like “template virtual class basic_istream;” or “template typeid class basic_istream;”. But we shouldn’t use that in a vendor extension without the committee’s blessing.)

John.

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

libc++ should declare explicit instantiations of just the functions it actually intends to provide in the library instead of declaring an instantiation of the entire class-template and then trying to retroactively patch over the problem with a mess of attributes on every declaration. You should be able to then just give the templates type_visibility(“default”) and visibility(“hidden”). You can put visibility(“default”) directly on the instantiation and it should override any attributes from the template.

That’s a very good point, somehow I hadn’t thought of it before.

One problem is that attributes like internal_linkage need to appear on the first declaration, and can’t be added later when declaring an instantiation.
Though I’m not sure if that restriction is artificial, at least in this particular case.

Well, I can’t imagine why you would declare an internal-linkage explicit instantiation, but if you have a need to, I don’t see any reason we couldn’t allow that.

Sorry. I don’t want to declare an explicit instantiation as having internal linkage. But I might want to declare everything except for an explicit instantiation as having internal linkage.
But since the attribute must appear on the first declaration, it’s not possible to override it.

Even if we made Clang play nice with our design or provide magic to allow libc++ to act in a sane manner, GCC is still an issue.

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

libc++ should declare explicit instantiations of just the functions it actually intends to provide in the library instead of declaring an instantiation of the entire class-template and then trying to retroactively patch over the problem with a mess of attributes on every declaration. You should be able to then just give the templates type_visibility(“default”) and visibility(“hidden”). You can put visibility(“default”) directly on the instantiation and it should override any attributes from the template.

That’s a very good point, somehow I hadn’t thought of it before.

One problem is that attributes like internal_linkage need to appear on the first declaration, and can’t be added later when declaring an instantiation.
Though I’m not sure if that restriction is artificial, at least in this particular case.

Well, I can’t imagine why you would declare an internal-linkage explicit instantiation, but if you have a need to, I don’t see any reason we couldn’t allow that.

Sorry. I don’t want to declare an explicit instantiation as having internal linkage. But I might want to declare everything except for an explicit instantiation as having internal linkage.
But since the attribute must appear on the first declaration, it’s not possible to override it.

Even if we made Clang play nice with our design or provide magic to allow libc++ to act in a sane manner, GCC is still an issue.

There’s another option, though. We could also just ditch internal_linkage (and always_inline for that matter) entirely, since I don’t think anybody knows whether we need to enable TUs built with different libc++ versions to interoperate. It’s “supported” right now, but it’s not clear that we actually need it nor support it properly. If we did that, we’d be down to a simple visibility problem, which we can solve using what John suggested. The main benefit I see is that we’d get rid of 95% of the visibility annotations in libc++: we’d simply build with -fvisibility=hidden and export exactly what we want. That would be more sane.

Louis

  • llvm-dev (sorry Chandler, I’m not accustomed to which topics should be discussed on which lists yet)

This is probably much more of a question for the Clang list…

Thanks Hubert. That makes some amount of sense. So in that case, it would either be

  1. A bug that Clang is not emitting __init in the object file since it has hidden visibility, and it can’t tell whether we’re going to try to link dynamically or statically with the other module that provides it.
  2. The intended design that visibility attributes interact very poorly with extern template declarations. This would be sad, since libc++ is a pretty big user of both, and it really isn’t working that well for us.

In both cases, it’s not a conformance bug with the Standard, as Hubert points out. It would be lovely if someone more knowledgeable about the visibility attributes could shed some light on whether that’s intended and we need to invent something new, or whether it is correct to consider it a bug and we should fix it (somehow).

libc++ should declare explicit instantiations of just the functions it actually intends to provide in the library instead of declaring an instantiation of the entire class-template and then trying to retroactively patch over the problem with a mess of attributes on every declaration. You should be able to then just give the templates type_visibility(“default”) and visibility(“hidden”). You can put visibility(“default”) directly on the instantiation and it should override any attributes from the template.

That’s a very good point, somehow I hadn’t thought of it before.

One problem is that attributes like internal_linkage need to appear on the first declaration, and can’t be added later when declaring an instantiation.
Though I’m not sure if that restriction is artificial, at least in this particular case.

Well, I can’t imagine why you would declare an internal-linkage explicit instantiation, but if you have a need to, I don’t see any reason we couldn’t allow that.

Sorry. I don’t want to declare an explicit instantiation as having internal linkage. But I might want to declare everything except for an explicit instantiation as having internal linkage.
But since the attribute must appear on the first declaration, it’s not possible to override it.

Even if we made Clang play nice with our design or provide magic to allow libc++ to act in a sane manner, GCC is still an issue.

There’s another option, though. We could also just ditch internal_linkage (and always_inline for that matter) entirely, since I don’t think anybody knows whether we need to enable TUs built with different libc++ versions to interoperate. It’s “supported” right now, but it’s not clear that we actually need it nor support it properly. If we did that, we’d be down to a simple visibility problem, which we can solve using what John suggested. The main benefit I see is that we’d get rid of 95% of the visibility annotations in libc++: we’d simply build with -fvisibility=hidden and export exactly what we want. That would be more sane.

Well, never mind this. It’s apparently not an option since clients (like libfuzzer) are relying on this. Someone mentioned in IRC that libfuzzer is linked statically against a libc++ they build themselves. And then a user application using (a possibly different version of) libc++ can link statically against libfuzzer. While I think this is a brittle setup (and it only works because libc++ is currently throwing always_inline really hard at the problem), it’s probably not reasonable to stop supporting the use case since people have been relying on it.

Louis

I haven’t looked at what libfuzzer does, but as you’ve described it, I’d say this doesn’t seem a reasonable use-case. If you want to link libc++ (or any other library!) hidden inside your library, you should be using a version-script to only expose symbols that you intend to expose.

In XRay, we go out of our way to not use any standard-library defined symbols that need linkage. We have a tests which ensure that we are able to link the XRay runtime without the standard library dependency. This has been discussed before, I think, precisely in this context.

The options for libFuzzer might be more limited since it’s a special use-case, and I’m sure unless the fuzzer runtime removes dependencies on standard library symbols that libc++ shouldn’t be prioritising this use-case. Another way of saying this is that the cost of ensuring that libFuzzer continues to work despite changes to libc++ should be borne by libFuzzer, not the other way around.

I don’t know how much work is required to use a work-around as James describes with symbol visibility or “privatising” the symbols libFuzzer uses from libc++. It could be less than trying to remove the dependency on libc++ instead.

-- Dean

I haven’t looked at what libfuzzer does, but as you’ve described it, I’d say this doesn’t seem a reasonable use-case. If you want to link libc++ (or any other library!) hidden inside your library, you should be using a version-script to only expose symbols that you intend to expose.

In XRay, we go out of our way to not use any standard-library defined symbols that need linkage. We have a tests which ensure that we are able to link the XRay runtime without the standard library dependency. This has been discussed before, I think, precisely in this context.

The options for libFuzzer might be more limited since it’s a special use-case, and I’m sure unless the fuzzer runtime removes dependencies on standard library symbols that libc++ shouldn’t be prioritising this use-case. Another way of saying this is that the cost of ensuring that libFuzzer continues to work despite changes to libc++ should be borne by libFuzzer, not the other way around.

Well, that’s also my impression, but I don’t know how we can actually change this given there are clients using it and the status quo usually wins when no consensus can be reached. What I suggest is that we instead start implementing my proposal at http://lists.llvm.org/pipermail/cfe-dev/2018-July/058457.html (I’m waiting for reviews on https://reviews.llvm.org/D49240 BTW), which will solve most concrete problems we have right now. In the not too distant future, we can then evaluate the feasibility of removing support for interoperability of TUs built with different versions and the benefits of doing so — as a separate effort (which I’d be glad to volunteer for).

My primary concern at this point is about moving forward and solving the concrete problems we have with the use of always_inline, and I’m willing to compromise to get there, and to do it in stages if I have to.

Cheers,
Louis

I might be completely wrong about this, but I don’t think libFuzzer relies on __always_inline, at least not intentionally. The way libFuzzer statically links libc++ is more complicated than simply merging all object files into a single archive. What we do is first compiling libc++ with _LIBCPP_ABI_VERSION=Fuzzer so all symbols that would normally end up in std::__1 (or __2 if you use ABI v2) namespace end up in std::__Fuzzer namespace (this may not even be needed given what follows). We also also disable the default visibility annotations by setting _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS and compile libc++ with -fvisibility=hidden set so all symbols should end up as hidden. We then link all libFuzzer and libc++ object files into a single relocatable .o file and we use objcopy --localize-hidden to make all symbols that were marked as hidden (i.e. all libc++ symbols) local. Given that, there should be no collisions between libFuzzer’s internal libc++ copy and libc++ application is using (even when linking libc++ statically), so I don’t think removing __always_inline would make any difference for this use case.

I might be completely wrong about this, but I don’t think libFuzzer relies on __always_inline, at least not intentionally. The way libFuzzer statically links libc++ is more complicated than simply merging all object files into a single archive. What we do is first compiling libc++ with _LIBCPP_ABI_VERSION=Fuzzer so all symbols that would normally end up in std::__1 (or __2 if you use ABI v2) namespace end up in std::__Fuzzer namespace (this may not even be needed given what follows). We also also disable the default visibility annotations by setting _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS and compile libc++ with -fvisibility=hidden set so all symbols should end up as hidden. We then link all libFuzzer and libc++ object files into a single relocatable .o file and we use objcopy --localize-hidden to make all symbols that were marked as hidden (i.e. all libc++ symbols) local. Given that, there should be no collisions between libFuzzer’s internal libc++ copy and libc++ application is using (even when linking libc++ statically), so I don’t think removing __always_inline would make any difference for this use case.

If that’s how it works, then indeed I don’t think libFuzzer is a problem. The part that saves us is the one where hidden symbols are marked as local. In that case, it may be worthwhile to consider a complete redesign of libc++’s approach to visibility, which could yield significant simplifications. I’ll think about that once we’ve solved the immediate problem of __always_inline__.

Louis