* [fsverity-utils PATCH] override CFLAGS too
@ 2020-10-26 20:48 luca.boccassi
2020-10-26 22:10 ` Eric Biggers
0 siblings, 1 reply; 8+ messages in thread
From: luca.boccassi @ 2020-10-26 20:48 UTC (permalink / raw)
To: linux-fscrypt
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)
--
2.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [fsverity-utils PATCH] override CFLAGS too
2020-10-26 20:48 [fsverity-utils PATCH] override CFLAGS too luca.boccassi
@ 2020-10-26 22:10 ` Eric Biggers
2020-10-27 10:30 ` Luca Boccassi
2020-10-28 16:47 ` [fsverity-utils PATCH] override CFLAGS too Jes Sorensen
0 siblings, 2 replies; 8+ messages in thread
From: Eric Biggers @ 2020-10-26 22:10 UTC (permalink / raw)
To: luca.boccassi, Jes Sorensen; +Cc: linux-fscrypt
[+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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [fsverity-utils PATCH] override CFLAGS too
2020-10-26 22:10 ` Eric Biggers
@ 2020-10-27 10:30 ` Luca Boccassi
2020-10-28 17:17 ` Eric Biggers
2020-10-28 16:47 ` [fsverity-utils PATCH] override CFLAGS too Jes Sorensen
1 sibling, 1 reply; 8+ messages in thread
From: Luca Boccassi @ 2020-10-27 10:30 UTC (permalink / raw)
To: Eric Biggers, Jes Sorensen; +Cc: linux-fscrypt
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)
##############################################################################
--
2.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [fsverity-utils PATCH] override CFLAGS too
2020-10-26 22:10 ` Eric Biggers
2020-10-27 10:30 ` Luca Boccassi
@ 2020-10-28 16:47 ` Jes Sorensen
1 sibling, 0 replies; 8+ messages in thread
From: Jes Sorensen @ 2020-10-28 16:47 UTC (permalink / raw)
To: Eric Biggers, luca.boccassi; +Cc: linux-fscrypt, malmond
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [fsverity-utils PATCH] override CFLAGS too
2020-10-27 10:30 ` Luca Boccassi
@ 2020-10-28 17:17 ` Eric Biggers
2020-10-28 17:27 ` Luca Boccassi
0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2020-10-28 17:17 UTC (permalink / raw)
To: Luca Boccassi; +Cc: Jes Sorensen, linux-fscrypt
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)
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [fsverity-utils PATCH] override CFLAGS too
2020-10-28 17:17 ` Eric Biggers
@ 2020-10-28 17:27 ` Luca Boccassi
2020-10-28 18:27 ` [fsverity-utils PATCH] Makefile: adjust CFLAGS overriding Eric Biggers
0 siblings, 1 reply; 8+ messages in thread
From: Luca Boccassi @ 2020-10-28 17:27 UTC (permalink / raw)
To: Eric Biggers; +Cc: Jes Sorensen, linux-fscrypt
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.
--
Kind regards,
Luca Boccassi
^ permalink raw reply [flat|nested] 8+ messages in thread
* [fsverity-utils PATCH] Makefile: adjust CFLAGS overriding
2020-10-28 17:27 ` Luca Boccassi
@ 2020-10-28 18:27 ` Eric Biggers
2020-10-28 18:38 ` Luca Boccassi
0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2020-10-28 18:27 UTC (permalink / raw)
To: linux-fscrypt; +Cc: Luca Boccassi, Jes Sorensen
From: Eric Biggers <ebiggers@google.com>
Make any user-specified CFLAGS only replace flags that affect the
resulting binary. Currently that means just "-O2". Always add the
warning flags, although they can still be disabled by -Wno-*. This
seems to be closer to what people want; see the discussion at
https://lkml.kernel.org/linux-fscrypt/20201026204831.3337360-1-luca.boccassi@gmail.com/T/#u
Also fix up scripts/run-tests.sh to use appropriate CFLAGS. That is,
don't specify -Wall since the Makefile now adds it, always specify
-Werror, and usually specify an optimization level too.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
Makefile | 7 ++++--
scripts/run-tests.sh | 54 ++++++++++++++++++++++----------------------
2 files changed, 32 insertions(+), 29 deletions(-)
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)
override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh
index 8a03968..a47cc48 100755
--- a/scripts/run-tests.sh
+++ b/scripts/run-tests.sh
@@ -39,7 +39,7 @@ exec 2> >(tee -ia run-tests.log 1>&2)
MAKE="make -j$(getconf _NPROCESSORS_ONLN)"
log "Build and test with statically linking"
-$MAKE
+$MAKE CFLAGS="-Werror"
if ldd fsverity | grep libfsverity.so; then
fail "fsverity binary should be statically linked to libfsverity by default"
fi
@@ -51,7 +51,7 @@ if nm libfsverity.a | grep ' T ' | grep -v " libfsverity_"; then
fi
log "Build and test with dynamic linking"
-$MAKE USE_SHARED_LIB=1 check
+$MAKE CFLAGS="-Werror" USE_SHARED_LIB=1 check
if ! ldd fsverity | grep libfsverity.so; then
fail "fsverity binary should be dynamically linked to libfsverity when USE_SHARED_LIB=1"
fi
@@ -75,7 +75,7 @@ c++ -Wall -Werror "$TMPDIR/test.cc" -Iinclude -L. -lfsverity -o "$TMPDIR/test"
rm "${TMPDIR:?}"/*
log "Check that build doesn't produce untracked files"
-$MAKE all test_programs
+$MAKE CFLAGS="-Werror" all test_programs
if git status --short | grep -q '^??'; then
git status
fail "Build produced untracked files (check 'git status'). Missing gitignore entry?"
@@ -120,64 +120,64 @@ if [ -s "$list" ]; then
fi
rm "$list"
-log "Build and test with gcc"
-$MAKE CC=gcc check
-
-log "Build and test with gcc (-Wall + -Werror)"
-$MAKE CC=gcc CFLAGS="-Wall -Werror" check
+log "Build and test with gcc (-O2)"
+$MAKE CC=gcc CFLAGS="-O2 -Werror" check
log "Build and test with gcc (-O3)"
-$MAKE CC=gcc CFLAGS="-O3 -Wall -Werror" check
+$MAKE CC=gcc CFLAGS="-O3 -Werror" check
log "Build and test with gcc (32-bit)"
-$MAKE CC=gcc CFLAGS="-m32 -O2 -Wall -Werror" check
-
-log "Build and test with clang"
-$MAKE CC=clang check
+$MAKE CC=gcc CFLAGS="-O2 -Werror -m32" check
-log "Build and test with clang (-Wall + -Werror)"
-$MAKE CC=clang CFLAGS="-Wall -Werror" check
+log "Build and test with clang (-O2)"
+$MAKE CC=clang CFLAGS="-O2 -Werror" check
log "Build and test with clang (-O3)"
-$MAKE CC=clang CFLAGS="-O3 -Wall -Werror" check
+$MAKE CC=clang CFLAGS="-O3 -Werror" check
log "Build and test with clang + UBSAN"
-$MAKE CC=clang CFLAGS="-fsanitize=undefined -fno-sanitize-recover=undefined" \
+$MAKE CC=clang \
+ CFLAGS="-O2 -Werror -fsanitize=undefined -fno-sanitize-recover=undefined" \
check
log "Build and test with clang + ASAN"
-$MAKE CC=clang CFLAGS="-fsanitize=address -fno-sanitize-recover=address" check
+$MAKE CC=clang \
+ CFLAGS="-O2 -Werror -fsanitize=address -fno-sanitize-recover=address" \
+ check
log "Build and test with clang + unsigned integer overflow sanitizer"
-$MAKE CC=clang CFLAGS="-fsanitize=unsigned-integer-overflow -fno-sanitize-recover=unsigned-integer-overflow" \
+$MAKE CC=clang \
+ CFLAGS="-O2 -Werror -fsanitize=unsigned-integer-overflow -fno-sanitize-recover=unsigned-integer-overflow" \
check
log "Build and test with clang + CFI"
-$MAKE CC=clang CFLAGS="-fsanitize=cfi -flto -fvisibility=hidden" check
+$MAKE CC=clang CFLAGS="-O2 -Werror -fsanitize=cfi -flto -fvisibility=hidden" \
+ check
log "Build and test with valgrind"
$MAKE TEST_WRAPPER_PROG="valgrind --quiet --error-exitcode=100 --leak-check=full --errors-for-leak-kinds=all" \
- check
+ CFLAGS="-O2 -Werror" check
log "Build and test using BoringSSL instead of OpenSSL"
BSSL=$HOME/src/boringssl
-$MAKE LDFLAGS="-L$BSSL/build/crypto" CPPFLAGS="-I$BSSL/include" \
- LDLIBS="-lcrypto -lpthread" check
+$MAKE CFLAGS="-O2 -Werror" LDFLAGS="-L$BSSL/build/crypto" \
+ CPPFLAGS="-I$BSSL/include" LDLIBS="-lcrypto -lpthread" check
log "Build and test using -funsigned-char"
-$MAKE CFLAGS="-funsigned-char -Wall -Werror" check
+$MAKE CFLAGS="-O2 -Werror -funsigned-char" check
log "Build and test using -fsigned-char"
-$MAKE CFLAGS="-fsigned-char -Wall -Werror" check
+$MAKE CFLAGS="-O2 -Werror -fsigned-char" check
log "Run sparse"
./scripts/run-sparse.sh
log "Run clang static analyzer"
-scan-build --status-bugs make all test_programs
+scan-build --status-bugs make CFLAGS="-O2 -Werror" all test_programs
log "Run gcc static analyzer"
-$MAKE CC=gcc CFLAGS="-fanalyzer -Werror" all test_programs
+# Using -O2 here produces a false positive report in fsverity_cmd_sign().
+$MAKE CC=gcc CFLAGS="-O0 -Werror -fanalyzer" all test_programs
log "Run shellcheck"
shellcheck scripts/*.sh 1>&2
--
2.29.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [fsverity-utils PATCH] Makefile: adjust CFLAGS overriding
2020-10-28 18:27 ` [fsverity-utils PATCH] Makefile: adjust CFLAGS overriding Eric Biggers
@ 2020-10-28 18:38 ` Luca Boccassi
0 siblings, 0 replies; 8+ messages in thread
From: Luca Boccassi @ 2020-10-28 18:38 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-fscrypt, Jes Sorensen
On Wed, 28 Oct 2020 at 18:29, Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> Make any user-specified CFLAGS only replace flags that affect the
> resulting binary. Currently that means just "-O2". Always add the
> warning flags, although they can still be disabled by -Wno-*. This
> seems to be closer to what people want; see the discussion at
> https://lkml.kernel.org/linux-fscrypt/20201026204831.3337360-1-luca.boccassi@gmail.com/T/#u
>
> Also fix up scripts/run-tests.sh to use appropriate CFLAGS. That is,
> don't specify -Wall since the Makefile now adds it, always specify
> -Werror, and usually specify an optimization level too.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> Makefile | 7 ++++--
> scripts/run-tests.sh | 54 ++++++++++++++++++++++----------------------
> 2 files changed, 32 insertions(+), 29 deletions(-)
Acked-by: Luca Boccassi <bluca@debian.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-10-29 2:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-26 20:48 [fsverity-utils PATCH] override CFLAGS too luca.boccassi
2020-10-26 22:10 ` Eric Biggers
2020-10-27 10:30 ` Luca Boccassi
2020-10-28 17:17 ` Eric Biggers
2020-10-28 17:27 ` Luca Boccassi
2020-10-28 18:27 ` [fsverity-utils PATCH] Makefile: adjust CFLAGS overriding Eric Biggers
2020-10-28 18:38 ` Luca Boccassi
2020-10-28 16:47 ` [fsverity-utils PATCH] override CFLAGS too Jes Sorensen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.