Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1333138 > unrolled thread

[PATCH] branch tracer: fix freak link error

Back to article view | Back to linux.kernel

Contents

  [PATCH] branch tracer: fix freak link error Arnd Bergmann <arnd@arndb.de> - 2016-02-12 22:30 +0100
    Re: [PATCH] branch tracer: fix freak link error Arnd Bergmann <arnd@arndb.de> - 2016-02-12 22:50 +0100
      Re: [PATCH] branch tracer: fix freak link error Arnd Bergmann <arnd@arndb.de> - 2016-02-12 23:00 +0100
        Re: [PATCH] branch tracer: fix freak link error Arnd Bergmann <arnd@arndb.de> - 2016-02-12 23:20 +0100
        Re: [PATCH] branch tracer: fix freak link error Steven Rostedt <rostedt@goodmis.org> - 2016-02-12 23:20 +0100
    Re: [PATCH] branch tracer: fix freak link error Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-12 22:50 +0100
    Re: [PATCH] branch tracer: fix freak link error Steven Rostedt <rostedt@goodmis.org> - 2016-02-15 18:50 +0100
      Re: [PATCH] branch tracer: fix freak link error Arnd Bergmann <arnd@arndb.de> - 2016-02-15 21:20 +0100
        Re: [PATCH] branch tracer: fix freak link error Steven Rostedt <rostedt@goodmis.org> - 2016-02-15 21:30 +0100
          Re: [PATCH] branch tracer: fix freak link error Arnd Bergmann <arnd@arndb.de> - 2016-02-16 10:30 +0100

#1333138 — [PATCH] branch tracer: fix freak link error

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

[toc] | [next] | [standalone]

#1333150

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

[toc] | [prev] | [next] | [standalone]

#1333158

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

[toc] | [prev] | [next] | [standalone]

#1333172

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

[toc] | [prev] | [next] | [standalone]

#1333176

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

[toc] | [prev] | [next] | [standalone]

#1333156

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
> 
> 

[toc] | [prev] | [next] | [standalone]

#1334665

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

[toc] | [prev] | [next] | [standalone]

#1334788

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

[toc] | [prev] | [next] | [standalone]

#1334795

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

[toc] | [prev] | [next] | [standalone]

#1335216

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web