| Message ID | 3cc24b4a-a4f4-ea25-501c-46c02ef68cd4@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Oct 26, 2016 at 01:16:13AM +0200, Andreas Cadhalpun wrote: > On 25.10.2016 23:34, Carl Eugen Hoyos wrote: > > 2016-10-25 19:19 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > > > >> + # LTO could optimize out the test functions without this > >> + echo "#if defined(__GNUC__) && __GNUC__ >= 4" > >> + echo " #define USED __attribute__((used))" > >> + echo "#else" > >> + echo " #define USED" > >> + echo "#endif" > > > > Why is the ugly #if - #else - #endif necessary? > > I'm under the impression that __attribute__((used)) is not available for all compilers, > see e.g. libavutil/attributes.h. > But thinking more about it, a better way is to actually use the functions so that > LTO can't optimize them out. This avoids the ugly preprocessor checks. > > > Please mention ticket #5909 if it is related. > > Sure, new patch attached. > > By the way, this is not a regression in 3.1.5, but a fix included in that > release allowed them to drop their distro-specific patch, exposing the problem > with LTO. > > Best regards, > Andreas > > configure | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > 742684cf379693d08075d43fdfb75ed5e2e936c6 0001-configure-make-sure-LTO-does-not-optimize-out-the-te.patch > From bb289a0b2b0948afa99227bcff188301c1143624 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > Date: Tue, 25 Oct 2016 19:09:46 +0200 > Subject: [PATCH] configure: make sure LTO does not optimize out the test > functions > > Fixes trac ticket #5909 > > Bud-Id: https://bugs.gentoo.org/show_bug.cgi?id=598054 > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > configure | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/configure b/configure > index 481f692..54faef1 100755 > --- a/configure > +++ b/configure > @@ -1150,7 +1150,12 @@ check_func_headers(){ > for func in $funcs; do > echo "long check_$func(void) { return (long) $func; }" > done > - echo "int main(void) { return 0; }" > + echo "int main(void) { int ret = 0;" > + # LTO could optimize out the test functions without this > + for func in $funcs; do > + echo " ret |= ((intptr_t)check_$func) & 0xFFFF;" > + done > + echo "return ret; }" breaks configure i get this: ERROR: LoadLibrary/dlopen not found for avisynth If you think configure made a mistake, make sure you are using the latest version from Git. If the latest version fails, report the problem to the ffmpeg-user@ffmpeg.org mailing list or IRC #ffmpeg on irc.freenode.net. Include the log file "config.log" produced by configure as this will help solve the problem. [...]
2016-10-26 1:16 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > On 25.10.2016 23:34, Carl Eugen Hoyos wrote: >> 2016-10-25 19:19 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> >>> + # LTO could optimize out the test functions without this >>> + echo "#if defined(__GNUC__) && __GNUC__ >= 4" >>> + echo " #define USED __attribute__((used))" >>> + echo "#else" >>> + echo " #define USED" >>> + echo "#endif" >> >> Why is the ugly #if - #else - #endif necessary? > > I'm under the impression that __attribute__((used)) is not available > for all compilers, Yes, but __attribute__((foo_bar)) does not break compilation here. > see e.g. libavutil/attributes.h. This is not a good reference, you cannot test - for example - icc like this. > But thinking more about it, a better way is to actually use the functions so that > LTO can't optimize them out. This avoids the ugly preprocessor checks. Imo, they were only ugly as long as they were enclosed in #if - #endif ;-) >> Please mention ticket #5909 if it is related. > > Sure, new patch attached. > > By the way, this is not a regression in 3.1.5, but a fix included in > that release allowed them to drop their distro-specific patch, > exposing the problem with LTO. Yes, they are adding a few unneeded flags that make FFmpeg slower and then they add lto to make compilation slower... (But it is still a regression: 3.1 should not have seen the patch, it doesn't support new incompatible external libraries, and the issue was neither a regression nor security-related.) Carl Eugen
On 26.10.2016 10:52, Carl Eugen Hoyos wrote: > 2016-10-26 1:16 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> I'm under the impression that __attribute__((used)) is not available >> for all compilers, > Yes, but __attribute__((foo_bar)) does not break compilation here. Have you tested with MSVC? > (But it is still a regression: 3.1 should not have seen the patch, > it doesn't support new incompatible external libraries, and the > issue was neither a regression nor security-related.) This was not about a new external library, the support was added in 2015 (commit 99eabcd), and I think fixing build failures is quite sensible for release branches. On 26.10.2016 10:55, Carl Eugen Hoyos wrote: > 2016-10-26 1:35 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > >> I forgot to include stdint.h. Fixed patch attached. > > Why don't you cast to (int)? Because gcc doesn't like it: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] Best regards, Andreas
2016-10-26 21:36 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > On 26.10.2016 10:52, Carl Eugen Hoyos wrote: >> 2016-10-26 1:16 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >>> I'm under the impression that __attribute__((used)) is not available >>> for all compilers, > >> Yes, but __attribute__((foo_bar)) does not break compilation here. > > Have you tested with MSVC? No. >> (But it is still a regression: 3.1 should not have seen the patch, >> it doesn't support new incompatible external libraries, and the >> issue was neither a regression nor security-related.) > > This was not about a new external library, the support was added > in 2015 (commit 99eabcd), and I think fixing build failures is quite > sensible for release branches. Sorry for bringing this up. > On 26.10.2016 10:55, Carl Eugen Hoyos wrote: >> 2016-10-26 1:35 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> >>> I forgot to include stdint.h. Fixed patch attached. >> >> Why don't you cast to (int)? > > Because gcc doesn't like it: > warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] I don't see the problem with this warning but I guess you could use long, no? Carl Eugen
On Wed, Oct 26, 2016 at 10:57 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-10-26 21:36 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> On 26.10.2016 10:52, Carl Eugen Hoyos wrote: >>> 2016-10-26 1:16 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >>>> I'm under the impression that __attribute__((used)) is not available >>>> for all compilers, >> >>> Yes, but __attribute__((foo_bar)) does not break compilation here. >> >> Have you tested with MSVC? > > No. > >>> (But it is still a regression: 3.1 should not have seen the patch, >>> it doesn't support new incompatible external libraries, and the >>> issue was neither a regression nor security-related.) >> >> This was not about a new external library, the support was added >> in 2015 (commit 99eabcd), and I think fixing build failures is quite >> sensible for release branches. > > Sorry for bringing this up. > >> On 26.10.2016 10:55, Carl Eugen Hoyos wrote: >>> 2016-10-26 1:35 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >>> >>>> I forgot to include stdint.h. Fixed patch attached. >>> >>> Why don't you cast to (int)? >> >> Because gcc doesn't like it: >> warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > > I don't see the problem with this warning but I guess you could use > long, no? > long is still 32-bit on some 64-bit platforms. intptr_t is the type designed for this, whats wrong with using it? - Hendrik
On 26.10.2016 22:57, Carl Eugen Hoyos wrote: > 2016-10-26 21:36 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> On 26.10.2016 10:52, Carl Eugen Hoyos wrote: >>> 2016-10-26 1:16 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >>>> I'm under the impression that __attribute__((used)) is not available >>>> for all compilers, >> >>> Yes, but __attribute__((foo_bar)) does not break compilation here. >> >> Have you tested with MSVC? > > No. Then it's safer to assume it doesn't work. >> On 26.10.2016 10:55, Carl Eugen Hoyos wrote: >>> 2016-10-26 1:35 GMT+02:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >>> >>>> I forgot to include stdint.h. Fixed patch attached. >>> >>> Why don't you cast to (int)? >> >> Because gcc doesn't like it: >> warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > > I don't see the problem with this warning but I guess you could use > long, no? As Hendrik said, intptr_t is the correct type here, so I see no reason not to use. Best regards, Andreas
From bb289a0b2b0948afa99227bcff188301c1143624 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> Date: Tue, 25 Oct 2016 19:09:46 +0200 Subject: [PATCH] configure: make sure LTO does not optimize out the test functions Fixes trac ticket #5909 Bud-Id: https://bugs.gentoo.org/show_bug.cgi?id=598054 Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- configure | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 481f692..54faef1 100755 --- a/configure +++ b/configure @@ -1150,7 +1150,12 @@ check_func_headers(){ for func in $funcs; do echo "long check_$func(void) { return (long) $func; }" done - echo "int main(void) { return 0; }" + echo "int main(void) { int ret = 0;" + # LTO could optimize out the test functions without this + for func in $funcs; do + echo " ret |= ((intptr_t)check_$func) & 0xFFFF;" + done + echo "return ret; }" } | check_ld "cc" "$@" && enable $funcs && enable_safe $headers } -- 2.9.3