diff mbox

build: clean up $CFLAGS handling in the makefile

Message ID CANeU7Qm8YHpYLLW8O=y=m9m6kRAD_5MiTnS79N5ffCk-PVcxFQ@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Christopher Li Nov. 1, 2017, 12:56 a.m. UTC
On Tue, Oct 31, 2017 at 2:57 PM, Jeff Layton <jlayton@kernel.org> wrote:
> As Luc and Josh pointed out, I think we want $(CFLAGS) last here. That
> allows you to pass in options that can supersede what the makefile puts
> in there.
> ...other than that, I'm fine with this. Assuming you fix that, you can
> add:

I update the patch here and git url:
https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags-v2

Chris

From 55496234238e127c05807550fb432e1125a85710 Mon Sep 17 00:00:00 2001
From: Christopher Li <sparse@chrisli.org>
Date: Sun, 29 Oct 2017 19:16:44 +0800
Subject: [PATCH] Makefile: provide CFLAGS for command line override.

Avoid assign to CFLAGS in Makefile.
Rename BASIC_CFLAGS to COMMON_CFLAGS.
Use PKG_CFLAGS to store external package related cflags.

Signed-off-by: Christopher Li <sparse@chrisli.org>
Acked-by: Jeff Layton <jlayton@kernel.org>
---
 Makefile | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

$(LLVM_VERSION) installed.)

Comments

Luc Van Oostenryck Nov. 1, 2017, 2:17 p.m. UTC | #1
On Tue, Oct 31, 2017 at 08:56:47PM -0400, Christopher Li wrote:
> On Tue, Oct 31, 2017 at 2:57 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > As Luc and Josh pointed out, I think we want $(CFLAGS) last here. That
> > allows you to pass in options that can supersede what the makefile puts
> > in there.
> > ...other than that, I'm fine with this. Assuming you fix that, you can
> > add:
> 
> I update the patch here and git url:
> https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags-v2

This is still not correct. It still doesn't allow to compile sparse
via 'make CFLAGS=<some extra flags, possibly none>'.

Visibly this hasn't been tested.

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Nov. 5, 2017, 12:45 a.m. UTC | #2
On Wed, Nov 1, 2017 at 10:17 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:

> This is still not correct. It still doesn't allow to compile sparse
> via 'make CFLAGS=<some extra flags, possibly none>'.
>
> Visibly this hasn't been tested.

Sorry about that.

There is one spot I miss after solving conflict of rebase master.

How about this one:

https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags-v3

Thanks

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck Nov. 5, 2017, 4:57 p.m. UTC | #3
On Sun, Nov 05, 2017 at 08:45:53AM +0800, Christopher Li wrote:
> On Wed, Nov 1, 2017 at 10:17 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> 
> > This is still not correct. It still doesn't allow to compile sparse
> > via 'make CFLAGS=<some extra flags, possibly none>'.
> >
> > Visibly this hasn't been tested.
> 
> Sorry about that.
> 
> There is one spot I miss after solving conflict of rebase master.

You rejected the patch, wrote your own version and you had to rebase it
and you had a conflict?
 
> How about this one:
> 
> https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags-v3

Much better!
Now you can give it a try at 'make CFLAGS=-O3 selfcheck'.


-- Luc Van Oostenryck 
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Nov. 9, 2017, 9:10 p.m. UTC | #4
On Mon, Nov 6, 2017 at 12:57 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Much better!
> Now you can give it a try at 'make CFLAGS=-O3 selfcheck'.
>
The selfcheck on sparse-llvm.sc is independent of the CFLAGS changes.
BTW, I never saw the brakage on FC 26. because "llvm-config --cflags" does not
add new search path.

Here is the V4:

https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/commit/?h=cflags-v4

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck Nov. 9, 2017, 9:26 p.m. UTC | #5
On Fri, Nov 10, 2017 at 05:10:48AM +0800, Christopher Li wrote:
> On Mon, Nov 6, 2017 at 12:57 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > Much better!
> > Now you can give it a try at 'make CFLAGS=-O3 selfcheck'.
> >
> The selfcheck on sparse-llvm.sc is independent of the CFLAGS changes.

On the contrary, it's very much at the core of the way the
different CFLAGS variable are used.

Everywhere you will use something like:
   <sometarget>.o: <some CFLAGS variant> += <something>
you will also need to add <sometarget>.sc
It's the way you solve the problem and it may seems to you 
independent of the CFLAGS changes and yet in my version of
the CFLAGS changes this bug was solved automatically by how
the CFLAGS were used (I discovered the bug while writting the
patch; you know this "ohoh, this can't possibly be correct").

But well, more than enough time have already spend on these CFLAGS.

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Nov. 9, 2017, 10:18 p.m. UTC | #6
On Fri, Nov 10, 2017 at 5:26 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>> The selfcheck on sparse-llvm.sc is independent of the CFLAGS changes.
>
> On the contrary, it's very much at the core of the way the
> different CFLAGS variable are used.

I mean the bug is triggerable even before the CFLAGS patch applied.

>
> Everywhere you will use something like:
>    <sometarget>.o: <some CFLAGS variant> += <something>
> you will also need to add <sometarget>.sc
> It's the way you solve the problem and it may seems to you
> independent of the CFLAGS changes and yet in my version of
> the CFLAGS changes this bug was solved automatically by how
> the CFLAGS were used (I discovered the bug while writting the
> patch; you know this "ohoh, this can't possibly be correct").

I see. You avoid using target specific variables. That might be
a good idea. I was looking for the smallest fix to so that
don't impact too much on the other Makefile changes.

I don't like to have CFLAGS += for every thing though.
It only works if the options always append from the tail.
If it is order sensitive, need to insert in the middle, then
you will need to have sub variable to group from any way,
like your CPPFLAGS.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck Nov. 9, 2017, 10:55 p.m. UTC | #7
On Thu, Nov 9, 2017 at 11:18 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Fri, Nov 10, 2017 at 5:26 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>>> The selfcheck on sparse-llvm.sc is independent of the CFLAGS changes.
>>
>> On the contrary, it's very much at the core of the way the
>> different CFLAGS variable are used.
>
> I mean the bug is triggerable even before the CFLAGS patch applied.
>
>>
>> Everywhere you will use something like:
>>    <sometarget>.o: <some CFLAGS variant> += <something>
>> you will also need to add <sometarget>.sc
>> It's the way you solve the problem and it may seems to you
>> independent of the CFLAGS changes and yet in my version of
>> the CFLAGS changes this bug was solved automatically by how
>> the CFLAGS were used (I discovered the bug while writting the
>> patch; you know this "ohoh, this can't possibly be correct").
>
> I see. You avoid using target specific variables. That might be
> a good idea. I was looking for the smallest fix to so that
> don't impact too much on the other Makefile changes.
>
> I don't like to have CFLAGS += for every thing though.
> It only works if the options always append from the tail.
> If it is order sensitive, need to insert in the middle, then
> you will need to have sub variable to group from any way,
> like your CPPFLAGS.

In the big makefile cleanup series, I've done slightly
differently but yes, no more target specific vars,
only <target>_CFLAGS and using 'cflags' for
flags private to sparse.
Aesthetically, it's not what I found the most pleasing
but this does well the job (SPARSE_CFLAGS, _CFLAGS,
C_FLAGS,  ... could have been used too).

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Makefile b/Makefile
index d0341764..66cb1ae1 100644
--- a/Makefile
+++ b/Makefile
@@ -12,8 +12,8 @@  OS = linux


 CC = gcc
-CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
-CFLAGS += -Wall -Wwrite-strings
+COMMON_CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
+COMMON_CFLAGS += -Wall -Wwrite-strings
 LDFLAGS += -g
 LD = gcc
 AR = ar
@@ -21,7 +21,7 @@  PKG_CONFIG = pkg-config
 CHECKER = ./cgcc -no-compile
 CHECKER_FLAGS =

-ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
+ALL_CFLAGS = $(COMMON_CFLAGS) $(PKG_CFLAGS) $(CFLAGS)
 #
 # For debugging, put this in local.mk:
 #
@@ -44,13 +44,13 @@  LLVM_CONFIG:=llvm-config
 HAVE_LLVM:=$(shell $(LLVM_CONFIG) --version >/dev/null 2>&1 && echo 'yes')

 GCC_BASE := $(shell $(CC) --print-file-name=)
-BASIC_CFLAGS = -DGCC_BASE=\"$(GCC_BASE)\"
+COMMON_CFLAGS += -DGCC_BASE=\"$(GCC_BASE)\"

 MULTIARCH_TRIPLET := $(shell $(CC) -print-multiarch 2>/dev/null)
-BASIC_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
+COMMON_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"

 ifeq ($(HAVE_GCC_DEP),yes)
-BASIC_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
+COMMON_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
 endif

 DESTDIR=
@@ -83,7 +83,7 @@  PROGRAMS += test-inspect
 INST_PROGRAMS += test-inspect
 test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
 test-inspect_OBJS := test-inspect.o $(test-inspect_EXTRA_DEPS)
-$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): CFLAGS += $(GTK_CFLAGS)
+$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): PKG_CFLAGS += $(GTK_CFLAGS)
 test-inspect_EXTRA_OBJS := $(GTK_LIBS)
 else
 $(warning Your system does not have gtk3/gtk2, disabling test-inspect)
@@ -101,7 +101,7 @@  LLVM_LIBS := $(shell $(LLVM_CONFIG) --libs)
 LLVM_LIBS += $(shell $(LLVM_CONFIG) --system-libs 2>/dev/null)
 PROGRAMS += $(LLVM_PROGS)
 INST_PROGRAMS += sparse-llvm sparsec
-sparse-llvm.o: BASIC_CFLAGS += $(LLVM_CFLAGS)
+sparse-llvm.o: PKG_CFLAGS += $(LLVM_CFLAGS)
 sparse-llvm_EXTRA_OBJS := $(LLVM_LIBS) $(LLVM_LDFLAGS)
 else
 $(warning LLVM 3.0 or later required. Your system has version