build: clean up $CFLAGS handling in the makefile

From: Jeff Layton <[email protected]>

The fedora packaging tools want to provide a base set of $CFLAGS when building packages, but that fails when building sparse.

There are a couple of build targets in the makefile that add options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to the compiler, and that ends up without those extra options, if CFLAGS= was passed to the make command.

This patch changes the code to append the options to $ALL_CFLAGS instead of $CFLAGS. At the same time, passing a CFLAGS argument to make ends up with the initial CFLAGS assignment being clobbered such that we lose the options that are assigned to it internally. Fold the internal usage of CFLAGS into BASIC_CFLAGS. They both just end up as part of ALL_CFLAGS anyway, so this should be functionally equivalent.

Cc Luc Van Oostenryck <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 Makefile | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

I sent this last week in a reply to a different thread. Re-posting it here since it got no replies.

Quoted 17 lines

From: Jeff Layton <[email protected]>

The fedora packaging tools want to provide a base set of $CFLAGS when building packages, but that fails when building sparse.

There are a couple of build targets in the makefile that add options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to the compiler, and that ends up without those extra options, if CFLAGS= was passed to the make command.

This patch changes the code to append the options to $ALL_CFLAGS instead of $CFLAGS. At the same time, passing a CFLAGS argument to make ends up with the initial CFLAGS assignment being clobbered such that we lose the options that are assigned to it internally. Fold the internal usage of CFLAGS into BASIC_CFLAGS. They both just end up as part of ALL_CFLAGS anyway, so this should be functionally equivalent.

Quoted 9 lines

Cc Luc Van Oostenryck <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 Makefile | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

I sent this last week in a reply to a different thread. Re-posting it here since it got no replies.

Hi,

I'm fine with this patch but I would prefer to go one step further and get rid of BASIC_CFLAGS/ALL_CFLAGS while at the same time being more explicit. I have in mind something like:

From 258efc92cff090bba5ea14a05819514779d7992a Mon Sep 17 00:00:00 2001
From: Luc Van Oostenryck <[email protected]>
Date: Wed, 25 Oct 2017 14:38:01 +0200
Subject: [PATCH] build: allow to override CFLAGS via the command line

Some distros (fedora) and some developers want to specifiy their own CFLAGS. OTOH most of CFLAGS's content is automatically contructed or must not be changed.

This patch, allow to extend CFLAGS via the command line, like:
make CFLAGS=-some-extra-flags

The patch also get rid of the confusing CFLAGS/BASIC_CFLAGS/ALL_CFLAGS

With this change, target specific CFLAGS can be given either via

        target.o: CFLAGS += -some-specific-flags

or via
target_CFLAGS = -some-specific-flags

Signed-off-by Luc Van Oostenryck <[email protected]>

Hi Luc and Jeff,

That get me curious how other package using CFLAGS in their make file generated by automake and cmake.

I have look at gcc, clang and a few others. My observation is that, it is up to the package to provide what flags to be override. There is no fix way to assign and override CFLAGS.

For example, This is one of the line I found in gcc, very typical:

$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(libz_a_CFLAGS) $(CFLAGS) -c

The CFLAGS variable does not contain every thing.
Also explicit overriding the CFLAGS in Makefile is rare. There are some example doing that. But there is a lot of Makefile don't explicit override CFLAGS at all.

I also found case that CFLAGS was detect and assigned by ./configure as part of @CFLAGS@ template. For those case, it is very easy to mistakenly over written CFLAGS set by ./configure.

For cmake. I have seen some package using C_FLAGS and clang is using CXX.Flags.

Over all I think having ALL_CFLAGS is fine.

BASIC_CFLAGS might be a bad name.
Our current usage case, we can divide that into three groups. I still want some name space for CFLAGS instead of adding every thing to CFLAGS.

ALL_CFLAGS = $(CFLAGS) $(PKG_CFLAGS) $(COMMON_CFLAGS)

COMMON_CFLAGS is the CFLAGS shared by all objects. It serve as the base line.

PKG_CFLAGS is the CFLAGS introduce by external packages. like LLVM, libxml, gtk etc.

CFLAGS just for others to assign from the command line.

That should handle all the CFLAGS we have in sparse right now.

Chris

Quoted 53 lines · Luc Van Oostenryck

Quoted 17 lines

From: Jeff Layton <[email protected]>

The fedora packaging tools want to provide a base set of $CFLAGS when building packages, but that fails when building sparse.

There are a couple of build targets in the makefile that add options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to the compiler, and that ends up without those extra options, if CFLAGS= was passed to the make command.

This patch changes the code to append the options to $ALL_CFLAGS instead of $CFLAGS. At the same time, passing a CFLAGS argument to make ends up with the initial CFLAGS assignment being clobbered such that we lose the options that are assigned to it internally. Fold the internal usage of CFLAGS into BASIC_CFLAGS. They both just end up as part of ALL_CFLAGS anyway, so this should be functionally equivalent.

Quoted 9 lines

Cc Luc Van Oostenryck <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 Makefile | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

I sent this last week in a reply to a different thread. Re-posting it here since it got no replies.

Hi,

I'm fine with this patch but I would prefer to go one step further and get rid of BASIC_CFLAGS/ALL_CFLAGS while at the same time being more explicit. I have in mind something like:

From 258efc92cff090bba5ea14a05819514779d7992a Mon Sep 17 00:00:00 2001
From: Luc Van Oostenryck <[email protected]>
Date: Wed, 25 Oct 2017 14:38:01 +0200
Subject: [PATCH] build: allow to override CFLAGS via the command line

Some distros (fedora) and some developers want to specifiy their own CFLAGS. OTOH most of CFLAGS's content is automatically contructed or must not be changed.

This patch, allow to extend CFLAGS via the command line, like:
make CFLAGS=-some-extra-flags

The patch also get rid of the confusing CFLAGS/BASIC_CFLAGS/ALL_CFLAGS

With this change, target specific CFLAGS can be given either via

        target.o: CFLAGS += -some-specific-flags

or via
target_CFLAGS = -some-specific-flags

Signed-off-by Luc Van Oostenryck <[email protected]>

It's not clear to me what Chris is suggesting below. It sounds sort of like you're OK with the patch I originally suggested, but want different variable names?

Perhaps instead of describing it, you could roll up an alternative patch that shows what you would prefer to see here?

Perhaps instead of describing it, you could roll up an alternative patch that shows what you would prefer to see here?

Quoted 3 lines · Jeff Layton

Perhaps instead of describing it, you could roll up an alternative patch that shows what you would prefer to see here?

Here it is the patch.

I push it on https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags

Chris

From 9fc170ec5e5814ae858922496dea668bb80dbf34 Mon Sep 17 00:00:00 2001
From: Christopher Li <[email protected]>
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.

The goal of the initial patch, as stated by Jeff, was:
"The fedora packaging tools want to *override* $CFLAGS when building sparse"

This version doesn't allow to override CFLAGS. See what happens with 'make CFLAGS=-O3' for example.

Quoted 5 lines · Luc Van Oostenryck

The goal of the initial patch, as stated by Jeff, was:
"The fedora packaging tools want to *override* $CFLAGS when building sparse"

This version doesn't allow to override CFLAGS. See what happens with 'make CFLAGS=-O3' for example.

The original patch proposed by Jeff does not allow usage of "make CFLAGS=-O3" either. I take a look at your patch, it behave the same way in this regard. Do you mean I should change the commit title to *append* CFLAGS instead of override?

May be Jeff can clarify the intention?

BTW, I am actually leaning toward just expose some thing like CLI_CFLAGS to be override by package builders. I am not that convince override CFLAGS is established stander practices from the project that I have look at.

On the other hand, it is trivial to avoid using CFLAGS in sparse Makefile. If Jeff really want CFLAGS instead of CLI_CFLAGS, it is not a big a deal.

make CFLAGS='-arbitrary -flags' is quite common, and people generally expect it to work, though it isn't *shocking* when it breaks. Though the exact boundary for what should get overridden by that and what should remain seems a bit fuzzy. Roughly speaking, if the application normally wants to build with "-O2 -ffooish-bar -Wall -Wanother -I/some/path" , then make CFLAGS='-Os -arbitrary -flags' should build with "-Os -arbitrary -flags -Wall -Wanother -Isome-path`. I think.

I think it's worth not putting anything into CFLAGS that would break the build if missing.

For instance, git's Makefile specifically says "# CFLAGS and LDFLAGS are for the users to override from the command line.", and then just has "CFLAGS = -g -O2 -Wall". It then has additional variables like

Quoted 3 lines · Luc Van Oostenryck

The goal of the initial patch, as stated by Jeff, was:
"The fedora packaging tools want to *override* $CFLAGS when building sparse"

Quoted 2 lines

This version doesn't allow to override CFLAGS. See what happens with 'make CFLAGS=-O3' for example.

The original patch proposed by Jeff does not allow usage of "make CFLAGS=-O3" either. I take a look at your patch, it behave the same way in this regard. Do you mean I should change the commit title to *append* CFLAGS instead of override?

Since for flags like -O[0123] but also for all flags like -f[no-]xxx and all sort of others settings in GCC and in a multitude of others programs it's the last value that matters, appending to CFLAGS *is* the usual way to *override* previous settings.
Jeff's patch allowed that, mine too but yours not.

Quoted 3 lines · Jeff Layton

Perhaps instead of describing it, you could roll up an alternative patch that shows what you would prefer to see here?

Here it is the patch.

I push it on https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags

Chris

From 9fc170ec5e5814ae858922496dea668bb80dbf34 Mon Sep 17 00:00:00 2001
From: Christopher Li <[email protected]>
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 <[email protected]>

---
 Makefile | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Makefile +0 -1

index 76902b7..ef47358 100644
@@ -12,14 +12,14 @@ OS = linux
 
 
 CC = gcc
-CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g

Makefile +5 -5

 #
 # For debugging, put this in local.mk:
 #

@@ -35,13 +35,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=

@@ -72,7 +72,7 @@ GTK2_LIBS := $(shell $(PKG_CONFIG) --libs gtk+-2.0)
 PROGRAMS += test-inspect
 INST_PROGRAMS += test-inspect
 test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
-test-inspect.o $(test-inspect_EXTRA_DEPS): BASIC_CFLAGS += $(GTK2_CFLAGS)
+test-inspect.o $(test-inspect_EXTRA_DEPS): PKG_CFLAGS += $(GTK2_CFLAGS)
 test-inspect_EXTRA_OBJS := $(GTK2_LIBS)
 else
 $(warning Your system does not have libgtk2, disabling test-inspect)

@@ -89,7 +89,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

$(LLVM_VERSION) installed.)

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:

Quoted 5 lines · Jeff Layton

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.

Quoted 4 lines · Luc Van Oostenryck

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

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

Quoted 3 lines · Luc Van Oostenryck

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.

Quoted 1 line

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").

Quoted 4 lines · Luc Van Oostenryck

Quoted 1 line

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.

Quoted 9 lines

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.