Re: [PATCH] [SUBMITTED 20170314] [v333kbuild: disable -ffunction-sections on gcc-4.7 with ftrace

[Masahiro Yamada]

Hi Arnd,

2017-03-29 18:30 GMT+09:00 Arnd Bergmann <arnd@xxxxxxxx>:
> On Wed, Mar 29, 2017 at 4:07 AM, Masahiro Yamada
> <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>> Hi Arnd,
>>
>>
>> 2017-03-15 6:37 GMT+09:00 Arnd Bergmann <arnd@xxxxxxxx>:
>>> 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@xxxxxxxx>
>>> Cc: Namhyung Kim <namhyung.with.foss@xxxxxxxxx>
>>> ---
>>> 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.

I won't argue.  Fixes is reviewer-friendly
for finding the related commit.

I just personally used the "Fix" keyword as a hint for priority
because there are still many kbuild patches piled up,
and I have not been able to catch up yet.



>> [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@xxxxxxxxx>
> 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@xxxxxxxxx>
>     Signed-off-by: Behan Webster <behanw@xxxxxxxxxxxxxxxxxx>
>     Reviewed-by: Jan-Simon MÃller <dl9pf@xxxxxx>
>
> 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.

Ah, I see.

I'm also moving
KBUILD_CFLAGS  += $(call cc-option,-ffunction-sections,)
below
CC_FLAGS_FTRACE := -pg
to fix the 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.


I agree.


At least, 03497934e21 looks good to be merged.
(I need to get access to Mark, and ask him to send this patch.)

Then, swap the -ffunction-sections and -pg order.

I hope this will make you and clang guys happy.


-- 
Best Regards
Masahiro Yamada