On Tue, 14 Feb 2023 at 14:33, Bartosz Golaszewski
<bartosz.golaszew...@linaro.org> wrote:
>
> On Tue, 14 Feb 2023 at 11:52, Adhemerval Zanella Netto
> <adhemerval.zane...@linaro.org> wrote:
> >
> >
> >
> > 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?
>
> 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
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__
And now it works fine with clang. Am I right to assume this should work?
Bart
_______________________________________________
linaro-toolchain mailing list -- linaro-toolchain@lists.linaro.org
To unsubscribe send an email to linaro-toolchain-le...@lists.linaro.org
