warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Linus Torvalds]

So almost all of our __builtin_return_address() users use a zero
argument, but we have a couple in-tree. They all seem to be related to
tracing:

  kernel/trace/trace_irqsoff.c
  kernel/trace/trace_sched_wakeup.c

and it causes a fair number of lines of warning noise on current
kernels with at least gcc-6.1.1. See

    https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

that says

   "-Wframe-address:
        Warn when the ‘__builtin_frame_address’ or
‘__builtin_return_address’ is called with an argument greater than 0.
Such calls may return indeterminate values or crash the program. The
warning is included in -Wall"

I can just add a

  KBUILD_CFLAGS  += $(call cc-disable-warning,frame-address,)

in the main Makefile, and that is indeed what I have in my tree right
now and will likely commit soon. The warning isn't actually helpful
for us, and there doesn't seem to be any other way to turn it off. At
the same time, there is a somewhat valid reason for that warning
existing, so I'm wondering if the tracing code could perhaps try to
change.

I detest having lots of warnings when doing my test builds, because
the uninteresting warnings often hide real ones that might actually
matter.

Comments?

             Linus

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Steven Rostedt]

On Wed, 27 Jul 2016 16:00:54 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> I can just add a
> 
>   KBUILD_CFLAGS  += $(call cc-disable-warning,frame-address,)

I like this solution.

> 
> in the main Makefile, and that is indeed what I have in my tree right
> now and will likely commit soon. The warning isn't actually helpful
> for us, and there doesn't seem to be any other way to turn it off. At
> the same time, there is a somewhat valid reason for that warning
> existing, so I'm wondering if the tracing code could perhaps try to
> change.
> 
> I detest having lots of warnings when doing my test builds, because
> the uninteresting warnings often hide real ones that might actually
> matter.
> 
> Comments?

We are well aware of the danger of using __builtin_return_address() of
 > 0. In fact that's part of the reason for having the "thunk" code in
x86 (See arch/x86/entry/thunk_{64,32}.S). Because it adds extra frames
when tracking irqs off sections, to prevent the
__builtin_return_address() from accessing bad areas. In fact the
thunk_32.S states: "Trampoline to trace irqs off. (otherwise
CALLER_ADDR1 might crash)".

Now if we can get a better fast arch generic way of getting the same
information, we could switch to that. Perhaps the wakeup tracing could
use part of stacktrace, as that's not as high volume as the irq tracing
is. But the irq tracing does this at every instance interrupts are
disabled and enabled. A bit too much impact on the system to be calling
into the stack trace code every time interrupts are disable or enabled.

I can still look to see if that information is even useful, as we do a
stack dump when we find a new max latency.

-- Steve

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Linus Torvalds]

On Wed, Jul 27, 2016 at 6:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 27 Jul 2016 16:00:54 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>
>> I can just add a
>>
>>   KBUILD_CFLAGS  += $(call cc-disable-warning,frame-address,)
>
> I like this solution.

Ok. Pushed out. As long as people are aware of this, and are hopefully
at least looking for potential alternatives, it's fine.

I've squashed most of the warnings I see in my allmodconfig build.

The remaining ones are mostly objtool warnings (Josh added to cc: I
get both a "objtool: x86 instruction decoder differs from kernel"
warning, and several new "sibling call from callable instruction with
changed frame pointer" warnings).

There's also a couple of really annoying warnings from gcc:

  drivers/sfi/sfi_core.c:175:53: warning: self-comparison always
evaluates to true [-Wtautological-compare]

which is a classic case of compiler people thinking that "comparing
things to itself is stupid", but it comes from using general-case
macros that then sometimes end up having simple uses where one part of
the comparison ends up being trivially true.

Since the "fix" (to avoid a generic macro helper and use special-case
simpler tests) is likely much worse than what the compiler actually
warns about, I suspect I will be just disabling that silly compiler
warning.

People who love being warned about tautological compares, speak up now
about your preferred alternative, or forever hold your peace. I do
*not* want to have stupid warnings show up by default, because then
people will just ignore the real ones when they pop up. That already
happens much too frequently.

                Linus

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Steven Rostedt]

On Wed, 27 Jul 2016 20:21:50 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Jul 27, 2016 at 6:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Wed, 27 Jul 2016 16:00:54 -0700
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:  
> >>
> >> I can just add a
> >>
> >>   KBUILD_CFLAGS  += $(call cc-disable-warning,frame-address,)  
> >
> > I like this solution.  
> 
> Ok. Pushed out. As long as people are aware of this, and are hopefully
> at least looking for potential alternatives, it's fine.

I just looked at your patch. Would this work if you moved that
KBUILD_CFLAGS to the tracing directory? Something like the below (never
compiled or tested).

This way we still get warnings from other users.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---

diff --git a/Makefile b/Makefile
index 393b615..d384848 100644
--- a/Makefile
+++ b/Makefile
@@ -620,7 +620,6 @@ include arch/$(SRCARCH)/Makefile
 
 KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
 KBUILD_CFLAGS	+= $(call cc-disable-warning,maybe-uninitialized,)
-KBUILD_CFLAGS	+= $(call cc-disable-warning,frame-address,)
 
 ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 KBUILD_CFLAGS	+= -Os
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index 979e7bf..48079dd 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -1,4 +1,7 @@
 
+# We are fully aware of the dangers of __builtin_return_address() > 0
+KBUILD_CFLAGS += $(call cc-disable-warning,frame-address,)
+
 # Do not instrument the tracer itself:
 
 ifdef CONFIG_FUNCTION_TRACER

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Josh Poimboeuf]

On Wed, Jul 27, 2016 at 08:21:50PM -0700, Linus Torvalds wrote:
> The remaining ones are mostly objtool warnings (Josh added to cc: I
> get both a "objtool: x86 instruction decoder differs from kernel"
> warning

Ok, this should be an easy fix.

> and several new "sibling call from callable instruction with
> changed frame pointer" warnings).

These all seem to be caused by a new switch statement optimization in
gcc 6 which objtool doesn't know about yet.  I'll try to come up with a
patch to fix it soon.

-- 
Josh

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Linus Torvalds]

On Wed, Jul 27, 2016 at 8:52 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> I just looked at your patch. Would this work if you moved that
> KBUILD_CFLAGS to the tracing directory? Something like the below (never
> compiled or tested).

I tried something like that, but the CFLAGS games the tracing code
does made my thing result in

  kernel/trace/Makefile:20: *** Recursive variable 'KBUILD_CFLAGS'
references itself (eventually).  Stop.

but yes, if you have the magic fingers to make it work, limiting the
-Wno-frame-address to just the tracing code sounds like the
RightThing(tm).

              Linus

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Steven Rostedt]

On Thu, 28 Jul 2016 18:25:33 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I tried something like that, but the CFLAGS games the tracing code
> does made my thing result in
> 
>   kernel/trace/Makefile:20: *** Recursive variable 'KBUILD_CFLAGS'
> references itself (eventually).  Stop.
> 
> but yes, if you have the magic fingers to make it work, limiting the
> -Wno-frame-address to just the tracing code sounds like the
> RightThing(tm).

Take 2:

It compiles for me, but I don't have a compiler that has that warning
to test with. See if this works. BTW, it looks like that last comma
before the ending parenthesis is not needed.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/Makefile b/Makefile
index 393b615..d384848 100644
--- a/Makefile
+++ b/Makefile
@@ -620,7 +620,6 @@ include arch/$(SRCARCH)/Makefile
 
 KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
 KBUILD_CFLAGS	+= $(call cc-disable-warning,maybe-uninitialized,)
-KBUILD_CFLAGS	+= $(call cc-disable-warning,frame-address,)
 
 ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 KBUILD_CFLAGS	+= -Os
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index 979e7bf..d0a1617 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -1,4 +1,8 @@
 
+# We are fully aware of the dangers of __builtin_return_address()
+FRAME_CFLAGS := $(call cc-disable-warning,frame-address)
+KBUILD_CFLAGS += $(FRAME_CFLAGS)
+
 # Do not instrument the tracer itself:
 
 ifdef CONFIG_FUNCTION_TRACER

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Steven Rostedt]

On Thu, 28 Jul 2016 22:30:43 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 28 Jul 2016 18:25:33 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > I tried something like that, but the CFLAGS games the tracing code
> > does made my thing result in
> > 
> >   kernel/trace/Makefile:20: *** Recursive variable 'KBUILD_CFLAGS'
> > references itself (eventually).  Stop.
> > 
> > but yes, if you have the magic fingers to make it work, limiting the
> > -Wno-frame-address to just the tracing code sounds like the
> > RightThing(tm).  
> 
> Take 2:
> 
> It compiles for me, but I don't have a compiler that has that warning
> to test with. See if this works. BTW, it looks like that last comma
> before the ending parenthesis is not needed.

Linus, did you get a chance to test this? I could send a formal patch
if needed. I only have a 4.9 compiler, so I'm not seeing the warnings
others have reported.

-- Steve

> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> diff --git a/Makefile b/Makefile
> index 393b615..d384848 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -620,7 +620,6 @@ include arch/$(SRCARCH)/Makefile
>  
>  KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
>  KBUILD_CFLAGS	+= $(call cc-disable-warning,maybe-uninitialized,)
> -KBUILD_CFLAGS	+= $(call cc-disable-warning,frame-address,)
>  
>  ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
>  KBUILD_CFLAGS	+= -Os
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index 979e7bf..d0a1617 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -1,4 +1,8 @@
>  
> +# We are fully aware of the dangers of __builtin_return_address()
> +FRAME_CFLAGS := $(call cc-disable-warning,frame-address)
> +KBUILD_CFLAGS += $(FRAME_CFLAGS)
> +
>  # Do not instrument the tracer itself:
>  
>  ifdef CONFIG_FUNCTION_TRACER

Re: warning: calling ‘__builtin_return_address’ with a nonzero argument is unsafe

[Linus Torvalds]

On Mon, Aug 1, 2016 at 2:10 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>> It compiles for me, but I don't have a compiler that has that warning
>> to test with. See if this works. BTW, it looks like that last comma
>> before the ending parenthesis is not needed.
>
> Linus, did you get a chance to test this? I could send a formal patch
> if needed. I only have a 4.9 compiler, so I'm not seeing the warnings
> others have reported.

It seems to work for me. Warnings are still suppressed, and the
-Wno-frame-address flag seems to be nicely limited to just the build
of the kernel/trace/ subdirectory.

So feel free to add that patch to the tracing tree, with my tested-by.

           Linus