diff mbox series

[fsverity-utils] override CFLAGS too

Message ID 20201026204831.3337360-1-luca.boccassi@gmail.com (mailing list archive)
State Superseded
Headers show
Series [fsverity-utils] override CFLAGS too | expand

Commit Message

Luca Boccassi Oct. 26, 2020, 8:48 p.m. UTC 
From: Romain Perier <romain.perier@gmail.com>

Currently, CFLAGS are defined by default. It has to effect to define its
c-compiler options only when the variable is not defined on the cmdline
by the user, it is not possible to merge or mix both, while it could
be interesting for using the app warning cflags or the pkg-config
cflags, while using the distributor flags. Most distributions packages
use their own compilation flags, typically for hardening purpose but not
only. This fixes the issue by using the override keyword.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
Currently used in Debian, were we want to append context-specific
compiler flags (eg: for compiler hardening options) without
removing the default flags

 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Eric Biggers Oct. 26, 2020, 10:10 p.m. UTC | #1 
[+Jes Sorensen]

On Mon, Oct 26, 2020 at 08:48:31PM +0000, luca.boccassi@gmail.com wrote:
> From: Romain Perier <romain.perier@gmail.com>
> 
> Currently, CFLAGS are defined by default. It has to effect to define its
> c-compiler options only when the variable is not defined on the cmdline
> by the user, it is not possible to merge or mix both, while it could
> be interesting for using the app warning cflags or the pkg-config
> cflags, while using the distributor flags. Most distributions packages
> use their own compilation flags, typically for hardening purpose but not
> only. This fixes the issue by using the override keyword.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
> Currently used in Debian, were we want to append context-specific
> compiler flags (eg: for compiler hardening options) without
> removing the default flags
> 
>  Makefile | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 6c6c8c9..5020cac 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -35,14 +35,15 @@
>  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
>  	      then echo $(1); fi)
>  
> -CFLAGS ?= -O2 -Wall -Wundef					\
> +override CFLAGS := -O2 -Wall -Wundef				\
>  	$(call cc-option,-Wdeclaration-after-statement)		\
>  	$(call cc-option,-Wimplicit-fallthrough)		\
>  	$(call cc-option,-Wmissing-field-initializers)		\
>  	$(call cc-option,-Wmissing-prototypes)			\
>  	$(call cc-option,-Wstrict-prototypes)			\
>  	$(call cc-option,-Wunused-parameter)			\
> -	$(call cc-option,-Wvla)
> +	$(call cc-option,-Wvla)					\
> +	$(CFLAGS)
>  
>  override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)

I had it like this originally, but Jes requested that it be changed to the
current way for rpm packaging.  See the thread:
https://lkml.kernel.org/linux-fscrypt/20200515205649.1670512-3-Jes.Sorensen@gmail.com/T/#u

Can we come to an agreement on one way to do it?

To me, the approach with 'override' makes more sense.  The only non-warning
option is -O2, and if someone doesn't want that, they can just specify
CFLAGS=-O0 and it will override -O2 (since the last option "wins").

Jes, can you explain why that way doesn't work with rpm?

- Eric
Luca Boccassi Oct. 27, 2020, 10:30 a.m. UTC | #2 
On Mon, 2020-10-26 at 15:10 -0700, Eric Biggers wrote:
> [+Jes Sorensen]
> 
> On Mon, Oct 26, 2020 at 08:48:31PM +0000, luca.boccassi@gmail.com wrote:
> > From: Romain Perier <romain.perier@gmail.com>
> > 
> > Currently, CFLAGS are defined by default. It has to effect to define its
> > c-compiler options only when the variable is not defined on the cmdline
> > by the user, it is not possible to merge or mix both, while it could
> > be interesting for using the app warning cflags or the pkg-config
> > cflags, while using the distributor flags. Most distributions packages
> > use their own compilation flags, typically for hardening purpose but not
> > only. This fixes the issue by using the override keyword.
> > 
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> > Currently used in Debian, were we want to append context-specific
> > compiler flags (eg: for compiler hardening options) without
> > removing the default flags
> > 
> >  Makefile | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 6c6c8c9..5020cac 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -35,14 +35,15 @@
> >  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
> >  	      then echo $(1); fi)
> >  
> > -CFLAGS ?= -O2 -Wall -Wundef					\
> > +override CFLAGS := -O2 -Wall -Wundef				\
> >  	$(call cc-option,-Wdeclaration-after-statement)		\
> >  	$(call cc-option,-Wimplicit-fallthrough)		\
> >  	$(call cc-option,-Wmissing-field-initializers)		\
> >  	$(call cc-option,-Wmissing-prototypes)			\
> >  	$(call cc-option,-Wstrict-prototypes)			\
> >  	$(call cc-option,-Wunused-parameter)			\
> > -	$(call cc-option,-Wvla)
> > +	$(call cc-option,-Wvla)					\
> > +	$(CFLAGS)
> >  
> >  override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
> 
> I had it like this originally, but Jes requested that it be changed to the
> current way for rpm packaging.  See the thread:
> https://lkml.kernel.org/linux-fscrypt/20200515205649.1670512-3-Jes.Sorensen@gmail.com/T/#u
> 
> Can we come to an agreement on one way to do it?
> 
> To me, the approach with 'override' makes more sense.  The only non-warning
> option is -O2, and if someone doesn't want that, they can just specify
> CFLAGS=-O0 and it will override -O2 (since the last option "wins").
> 
> Jes, can you explain why that way doesn't work with rpm?

I see - I'm pretty sure what we want to override is the optimization
flag (and any other flag that affect the binary, eg: debugging
symbols), but not the other flags that you set (eg: warnings).

So, how about this v2:

From b48d09b1868cfa50b2cd61eec2951f722943e421 Mon Sep 17 00:00:00 2001
From: Romain Perier <romain.perier@gmail.com>
Date: Sun, 19 Apr 2020 09:24:09 +0200
Subject: [PATCH] override CFLAGS too

Currently, CFLAGS are defined by default. It has to effect to define its
c-compiler options only when the variable is not defined on the cmdline
by the user, it is not possible to merge or mix both, while it could
be interesting for using the app warning cflags or the pkg-config
cflags, while using the distributor flags. Most distributions packages
use their own compilation flags, typically for hardening purpose but not
only. This fixes the issue by using the override keyword.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 Makefile | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 6c6c8c9..82abfe9 100644
--- a/Makefile
+++ b/Makefile
@@ -35,14 +35,19 @@
 cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
 	      then echo $(1); fi)
 
-CFLAGS ?= -O2 -Wall -Wundef					\
+# Flags that callers can override
+CFLAGS ?= -O2
+
+# Flags that we always want to use
+INTERNAL_CFLAGS := -Wall -Wundef				\
 	$(call cc-option,-Wdeclaration-after-statement)		\
 	$(call cc-option,-Wimplicit-fallthrough)		\
 	$(call cc-option,-Wmissing-field-initializers)		\
 	$(call cc-option,-Wmissing-prototypes)			\
 	$(call cc-option,-Wstrict-prototypes)			\
 	$(call cc-option,-Wunused-parameter)			\
-	$(call cc-option,-Wvla)
+	$(call cc-option,-Wvla)					\
+	$(CFLAGS)
 
 override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
 
@@ -65,7 +70,7 @@ PKGCONF         ?= pkg-config
 .build-config: FORCE
 	@flags=$$(							\
 		echo 'CC=$(CC)';					\
-		echo 'CFLAGS=$(CFLAGS)';				\
+		echo 'CFLAGS=$(INTERNAL_CFLAGS)';			\
 		echo 'CPPFLAGS=$(CPPFLAGS)';				\
 		echo 'LDFLAGS=$(LDFLAGS)';				\
 		echo 'LDLIBS=$(LDLIBS)';				\
@@ -98,7 +103,7 @@ endif
 #### Library
 
 SOVERSION       := 0
-LIB_CFLAGS      := $(CFLAGS) -fvisibility=hidden
+LIB_CFLAGS      := $(INTERNAL_CFLAGS) -fvisibility=hidden
 LIB_SRC         := $(wildcard lib/*.c)
 LIB_HEADERS     := $(wildcard lib/*.h) $(COMMON_HEADERS)
 STATIC_LIB_OBJ  := $(LIB_SRC:.c=.o)
@@ -121,7 +126,7 @@ DEFAULT_TARGETS += libfsverity.a
 # Create shared library
 libfsverity.so.$(SOVERSION):$(SHARED_LIB_OBJ)
 	$(QUIET_CCLD) $(CC) -o $@ -Wl,-soname=$@ -shared $+ \
-		$(CFLAGS) $(LDFLAGS) $(LDLIBS)
+		$(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
 
 DEFAULT_TARGETS += libfsverity.so.$(SOVERSION)
 
@@ -160,23 +165,23 @@ TEST_PROGRAMS     := $(TEST_PROG_SRC:programs/%.c=%)
 
 # Compile program object files
 $(ALL_PROG_OBJ): %.o: %.c $(ALL_PROG_HEADERS) .build-config
-	$(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(CFLAGS) $<
+	$(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
 
 # Link the fsverity program
 ifdef USE_SHARED_LIB
 fsverity: $(FSVERITY_PROG_OBJ) libfsverity.so
 	$(QUIET_CCLD) $(CC) -o $@ $(FSVERITY_PROG_OBJ) \
-		$(CFLAGS) $(LDFLAGS) -L. -lfsverity
+		$(INTERNAL_CFLAGS) $(LDFLAGS) -L. -lfsverity
 else
 fsverity: $(FSVERITY_PROG_OBJ) libfsverity.a
-	$(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS)
+	$(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
 endif
 
 DEFAULT_TARGETS += fsverity
 
 # Link the test programs
 $(TEST_PROGRAMS): %: programs/%.o $(PROG_COMMON_OBJ) libfsverity.a
-	$(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS)
+	$(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
 
 ##############################################################################
Jes Sorensen Oct. 28, 2020, 4:47 p.m. UTC | #3 
On 10/26/20 6:10 PM, Eric Biggers wrote:
> [+Jes Sorensen]
> 
> On Mon, Oct 26, 2020 at 08:48:31PM +0000, luca.boccassi@gmail.com wrote:
>> From: Romain Perier <romain.perier@gmail.com>
>>
>> Currently, CFLAGS are defined by default. It has to effect to define its
>> c-compiler options only when the variable is not defined on the cmdline
>> by the user, it is not possible to merge or mix both, while it could
>> be interesting for using the app warning cflags or the pkg-config
>> cflags, while using the distributor flags. Most distributions packages
>> use their own compilation flags, typically for hardening purpose but not
>> only. This fixes the issue by using the override keyword.
>>
>> Signed-off-by: Romain Perier <romain.perier@gmail.com>
>> ---
>> Currently used in Debian, were we want to append context-specific
>> compiler flags (eg: for compiler hardening options) without
>> removing the default flags
>>
>>  Makefile | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 6c6c8c9..5020cac 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -35,14 +35,15 @@
>>  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
>>  	      then echo $(1); fi)
>>  
>> -CFLAGS ?= -O2 -Wall -Wundef					\
>> +override CFLAGS := -O2 -Wall -Wundef				\
>>  	$(call cc-option,-Wdeclaration-after-statement)		\
>>  	$(call cc-option,-Wimplicit-fallthrough)		\
>>  	$(call cc-option,-Wmissing-field-initializers)		\
>>  	$(call cc-option,-Wmissing-prototypes)			\
>>  	$(call cc-option,-Wstrict-prototypes)			\
>>  	$(call cc-option,-Wunused-parameter)			\
>> -	$(call cc-option,-Wvla)
>> +	$(call cc-option,-Wvla)					\
>> +	$(CFLAGS)
>>  
>>  override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
> 
> I had it like this originally, but Jes requested that it be changed to the
> current way for rpm packaging.  See the thread:
> https://lkml.kernel.org/linux-fscrypt/20200515205649.1670512-3-Jes.Sorensen@gmail.com/T/#u
> 
> Can we come to an agreement on one way to do it?
> 
> To me, the approach with 'override' makes more sense.  The only non-warning
> option is -O2, and if someone doesn't want that, they can just specify
> CFLAGS=-O0 and it will override -O2 (since the last option "wins").
> 
> Jes, can you explain why that way doesn't work with rpm?

I don't remember all the details and I haven't looked at this in a
while. Matthew Almond has helpfully offered to look into it.

Jes
Eric Biggers Oct. 28, 2020, 5:17 p.m. UTC | #4 
On Tue, Oct 27, 2020 at 10:30:20AM +0000, Luca Boccassi wrote:
> On Mon, 2020-10-26 at 15:10 -0700, Eric Biggers wrote:
> > [+Jes Sorensen]
> > 
> > On Mon, Oct 26, 2020 at 08:48:31PM +0000, luca.boccassi@gmail.com wrote:
> > > From: Romain Perier <romain.perier@gmail.com>
> > > 
> > > Currently, CFLAGS are defined by default. It has to effect to define its
> > > c-compiler options only when the variable is not defined on the cmdline
> > > by the user, it is not possible to merge or mix both, while it could
> > > be interesting for using the app warning cflags or the pkg-config
> > > cflags, while using the distributor flags. Most distributions packages
> > > use their own compilation flags, typically for hardening purpose but not
> > > only. This fixes the issue by using the override keyword.
> > > 
> > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > > ---
> > > Currently used in Debian, were we want to append context-specific
> > > compiler flags (eg: for compiler hardening options) without
> > > removing the default flags
> > > 
> > >  Makefile | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 6c6c8c9..5020cac 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -35,14 +35,15 @@
> > >  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
> > >  	      then echo $(1); fi)
> > >  
> > > -CFLAGS ?= -O2 -Wall -Wundef					\
> > > +override CFLAGS := -O2 -Wall -Wundef				\
> > >  	$(call cc-option,-Wdeclaration-after-statement)		\
> > >  	$(call cc-option,-Wimplicit-fallthrough)		\
> > >  	$(call cc-option,-Wmissing-field-initializers)		\
> > >  	$(call cc-option,-Wmissing-prototypes)			\
> > >  	$(call cc-option,-Wstrict-prototypes)			\
> > >  	$(call cc-option,-Wunused-parameter)			\
> > > -	$(call cc-option,-Wvla)
> > > +	$(call cc-option,-Wvla)					\
> > > +	$(CFLAGS)
> > >  
> > >  override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
> > 
> > I had it like this originally, but Jes requested that it be changed to the
> > current way for rpm packaging.  See the thread:
> > https://lkml.kernel.org/linux-fscrypt/20200515205649.1670512-3-Jes.Sorensen@gmail.com/T/#u
> > 
> > Can we come to an agreement on one way to do it?
> > 
> > To me, the approach with 'override' makes more sense.  The only non-warning
> > option is -O2, and if someone doesn't want that, they can just specify
> > CFLAGS=-O0 and it will override -O2 (since the last option "wins").
> > 
> > Jes, can you explain why that way doesn't work with rpm?
> 
> I see - I'm pretty sure what we want to override is the optimization
> flag (and any other flag that affect the binary, eg: debugging
> symbols), but not the other flags that you set (eg: warnings).
> 
> So, how about this v2:
> 
> From b48d09b1868cfa50b2cd61eec2951f722943e421 Mon Sep 17 00:00:00 2001
> From: Romain Perier <romain.perier@gmail.com>
> Date: Sun, 19 Apr 2020 09:24:09 +0200
> Subject: [PATCH] override CFLAGS too
> 
> Currently, CFLAGS are defined by default. It has to effect to define its
> c-compiler options only when the variable is not defined on the cmdline
> by the user, it is not possible to merge or mix both, while it could
> be interesting for using the app warning cflags or the pkg-config
> cflags, while using the distributor flags. Most distributions packages
> use their own compilation flags, typically for hardening purpose but not
> only. This fixes the issue by using the override keyword.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  Makefile | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 6c6c8c9..82abfe9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -35,14 +35,19 @@
>  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
>  	      then echo $(1); fi)
>  
> -CFLAGS ?= -O2 -Wall -Wundef					\
> +# Flags that callers can override
> +CFLAGS ?= -O2
> +
> +# Flags that we always want to use
> +INTERNAL_CFLAGS := -Wall -Wundef				\
>  	$(call cc-option,-Wdeclaration-after-statement)		\
>  	$(call cc-option,-Wimplicit-fallthrough)		\
>  	$(call cc-option,-Wmissing-field-initializers)		\
>  	$(call cc-option,-Wmissing-prototypes)			\
>  	$(call cc-option,-Wstrict-prototypes)			\
>  	$(call cc-option,-Wunused-parameter)			\
> -	$(call cc-option,-Wvla)
> +	$(call cc-option,-Wvla)					\
> +	$(CFLAGS)
>  
>  override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
>  
> @@ -65,7 +70,7 @@ PKGCONF         ?= pkg-config
>  .build-config: FORCE
>  	@flags=$$(							\
>  		echo 'CC=$(CC)';					\
> -		echo 'CFLAGS=$(CFLAGS)';				\
> +		echo 'CFLAGS=$(INTERNAL_CFLAGS)';			\
>  		echo 'CPPFLAGS=$(CPPFLAGS)';				\
>  		echo 'LDFLAGS=$(LDFLAGS)';				\
>  		echo 'LDLIBS=$(LDLIBS)';				\
> @@ -98,7 +103,7 @@ endif
>  #### Library
>  
>  SOVERSION       := 0
> -LIB_CFLAGS      := $(CFLAGS) -fvisibility=hidden
> +LIB_CFLAGS      := $(INTERNAL_CFLAGS) -fvisibility=hidden
>  LIB_SRC         := $(wildcard lib/*.c)
>  LIB_HEADERS     := $(wildcard lib/*.h) $(COMMON_HEADERS)
>  STATIC_LIB_OBJ  := $(LIB_SRC:.c=.o)
> @@ -121,7 +126,7 @@ DEFAULT_TARGETS += libfsverity.a
>  # Create shared library
>  libfsverity.so.$(SOVERSION):$(SHARED_LIB_OBJ)
>  	$(QUIET_CCLD) $(CC) -o $@ -Wl,-soname=$@ -shared $+ \
> -		$(CFLAGS) $(LDFLAGS) $(LDLIBS)
> +		$(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
>  
>  DEFAULT_TARGETS += libfsverity.so.$(SOVERSION)
>  
> @@ -160,23 +165,23 @@ TEST_PROGRAMS     := $(TEST_PROG_SRC:programs/%.c=%)
>  
>  # Compile program object files
>  $(ALL_PROG_OBJ): %.o: %.c $(ALL_PROG_HEADERS) .build-config
> -	$(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(CFLAGS) $<
> +	$(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
>  
>  # Link the fsverity program
>  ifdef USE_SHARED_LIB
>  fsverity: $(FSVERITY_PROG_OBJ) libfsverity.so
>  	$(QUIET_CCLD) $(CC) -o $@ $(FSVERITY_PROG_OBJ) \
> -		$(CFLAGS) $(LDFLAGS) -L. -lfsverity
> +		$(INTERNAL_CFLAGS) $(LDFLAGS) -L. -lfsverity
>  else
>  fsverity: $(FSVERITY_PROG_OBJ) libfsverity.a
> -	$(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS)
> +	$(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
>  endif
>  
>  DEFAULT_TARGETS += fsverity
>  
>  # Link the test programs
>  $(TEST_PROGRAMS): %: programs/%.o $(PROG_COMMON_OBJ) libfsverity.a
> -	$(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS)
> +	$(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
>  
>  ##############################################################################

I think the following accomplishes the same thing much more easily:

diff --git a/Makefile b/Makefile
index 6c6c8c9..cff8d36 100644
--- a/Makefile
+++ b/Makefile
@@ -35,14 +35,17 @@
 cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
 	      then echo $(1); fi)
 
-CFLAGS ?= -O2 -Wall -Wundef					\
+CFLAGS ?= -O2
+
+override CFLAGS := -Wall -Wundef				\
 	$(call cc-option,-Wdeclaration-after-statement)		\
 	$(call cc-option,-Wimplicit-fallthrough)		\
 	$(call cc-option,-Wmissing-field-initializers)		\
 	$(call cc-option,-Wmissing-prototypes)			\
 	$(call cc-option,-Wstrict-prototypes)			\
 	$(call cc-option,-Wunused-parameter)			\
-	$(call cc-option,-Wvla)
+	$(call cc-option,-Wvla)					\
+	$(CFLAGS)
Luca Boccassi Oct. 28, 2020, 5:27 p.m. UTC | #5 
On Wed, 2020-10-28 at 10:17 -0700, Eric Biggers wrote:
> On Tue, Oct 27, 2020 at 10:30:20AM +0000, Luca Boccassi wrote:
> > On Mon, 2020-10-26 at 15:10 -0700, Eric Biggers wrote:
> > > [+Jes Sorensen]
> > > 
> > > On Mon, Oct 26, 2020 at 08:48:31PM +0000, luca.boccassi@gmail.com wrote:
> > > > From: Romain Perier <romain.perier@gmail.com>
> > > > 
> > > > Currently, CFLAGS are defined by default. It has to effect to define its
> > > > c-compiler options only when the variable is not defined on the cmdline
> > > > by the user, it is not possible to merge or mix both, while it could
> > > > be interesting for using the app warning cflags or the pkg-config
> > > > cflags, while using the distributor flags. Most distributions packages
> > > > use their own compilation flags, typically for hardening purpose but not
> > > > only. This fixes the issue by using the override keyword.
> > > > 
> > > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > > > ---
> > > > Currently used in Debian, were we want to append context-specific
> > > > compiler flags (eg: for compiler hardening options) without
> > > > removing the default flags
> > > > 
> > > >  Makefile | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/Makefile b/Makefile
> > > > index 6c6c8c9..5020cac 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -35,14 +35,15 @@
> > > >  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
> > > >  	      then echo $(1); fi)
> > > >  
> > > > -CFLAGS ?= -O2 -Wall -Wundef					\
> > > > +override CFLAGS := -O2 -Wall -Wundef				\
> > > >  	$(call cc-option,-Wdeclaration-after-statement)		\
> > > >  	$(call cc-option,-Wimplicit-fallthrough)		\
> > > >  	$(call cc-option,-Wmissing-field-initializers)		\
> > > >  	$(call cc-option,-Wmissing-prototypes)			\
> > > >  	$(call cc-option,-Wstrict-prototypes)			\
> > > >  	$(call cc-option,-Wunused-parameter)			\
> > > > -	$(call cc-option,-Wvla)
> > > > +	$(call cc-option,-Wvla)					\
> > > > +	$(CFLAGS)
> > > >  
> > > >  override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
> > > 
> > > I had it like this originally, but Jes requested that it be changed to the
> > > current way for rpm packaging.  See the thread:
> > > https://lkml.kernel.org/linux-fscrypt/20200515205649.1670512-3-Jes.Sorensen@gmail.com/T/#u
> > > 
> > > Can we come to an agreement on one way to do it?
> > > 
> > > To me, the approach with 'override' makes more sense.  The only non-warning
> > > option is -O2, and if someone doesn't want that, they can just specify
> > > CFLAGS=-O0 and it will override -O2 (since the last option "wins").
> > > 
> > > Jes, can you explain why that way doesn't work with rpm?
> > 
> > I see - I'm pretty sure what we want to override is the optimization
> > flag (and any other flag that affect the binary, eg: debugging
> > symbols), but not the other flags that you set (eg: warnings).
> > 
> > So, how about this v2:
> > 
> > From b48d09b1868cfa50b2cd61eec2951f722943e421 Mon Sep 17 00:00:00 2001
> > From: Romain Perier <romain.perier@gmail.com>
> > Date: Sun, 19 Apr 2020 09:24:09 +0200
> > Subject: [PATCH] override CFLAGS too
> > 
> > Currently, CFLAGS are defined by default. It has to effect to define its
> > c-compiler options only when the variable is not defined on the cmdline
> > by the user, it is not possible to merge or mix both, while it could
> > be interesting for using the app warning cflags or the pkg-config
> > cflags, while using the distributor flags. Most distributions packages
> > use their own compilation flags, typically for hardening purpose but not
> > only. This fixes the issue by using the override keyword.
> > 
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> >  Makefile | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 6c6c8c9..82abfe9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -35,14 +35,19 @@
> >  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
> >  	      then echo $(1); fi)
> >  
> > -CFLAGS ?= -O2 -Wall -Wundef					\
> > +# Flags that callers can override
> > +CFLAGS ?= -O2
> > +
> > +# Flags that we always want to use
> > +INTERNAL_CFLAGS := -Wall -Wundef				\
> >  	$(call cc-option,-Wdeclaration-after-statement)		\
> >  	$(call cc-option,-Wimplicit-fallthrough)		\
> >  	$(call cc-option,-Wmissing-field-initializers)		\
> >  	$(call cc-option,-Wmissing-prototypes)			\
> >  	$(call cc-option,-Wstrict-prototypes)			\
> >  	$(call cc-option,-Wunused-parameter)			\
> > -	$(call cc-option,-Wvla)
> > +	$(call cc-option,-Wvla)					\
> > +	$(CFLAGS)
> >  
> >  override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
> >  
> > @@ -65,7 +70,7 @@ PKGCONF         ?= pkg-config
> >  .build-config: FORCE
> >  	@flags=$$(							\
> >  		echo 'CC=$(CC)';					\
> > -		echo 'CFLAGS=$(CFLAGS)';				\
> > +		echo 'CFLAGS=$(INTERNAL_CFLAGS)';			\
> >  		echo 'CPPFLAGS=$(CPPFLAGS)';				\
> >  		echo 'LDFLAGS=$(LDFLAGS)';				\
> >  		echo 'LDLIBS=$(LDLIBS)';				\
> > @@ -98,7 +103,7 @@ endif
> >  #### Library
> >  
> >  SOVERSION       := 0
> > -LIB_CFLAGS      := $(CFLAGS) -fvisibility=hidden
> > +LIB_CFLAGS      := $(INTERNAL_CFLAGS) -fvisibility=hidden
> >  LIB_SRC         := $(wildcard lib/*.c)
> >  LIB_HEADERS     := $(wildcard lib/*.h) $(COMMON_HEADERS)
> >  STATIC_LIB_OBJ  := $(LIB_SRC:.c=.o)
> > @@ -121,7 +126,7 @@ DEFAULT_TARGETS += libfsverity.a
> >  # Create shared library
> >  libfsverity.so.$(SOVERSION):$(SHARED_LIB_OBJ)
> >  	$(QUIET_CCLD) $(CC) -o $@ -Wl,-soname=$@ -shared $+ \
> > -		$(CFLAGS) $(LDFLAGS) $(LDLIBS)
> > +		$(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
> >  
> >  DEFAULT_TARGETS += libfsverity.so.$(SOVERSION)
> >  
> > @@ -160,23 +165,23 @@ TEST_PROGRAMS     := $(TEST_PROG_SRC:programs/%.c=%)
> >  
> >  # Compile program object files
> >  $(ALL_PROG_OBJ): %.o: %.c $(ALL_PROG_HEADERS) .build-config
> > -	$(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(CFLAGS) $<
> > +	$(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
> >  
> >  # Link the fsverity program
> >  ifdef USE_SHARED_LIB
> >  fsverity: $(FSVERITY_PROG_OBJ) libfsverity.so
> >  	$(QUIET_CCLD) $(CC) -o $@ $(FSVERITY_PROG_OBJ) \
> > -		$(CFLAGS) $(LDFLAGS) -L. -lfsverity
> > +		$(INTERNAL_CFLAGS) $(LDFLAGS) -L. -lfsverity
> >  else
> >  fsverity: $(FSVERITY_PROG_OBJ) libfsverity.a
> > -	$(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS)
> > +	$(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
> >  endif
> >  
> >  DEFAULT_TARGETS += fsverity
> >  
> >  # Link the test programs
> >  $(TEST_PROGRAMS): %: programs/%.o $(PROG_COMMON_OBJ) libfsverity.a
> > -	$(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS)
> > +	$(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS)
> >  
> >  ##############################################################################
> 
> I think the following accomplishes the same thing much more easily:
> 
> diff --git a/Makefile b/Makefile
> index 6c6c8c9..cff8d36 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -35,14 +35,17 @@
>  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
>  	      then echo $(1); fi)
>  
> -CFLAGS ?= -O2 -Wall -Wundef					\
> +CFLAGS ?= -O2
> +
> +override CFLAGS := -Wall -Wundef				\
>  	$(call cc-option,-Wdeclaration-after-statement)		\
>  	$(call cc-option,-Wimplicit-fallthrough)		\
>  	$(call cc-option,-Wmissing-field-initializers)		\
>  	$(call cc-option,-Wmissing-prototypes)			\
>  	$(call cc-option,-Wstrict-prototypes)			\
>  	$(call cc-option,-Wunused-parameter)			\
> -	$(call cc-option,-Wvla)
> +	$(call cc-option,-Wvla)					\
> +	$(CFLAGS)

It does indeed, that works for me, thanks.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 6c6c8c9..5020cac 100644
--- a/Makefile
+++ b/Makefile
@@ -35,14 +35,15 @@ 
 cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \
 	      then echo $(1); fi)
 
-CFLAGS ?= -O2 -Wall -Wundef					\
+override CFLAGS := -O2 -Wall -Wundef				\
 	$(call cc-option,-Wdeclaration-after-statement)		\
 	$(call cc-option,-Wimplicit-fallthrough)		\
 	$(call cc-option,-Wmissing-field-initializers)		\
 	$(call cc-option,-Wmissing-prototypes)			\
 	$(call cc-option,-Wstrict-prototypes)			\
 	$(call cc-option,-Wunused-parameter)			\
-	$(call cc-option,-Wvla)
+	$(call cc-option,-Wvla)					\
+	$(CFLAGS)
 
 override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)