Message-ID: <CAK8P3a101M6u2hjkEh47t5ZWfpGk80uHuFYFgfJi+vCLmvzaQg@mail.gmail.com>
Date:   Wed, 29 Mar 2017 11:30:57 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Michal Marek <mmarek@...e.com>,
        Nicholas Piggin <npiggin@...il.com>,
        Namhyung Kim <namhyung.with.foss@...il.com>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        masahiroy@...nel.org
Subject: Re: [PATCH] [SUBMITTED 20170314] [v333kbuild: disable
 -ffunction-sections on gcc-4.7 with ftrace

On Wed, Mar 29, 2017 at 4:07 AM, Masahiro Yamada
<yamada.masahiro@...ionext.com> wrote:
> Hi Arnd,
>
>
> 2017-03-15 6:37 GMT+09:00 Arnd Bergmann <arnd@...db.de>:
>> When ftrace is enabled and we build with gcc-4.7 or older, we
>> get a warning for each file on architectures that select
>> CONFIG_LD_DEAD_CODE_DATA_ELIMINATION:
>>
>> warning: -ffunction-sections disabled; it makes profiling impossible [enabled by default]
>>
>> This turns off function sections in that specific case, leaving
>> it enabled for all other configurations.
>>
>> Fixes: b67067f1176d ("kbuild: allow archs to select link dead code/data elimination")
>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>> Cc: Namhyung Kim <namhyung.with.foss@...il.com>
>> ---
>> v2: accidentally resend the same patch as before
>> v3: send the exact same patch once more, without doing the change I wanted
>> v4: actually fixed version number in check as pointed out by Namhyung Kim (I hope)
>> ---
>>  Makefile | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 6e7e644a0b84..3a964fa3a787 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -662,7 +662,11 @@ KBUILD_CFLAGS      += -Wextra
>>  KBUILD_CFLAGS  += $(call cc-disable-warning,frame-address,)
>>
>>  ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
>> +ifdef CONFIG_FUNCTION_TRACER
>> +KBUILD_CFLAGS  += $(call cc-ifversion, -ge,0408,$(call cc-option,-ffunction-sections,))
>> +else
>>  KBUILD_CFLAGS  += $(call cc-option,-ffunction-sections,)
>> +endif
>>  KBUILD_CFLAGS  += $(call cc-option,-fdata-sections,)
>>  endif
>>
>
>
> I have two questions.
>
> [1]
> How did you produce this warning?
> I do not see any architecture that selects this option at this point of time.

I saw it on ARM randconfig builds

> Did you edit Kconfig locally to select LD_DEAD_CODE_DATA_ELIMINATION?
> If so, is this patch not so urgent as the "Fixes" tag claims?

Good point, I forgot that I had enabled this manually on ARM in an
earlier patch in my randconfig-fixup series. I thought this was selected
on powerpc, but that part of Nick's series apparently didn't make it in
yet.

I don't see the 'Fixes' tag as claiming the patch to be urgent, just
documenting what patch introduced the problem, but you could
argue here that it only gets introduced by a future patch that
actually selects the symbol.

> [2]
> This question is more technical.
>
> The cause of the problem seems that GCC warns it cannot support the
> two at the same time,
> but it does handle it as an error.  So, cc-option assumes this
> combination is OK.
>
> Is it a good idea to add -Werror to cc-option checking?

Hmm, I've actually been running with that change as a side-effect
of having the llvmlinux patches applied, which contain this one:

commit 03497934e211325fc2913476897daf7a5ddb008a
Author: Mark Charlebois <charlebm@...il.com>
Date:   Mon Sep 22 14:33:05 2014 -0700

    kbuild, LLVMLinux: Add -Werror to cc-option to support clang

    Clang will warn about unknown warnings but will not return false
    unless -Werror is set. GCC will return false if an unknown
    warning is passed.

    Adding -Werror make both compiler behave the same.

    Signed-off-by: Mark Charlebois <charlebm@...il.com>
    Signed-off-by: Behan Webster <behanw@...verseincode.com>
    Reviewed-by: Jan-Simon Möller <dl9pf@....de>

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 285acc57c0a4..3629bd9c7e79 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -116,12 +116,12 @@ CC_OPTION_CFLAGS = $(filter-out
$(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
 # Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)

 cc-option = $(call try-run,\
-       $(CC) $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) $(1) -c -x c
/dev/null -o "$$TMP",$(1),$(2))
+       $(CC) -Werror $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) $(1) -c
-x c /dev/null -o "$$TMP",$(1),$(2))

 # cc-option-yn
 # Usage: flag := $(call cc-option-yn,-march=winchip-c6)
 cc-option-yn = $(call try-run,\
-       $(CC) $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) $(1) -c -x c
/dev/null -o "$$TMP",y,n)
+       $(CC) -Werror $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) $(1) -c
-x c /dev/null -o "$$TMP",y,n)

 # cc-option-align
 # Prefix align with either -falign or -malign
@@ -131,7 +131,7 @@ cc-option-align = $(subst -functions=0,,\
 # cc-disable-warning
 # Usage: cflags-y += $(call cc-disable-warning,unused-but-set-variable)
 cc-disable-warning = $(call try-run,\
-       $(CC) $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) -W$(strip $(1))
-c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1)))
+       $(CC) -Werror $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) -W$(strip
$(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1)))

 # cc-name
 # Expands to either gcc or clang

This is identical to your version, except it applies the same
thing to cc-disable-warning. I think this is good to get merged,
and there are probably a couple of other patches in that series
that we may want to look at again.

   Arnd

Powered by blists - more mailing lists