libxmp fails to build with gcc link-time optimization (LTO) enabled #385
Comments
|
Googling points to some hints at alsa-project/alsa-lib#6
|
|
Tried lto on my old fedora-27 setup with binutils-2.29-13.fc27.x86_64,
I tried adding |
|
I also found this Fedora developers mailing list post, which suggests diff -up libxmp-4.5.0/include/xmp.h.lto libxmp-4.5.0/include/xmp.h
--- libxmp-4.5.0/include/xmp.h.lto 2021-06-10 02:24:24.000000000 +0200
+++ libxmp-4.5.0/include/xmp.h 2021-06-22 21:34:03.394649770 +0200
@@ -22,7 +22,7 @@ extern "C" {
#elif defined(__OS2__) && defined(__WATCOMC__) && defined(__SW_BD)
# define LIBXMP_EXPORT __declspec(dllexport)
#elif (defined(__GNUC__) || defined(__clang__) || defined(__HP_cc)) && defined(XMP_SYM_VISIBILITY)
-# define LIBXMP_EXPORT __attribute__((visibility ("default")))
+# define LIBXMP_EXPORT __attribute__((externally_visible))
#elif defined(__SUNPRO_C) && defined(XMP_LDSCOPE_GLOBAL)
# define LIBXMP_EXPORT __global
#elif defined(EMSCRIPTEN)fixes the issue for me on Fedora 34 (gcc-10.3.1). |
|
Hm, but the test suite fails to link: |
|
The problem with (It doesn't fix the export issue I'm having on fc27 with fsf gcc8/9 though, |
|
@sezero Unfortunately, with your patch it's failing as in the original report: |
|
After compiling with |
|
Does configure report that externally_visible is supported? (Note that my patch applies externally_visible only to versioned syms in control.c) Updated the patch btw (there was a minor -Werror issue) |
|
Scratch that. I haven't run |
|
That's great! To confirm: With the patch applied:
Yes? |
|
Correct. There are no backwards-incompatible ABI changes, so I simply updated the existing libxmp on my system and xmp still works fine. |
this seems to help with lto-enabled builds, see issue reported by fedora people at: #385. also tweak the visibility attribute checks in configure.
|
OK, pushed the patch as a091de5 Keeping this open so that @AliceLR, @cmatsuoka and others can test and @rathann: if you can try building xmp-cli (from git), that'd be great. |
|
I wonder if declaring versioned symbols using the gcc 10 symver attribute instead of asm could also work. Something like: and removing the asm lines. |
|
I don't have a gcc10 build to test that. (Even with my patch applied, the export/link issues I experienced (see #385 (comment)) hasn't actually gone, I'm guessing this is somehow a gcc issue.) |
|
From what I understood LTO doesn't play well with asm-based versioned symbols, but attribute |
|
@sezero I've just tested building xmp from git, it builds against patched libxmp and works just fine, too. |
Great, thank you for testing and feedback! |
|
@rathann I'm just curious, does cmatsuoka@3d488c0 work too, keeping ABI compatibility with previous versions? |
We need a configury check for it: If it is found, we define
The |
But is Fedora using LTO by default with gcc < 10 as well? If so, we'll need to check how other projects are handling it. |
Possibly. @rathann? As for compiler support: Does clang support this new attribute? |
|
Fedora enabled LTO by default in release 33, which shipped with gcc-10.2.1 (and was updated to 10.3.1 subsequently), so the answer is no, it's used with gcc-10 or later only by default. I'll test @cmatsuoka 's patch later. |
|
I've confirmed that both patches fix the issue in my Fedora 34 VM (gcc 11.1.1). I don't know enough about this particular problem to have any other useful feedback. |
|
@cmatsuoka, @rathann, @AliceLR: Is the patch below correct and works? |
|
This patch uses |
Eww, copy/paste srew-up, sorry. Fixed one (hopefully) below: |
More eww.. Fixed one attached and inlined below: symver_patch.txt diff --git a/src/common.h b/src/common.h
index 9b88f43..7998988 100644
--- a/src/common.h
+++ b/src/common.h
@@ -29,6 +29,11 @@
#else
#define LIBXMP_EXPORT_VERSIONED __attribute__((visibility("default")))
#endif
+#if defined(__GNUC__) && (__GNUC__ >= 10)
+#define LIBXMP_ATTRIB_SYMVER(_sym) __attribute__((__symver__(_sym)))
+#else
+#define LIBXMP_ATTRIB_SYMVER(_sym)
+#endif /* gcc >= 10 */
#endif
#endif
diff --git a/src/control.c b/src/control.c
index 9644374..ff581aa 100644
--- a/src/control.c
+++ b/src/control.c
@@ -310,18 +310,20 @@ int xmp_channel_vol(xmp_context opaque, int chn, int vol)
}
#ifdef USE_VERSIONED_SYMBOLS
-LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v40__(xmp_context, int, int);
+LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v40__(xmp_context, int, int) LIBXMP_ATTRIB_SYMVER("xmp_set_player@XMP_4.0");
LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v41__(xmp_context, int, int)
- __attribute__((alias("xmp_set_player_v40__")));
+ __attribute__((alias("xmp_set_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_set_player@XMP_4.1");
LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v43__(xmp_context, int, int)
- __attribute__((alias("xmp_set_player_v40__")));
+ __attribute__((alias("xmp_set_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_set_player@XMP_4.3");
LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v44__(xmp_context, int, int)
- __attribute__((alias("xmp_set_player_v40__")));
+ __attribute__((alias("xmp_set_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_set_player@@XMP_4.4");
+#if !defined(__GNUC__) || (__GNUC__ < 10)
asm(".symver xmp_set_player_v40__, xmp_set_player@XMP_4.0");
asm(".symver xmp_set_player_v41__, xmp_set_player@XMP_4.1");
asm(".symver xmp_set_player_v43__, xmp_set_player@XMP_4.3");
asm(".symver xmp_set_player_v44__, xmp_set_player@@XMP_4.4");
+#endif
#define xmp_set_player__ xmp_set_player_v40__
#else
@@ -428,21 +430,23 @@ int xmp_set_player__(xmp_context opaque, int parm, int val)
}
#ifdef USE_VERSIONED_SYMBOLS
-LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v40__(xmp_context, int);
+LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v40__(xmp_context, int) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.0");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v41__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.1");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v42__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.2");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v43__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.3");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v44__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@@XMP_4.4");
+#if !defined(__GNUC__) || (__GNUC__ < 10)
asm(".symver xmp_get_player_v40__, xmp_get_player@XMP_4.0");
asm(".symver xmp_get_player_v41__, xmp_get_player@XMP_4.1");
asm(".symver xmp_get_player_v42__, xmp_get_player@XMP_4.2");
asm(".symver xmp_get_player_v43__, xmp_get_player@XMP_4.3");
asm(".symver xmp_get_player_v44__, xmp_get_player@@XMP_4.4");
+#endif
#define xmp_get_player__ xmp_get_player_v40__
#else |
|
Perhaps we could write a macro that can be used to declare versioned symbols and switch between asm or attribute in the macro definition. I'll try it and propose a change if it looks good enough. |
It didn't, @sezero's solution is more verbose but less ugly/magic. The only thing I'd change is to test for availablliity of attribute |
|
I don't have a setup with gcc10+ so I don't have a configure.ac solution. |
This should do the trick: cmatsuoka@cc4e53d (From a quick glance at the current |
The attribute is new, so
Have we actually ever tested that compiler along with its visibility support? |
|
OK, here is a patch incorporating the autotools solution of @cmatsuoka, diff --git a/configure.ac b/configure.ac
index 909cc96..6c4639e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -139,12 +139,18 @@ dnl https://github.com/libxmp/libxmp/issues/385
if test $ac_cv_c_flag_w_all = yes; then
CFLAGS="${CFLAGS} -Werror"
fi
- XMP_TRY_COMPILE(whether compiler understands externally_visible attribute,
+ XMP_TRY_COMPILE(whether compiler understands attribute externally_visible,
ac_cv_c_attribute_visibility_external,[],[
__attribute__((visibility("default"),externally_visible)) int foo(void);
int foo(void) { return 0; }],
[CFLAGS="${old_CFLAGS} -DHAVE_EXTERNAL_VISIBILITY"],
[CFLAGS="${old_CFLAGS}"])
+
+ XMP_TRY_COMPILE(whether compiler understands attribute __symver__,
+ ac_cv_c_attribute_symver,[-Werror=attributes],[
+void foo(void) __attribute__((__symver__("foo@bar")));],
+ [CFLAGS="${CFLAGS} -DHAVE_ATTRIBUTE_SYMVER"],
+ [CFLAGS="${CFLAGS}"])
fi
dnl for clang
diff --git a/lite/configure.ac b/lite/configure.ac
index e094b42..501b66b 100644
--- a/lite/configure.ac
+++ b/lite/configure.ac
@@ -138,12 +138,18 @@ dnl https://github.com/libxmp/libxmp/issues/385
if test $ac_cv_c_flag_w_all = yes; then
CFLAGS="${CFLAGS} -Werror"
fi
- XMP_TRY_COMPILE(whether compiler understands externally_visible attribute,
+ XMP_TRY_COMPILE(whether compiler understands attribute externally_visible,
ac_cv_c_attribute_visibility_external,[],[
__attribute__((visibility("default"),externally_visible)) int foo(void);
int foo(void) { return 0; }],
[CFLAGS="${old_CFLAGS} -DHAVE_EXTERNAL_VISIBILITY"],
[CFLAGS="${old_CFLAGS}"])
+
+ XMP_TRY_COMPILE(whether compiler understands attribute __symver__,
+ ac_cv_c_attribute_symver,[-Werror=attributes],[
+void foo(void) __attribute__((__symver__("foo@bar")));],
+ [CFLAGS="${CFLAGS} -DHAVE_ATTRIBUTE_SYMVER"],
+ [CFLAGS="${CFLAGS}"])
fi
dnl for clang
diff --git a/src/common.h b/src/common.h
index 9b88f43..a18bf0d 100644
--- a/src/common.h
+++ b/src/common.h
@@ -29,6 +29,11 @@
#else
#define LIBXMP_EXPORT_VERSIONED __attribute__((visibility("default")))
#endif
+#ifdef HAVE_ATTRIBUTE_SYMVER
+#define LIBXMP_ATTRIB_SYMVER(_sym) __attribute__((__symver__(_sym)))
+#else
+#define LIBXMP_ATTRIB_SYMVER(_sym)
+#endif
#endif
#endif
diff --git a/src/control.c b/src/control.c
index 9644374..7cb913e 100644
--- a/src/control.c
+++ b/src/control.c
@@ -310,18 +310,20 @@ int xmp_channel_vol(xmp_context opaque, int chn, int vol)
}
#ifdef USE_VERSIONED_SYMBOLS
-LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v40__(xmp_context, int, int);
+LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v40__(xmp_context, int, int) LIBXMP_ATTRIB_SYMVER("xmp_set_player@XMP_4.0");
LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v41__(xmp_context, int, int)
- __attribute__((alias("xmp_set_player_v40__")));
+ __attribute__((alias("xmp_set_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_set_player@XMP_4.1");
LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v43__(xmp_context, int, int)
- __attribute__((alias("xmp_set_player_v40__")));
+ __attribute__((alias("xmp_set_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_set_player@XMP_4.3");
LIBXMP_EXPORT_VERSIONED extern int xmp_set_player_v44__(xmp_context, int, int)
- __attribute__((alias("xmp_set_player_v40__")));
+ __attribute__((alias("xmp_set_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_set_player@@XMP_4.4");
+#ifndef HAVE_ATTRIBUTE_SYMVER
asm(".symver xmp_set_player_v40__, xmp_set_player@XMP_4.0");
asm(".symver xmp_set_player_v41__, xmp_set_player@XMP_4.1");
asm(".symver xmp_set_player_v43__, xmp_set_player@XMP_4.3");
asm(".symver xmp_set_player_v44__, xmp_set_player@@XMP_4.4");
+#endif
#define xmp_set_player__ xmp_set_player_v40__
#else
@@ -428,21 +430,23 @@ int xmp_set_player__(xmp_context opaque, int parm, int val)
}
#ifdef USE_VERSIONED_SYMBOLS
-LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v40__(xmp_context, int);
+LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v40__(xmp_context, int) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.0");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v41__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.1");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v42__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.2");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v43__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@XMP_4.3");
LIBXMP_EXPORT_VERSIONED extern int xmp_get_player_v44__(xmp_context, int)
- __attribute__((alias("xmp_get_player_v40__")));
+ __attribute__((alias("xmp_get_player_v40__"))) LIBXMP_ATTRIB_SYMVER("xmp_get_player@@XMP_4.4");
+#ifndef HAVE_ATTRIBUTE_SYMVER
asm(".symver xmp_get_player_v40__, xmp_get_player@XMP_4.0");
asm(".symver xmp_get_player_v41__, xmp_get_player@XMP_4.1");
asm(".symver xmp_get_player_v42__, xmp_get_player@XMP_4.2");
asm(".symver xmp_get_player_v43__, xmp_get_player@XMP_4.3");
asm(".symver xmp_get_player_v44__, xmp_get_player@@XMP_4.4");
+#endif
#define xmp_get_player__ xmp_get_player_v40__
#else |
|
Could you open a PR with that so we can also run it through the regression tests? In the meantime I'll set up a dev environment on a Fedora 34 VM. |
e.g. with gcc >= 10. Co-authored-by: Claudio Matsuoka<notifications@github.com> Closes: libxmp#385
Done: #391
Is Travis fixed? |
I think we should drop Travis and move windows and mac CI tests to github actions, wdyt? |
Whatever works - so long that our unit tests are run |
|
Ok, I'll check how to port existing tests from travis to gh actions. |
|
In the meantime, we should fix the travis url so that existing PRs are run |
e.g. with gcc >= 10. Co-authored-by: Claudio Matsuoka <cmatsuoka@gmail.com> Closes: libxmp#385
I just tried changing |
That sounds OK to me. edit: added actions for the sanitizers here: #392. |
e.g. with gcc >= 10. Co-authored-by: Claudio Matsuoka <cmatsuoka@gmail.com> Closes: libxmp#385
e.g. with gcc >= 10. Co-authored-by: Claudio Matsuoka <cmatsuoka@gmail.com> Closes: libxmp#385
e.g. with gcc >= 10. Co-authored-by: Claudio Matsuoka <cmatsuoka@gmail.com> Closes: libxmp#385
e.g. with gcc >= 10. Co-authored-by: Claudio Matsuoka <cmatsuoka@gmail.com> Closes: libxmp#385
When building on Fedora (33 and later) libxmp 4.4.1 and 4.5.0 fail to link if LTO was enabled (
CFLAGS+=-flto=auto -ffat-lto-objects):This is with gcc-10.3.1.
The text was updated successfully, but these errors were encountered: