linux-kernel - Re: [PATCH 3/4] kbuild: re-order the code to not parse unnecessary variables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Oct 2017 09:08:57 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Sam Ravnborg <sam@...nborg.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] kbuild: re-order the code to not parse unnecessary variables

2017-10-10 7:03 GMT+09:00 Doug Anderson <dianders@...omium.org>:
> Hi,
>
> On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
> <yamada.masahiro@...ionext.com> wrote:
>> The top Makefile is divided into some sections such as mixed targets,
>> config targets, build targets, etc.
>>
>> When we build mixed targets, Kbuild just invokes submake to process
>> them one by one.  In this case, compiler-related variables like CC,
>> KBUILD_CFLAGS, etc. are unneeded.
>>
>> Check what kind of targets we are building first, then parse necessary
>> variables for building them.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
>> ---
>>
>>  Makefile | 233 ++++++++++++++++++++++++++++++++-------------------------------
>>  1 file changed, 118 insertions(+), 115 deletions(-)
>
> I'm even further outside my comfort zone with this big of a change,
> but I will say that as far as I can tell this seems like a good
> change.  If it were me I'd have probably broken it up into several
> tinier changes that were each massively easy to check/verify, but
> presumably for those who know the Makefile better then rolling them
> together is fine.  ;)
>
> I're spent some time reviewing this (not tons--maybe an hour or so),
> but IMHO I don't know this well enough to add a Reviewed-by tag.
>
>
>> diff --git a/Makefile b/Makefile
>> index 39a7c03..a4fd682 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -186,15 +186,6 @@ ifeq ("$(origin M)", "command line")
>>    KBUILD_EXTMOD := $(M)
>>  endif
>>
>> -# If building an external module we do not care about the all: rule
>> -# but instead _all depend on modules
>> -PHONY += all
>> -ifeq ($(KBUILD_EXTMOD),)
>> -_all: all
>> -else
>> -_all: modules
>> -endif
>> -
>>  ifeq ($(KBUILD_SRC),)
>>          # building in the source tree
>>          srctree := .
>> @@ -206,6 +197,9 @@ else
>>                  srctree := $(KBUILD_SRC)
>>          endif
>>  endif
>> +
>> +export KBUILD_CHECKSRC KBUILD_EXTMOD KBUILD_SRC
>
> The old Makefile also used to export KBUILD_SRC, but I'm not sure why.
> Shouldn't this be implicit because KBUILD_SRC always comes from a
> command line parameter or environment variable?


As the comment line around line 109 says,
"KBUILD_SRC is not intended to be used by the regular user (for now)"


It is set by "sub-make" around line 143:

sub-make:
         $(Q)$(MAKE) -C $(KBUILD_OUTPUT) KBUILD_SRC=$(CURDIR) \
         -f $(CURDIR)/Makefile $(filter-out _all sub-make,$(MAKECMDGOALS))



I'd like to improve it in the future,
but currently KBUILD_SRC works like that.

But, you are right.
I also thought "export KBUILD_SRC" is redundant.
KBUILD_SRC=$(CURDIR) implies exporting it.


I see some more redundant exporting
for variables we know they only come from command line or environment.


I think cleaning-up is OK, but should be
separated to another patch just in case.
(my commit claims I am just changing the code order...)




-- 
Best Regards
Masahiro Yamada