Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gcc: always add libatomic to the image #9395

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mglae
Copy link
Contributor

@mglae mglae commented Oct 14, 2024

gcc is good in generating inline code for atomic operations. But at least in a few cases function calls to libatomic are generated.

The error can be surprising, see #9391

For Generic the lib is only ~30 KiBytes, size is no matter.

Fixes #9391, inpuststream.adaptive 22.1.6 build for Generic and runtime tested

gcc is good in generating inline code for atomic operations. But at least in a
few cases function calls to libatomic are generated.

For Generic the lib is only ~30 KiBytes, size is no matter.
@heitbaum
Copy link
Contributor

Probably prefer not to see this go in unless we really need it - I agree it is benign. But given that kodi itself does not use libatomic - xbmc/xbmc@1673f47 - should we?

Looks to be a Debian gcc-12.2.0 - issue.

Working with @CastagnaIT to see if we can update inputstream.adaptive to only include -latomic if it is present / versus forced. Immediate workaround is to patch out the forced -latomic.

@mglae
Copy link
Contributor Author

mglae commented Oct 15, 2024

Looks to be a Debian gcc-12.2.0 - issue.

I'm seeing it in LE13 and in my private test build in OpenSuSE 15.5 with gcc 10.x. After latest code changes in inputstream.adaptive the compiler is generating references to libatomic. Use objdump -T "so file"|grep __atomic

Immediate workaround is to patch out the forced -latomic.

This result in kodi crashing with kodi.sh[1011]: /usr/lib/kodi/kodi.bin: symbol lookup error: /storage/.kodi/addons/inputstream.adaptive/inputstream.adaptive.so.22.1.6: undefined symbol: __atomic_store_16

But given that kodi itself does not use libatomic [..] should we?

It is just luck that gcc currently does not generate libatomc references when compiling kodi source. That can change any time.


Another possibility is to link with static libatomic, this will only change the build system.

@CastagnaIT
Copy link

idk why only ISA addon require this,
its possible that the problem is that i use std::atomic with a struct std::atomic<myStruct>
https://github.com/xbmc/inputstream.adaptive/blob/0b8bdbb0004994cb584ed64420c177f4a70d07cf/src/CompResources.h#L90

instead on kodi sources there are uses only with base data types (e.g. bool, float...) with an expection on Windows platform code that however is not affected by this problem

if you have some better cmake change proposal for the ISA addon let me know
i can test build under ubuntu that complains of same problem
im run out of ideas since im not so expert of cmake things

@heitbaum
Copy link
Contributor

@mglae tested now, and yes you are right - those objects are referenced on the 22.1.6 build

$ objdump -T build.LibreELEC-Generic.x86_64-13.0-devel/install_pkg/inputstream.adaptive-22.1.6-Piers/usr/lib/kodi/addons/inputstream.adaptive/inputstream.adaptive.so.22.1.6|grep __at
omic
0000000000000000      DF *UND*  0000000000000000 (GLIBCXX_3.4.21) _ZNSt28__atomic_futex_unsigned_base19_M_futex_wait_untilEPjjbNSt6chrono8durationIlSt5ratioILl1ELl1EEEENS2_IlS3_ILl1ELl1000000000EEEE
0000000000000000      DF *UND*  0000000000000000 (GLIBCXX_3.4.21) _ZNSt28__atomic_futex_unsigned_base19_M_futex_notify_allEPj
0000000000000000      D  *UND*  0000000000000000              __atomic_load_16
0000000000000000      D  *UND*  0000000000000000              __atomic_store_16

whereas in 22.1.5 they are not.

$ objdump -T build.LibreELEC-Generic.x86_64-13.0-devel/install_pkg/inputstream.adaptive-22.1.5-Piers/usr/lib/kodi/addons/inputstream.adaptive/inputstream.adaptive.so.22.1.5|grep __at
omic
0000000000000000      DF *UND*  0000000000000000 (GLIBCXX_3.4.21) _ZNSt28__atomic_futex_unsigned_base19_M_futex_wait_untilEPjjbNSt6chrono8durationIlSt5ratioILl1ELl1EEEENS2_IlS3_ILl1ELl1000000000EEEE
0000000000000000      DF *UND*  0000000000000000 (GLIBCXX_3.4.21) _ZNSt28__atomic_futex_unsigned_base19_M_futex_notify_allEPj

@CastagnaIT - it looks like because of std::atomic<ScreenInfo> m_screenInfo; (using atomic on the struct) - this requires -latomic on all platforms (as the native gcc/CPU atomic does not handle atomic structs). I'm not knowledgeable enough to comment on the use of std::atomic on a struct.

@CastagnaIT
Copy link

good to know that it should be used with primitive types only
so i think the best is replace it with a separate mutex

@CastagnaIT
Copy link

CastagnaIT commented Oct 16, 2024

i opened a PR to remove in full the atomic code
xbmc/inputstream.adaptive#1701
i will made a test under ubuntu just to be sure
then i will make a new ISA release

@CastagnaIT
Copy link

new release done, i think you can close this pr

@mglae
Copy link
Contributor Author

mglae commented Oct 16, 2024

@CastagnaIT Thanks for your effort.


I'm still keeping the PR open, The major information of #9391 for me is that armv7 was working OOTB by just have included libatomic. Let all platforms behave equal. It is just an optimization that libatomic is not needed on X86_64 and aarch64 most of the time.

Furthermore "Backport Required" is removed to keep LE12.0.x images compatible to 12.0.0. We have to use static linking on X86_64 and aarch64 if libatomic is really required in the remaining support time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] inputstream.adaptive 22.1.6 fails build with libatomic
3 participants