[PATCH 0/6] build: various updates

[PATCH 0/6] build: various updates

[Uwe Kleine-König]

Hello,

this series motiviation is to support CPPFLAGS which is needed for
Debian packaging and includes the stuff I found while adding this.

Best regards
Uwe

Uwe Kleine-König (6):
  build: make PREFIX overwritable from the environment
  build: put comment about local.mk to the place where it is included
  build: drop BASIC_CFLAGS and ALL_CFLAGS
  build: drop -g from LDFLAGS
  build: pass standard make variables to compiler and linker
  build: replace *_EXTRA_OBJS by local assignments to LDFLAGS and
    LOADLIBES

 Makefile | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

-- 
2.14.2

[PATCH 1/6] build: make PREFIX overwritable from the environment

[Uwe Kleine-König]

This way I can just use

	env PREFIX=/usr make install

on the command line to install sparse into the system.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d0341764158e..e011df3cc0dc 100644
--- a/Makefile
+++ b/Makefile
@@ -54,7 +54,7 @@ BASIC_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
 endif
 
 DESTDIR=
-PREFIX=$(HOME)
+PREFIX ?= $(HOME)
 BINDIR=$(PREFIX)/bin
 LIBDIR=$(PREFIX)/lib
 MANDIR=$(PREFIX)/share/man
-- 
2.14.2

[PATCH 2/6] build: put comment about local.mk to the place where it is included

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Makefile | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index e011df3cc0dc..ddb4c5e3de02 100644
--- a/Makefile
+++ b/Makefile
@@ -22,11 +22,6 @@ CHECKER = ./cgcc -no-compile
 CHECKER_FLAGS =
 
 ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
-#
-# For debugging, put this in local.mk:
-#
-#     CFLAGS += -O0 -DDEBUG -g3 -gdwarf-2
-#
 
 HAVE_LIBXML:=$(shell $(PKG_CONFIG) --exists libxml-2.0 2>/dev/null && echo 'yes')
 HAVE_GCC_DEP:=$(shell touch .gcc-test.c && 				\
@@ -167,6 +162,10 @@ SED_PC_CMD = 's|@version@|$(VERSION)|g;		\
 
 
 # Allow users to override build settings without dirtying their trees
+# For debugging, put this in local.mk:
+#
+#     CFLAGS += -O0 -DDEBUG -g3 -gdwarf-2
+#
 -include local.mk
 
 
-- 
2.14.2

Re: [PATCH 2/6] build: put comment about local.mk to the place where it is included

[Christopher Li]

On Wed, Oct 4, 2017 at 6:26 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
>  # Allow users to override build settings without dirtying their trees
> +# For debugging, put this in local.mk:
> +#
> +#     CFLAGS += -O0 -DDEBUG -g3 -gdwarf-2
> +#
>  -include local.mk

This and the first patch is fine. Subject to impact of the
debug-target-v3 branch.

Chris

[PATCH 3/6] build: drop BASIC_CFLAGS and ALL_CFLAGS

[Uwe Kleine-König]

There is no good reason to not use plain CFLAGS for all usages.
This simplifies understanding the Makefile for the casual reader.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Makefile | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index ddb4c5e3de02..069dae6c8fdb 100644
--- a/Makefile
+++ b/Makefile
@@ -21,8 +21,6 @@ PKG_CONFIG = pkg-config
 CHECKER = ./cgcc -no-compile
 CHECKER_FLAGS =
 
-ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
-
 HAVE_LIBXML:=$(shell $(PKG_CONFIG) --exists libxml-2.0 2>/dev/null && echo 'yes')
 HAVE_GCC_DEP:=$(shell touch .gcc-test.c && 				\
 		$(CC) -c -Wp,-MD,.gcc-test.d .gcc-test.c 2>/dev/null && \
@@ -39,13 +37,13 @@ LLVM_CONFIG:=llvm-config
 HAVE_LLVM:=$(shell $(LLVM_CONFIG) --version >/dev/null 2>&1 && echo 'yes')
 
 GCC_BASE := $(shell $(CC) --print-file-name=)
-BASIC_CFLAGS = -DGCC_BASE=\"$(GCC_BASE)\"
+CFLAGS += -DGCC_BASE=\"$(GCC_BASE)\"
 
 MULTIARCH_TRIPLET := $(shell $(CC) -print-multiarch 2>/dev/null)
-BASIC_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
+CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
 
 ifeq ($(HAVE_GCC_DEP),yes)
-BASIC_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
+CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
 endif
 
 DESTDIR=
@@ -96,7 +94,7 @@ LLVM_LIBS := $(shell $(LLVM_CONFIG) --libs)
 LLVM_LIBS += $(shell $(LLVM_CONFIG) --system-libs 2>/dev/null)
 PROGRAMS += $(LLVM_PROGS)
 INST_PROGRAMS += sparse-llvm sparsec
-sparse-llvm.o: BASIC_CFLAGS += $(LLVM_CFLAGS)
+sparse-llvm.o: CFLAGS += $(LLVM_CFLAGS)
 sparse-llvm_EXTRA_OBJS := $(LLVM_LIBS) $(LLVM_LDFLAGS)
 else
 $(warning LLVM 3.0 or later required. Your system has version $(LLVM_VERSION) installed.)
@@ -122,7 +120,7 @@ LIB_OBJS= target.o parse.o tokenize.o pre-process.o symbol.o lib.o scope.o \
 LIB_FILE= libsparse.a
 SLIB_FILE= libsparse.so
 
-# If you add $(SLIB_FILE) to this, you also need to add -fpic to BASIC_CFLAGS above.
+# If you add $(SLIB_FILE) to this, you also need to add -fpic to CFLAGS above.
 # Doing so incurs a noticeable performance hit, and Sparse does not have a
 # stable shared library interface, so this does not occur by default.  If you
 # really want a shared library, you may want to build Sparse twice: once
@@ -212,10 +210,10 @@ c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
 pre-process.sc: CHECKER_FLAGS += -Wno-vla
 
 %.o: %.c $(LIB_H)
-	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -o $@ -c $(CFLAGS) $<
 
 %.sc: %.c sparse
-	$(QUIET_CHECK) $(CHECKER) $(CHECKER_FLAGS) -c $(ALL_CFLAGS) $<
+	$(QUIET_CHECK) $(CHECKER) $(CHECKER_FLAGS) -c $(CFLAGS) $<
 
 ALL_OBJS :=  $(LIB_OBJS) $(foreach p,$(PROGRAMS),$(p).o $($(p)_EXTRA_DEPS))
 selfcheck: $(ALL_OBJS:.o=.sc)
-- 
2.14.2

Re: [PATCH 3/6] build: drop BASIC_CFLAGS and ALL_CFLAGS

[Christopher Li]

On Wed, Oct 4, 2017 at 6:26 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> There is no good reason to not use plain CFLAGS for all usages.
> This simplifies understanding the Makefile for the casual reader.

Have more fine grain variable allow partial overwrite some
part of the CFLAG in some specific locations. For example
some flags have order over other flags etc.

I don't see a strong reason to change it. The change did not gain
any function.

Chris

Re: [PATCH 3/6] build: drop BASIC_CFLAGS and ALL_CFLAGS

[Luc Van Oostenryck]

On Fri, Oct 6, 2017 at 9:09 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Wed, Oct 4, 2017 at 6:26 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>> There is no good reason to not use plain CFLAGS for all usages.
>> This simplifies understanding the Makefile for the casual reader.
>
> Have more fine grain variable allow partial overwrite some
> part of the CFLAG in some specific locations. For example
> some flags have order over other flags etc.

And having several names make possible to use the wrong name.

> I don't see a strong reason to change it. The change did not gain
> any function.

Your version of the patch has (at least) two bugs that aren't present in this
version *because* this version (or mine) use the plain CFLAGS for everything.

-- Luc

[PATCH 4/6] build: drop -g from LDFLAGS

[Uwe Kleine-König]

-g is a compiler option that is ignored by the linker. So it should be
included in CFLAGS (it already is) but not LDFLAGS.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index 069dae6c8fdb..39b34f90107d 100644
--- a/Makefile
+++ b/Makefile
@@ -14,7 +14,6 @@ OS = linux
 CC = gcc
 CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
 CFLAGS += -Wall -Wwrite-strings
-LDFLAGS += -g
 LD = gcc
 AR = ar
 PKG_CONFIG = pkg-config
-- 
2.14.2

Re: [PATCH 4/6] build: drop -g from LDFLAGS

[Christopher Li]

On Wed, Oct 4, 2017 at 6:26 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> -g is a compiler option that is ignored by the linker. So it should be
> included in CFLAGS (it already is) but not LDFLAGS.

Our linker "LD = gcc" so it might have impact on gcc as the finial
linking program?
I am actually not sure, I can be wrong.

Chris

[PATCH 5/6] build: pass standard make variables to compiler and linker

[Uwe Kleine-König]

Add all variables that make's builtin rules pass to compiler and linker.

I don't know a user of TARGET_ARCH but Debian packages use CPPFLAGS to
pass -D_FORTIFY_SOURCE=2 for hardening.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 39b34f90107d..f6c577c02143 100644
--- a/Makefile
+++ b/Makefile
@@ -190,13 +190,13 @@ compile_EXTRA_DEPS = compile-i386.o
 
 $(foreach p,$(PROGRAMS),$(eval $(p): $($(p)_EXTRA_DEPS) $(LIBS)))
 $(PROGRAMS): % : %.o 
-	$(QUIET_LINK)$(LD) $(LDFLAGS) -o $@ $^ $($@_EXTRA_OBJS)
+	$(QUIET_LINK)$(LD) $(LDFLAGS) $(TARGET_ARCH) $^ $(LOADLIBES) $(LDLIBS) -o $@ $($@_EXTRA_OBJS)
 
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(AR) rcs $@ $(LIB_OBJS)
 
 $(SLIB_FILE): $(LIB_OBJS)
-	$(QUIET_LINK)$(CC) $(LDFLAGS) -Wl,-soname,$@ -shared -o $@ $(LIB_OBJS)
+	$(QUIET_LINK)$(CC) $(LDFLAGS) $(TARGET_ARCH) -Wl,-soname,$@ -shared $^ $(LOADLIBES) $(LDLIBS) -o $@
 
 DEP_FILES := $(wildcard .*.o.d)
 
@@ -209,10 +209,10 @@ c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
 pre-process.sc: CHECKER_FLAGS += -Wno-vla
 
 %.o: %.c $(LIB_H)
-	$(QUIET_CC)$(CC) -o $@ -c $(CFLAGS) $<
+	$(QUIET_CC)$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -o $@ $<
 
 %.sc: %.c sparse
-	$(QUIET_CHECK) $(CHECKER) $(CHECKER_FLAGS) -c $(CFLAGS) $<
+	$(QUIET_CHECK) $(CHECKER) $(CHECKER_FLAGS) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $<
 
 ALL_OBJS :=  $(LIB_OBJS) $(foreach p,$(PROGRAMS),$(p).o $($(p)_EXTRA_DEPS))
 selfcheck: $(ALL_OBJS:.o=.sc)
-- 
2.14.2

Re: [PATCH 5/6] build: pass standard make variables to compiler and linker

[Luc Van Oostenryck]

On Wed, Oct 04, 2017 at 03:26:04PM +0200, Uwe Kleine-König wrote:
> Add all variables that make's builtin rules pass to compiler and linker.
> 
> I don't know a user of TARGET_ARCH but Debian packages use CPPFLAGS to
> pass -D_FORTIFY_SOURCE=2 for hardening.

I've looked a bit at this. What I think is:
- LOADLIBES & LDLIBS are standard (g)make variables but I don't think
  there is any reasons to allow them to be overriden from the command
  line or the environment.
- TARGET_ARCH is a strange beast, it seems the intention was for
  something like '-march=armv7' which should be OK here but the only
  use I saw was to hold the output of 'uname -m'.
  I think it's better to not have it here
- of course the change for CPPFLAGS is completly legitimate.

-- Luc

[PATCH 6/6] build: replace *_EXTRA_OBJS by local assignments to LDFLAGS and LOADLIBES

[Uwe Kleine-König]

This allows to drop the magic to handle *_EXTRA_OBJS.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Makefile | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index f6c577c02143..50e7b99b6107 100644
--- a/Makefile
+++ b/Makefile
@@ -62,8 +62,8 @@ INST_MAN1=sparse.1 cgcc.1
 ifeq ($(HAVE_LIBXML),yes)
 PROGRAMS+=c2xml
 INST_PROGRAMS+=c2xml
-c2xml_EXTRA_OBJS = `$(PKG_CONFIG) --libs libxml-2.0`
-LIBXML_CFLAGS := $(shell $(PKG_CONFIG) --cflags libxml-2.0)
+c2xml: LOADLIBES += $(shell $(PKG_CONFIG) --libs libxml-2.0)
+c2xml: CFLAGS += $(shell $(PKG_CONFIG) --cflags libxml-2.0)
 else
 $(warning Your system does not have libxml, disabling c2xml)
 endif
@@ -76,7 +76,7 @@ INST_PROGRAMS += test-inspect
 test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
 test-inspect_OBJS := test-inspect.o $(test-inspect_EXTRA_DEPS)
 $(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): CFLAGS += $(GTK_CFLAGS)
-test-inspect_EXTRA_OBJS := $(GTK_LIBS)
+test-inspect: LOADLIBES += $(GTK_LIBS)
 else
 $(warning Your system does not have gtk3/gtk2, disabling test-inspect)
 endif
@@ -94,7 +94,8 @@ LLVM_LIBS += $(shell $(LLVM_CONFIG) --system-libs 2>/dev/null)
 PROGRAMS += $(LLVM_PROGS)
 INST_PROGRAMS += sparse-llvm sparsec
 sparse-llvm.o: CFLAGS += $(LLVM_CFLAGS)
-sparse-llvm_EXTRA_OBJS := $(LLVM_LIBS) $(LLVM_LDFLAGS)
+sparse-llvm: LDFLAGS += $(LLVM_LDFLAGS)
+sparse-llvm: LOADLIBES += $(LLVM_LIBS)
 else
 $(warning LLVM 3.0 or later required. Your system has version $(LLVM_VERSION) installed.)
 endif
@@ -190,7 +191,7 @@ compile_EXTRA_DEPS = compile-i386.o
 
 $(foreach p,$(PROGRAMS),$(eval $(p): $($(p)_EXTRA_DEPS) $(LIBS)))
 $(PROGRAMS): % : %.o 
-	$(QUIET_LINK)$(LD) $(LDFLAGS) $(TARGET_ARCH) $^ $(LOADLIBES) $(LDLIBS) -o $@ $($@_EXTRA_OBJS)
+	$(QUIET_LINK)$(LD) $(LDFLAGS) $(TARGET_ARCH) $^ $(LOADLIBES) $(LDLIBS) -o $@
 
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(AR) rcs $@ $(LIB_OBJS)
-- 
2.14.2

Re: [PATCH 6/6] build: replace *_EXTRA_OBJS by local assignments to LDFLAGS and LOADLIBES

[Christopher Li]

On Wed, Oct 4, 2017 at 6:26 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> ---
>  Makefile | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f6c577c02143..50e7b99b6107 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -62,8 +62,8 @@ INST_MAN1=sparse.1 cgcc.1
>  ifeq ($(HAVE_LIBXML),yes)
>  PROGRAMS+=c2xml
>  INST_PROGRAMS+=c2xml
> -c2xml_EXTRA_OBJS = `$(PKG_CONFIG) --libs libxml-2.0`
> -LIBXML_CFLAGS := $(shell $(PKG_CONFIG) --cflags libxml-2.0)
> +c2xml: LOADLIBES += $(shell $(PKG_CONFIG) --libs libxml-2.0)
> +c2xml: CFLAGS += $(shell $(PKG_CONFIG) --cflags libxml-2.0)


This is by design.

The problem with target specific variable is that, you can't easily
access it out side of the target specific scope.

For example in the debug target branch, it go through a loop
to process all objects, including the extra objects. You can't do that if
you use target specific variables.

 $(foreach @F,$(PROGRAMS),$(eval $(@F): $(EXTRA_OBJS) $(LIBS)))
-$(foreach @F,$(PROGRAMS),$(eval debug/$(@F): $(addprefix debug/,
$(EXTRA_OBJS)) $(DBG_LIBS)))
+$(foreach @F,$(PROGRAMS),$(eval dbgbuild/$(@F): $(addprefix
dbgbuild/, $(EXTRA_OBJS)) $(DBG_LIBS)))

Chris

Re: [PATCH 6/6] build: replace *_EXTRA_OBJS by local assignments to LDFLAGS and LOADLIBES

[Luc Van Oostenryck]

On Wed, Oct 04, 2017 at 03:26:05PM +0200, Uwe Kleine-König wrote:
> This allows to drop the magic to handle *_EXTRA_OBJS.

I like it, especially for CFLAGS and LDFLAGS, but it also make things
slightly more complex to correctly support selfcheck.

-- Luc Van Oostenryck

Re: [PATCH 0/6] build: various updates

[Christopher Li]

On Wed, Oct 4, 2017 at 6:25 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> Hello,
>
> this series motiviation is to support CPPFLAGS which is needed for
> Debian packaging and includes the stuff I found while adding this.

Sorry I was traveling in the last two days. Not able to reply
it earlier.

The first patch is applicable. However the others have
major conflict with the debug build change:

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

I plan to merge that first because it has deeper impact.
It also have impact on your patch too. I will comment that
on separate email reply to your patch.

Can you give the debug-target-v3 a review?

Thanks

Chris

Re: [PATCH 0/6] build: various updates

[Christopher Li]

On Fri, Oct 6, 2017 at 1:10 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> Note I don't feel strong for most of my changes. There is little user
> visible change intended by them, just making the build system more
> standard by using expected variable names, which might also be subjective?!

Exactly. I also CC sparse mailing list.

>
>> Can you give the debug-target-v3 a review?
>
> I don't like
>         Makefile: Adding OPT variable for optmize level

Thanks for the review.

>
> I'd expect a Makefile to do something like that:
>
>         CFLAGS ?= -O2 ...
>
> such that I can overrule it by doing
>
>         make CFLAGS="-O0 -g"

I can't see how that will work. CFLAGS has a lot of other options in it.
" -finline-functions -fno-strict-aliasing" just to name a few.
Overwrite the CFLAG
will drop those options as well, which is not desirable.

That is why the OPT variable was needed. Overwrite OPT will just change
the "-O2" an nothing else. Currently over write CFLAGS does not have the same
effect.

In general, overwrite CFLAGS in command line is a bad idea. Most of
the open source project I saw will not able to handle that properly.

>
> Commit
>
>         Makefile: introduce minimal target
>
> has a typo, s/minial/minimal/. And I wonder why it removes debug from

Thanks for catching that. I will update the V4 soon.

> the dependencies of check. This was added in "Makefile: release and
> debug target" needlessly?

Because this change make "all" depend on "debug" as well.
And "check" depend on "all". That makes "check" implicit depend on
"debug" already.

Before this change, "all" target actually did not build all possible target.
That is why I want to change "all" explicit build "release" and "debug" at the
same time.


>
> Other than that I don't see the motivation for this series and I'd
> always compile everything and wonder if
>
>         make CFLAGS="-DSPARSE_DEBUG"
>
> would be good enough.

The motivation is speed. For most user that use sparse to run
the kernel checking. We want sparse run as fast as possible.
However, we also want a debug mode of sparse can run more
validation and error diagnostic at the cost of speed.

One patch series I want to use this debug mode is the ptrlist
reference count patch. The ptrlist reference count has running
over head. However, it can catch the "nest loop modify" bug.
If that code path was triggered, it means a possible list
iteration corruption.

I kind of want sparse can have option  that run the debug
version at run time request. In other words, distributios
will ship both the release and debug version of sparse.
The release version of sparse will invoke the debug
options if there is debug option is specified in command line.

Chris