On 13/02/23 19:04, Bartosz Golaszewski wrote:
> On Mon, 13 Feb 2023 at 21:41, Adhemerval Zanella Netto
> <adhemerval.zane...@linaro.org> wrote:
>>
>>
>>
>> On 13/02/23 17:25, Bartosz Golaszewski wrote:
>>> On Mon, 13 Feb 2023 at 21:17, Adhemerval Zanella Netto
>>> <adhemerval.zane...@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 13/02/23 17:05, Bartosz Golaszewski wrote:
>>>>> On Mon, 13 Feb 2023 at 20:53, Adhemerval Zanella Netto
>>>>> <adhemerval.zane...@linaro.org> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 13/02/23 16:22, Bartosz Golaszewski wrote:
>>>>>>> On Mon, 13 Feb 2023 at 19:49, Adhemerval Zanella Netto
>>>>>>> <adhemerval.zane...@linaro.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 13/02/23 12:49, Bartosz Golaszewski wrote:
>>>>>>>>> Hey!
>>>>>>>>>
>>>>>>>>> I'm the author and maintainer of libgpiod. I'm currently getting ready
>>>>>>>>> to do a new major release. After giving some exposure to the release
>>>>>>>>> candidate, I noticed that when using clang, I can't link against the
>>>>>>>>> C++ bindings, while it works just fine in GCC.
>>>>>>>>>
>>>>>>>>> The tree in question is here:
>>>>>>>>> https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/log/
>>>>>>>>>
>>>>>>>>> You can trigger the linking program by trying to build the C++ tests
>>>>>>>>> with clang like that:
>>>>>>>>>
>>>>>>>>> CC=clang CXX=clang++ ./autogen.sh --enable-bindings-cxx --enable-tests
>>>>>>>>> && make -j16
>>>>>>>>>
>>>>>>>>> You'll get the following error:
>>>>>>>>>
>>>>>>>>> /usr/bin/ld: tests-chip.o:(.data+0x0): undefined reference to
>>>>>>>>> `typeinfo for gpiod::chip_closed'
>>>>>>>>> /usr/bin/ld: tests-line-request.o:(.data+0x0): undefined reference to
>>>>>>>>> `typeinfo for gpiod::request_released'
>>>>>>>>> /usr/bin/ld: .libs/gpiod-cxx-test: hidden symbol
>>>>>>>>> `_ZTIN5gpiod11chip_closedE' isn't defined
>>>>>>>>> /usr/bin/ld: final link failed: bad value
>>>>>>>>>
>>>>>>>>> The typoinfo is missing for exception types that should be visible to
>>>>>>>>> users of the library.
>>>>>>>>>
>>>>>>>>> The culprit is here:
>>>>>>>>> https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/cxx/gpiod.hpp#n26
>>>>>>>>>
>>>>>>>>> I added the GPIOD_CXX_BUILD macro in order to not re-export the
>>>>>>>>> visible symbols if any user of the library would include the gpiod.hpp
>>>>>>>>> header. When the library is being built, the symbols are visible, when
>>>>>>>>> someone includes the header, the symbols are hidden.
>>>>>>>>
>>>>>>>> But the typeid of the symbol, for instance gpiod, will still be
>>>>>>>> provided
>>>>>>>> by shared library if I understood correctly:
>>>>>>>>
>>>>>>>> libgpiod-llvm$ objdump -t ./bindings/cxx/tests/tests-chip.o
>>>>>>>> 2>/dev/null| grep -w _ZTIN5gpiod11chip_closedE
>>>>>>>> 0000000000000000 *UND* 0000000000000000 .hidden
>>>>>>>> _ZTIN5gpiod11chip_closedE
>>>>>>>> libgpiod-llvm$ objdump -t bindings/cxx/.libs/libgpiodcxx.so
>>>>>>>> 2>/dev/null| grep _ZTIN5gpiod11chip_closedE
>>>>>>>> 0000000000024b50 g O .data.rel.ro 0000000000000018
>>>>>>>> _ZTIN5gpiod11chip_closedE
>>>>>>>>
>>>>>>>> However, it seems that GCC is not applying the hidden attribute on the
>>>>>>>> typeid class:
>>>>>>>>
>>>>>>>> libgpiod-gcc$ objdump -t ./bindings/cxx/tests/tests-chip.o
>>>>>>>> 2>/dev/null| grep -w _ZTIN5gpiod11chip_closedE
>>>>>>>> 0000000000000000 w O
>>>>>>>> .data.rel.local.DW.ref._ZTIN5gpiod11chip_closedE
>>>>>>>> 0000000000000008 .hidden DW.ref._ZTIN5gpiod11chip_closedE
>>>>>>>> 0000000000000000 *UND* 0000000000000000
>>>>>>>> _ZTIN5gpiod11chip_closedE
>>>>>>>>
>>>>>>>> When it creates create the vague linking weak symbol
>>>>>>>> .data.rel.local.DW.ref._ZTIN5gpiod11chip_closedE.
>>>>>>>>
>>>>>>>> I am not sure why GCC is being permissive here, in fact IMHO this is
>>>>>>>> gcc issue. If I add the visibility explicitly using pragmas:
>>>>>>>>
>>>>>>>> diff --git a/bindings/cxx/gpiodcxx/exception.hpp
>>>>>>>> b/bindings/cxx/gpiodcxx/exception.hpp
>>>>>>>> index 98b7bc4..24ae698 100644
>>>>>>>> --- a/bindings/cxx/gpiodcxx/exception.hpp
>>>>>>>> +++ b/bindings/cxx/gpiodcxx/exception.hpp
>>>>>>>> @@ -17,6 +17,8 @@
>>>>>>>>
>>>>>>>> namespace gpiod {
>>>>>>>>
>>>>>>>> +#pragma GCC visibility push(hidden)
>>>>>>>> +
>>>>>>>> /**
>>>>>>>> * @ingroup gpiod_cxx
>>>>>>>> * @{
>>>>>>>> @@ -25,7 +27,7 @@ namespace gpiod {
>>>>>>>> /**
>>>>>>>> * @brief Exception thrown when an already closed chip is used.
>>>>>>>> */
>>>>>>>> -class GPIOD_CXX_API chip_closed : public ::std::logic_error
>>>>>>>> +class /*GPIOD_CXX_API*/ chip_closed : public ::std::logic_error
>>>>>>>> {
>>>>>>>> public:
>>>>>>>>
>>>>>>>> @@ -64,6 +66,8 @@ public:
>>>>>>>> virtual ~chip_closed();
>>>>>>>> };
>>>>>>>>
>>>>>>>> +#pragma GCC visibility pop
>>>>>>>> +
>>>>>>>> /**
>>>>>>>> * @brief Exception thrown when an already released line request is
>>>>>>>> used.
>>>>>>>> */
>>>>>>>>
>>>>>>>> I get an explicit linking error:
>>>>>>>>
>>>>>>>> /usr/bin/ld:
>>>>>>>> tests-chip.o:(.data.rel.local.DW.ref._ZTIN5gpiod11chip_closedE[DW.ref._ZTIN5gpiod11chip_closedE]+0x0):
>>>>>>>> undefined reference to `typeinfo for gpiod::chip_closed'
>>>>>>>>
>>>>>>>> Which is what I expect. So I suggest you to avoid adding the hidden
>>>>>>>> visibility on tests because since there are not linking static, they
>>>>>>>> should follow the default rules of ABI and hidden in this case does
>>>>>>>> not really make much sense.
>>>>>>>>
>>>>>>>
>>>>>>> I'm not sure I understand this. The tests are linked dynamically -
>>>>>>> just like any other program would. IIUC: I build libgpiodcxx making
>>>>>>> the exception symbols visible, then if anyone else (a program linking
>>>>>>> against libgpiodcxx) includes the header, the symbol is hidden.
>>>>>>
>>>>>> Hidden symbols linkage only works for TU within the same ELF object,
>>>>>> for instance within a shared library or a binary itself. They are used,
>>>>>> besides to trim the dynamic section, to also avoid symbol interposition
>>>>>> through PLT.
>>>>>>
>>>>>> So using hidden visibility for libgpiodcxx only makes sense if you
>>>>>> other TU reference the exported ABI (for instance if another class
>>>>>> from libgpiodcxx uses the chip_closed class). It does not make sense
>>>>>> for external ELF objects to use visibility hidden for public ABIs because
>>>>>> by ELF rules such symbols are not visible at static linking time.
>>>>>>
>>>>>
>>>>> Yes, I get that. Isn't this what I'm doing here though (and possibly
>>>>> failing)? When building libgpiodcxx, I pass -DGPIOD_CXX_BUILD to the
>>>>> compiler.
>>>>>
>>>>> This makes GPIOD_CXX_API become __attribute__((visibility("default")))
>>>>> and the resulting shared object has:
>>>>>
>>>>> $ nm bindings/cxx/.libs/libgpiodcxx.so | grep chip_closed
>>>>> 000000000000f770 T _ZN5gpiod11chip_closedaSEOS0_
>>>>> 000000000000f760 T _ZN5gpiod11chip_closedaSERKS0_
>>>>> 000000000000f740 T _ZN5gpiod11chip_closedC1EOS0_
>>>>> 000000000000f700 T
>>>>> _ZN5gpiod11chip_closedC1ERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
>>>>> 000000000000f720 T _ZN5gpiod11chip_closedC1ERKS0_
>>>>> 000000000000f740 T _ZN5gpiod11chip_closedC2EOS0_
>>>>> 000000000000f700 T
>>>>> _ZN5gpiod11chip_closedC2ERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
>>>>> 000000000000f720 T _ZN5gpiod11chip_closedC2ERKS0_
>>>>> 000000000000f790 T _ZN5gpiod11chip_closedD0Ev
>>>>> 000000000000f780 T _ZN5gpiod11chip_closedD1Ev
>>>>> 000000000000f780 T _ZN5gpiod11chip_closedD2Ev
>>>>> 0000000000024b38 D _ZTIN5gpiod11chip_closedE
>>>>> 000000000001b5d0 R _ZTSN5gpiod11chip_closedE
>>>>> 0000000000024ac0 D _ZTVN5gpiod11chip_closedE
>>>>>
>>>>> Specifically:
>>>>>
>>>>> $ nm bindings/cxx/.libs/libgpiodcxx.so | grep _ZTIN5gpiod11chip_closedE
>>>>> 0000000000024b38 D _ZTIN5gpiod11chip_closedE
>>>>>
>>>>> Looks good for linking. Now if someone includes the exception.hpp
>>>>> header, GPIOD_CXX_API will become
>>>>> __attribute__((visibility("hidden"))) but that shouldn't matter now
>>>>> for the program that's being built because libgpiocxx has this symbol
>>>>> and IT IS visible?
>>>>>
>>>>> I admit, I'm not an expert on this but I really can't see what I'm
>>>>> missing here.
>>>>
>>>> That's I am trying to explain in the first email, where the test-chip.o
>>>> object
>>>> is binding with the typeinfo as hidden:
>>>>
>>>> $ objdump -t ./bindings/cxx/tests/tests-chip.o 2>/dev/null| grep -w
>>>> _ZTIN5gpiod11chip_closedE
>>>> 0000000000000000 *UND* 0000000000000000 *.hidden*
>>>> _ZTIN5gpiod11chip_closedE
>>>>
>>>> The libgpiodcxx.so is correct, it exports the symbol visibility as
>>>> expected.
>>>> But, you also need to setup the correct symbol visibility on ABI headers
>>>> definition when building the tests. C/C++ visibility is not inferred from
>>>> the input objects, but rather from the TU itself; meaning that if you
>>>> define
>>>> hidden visibility for the tests, it will try to bind against hidden symbol
>>>> and not the global ones from the library.
>>>
>>> OH now I get it! tests-chip.o is the TU here and the final link fails. I
>>> see.
>>
>> Yeap, I might not have being clear about that (sorry for the confusion).
>>
>>>
>>> So is my whole re-export guard just pointless? Don't libraries do that
>>> at all? And so if some other library includes this header in its
>>> header it will be visible in this library too? What is the best
>>> practice here?
>>
>> For ELF afaik there is no much you can do for exported symbols, the best
>> practices are only to export that you really need and use namespace to avoid
>> symbol names clashes and ODR violations. Another option is to use static
>> linking, so you can always hidden visibility everywhere (and it still makes
>> a difference for PIE binaries, since you need to build for PIC).
>>
>> But libraries do use the hidden visibility to optimize internal calls (it
>> does show a lot of improvement and plays better with LTO). For instance,
>> CPython 3.10 used the -fno-semantic-interposition [1] to get a really nice
>> improvement.
>>
>> [1]
>> https://developers.redhat.com/blog/2020/06/25/red-hat-enterprise-linux-8-2-brings-faster-python-3-8-run-speeds
>>
>
> One thing I don't get is: what about this function:
> https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/cxx/internal.cpp#n12
>
> which clearly is visible outside its translation unit despite the
> default passed to the compiler being -fvisibility=hidden?
>
> Bart
On both clang and llvm the symbol is marked as local:
$ objdump -t bindings/cxx/.libs/libgpiodcxx.so 2>/dev/null| grep
throw_from_errno
0000000000010570 l F .text 0000000000000139
_ZN5gpiod16throw_from_errnoERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
c++filt
_ZN5gpiod16throw_from_errnoERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
gpiod::throw_from_errno(std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&)
Which won't be available to link. Are you seeing otherwise?
_______________________________________________
linaro-toolchain mailing list -- linaro-toolchain@lists.linaro.org
To unsubscribe send an email to linaro-toolchain-le...@lists.linaro.org
