* [PATCH] branch tracer: fix freak link error
@ 2016-02-12 21:26 Arnd Bergmann
2016-02-12 21:45 ` Nicolas Pitre
2016-02-15 17:47 ` Steven Rostedt
0 siblings, 2 replies; 10+ messages in thread
From: Arnd Bergmann @ 2016-02-12 21:26 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Arnd Bergmann, Nicolas Pitre,
Mauro Carvalho Chehab, Steven Rostedt, Ingo Molnar
In my randconfig tests, I came across a bug that involves several
components:
* gcc-4.9 through at least 5.3
* CONFIG_GCOV_PROFILE_ALL enabling -fprofile-arcs for all files
* CONFIG_PROFILE_ALL_BRANCHES overriding every if()
* The optimized implementation of do_div() that tries to
replace a library call with an division by multiplication
* code in drivers/media/dvb-frontends/zl10353.c doing
u32 adc_clock = 450560; /* 45.056 MHz */
if (state->config.adc_clock)
adc_clock = state->config.adc_clock;
do_div(value, adc_clock);
In this case, gcc fails to determine whether the divisor
in do_div() is __builtin_constant_p(). In particular, it
concludes that __builtin_constant_p(adc_clock) is false, while
__builtin_constant_p(!!adc_clock) is true.
That in turn throws off the logic in do_div() that also uses
__builtin_constant_p(), and instead of picking either the
constant- optimized division, and the code in ilog2() that uses
__builtin_constant_p() to figure out whether it knows the answer at
compile time. The result is a link error from failing to find
multiple symbols that should never have been called based on
the __builtin_constant_p():
dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
ERROR: "____ilog2_NaN" [drivers/media/dvb-frontends/zl10353.ko] undefined!
ERROR: "__aeabi_uldivmod" [drivers/media/dvb-frontends/zl10353.ko] undefined!
This patch avoids the problem by changing __trace_if() to check
whether the condition is known at compile-time to be nonzero, rather
than checking whether it is actually a constant.
I see this one link error in roughly one out of 1600 randconfig builds
on ARM, and the patch fixes all known instances.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: ab3c9c686e22 ("branch tracer, intel-iommu: fix build with CONFIG_BRANCH_TRACER=y")
---
include/linux/compiler.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index b5acbb404854..b5ff9881bef8 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
*/
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
#define __trace_if(cond) \
- if (__builtin_constant_p((cond)) ? !!(cond) : \
+ if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
({ \
int ______r; \
static struct ftrace_branch_data \
--
2.7.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-12 21:26 [PATCH] branch tracer: fix freak link error Arnd Bergmann
@ 2016-02-12 21:45 ` Nicolas Pitre
2016-02-12 21:49 ` Arnd Bergmann
2016-02-15 17:47 ` Steven Rostedt
1 sibling, 1 reply; 10+ messages in thread
From: Nicolas Pitre @ 2016-02-12 21:45 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, linux-arm-kernel, Mauro Carvalho Chehab,
Steven Rostedt, Ingo Molnar
On Fri, 12 Feb 2016, Arnd Bergmann wrote:
> In my randconfig tests, I came across a bug that involves several
> components:
>
> * gcc-4.9 through at least 5.3
> * CONFIG_GCOV_PROFILE_ALL enabling -fprofile-arcs for all files
> * CONFIG_PROFILE_ALL_BRANCHES overriding every if()
> * The optimized implementation of do_div() that tries to
> replace a library call with an division by multiplication
> * code in drivers/media/dvb-frontends/zl10353.c doing
>
> u32 adc_clock = 450560; /* 45.056 MHz */
> if (state->config.adc_clock)
> adc_clock = state->config.adc_clock;
> do_div(value, adc_clock);
>
> In this case, gcc fails to determine whether the divisor
> in do_div() is __builtin_constant_p(). In particular, it
> concludes that __builtin_constant_p(adc_clock) is false, while
> __builtin_constant_p(!!adc_clock) is true.
>
> That in turn throws off the logic in do_div() that also uses
> __builtin_constant_p(), and instead of picking either the
> constant- optimized division, and the code in ilog2() that uses
> __builtin_constant_p() to figure out whether it knows the answer at
> compile time. The result is a link error from failing to find
> multiple symbols that should never have been called based on
> the __builtin_constant_p():
>
> dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
> dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
> ERROR: "____ilog2_NaN" [drivers/media/dvb-frontends/zl10353.ko] undefined!
> ERROR: "__aeabi_uldivmod" [drivers/media/dvb-frontends/zl10353.ko] undefined!
>
> This patch avoids the problem by changing __trace_if() to check
> whether the condition is known at compile-time to be nonzero, rather
> than checking whether it is actually a constant.
>
> I see this one link error in roughly one out of 1600 randconfig builds
> on ARM, and the patch fixes all known instances.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: ab3c9c686e22 ("branch tracer, intel-iommu: fix build with CONFIG_BRANCH_TRACER=y")
It is certainly a worthwhile workaround if it solves all the known
cases.
Acked-by: Nicolas Pitre <nico@linaro.org>
This trick doesn't work with the test case I produced to demonstrate
this bug though. So we might be affected again in the future, or maybe
not if we're lucky.
> ---
> include/linux/compiler.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index b5acbb404854..b5ff9881bef8 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> */
> #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
> #define __trace_if(cond) \
> - if (__builtin_constant_p((cond)) ? !!(cond) : \
> + if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
> ({ \
> int ______r; \
> static struct ftrace_branch_data \
> --
> 2.7.0
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-12 21:45 ` Nicolas Pitre
@ 2016-02-12 21:49 ` Arnd Bergmann
2016-02-12 21:52 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2016-02-12 21:49 UTC (permalink / raw)
To: Nicolas Pitre
Cc: linux-kernel, linux-arm-kernel, Mauro Carvalho Chehab,
Steven Rostedt, Ingo Molnar
On Friday 12 February 2016 16:45:03 Nicolas Pitre wrote:
>
> It is certainly a worthwhile workaround if it solves all the known
> cases.
>
> Acked-by: Nicolas Pitre <nico@linaro.org>
>
> This trick doesn't work with the test case I produced to demonstrate
> this bug though. So we might be affected again in the future, or maybe
> not if we're lucky.
Yes, I think it's clear that there is still something interesting going
on with the compiler and nested __builtin_compatible_p().
I'm currently creating a couple of gcc bug reports and plan to finally submit
this one as well.
Now I have to check if it does anything to the spurious "maybe-uninitialized"
warnings that I saw creeping in with PROFILE_ALL_BRANCHES. My guess is
that it doesn't fix them (I submitted a patch to turn off those
warnings today when PROFILE_ALL_BRANCHES is set), but I'll try anyway.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-12 21:49 ` Arnd Bergmann
@ 2016-02-12 21:52 ` Arnd Bergmann
2016-02-12 22:12 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2016-02-12 21:52 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Nicolas Pitre, Ingo Molnar, Steven Rostedt, linux-kernel,
Mauro Carvalho Chehab
On Friday 12 February 2016 22:49:07 Arnd Bergmann wrote:
> Now I have to check if it does anything to the spurious "maybe-uninitialized"
> warnings that I saw creeping in with PROFILE_ALL_BRANCHES. My guess is
> that it doesn't fix them (I submitted a patch to turn off those
> warnings today when PROFILE_ALL_BRANCHES is set), but I'll try anyway.
Ok, that was easy: they are all still there, so we still need to
pass -Wno-maybe-uninitialized when using PROFILE_ALL_BRANCHES to
avoid flooding the log with useless warnings.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-12 21:52 ` Arnd Bergmann
@ 2016-02-12 22:12 ` Steven Rostedt
2016-02-12 22:18 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2016-02-12 22:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, Nicolas Pitre, Ingo Molnar, linux-kernel,
Mauro Carvalho Chehab
On Fri, 12 Feb 2016 22:52:10 +0100
Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 12 February 2016 22:49:07 Arnd Bergmann wrote:
> > Now I have to check if it does anything to the spurious "maybe-uninitialized"
> > warnings that I saw creeping in with PROFILE_ALL_BRANCHES. My guess is
> > that it doesn't fix them (I submitted a patch to turn off those
> > warnings today when PROFILE_ALL_BRANCHES is set), but I'll try anyway.
>
> Ok, that was easy: they are all still there, so we still need to
> pass -Wno-maybe-uninitialized when using PROFILE_ALL_BRANCHES to
> avoid flooding the log with useless warnings.
Yeah, those warnings were the reason I turned PROFILE_ALL_BRANCHES into
a tristate and not a boolean, otherwise a make allmodconfig or
allyesconfig would enable it and spam the compile with those warnings.
I simply ignore them, so adding a way to turn them off when that is
enabled is probably a good idea.
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-12 22:12 ` Steven Rostedt
@ 2016-02-12 22:18 ` Arnd Bergmann
0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2016-02-12 22:18 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-arm-kernel, Nicolas Pitre, Ingo Molnar, linux-kernel,
Mauro Carvalho Chehab
On Friday 12 February 2016 17:12:37 Steven Rostedt wrote:
> On Fri, 12 Feb 2016 22:52:10 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> > On Friday 12 February 2016 22:49:07 Arnd Bergmann wrote:
> > > Now I have to check if it does anything to the spurious "maybe-uninitialized"
> > > warnings that I saw creeping in with PROFILE_ALL_BRANCHES. My guess is
> > > that it doesn't fix them (I submitted a patch to turn off those
> > > warnings today when PROFILE_ALL_BRANCHES is set), but I'll try anyway.
> >
> > Ok, that was easy: they are all still there, so we still need to
> > pass -Wno-maybe-uninitialized when using PROFILE_ALL_BRANCHES to
> > avoid flooding the log with useless warnings.
>
> Yeah, those warnings were the reason I turned PROFILE_ALL_BRANCHES into
> a tristate and not a boolean, otherwise a make allmodconfig or
> allyesconfig would enable it and spam the compile with those warnings.
>
> I simply ignore them, so adding a way to turn them off when that is
> enabled is probably a good idea.
Ok. The series I sent today was titled "[PATCH 0/5] gcov fixes and
maybe-uninitialized warnings", and I disable the warnings for
both GCOV_PROFILE_ALL and PROFILE_ALL_BRANCHES there, along with
CC_OPTIMIZE_FOR_SIZE and UBSAN_SANITIZE_ALL that already do that
today.
The patch series also makes sure that all four options are disabled
in allmodconfig (today they are disabled because of CC_OPTIMIZE_FOR_SIZE)
and the remaining warnings I get in both allmodconfig and randconfig
are finally useful.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-12 21:26 [PATCH] branch tracer: fix freak link error Arnd Bergmann
2016-02-12 21:45 ` Nicolas Pitre
@ 2016-02-15 17:47 ` Steven Rostedt
2016-02-15 20:15 ` Arnd Bergmann
1 sibling, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2016-02-15 17:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, linux-arm-kernel, Nicolas Pitre,
Mauro Carvalho Chehab, Ingo Molnar
On Fri, 12 Feb 2016 22:26:42 +0100
Arnd Bergmann <arnd@arndb.de> wrote:
> In my randconfig tests, I came across a bug that involves several
> components:
>
>
[..]
>
> This patch avoids the problem by changing __trace_if() to check
> whether the condition is known at compile-time to be nonzero, rather
> than checking whether it is actually a constant.
>
> I see this one link error in roughly one out of 1600 randconfig builds
> on ARM, and the patch fixes all known instances.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: ab3c9c686e22 ("branch tracer, intel-iommu: fix build with CONFIG_BRANCH_TRACER=y")
Want me to pull this into my queue? I have another patch I need to send
to Linus and I can add this in that pull request.
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-15 17:47 ` Steven Rostedt
@ 2016-02-15 20:15 ` Arnd Bergmann
2016-02-15 20:26 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2016-02-15 20:15 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-arm-kernel, Nicolas Pitre,
Mauro Carvalho Chehab, Ingo Molnar
On Monday 15 February 2016 12:47:46 Steven Rostedt wrote:
> On Fri, 12 Feb 2016 22:26:42 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> > In my randconfig tests, I came across a bug that involves several
> > components:
> >
> >
>
> [..]
>
> >
> > This patch avoids the problem by changing __trace_if() to check
> > whether the condition is known at compile-time to be nonzero, rather
> > than checking whether it is actually a constant.
> >
> > I see this one link error in roughly one out of 1600 randconfig builds
> > on ARM, and the patch fixes all known instances.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Fixes: ab3c9c686e22 ("branch tracer, intel-iommu: fix build with CONFIG_BRANCH_TRACER=y")
>
> Want me to pull this into my queue? I have another patch I need to send
> to Linus and I can add this in that pull request.
That would be nice, yes.
Do you also want to add a 'Cc: stable@vger.kernel.org' tag?
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-15 20:15 ` Arnd Bergmann
@ 2016-02-15 20:26 ` Steven Rostedt
2016-02-16 9:17 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2016-02-15 20:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, linux-arm-kernel, Nicolas Pitre,
Mauro Carvalho Chehab, Ingo Molnar
On Mon, 15 Feb 2016 21:15:39 +0100
Arnd Bergmann <arnd@arndb.de> wrote:
> Do you also want to add a 'Cc: stable@vger.kernel.org' tag?
>
I currently have this in the Tags:
Acked-by: Nicolas Pitre <nico@linaro.org>
Fixes: ab3c9c686e22 ("branch tracer, intel-iommu: fix build with CONFIG_BRANCH_TRACER=y")
Cc: stable@vger.kernel.org # v2.6.30+
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] branch tracer: fix freak link error
2016-02-15 20:26 ` Steven Rostedt
@ 2016-02-16 9:17 ` Arnd Bergmann
0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2016-02-16 9:17 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Steven Rostedt, Nicolas Pitre, Ingo Molnar, linux-kernel,
Mauro Carvalho Chehab
On Monday 15 February 2016 15:26:39 Steven Rostedt wrote:
> On Mon, 15 Feb 2016 21:15:39 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> > Do you also want to add a 'Cc: stable@vger.kernel.org' tag?
> >
>
> I currently have this in the Tags:
>
> Acked-by: Nicolas Pitre <nico@linaro.org>
> Fixes: ab3c9c686e22 ("branch tracer, intel-iommu: fix build with CONFIG_BRANCH_TRACER=y")
> Cc: stable@vger.kernel.org # v2.6.30+
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>
Looks good, thanks!
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-02-16 9:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-12 21:26 [PATCH] branch tracer: fix freak link error Arnd Bergmann
2016-02-12 21:45 ` Nicolas Pitre
2016-02-12 21:49 ` Arnd Bergmann
2016-02-12 21:52 ` Arnd Bergmann
2016-02-12 22:12 ` Steven Rostedt
2016-02-12 22:18 ` Arnd Bergmann
2016-02-15 17:47 ` Steven Rostedt
2016-02-15 20:15 ` Arnd Bergmann
2016-02-15 20:26 ` Steven Rostedt
2016-02-16 9:17 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).