[cfe-dev] [llvm-dev] Is it really valid to discard externally instantiated functions from a TU when marked inline?
James Y Knight via cfe-dev
cfe-dev at lists.llvm.org
Thu Jul 19 08:49:39 PDT 2018
On Thu, Jul 19, 2018 at 10:16 AM Louis Dionne via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
>
>
> On Jul 19, 2018, at 09:30, Louis Dionne <ldionne at apple.com> wrote:
>
>
>
> On Jul 19, 2018, at 04:24, Eric Fiselier <eric at efcs.ca> wrote:
>
>
>
> On Wed, Jul 18, 2018 at 11:53 AM John McCall <rjmccall at apple.com> wrote:
>
>> On Jul 18, 2018, at 5:40 AM, Eric Fiselier <eric at efcs.ca> wrote:
>> On Wed, Jul 18, 2018 at 1:30 AM John McCall via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>> On Jul 11, 2018, at 5:29 PM, Louis Dionne via cfe-dev <
>>> cfe-dev at lists.llvm.org> wrote:
>>>
>>> - llvm-dev (sorry Chandler, I’m not accustomed to which topics should be
>>> discussed on which lists yet)
>>>
>>> On Jul 11, 2018, at 06:37, Chandler Carruth <chandlerc at gmail.com> wrote:
>>>
>>> This is probably much more of a question for the Clang list....
>>>
>>> On Tue, Jul 10, 2018 at 6:21 PM Hubert Tong via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> On Tue, Jul 10, 2018 at 7:20 PM, Louis Dionne via llvm-dev <
>>>> llvm-dev at lists.llvm.org> wrote:
>>>>
>>>>> 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 (
>>>>> http://eel.is/c++draft/temp.explicit#12):
>>>>>
>>>>> 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.
>>>
>>>
>>> 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.
>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180719/713264c9/attachment.html>
More information about the cfe-dev
mailing list