Detect symver attribute support #32
Conversation
vfazio
force-pushed
the
vfazio-detect-symver
branch
from
231add5
to
18682b5
Compare
On Microblaze, builds will fail when trying to add symver information because __attribute__((symver ..)) is not supported even though __has_attribute(__symver__) returns true. Support for symver needs to be detected via a compile test since __has_attribute can report false positives [0]. Add a configure compile check for __attribute__((symver ..)) to ensure it is supported and define a variable to advertise support. [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101766#c1 Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Signed-off-by: Vincent Fazio <vfazio@gmail.com>
vfazio
force-pushed
the
vfazio-detect-symver
branch
from
18682b5
to
cb45891
Compare
|
rebased and pushed a whitespace change. |
|
@vfazio Thanks for the bug report and PR! I am not familiar with the Microblaze platform and I am surprised that Do you know if GCC on Microblaze supports symver? I am asking because if By the way, as a temporary workaround, you can configure with --disable-symbol-versions and the build should work. |
I'm not an expert on Microblaze at all, but using the asm ".symver" syntax seems to allow the compile to work fine since we've already ported this patch to buildroot for xz 5.2.10 Here's a failing build log http://autobuild.buildroot.org/results/4dc/4dc0c88c1ed250dd5e1be492138bd6e1781128b4/build-end.log it looks like the handling for I didn't see any build/link errors when switching but i suppose that doesn't mean it's working as intended. |
|
The linked GCC bug 101766 gives an impression that The It's confusing if GCC doesn't support If it is in ELF, what does this print? It should print three lines whose rightmost column looks like this: If there are no XZ Utils currently has two variants of symbol versioning: (1) A GNU/Linux-specific version with extra symbols for compatibility with a broken patch in RHEL/CentOS 7 which has also been copied to a few other places. The (2) A generic version that works on GNU/Linux (without RHEL/CentOS 7 symbols) and FreeBSD (possibly also Solaris but not sure, it's not enabled by default on Solaris). With this the above list only has It sounds very likely that the patch from RHEL/CentOS 7 (which was used somewhere else too) doesn't affect Microblaze users and thus (2) could be good if symbol versions are supported. The (2) method doesn't require anything in the C code, so no On the other hand, if symbol versioning isn't supported at all, then the default in configure.ac should be changed so that on Microblaze it's equivalent to The proposed patch has subtle problems: (1) Autoconf tests that require When writing this kind of tests, Clang's While not too important for this particular test, (2) The CMake build isn't updated so with this patch CMake-based build will never use the Anyway, I want to understand the issue better before worrying about patches. Once the problem is understood, a patch is probably fairly easy to write. |
The patch was largely based on how the check has been adapted other places: https://github.com/smuellerDD/libkcapi/blob/master/m4/ac_check_attribute_symver.m4 I'm open to doing this an alternative way if it's more appropriate
|
|
Again, i'm not totally convinced the gcc toolchain itself shouldn't be fixed to include https://github.com/gcc-mirror/gcc/blob/master/gcc/config.gcc#L2369 I do not see "elfos.h" in any of the microblaze targets. Note that our target is: |
On MicroBlaze, GCC 12 is broken in sense that
__has_attribute(__symver__) returns true but it still doesn't
support the __symver__ attribute even though the platform is ELF
and symbol versioning is supported if using the traditional
__asm__(".symver ...") method. Avoiding the traditional method is
good because it breaks LTO (-flto) builds with GCC.
See also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101766
For now the only extra symbols in liblzma_linux.map are the
compatibility symbols with the patch that spread from RHEL/CentOS 7.
These require the use of __symver__ attribute or __asm__(".symver ...")
in the C code. Compatibility with the patch from CentOS 7 doesn't
seem valuable on MicroBlaze so use liblzma_generic.map on MicroBlaze
instead. It doesn't require anything special in the C code and thus
no LTO issues either.
An alternative would be to detect support for __symver__
attribute in configure.ac and CMakeLists.txt and fall back
to __asm__(".symver ...") but then LTO would be silently broken
on MicroBlaze. It sounds likely that MicroBlaze is a special
case so let's treat it as a such because that is simpler. If
a similar issue exists on some other platform too then hopefully
someone will report it and this can be reconsidered.
(This doesn't do the same fix in CMakeLists.txt. Perhaps it should
but perhaps CMake build of liblzma doesn't matter much on MicroBlaze.
The problem breaks the build so it's easy to notice and can be fixed
later.)
Thanks to Vincent Fazio for reporting the problem and proposing
a patch (in the end that solution wasn't used):
#32
Thanks! So it's a normal ELF target that supports symbol versioning. It's just the There are two possible solutions:
I committed a fix using the second method. I didn't do it for CMake-based build but I guess building liblzma with CMake on MicroBlaze isn't so important.
My Meson skills are non-existent for now so I don't know if the method in libfuse is correct. If user-supplied The other two first declare the function and then define it so they work even with In case Clang some day happened to support the attribute then being future-compatible with A somewhat similar test is already used in XZ Utils for the Of course there are multiple slightly different ways to write a working test. One just has to be really careful that the test program will never give a warning about an unrelated thing in the test program which would make the test fail when it shouldn't. Perhaps
I have no idea, sorry. Thanks! |
|
I can try testing a build of your commit sans patch to see if it works sometime next week |
|
i did a quick build off of master via buildroot without applying our patch and tested via qemu-system-microblazeel. Things seem to work OK. While i think i personally prefer the compile time check, even if that means i need to tweak it to be more accurate, it's ultimately your call and i'm OK with closing this PR if quirking microblaze is the solution you're happy with. But if gcc gets fixed (assuming it's actually a gcc bug), that means microblaze is now an edge case different from other architectures. |
|
as a test, i patched gcc's and recompiled xz 5.2.10 without the patch: |
|
Fixing GCC would be the best but I guess the current GCC versions have to be supported for some time anyway. I have understood that MicroBlaze is for embedded use so I feel quite OK by making it a special case. The way symbol versioning is used in XZ Utils means that the downsides are very small: it sounds fairly unlikely that the issues caused by the patch from RHEL/CentOS 7 would affect MicroBlaze use cases. So the solution I committed is specific to XZ Utils and not trivially usable for other projects. Checking for features is obviously better most of the time (instead of checking for CPU/OS/whatever) so in general I don't disagree with you. In this case I feel the problem likely exists on just one platform and a generic test would be more complex than what is currently used on other platforms. If there is a bug in the test for the I plan to put the workaround in 5.4.2 and also 5.2.11 at the same time, whenever a new bugfix release will be made. Before that, it's safe to use the commit with both 5.2.10 and 5.4.1. If GCC is fixed this year, perhaps this workaround can be omitted 2-3 years later when a new major release of XZ Utils is made. Thanks for reporting the problem and testing! |
|
GCC discussion |
|
Thanks for reporting this and helping us fix @vfazio and reporting to gcc! I am closing this since the issue seems resolved with our workaround. Let us know if there are any other issues that you find :) |
On MicroBlaze, GCC 12 is broken in sense that
__has_attribute(__symver__) returns true but it still doesn't
support the __symver__ attribute even though the platform is ELF
and symbol versioning is supported if using the traditional
__asm__(".symver ...") method. Avoiding the traditional method is
good because it breaks LTO (-flto) builds with GCC.
See also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101766
For now the only extra symbols in liblzma_linux.map are the
compatibility symbols with the patch that spread from RHEL/CentOS 7.
These require the use of __symver__ attribute or __asm__(".symver ...")
in the C code. Compatibility with the patch from CentOS 7 doesn't
seem valuable on MicroBlaze so use liblzma_generic.map on MicroBlaze
instead. It doesn't require anything special in the C code and thus
no LTO issues either.
An alternative would be to detect support for __symver__
attribute in configure.ac and CMakeLists.txt and fall back
to __asm__(".symver ...") but then LTO would be silently broken
on MicroBlaze. It sounds likely that MicroBlaze is a special
case so let's treat it as a such because that is simpler. If
a similar issue exists on some other platform too then hopefully
someone will report it and this can be reconsidered.
(This doesn't do the same fix in CMakeLists.txt. Perhaps it should
but perhaps CMake build of liblzma doesn't matter much on MicroBlaze.
The problem breaks the build so it's easy to notice and can be fixed
later.)
Thanks to Vincent Fazio for reporting the problem and proposing
a patch (in the end that solution wasn't used):
#32
This commit changes the version branch from 5.2.x to 5.4.x. This old stable 5.2.x branch is expected to be end-of-life. The package site [1] mention: """ 5.4.3 was released on 2023-05-04. A minor bug fix release 5.2.12 to the old stable branch was made on 2023-05-04. This is probably the last release in the 5.2.x series. """ For a change log since 5.2.10, see [2]. This commit removes the package patch since the new version includes alternate workarounds. See comment in [3]. The COPYING licence file hash has changed. A note about Doxygen-generated HTML was added in [4]. COPYING.GPLv3 license file hash has also changed, as the file was updated (http links changed by https) in [5]. [1] https://tukaani.org/xz/ [2] https://git.tukaani.org/?p=xz.git;a=blob;f=NEWS;h=7f83c81f61e8e6aa81525e44c072c76205eeb14b;hb=238b4e5458b4bd2cadefb768b8ea7c6b70a191ac [3] tukaani-project/xz#32 (comment) [4] tukaani-project/xz@f68f4b2 [5] tukaani-project/xz@5a7b930 Signed-off-by: Julien Olivain <ju.o@free.fr> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
This commit changes the version branch from 5.2.x to 5.4.x. This old stable 5.2.x branch is expected to be end-of-life. The package site [1] mention: """ 5.4.3 was released on 2023-05-04. A minor bug fix release 5.2.12 to the old stable branch was made on 2023-05-04. This is probably the last release in the 5.2.x series. """ For a change log since 5.2.10, see [2]. This commit removes the package patch since the new version includes alternate workarounds. See comment in [3]. The COPYING licence file hash has changed. A note about Doxygen-generated HTML was added in [4]. COPYING.GPLv3 license file hash has also changed, as the file was updated (http links changed by https) in [5]. [1] https://tukaani.org/xz/ [2] https://git.tukaani.org/?p=xz.git;a=blob;f=NEWS;h=7f83c81f61e8e6aa81525e44c072c76205eeb14b;hb=238b4e5458b4bd2cadefb768b8ea7c6b70a191ac [3] tukaani-project/xz#32 (comment) [4] tukaani-project/xz@f68f4b2 [5] tukaani-project/xz@5a7b930 Signed-off-by: Julien Olivain <ju.o@free.fr> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
On Microblaze builds will fail when trying to add symver information because
__attribute__((symver ..))is not supported even though__has_attribute(__symver__)returns true.Support for symver needs to be detected via a compile test since
__has_attributecan report false positives [0].Add a configure compile check for
__attribute__((symver ..))to ensure it is supported and define a variable to advertise support.[0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101766#c1
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
builds targeting the microblaze fail
Related Issue URL:
What is the new behavior?
Does this introduce a breaking change?
Other information
I tested compiles via GCC 12 and 9 for x86_64 and microblaze targets and didn't encounter issues.