* [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-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
* 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
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.