On 14/02/23 10:41, Bartosz Golaszewski wrote:
On Tue, 14 Feb 2023 at 14:33, Bartosz Golaszewski bartosz.golaszewski@linaro.org wrote:
On Tue, 14 Feb 2023 at 11:52, Adhemerval Zanella Netto adhemerval.zanella@linaro.org wrote:
On 13/02/23 19:04, Bartosz Golaszewski wrote:
On Mon, 13 Feb 2023 at 21:41, Adhemerval Zanella Netto adhemerval.zanella@linaro.org wrote:
On 13/02/23 17:25, Bartosz Golaszewski wrote:
On Mon, 13 Feb 2023 at 21:17, Adhemerval Zanella Netto adhemerval.zanella@linaro.org wrote: > > > > On 13/02/23 17:05, Bartosz Golaszewski wrote: >> On Mon, 13 Feb 2023 at 20:53, Adhemerval Zanella Netto >> adhemerval.zanella@linaro.org wrote: >>> >>> >>> >>> On 13/02/23 16:22, Bartosz Golaszewski wrote: >>>> On Mon, 13 Feb 2023 at 19:49, Adhemerval Zanella Netto >>>> adhemerval.zanella@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/... >>>>>> >>>>>> 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-b...
One thing I don't get is: what about this function: https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/cxx/...
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?
No, that's it. So if there's no __attribute__((visibility(x))) then a non-static symbol is *local*, but with __attribute__((visibility(x))), it's *hidden*?
Anyway, thanks for the explanation, I will remove the guard now.
Bart
No, that's it. So if there's no __attribute__((visibility(x))) then a non-static symbol is *local*, but with __attribute__((visibility(x))), it's *hidden*?
Without any visibility attribute, 'default' is set. The 'local' visibility will be set only for symbol local to the TU (static symbol for instance). You can override it for TU with the -fvisibility=<mode> (which will act on all TU), or by either #pragma or the visibility attribute.
I did the following:
diff --git a/bindings/cxx/gpiod.hpp b/bindings/cxx/gpiod.hpp index 8981db4..29d1877 100644 --- a/bindings/cxx/gpiod.hpp +++ b/bindings/cxx/gpiod.hpp @@ -26,7 +26,7 @@ #ifdef GPIOD_CXX_BUILD #define GPIOD_CXX_API __attribute__((visibility("default"))) #else -#define GPIOD_CXX_API __attribute__((visibility("hidden"))) +#define GPIOD_CXX_API #endif
#define __LIBGPIOD_GPIOD_CXX_INSIDE__
There is no need to use the 'default' (although making is explicit might more readable). What I would do would be similar to what gcc wiki suggests [1] (assuming just ELF):
--- #define DLL_PUBLIC __attribute__ ((visibility ("default"))) #define DLL_LOCAL __attribute__ ((visibility ("hidden")))
and then:
class DLL_PUBLIC ExternalABI { public: ExternalABI(void); // It inherits use the class visibility.
private: DLL_LOCAL void internalMethod(); };
class DLL_LOCAL InternalABI { public: InternalABI(void); }; ---
So calls to either ExternalABI::internalMethod() or InternalABI:: will use local references (no symbol interposition or PLT generation).
So there is no need to define GPIOD_CXX_BUILD (it is only required for COFF/WIN32 where you need to explicit set the linkage mode, dllexport or dllimport).
And now it works fine with clang. Am I right to assume this should work?
That is my understanding, yes. I might dig into because I think you hit a gcc bug, the __attribute__ ((visibility ("hidden"))) should affect the typeinfo as well.
Bart