* Re: [PATCH kernel] powerpc/makefile: Do not redefine $(CPP) for preprocessor
2021-04-22 22:58 ` Daniel Axtens
@ 2021-04-23 3:06 ` Alexey Kardashevskiy
2021-04-23 5:27 ` Christophe Leroy
2021-04-29 10:47 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Alexey Kardashevskiy @ 2021-04-23 3:06 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev
On 23/04/2021 08:58, Daniel Axtens wrote:
> Hi Alexey,
>
>> The $(CPP) (do only preprocessing) macro is already defined in Makefile.
>> However POWERPC redefines it and adds $(KBUILD_CFLAGS) which results
>> in flags duplication. Which is not a big deal by itself except for
>> the flags which depend on other flags and the compiler checks them
>> as it parses the command line.
>>
>> Specifically, scripts/Makefile.build:304 generates ksyms for .S files.
>> If clang+llvm+sanitizer are enabled, this results in
>> -fno-lto -flto -fsanitize=cfi-mfcall .... -fno-lto -flto -fsanitize=cfi-mfcall
>
> Checkpatch doesn't like this line:
> WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #14:
> -fno-lto -flto -fsanitize=cfi-mfcall .... -fno-lto -flto -fsanitize=cfi-mfcall
> However, it doesn't make sense to wrap the line so we should just ignore
> checkpatch here.
>
>> in the clang command line and triggers error:
>>
>> clang-13: error: invalid argument '-fsanitize=cfi-mfcall' only allowed with '-flto'
>>
>> This removes unnecessary CPP redifinition.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> arch/powerpc/Makefile | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> index c9d2c7825cd6..3a2f2001c62b 100644
>> --- a/arch/powerpc/Makefile
>> +++ b/arch/powerpc/Makefile
>> @@ -214,7 +214,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
>> KBUILD_AFLAGS += $(AFLAGS-y)
>> KBUILD_CFLAGS += $(call cc-option,-msoft-float)
>> KBUILD_CFLAGS += -pipe $(CFLAGS-y)
>> -CPP = $(CC) -E $(KBUILD_CFLAGS)
>
> I was trying to check the history to see why powerpc has its own
> definition. It seems to date back at least as far as merging the two
> powerpc platforms into one, maybe it was helpful then. I agree it
> doesn't seem to be needed now.
>
> Snowpatch claims that this breaks the build, but I haven't been able to
> reproduce the issue in either pmac32 or ppc64 defconfig. I have sent it
> off to a fork of mpe's linux-ci repo to see if any of those builds hit
> any issues: https://github.com/daxtens/linux-ci/actions
To be precise, you need LLVM + LTO + byte code (-emit-llvm-bc), I am not
even sure what is the point of having -flto without -emit-llvm-bc.
No flags duplication:
[fstn1-p1 1]$ /mnt/sdb/pbuild/llvm-no-lto/bin/clang-13 -emit-llvm-bc
-fno-lto -flto -fvisibility=hidden -fsanitize=cfi-mfcall
/mnt/sdb/pbuild/llvm-bugs/1/a.c
/usr/bin/ld: warning: cannot find entry symbol mit-llvm-bc; defaulting
to 00000000100003e0
/usr/bin/ld:
/usr/lib/powerpc64le-linux-gnu/crt1.o:(.data.rel.ro.local+0x8):
undefined reference to `main'
clang-13: error: linker command failed with exit code 1 (use -v to see
invocation)
=> command line is fine, the file is not (but it is for debugging this
problem).
Now I am adding the second -fno-lto:
[fstn1-p1 1]$ /mnt/sdb/pbuild/llvm-no-lto/bin/clang-13 -emit-llvm-bc
-fno-lto -flto -fvisibility=hidden -fsanitize=cfi-mfcall -fno-lto
/mnt/sdb/pbuild/llvm-bugs/1/a.c
clang-13: error: invalid argument '-fsanitize=cfi-mfcall' only allowed
with '-flto'
=> failed.
> Assuming that doesn't break, this patch looks good to me:
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Kind regards,
> Daniel
>
--
Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kernel] powerpc/makefile: Do not redefine $(CPP) for preprocessor
2021-04-22 22:58 ` Daniel Axtens
2021-04-23 3:06 ` Alexey Kardashevskiy
@ 2021-04-23 5:27 ` Christophe Leroy
2021-04-23 13:41 ` Michael Ellerman
2021-04-29 10:47 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Christophe Leroy @ 2021-04-23 5:27 UTC (permalink / raw)
To: Daniel Axtens, Alexey Kardashevskiy, linuxppc-dev
Le 23/04/2021 à 00:58, Daniel Axtens a écrit :
>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> index c9d2c7825cd6..3a2f2001c62b 100644
>> --- a/arch/powerpc/Makefile
>> +++ b/arch/powerpc/Makefile
>> @@ -214,7 +214,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
>> KBUILD_AFLAGS += $(AFLAGS-y)
>> KBUILD_CFLAGS += $(call cc-option,-msoft-float)
>> KBUILD_CFLAGS += -pipe $(CFLAGS-y)
>> -CPP = $(CC) -E $(KBUILD_CFLAGS)
>
> I was trying to check the history to see why powerpc has its own
> definition. It seems to date back at least as far as merging the two
> powerpc platforms into one, maybe it was helpful then. I agree it
> doesn't seem to be needed now.
>
I digged a bit deaper. It has been there since the introduction of arch/ppc/ in Linux 1.3.45
At the time, there was the exact same CPP definition in arch/mips/Makefile
The CPP definition in mips disappeared is Linux 2.1.44pre3.
Since that version, ppc has been the only one with such CPP re-definition.
> Snowpatch claims that this breaks the build, but I haven't been able to
> reproduce the issue in either pmac32 or ppc64 defconfig. I have sent it
> off to a fork of mpe's linux-ci repo to see if any of those builds hit
> any issues: https://github.com/daxtens/linux-ci/actions
>
By the way, I find snowpatch quite useless these days. It seems to delete the reports a few minutes
after the test. We are less than one day after the patch was submitted and the report of the build
failures are already gone.
Christophe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kernel] powerpc/makefile: Do not redefine $(CPP) for preprocessor
2021-04-23 5:27 ` Christophe Leroy
@ 2021-04-23 13:41 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-04-23 13:41 UTC (permalink / raw)
To: Christophe Leroy, Daniel Axtens, Alexey Kardashevskiy,
linuxppc-dev
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 23/04/2021 à 00:58, Daniel Axtens a écrit :
>>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>>> index c9d2c7825cd6..3a2f2001c62b 100644
>>> --- a/arch/powerpc/Makefile
>>> +++ b/arch/powerpc/Makefile
>>> @@ -214,7 +214,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
>>> KBUILD_AFLAGS += $(AFLAGS-y)
>>> KBUILD_CFLAGS += $(call cc-option,-msoft-float)
>>> KBUILD_CFLAGS += -pipe $(CFLAGS-y)
>>> -CPP = $(CC) -E $(KBUILD_CFLAGS)
>>
>> I was trying to check the history to see why powerpc has its own
>> definition. It seems to date back at least as far as merging the two
>> powerpc platforms into one, maybe it was helpful then. I agree it
>> doesn't seem to be needed now.
>>
>
> I digged a bit deaper. It has been there since the introduction of arch/ppc/ in Linux 1.3.45
> At the time, there was the exact same CPP definition in arch/mips/Makefile
>
> The CPP definition in mips disappeared is Linux 2.1.44pre3.
> Since that version, ppc has been the only one with such CPP re-definition.
>
>> Snowpatch claims that this breaks the build, but I haven't been able to
>> reproduce the issue in either pmac32 or ppc64 defconfig. I have sent it
>> off to a fork of mpe's linux-ci repo to see if any of those builds hit
>> any issues: https://github.com/daxtens/linux-ci/actions
>>
>
> By the way, I find snowpatch quite useless these days. It seems to delete the reports a few minutes
> after the test. We are less than one day after the patch was submitted and the report of the build
> failures are already gone.
Yeah, it's pretty annoying. It needs to send emails to the list with its
results, with a link to a location that retains the logs for some
reasonable period.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kernel] powerpc/makefile: Do not redefine $(CPP) for preprocessor
2021-04-22 22:58 ` Daniel Axtens
2021-04-23 3:06 ` Alexey Kardashevskiy
2021-04-23 5:27 ` Christophe Leroy
@ 2021-04-29 10:47 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-04-29 10:47 UTC (permalink / raw)
To: Daniel Axtens, Alexey Kardashevskiy, linuxppc-dev; +Cc: Alexey Kardashevskiy
Daniel Axtens <dja@axtens.net> writes:
> Hi Alexey,
>
>> The $(CPP) (do only preprocessing) macro is already defined in Makefile.
>> However POWERPC redefines it and adds $(KBUILD_CFLAGS) which results
>> in flags duplication. Which is not a big deal by itself except for
>> the flags which depend on other flags and the compiler checks them
>> as it parses the command line.
>>
>> Specifically, scripts/Makefile.build:304 generates ksyms for .S files.
>> If clang+llvm+sanitizer are enabled, this results in
>> -fno-lto -flto -fsanitize=cfi-mfcall .... -fno-lto -flto -fsanitize=cfi-mfcall
>
> Checkpatch doesn't like this line:
> WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #14:
> -fno-lto -flto -fsanitize=cfi-mfcall .... -fno-lto -flto -fsanitize=cfi-mfcall
> However, it doesn't make sense to wrap the line so we should just ignore
> checkpatch here.
>
>> in the clang command line and triggers error:
>>
>> clang-13: error: invalid argument '-fsanitize=cfi-mfcall' only allowed with '-flto'
>>
>> This removes unnecessary CPP redifinition.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> arch/powerpc/Makefile | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> index c9d2c7825cd6..3a2f2001c62b 100644
>> --- a/arch/powerpc/Makefile
>> +++ b/arch/powerpc/Makefile
>> @@ -214,7 +214,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
>> KBUILD_AFLAGS += $(AFLAGS-y)
>> KBUILD_CFLAGS += $(call cc-option,-msoft-float)
>> KBUILD_CFLAGS += -pipe $(CFLAGS-y)
>> -CPP = $(CC) -E $(KBUILD_CFLAGS)
>
> I was trying to check the history to see why powerpc has its own
> definition. It seems to date back at least as far as merging the two
> powerpc platforms into one, maybe it was helpful then. I agree it
> doesn't seem to be needed now.
>
> Snowpatch claims that this breaks the build, but I haven't been able to
> reproduce the issue in either pmac32 or ppc64 defconfig. I have sent it
> off to a fork of mpe's linux-ci repo to see if any of those builds hit
> any issues: https://github.com/daxtens/linux-ci/actions
It does break the build.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread