[PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Ramsay Jones]

Introduce a $(CHECKER_FLAGS) variable to allow adding flags, using
target specific variable assignments, to specific $(CHECKER) command
invocations. In particular, in a new pre-process.cs target, include
'-Wno-vla' in the flags while checking pre-process.c.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Chris,

This is what I had in mind for the selfcheck of pre-process.c.
With this patch, selfcheck is clean for me on Linux, but not
on cygwin (I will look at fixing that later).

Thanks!

ATB,
Ramsay Jones

 Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 64146db..c20ea2c 100644
--- a/Makefile
+++ b/Makefile
@@ -19,6 +19,7 @@ LD = gcc
 AR = ar
 PKG_CONFIG = pkg-config
 CHECKER = ./cgcc -no-compile
+CHECKER_FLAGS =
 
 ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
 #
@@ -198,11 +199,13 @@ endif
 
 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) $<
 
 %.sc: %.c sparse
-	$(QUIET_CHECK) $(CHECKER) -c $(ALL_CFLAGS) $<
+	$(QUIET_CHECK) $(CHECKER) $(CHECKER_FLAGS) -c $(ALL_CFLAGS) $<
 
 ALL_OBJS :=  $(LIB_OBJS) $(foreach p,$(PROGRAMS),$(p).o $($(p)_EXTRA_DEPS))
 selfcheck: $(ALL_OBJS:.o=.sc)
-- 
2.13.0

Re: [PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Christopher Li]

On Sun, Jul 30, 2017 at 7:27 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Introduce a $(CHECKER_FLAGS) variable to allow adding flags, using
> target specific variable assignments, to specific $(CHECKER) command
> invocations. In particular, in a new pre-process.cs target, include
> '-Wno-vla' in the flags while checking pre-process.c.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>

Thanks, I want to apply it. But I have some very minor feed backs.

> +pre-process.sc: CHECKER_FLAGS += -Wno-vla
> +

You actually don't need to introduce CHECKER_FLAGS.
You can just do:

pre-process.sc: CFLAGS += -Wno-vla

The target specific CFLAGS will only impact pre-process.sc.

I would use CHECKER_FLAGS is for some thing impact all the
checker target.

>
>  %.sc: %.c sparse
> -       $(QUIET_CHECK) $(CHECKER) -c $(ALL_CFLAGS) $<
> +       $(QUIET_CHECK) $(CHECKER) $(CHECKER_FLAGS) -c $(ALL_CFLAGS) $<

If you use target specific CFLAGS, there is no need to use CHECKER_FLAGS.

Chris

Re: [PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Christopher Li]

On Sun, Jul 30, 2017 at 8:05 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Sun, Jul 30, 2017 at 7:27 PM, Ramsay Jones
>
> You actually don't need to introduce CHECKER_FLAGS.
> You can just do:

Ah, I see what you mean here. You want to distinguish
CFLAGS for gcc, CHECKER_FLAGS for sparse specific
flags.

It is different than what I have in mind, I think  that is fine
too.

I will apply.

Chris

Re: [PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Ramsay Jones]

On 31/07/17 01:12, Christopher Li wrote:
> On Sun, Jul 30, 2017 at 8:05 PM, Christopher Li <sparse@chrisli.org> wrote:
>> On Sun, Jul 30, 2017 at 7:27 PM, Ramsay Jones
>>
>> You actually don't need to introduce CHECKER_FLAGS.
>> You can just do:
> 
> Ah, I see what you mean here. You want to distinguish
> CFLAGS for gcc, CHECKER_FLAGS for sparse specific
> flags.

Err, ... well, yes and no! :-D

The main idea is to separate the 'additional' flags passed to
sparse for the $(CHECKER) target - not necessarily for sparse
specific flags.

In this case, for example, -W[no]-vla is also a gcc flag, viz:

  $ make CFLAGS=-Wno-vla pre-process.sc
  Makefile:69: Your system does not have libxml, disabling c2xml
  Makefile:82: Your system does not have libgtk2, disabling test-inspect
  Makefile:102: Your system does not have llvm, disabling sparse-llvm
       CHECK    pre-process.c
  $ 

  $ rm pre-process.o
  $ make CFLAGS=-Wno-vla pre-process.o
  Makefile:69: Your system does not have libxml, disabling c2xml
  Makefile:82: Your system does not have libgtk2, disabling test-inspect
  Makefile:102: Your system does not have llvm, disabling sparse-llvm
       CC       pre-process.o
  $ 

  $ rm pre-process.o
  $ make CFLAGS=-Wvla pre-process.o
  Makefile:69: Your system does not have libxml, disabling c2xml
  Makefile:82: Your system does not have libgtk2, disabling test-inspect
  Makefile:102: Your system does not have llvm, disabling sparse-llvm
       CC       pre-process.o
  pre-process.c: In function ‘expand’:
  pre-process.c:712:9: warning: ISO C90 forbids variable length array ‘args’ [-Wvla]
    struct arg args[nargs];
           ^
  pre-process.c: In function ‘dump_macro’:
  pre-process.c:2019:9: warning: ISO C90 forbids variable length array ‘args’ [-Wvla]
    struct token *args[nargs];
           ^
  $ 

So, yes, I initially didn't have the $(CHECKER_FLAGS) and simply
had the following addition:

  $ git diff
  diff --git a/Makefile b/Makefile
  index 64146db..8e4b0ae 100644
  --- a/Makefile
  +++ b/Makefile
  @@ -198,6 +198,8 @@ endif
 
   c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
 
  +pre-process.sc: CFLAGS += -Wno-vla
  +
   %.o: %.c $(LIB_H)
          $(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
 
  $ 

... which works just as well. [Note that gcc and sparse have a
different default for -Wvla].

Also, note that we are using cgcc for checker, so non-gcc flags
should never get to gcc anyway (or we have a bug).

So, if you prefer not to introduce another variable, I would be
equally fine with that. However, bear in mind that you will have
to remember to add -Wno-vla manually to CFLAGS if you invoke make
with CFLAGS on the command line.

ATB,
Ramsay Jones

Re: [PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Ramsay Jones]

On 31/07/17 16:05, Ramsay Jones wrote:
> 
> 
> On 31/07/17 01:12, Christopher Li wrote:
>> On Sun, Jul 30, 2017 at 8:05 PM, Christopher Li <sparse@chrisli.org> wrote:
>>> On Sun, Jul 30, 2017 at 7:27 PM, Ramsay Jones
>>>
>>> You actually don't need to introduce CHECKER_FLAGS.
>>> You can just do:
>>
>> Ah, I see what you mean here. You want to distinguish
>> CFLAGS for gcc, CHECKER_FLAGS for sparse specific
>> flags.
> 
> Err, ... well, yes and no! :-D
> 
> The main idea is to separate the 'additional' flags passed to
> sparse for the $(CHECKER) target - not necessarily for sparse
> specific flags.
> 
> In this case, for example, -W[no]-vla is also a gcc flag, viz:
> 
>   $ make CFLAGS=-Wno-vla pre-process.sc
>   Makefile:69: Your system does not have libxml, disabling c2xml
>   Makefile:82: Your system does not have libgtk2, disabling test-inspect
>   Makefile:102: Your system does not have llvm, disabling sparse-llvm
>        CHECK    pre-process.c
>   $ 
> 
>   $ rm pre-process.o
>   $ make CFLAGS=-Wno-vla pre-process.o
>   Makefile:69: Your system does not have libxml, disabling c2xml
>   Makefile:82: Your system does not have libgtk2, disabling test-inspect
>   Makefile:102: Your system does not have llvm, disabling sparse-llvm
>        CC       pre-process.o
>   $ 
> 
>   $ rm pre-process.o
>   $ make CFLAGS=-Wvla pre-process.o
>   Makefile:69: Your system does not have libxml, disabling c2xml
>   Makefile:82: Your system does not have libgtk2, disabling test-inspect
>   Makefile:102: Your system does not have llvm, disabling sparse-llvm
>        CC       pre-process.o
>   pre-process.c: In function ‘expand’:
>   pre-process.c:712:9: warning: ISO C90 forbids variable length array ‘args’ [-Wvla]
>     struct arg args[nargs];
>            ^
>   pre-process.c: In function ‘dump_macro’:
>   pre-process.c:2019:9: warning: ISO C90 forbids variable length array ‘args’ [-Wvla]
>     struct token *args[nargs];
>            ^
>   $ 
> 
> So, yes, I initially didn't have the $(CHECKER_FLAGS) and simply
> had the following addition:
> 
>   $ git diff
>   diff --git a/Makefile b/Makefile
>   index 64146db..8e4b0ae 100644
>   --- a/Makefile
>   +++ b/Makefile
>   @@ -198,6 +198,8 @@ endif
>  
>    c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
>  
>   +pre-process.sc: CFLAGS += -Wno-vla
>   +
>    %.o: %.c $(LIB_H)
>           $(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
>  
>   $ 
> 
> ... which works just as well. [Note that gcc and sparse have a
> different default for -Wvla].
> 
> Also, note that we are using cgcc for checker, so non-gcc flags
> should never get to gcc anyway (or we have a bug).
> 
> So, if you prefer not to introduce another variable, I would be
> equally fine with that. However, bear in mind that you will have
> to remember to add -Wno-vla manually to CFLAGS if you invoke make
> with CFLAGS on the command line.

Hmm, actually the following may be a better patch. what do you
think?

ATB,
Ramsay Jones

-- >8 --
diff --git a/Makefile b/Makefile
index 64146db..93c7db9 100644
--- a/Makefile
+++ b/Makefile
@@ -14,6 +14,7 @@ OS = linux
 CC = gcc
 CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
 CFLAGS += -Wall -Wwrite-strings
+BASIC_CFLAGS =
 LDFLAGS += -g
 LD = gcc
 AR = ar
@@ -24,7 +25,7 @@ ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
 #
 # For debugging, put this in local.mk:
 #
-#     CFLAGS += -O0 -DDEBUG -g3 -gdwarf-2
+#     BASIC_CFLAGS += -O0 -DDEBUG -g3 -gdwarf-2
 #
 
 HAVE_LIBXML:=$(shell $(PKG_CONFIG) --exists libxml-2.0 2>/dev/null && echo 'yes')
@@ -36,7 +37,7 @@ 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)\"
+BASIC_CFLAGS += -DGCC_BASE=\"$(GCC_BASE)\"
 
 MULTIARCH_TRIPLET := $(shell $(CC) -print-multiarch 2>/dev/null)
 BASIC_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
@@ -75,7 +76,7 @@ PROGRAMS += test-inspect
 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 += $(GTK2_CFLAGS)
+$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): BASIC_CFLAGS += $(GTK2_CFLAGS)
 test-inspect_EXTRA_OBJS := $(GTK2_LIBS)
 else
 $(warning Your system does not have libgtk2, disabling test-inspect)
@@ -196,7 +197,9 @@ ifneq ($(DEP_FILES),)
 include $(DEP_FILES)
 endif
 
-c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
+c2xml.o c2xml.sc: BASIC_CFLAGS += $(LIBXML_CFLAGS)
+
+pre-process.sc: BASIC_CFLAGS += -Wno-vla
 
 %.o: %.c $(LIB_H)
 	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<

Re: [PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Christopher Li]

On Mon, Jul 31, 2017 at 11:29 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
>>> Ah, I see what you mean here. You want to distinguish
>>> CFLAGS for gcc, CHECKER_FLAGS for sparse specific
>>> flags.
>>
>> Err, ... well, yes and no! :-D
>>
>> The main idea is to separate the 'additional' flags passed to
>> sparse for the $(CHECKER) target - not necessarily for sparse
>> specific flags.

Yes, my mental model was CHECKER_FLAGS for flags sparse
to use. I haven't notice the small detail that -Wvla works for gcc
as well.

> Hmm, actually the following may be a better patch. what do you
> think?

Hmm. When I write the BASIC_CFLAGS, I was thinking to use it for the
common part of the CFLAGS impact the whole directory or project. Some
thing as base line, like architecture related stuff etc, nothing fancy.

Each C file might have specific different requirement, like include path etc,
Those goes to CFLAGS. The stuff get overwrite more often should have
a slightly shorter name for readability.

At least that was my mental model. Either one of the three we discuss here
is acceptable. If you have to ask me to pick, my order goes to the simplest
one line change, then introducing $(CHECKER_CFLAGS), then this one.

Chris

Re: [PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Ramsay Jones]

On 31/07/17 16:51, Christopher Li wrote:
> On Mon, Jul 31, 2017 at 11:29 AM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>>
>>>> Ah, I see what you mean here. You want to distinguish
>>>> CFLAGS for gcc, CHECKER_FLAGS for sparse specific
>>>> flags.
>>>
>>> Err, ... well, yes and no! :-D
>>>
>>> The main idea is to separate the 'additional' flags passed to
>>> sparse for the $(CHECKER) target - not necessarily for sparse
>>> specific flags.
> 
> Yes, my mental model was CHECKER_FLAGS for flags sparse
> to use. I haven't notice the small detail that -Wvla works for gcc
> as well.
> 
>> Hmm, actually the following may be a better patch. what do you
>> think?
> 
> Hmm. When I write the BASIC_CFLAGS, I was thinking to use it for the
> common part of the CFLAGS impact the whole directory or project. Some
> thing as base line, like architecture related stuff etc, nothing fancy.

I thought of it more as a non-override-able list of cflags, which
are required for 'correct'/'desired' behaviour. The CFLAGS are
supposed to be user settable from the command line. So, for example,
if you wanted to compile c2xml and override the CFLAGS, then you must
know to add the output of '$(PKG_CONFIG) --cflags libxml-2.0' manually.

> Each C file might have specific different requirement, like include path etc,
> Those goes to CFLAGS. The stuff get overwrite more often should have
> a slightly shorter name for readability.

Hmm, but see above!

> At least that was my mental model. Either one of the three we discuss here
> is acceptable. If you have to ask me to pick, my order goes to the simplest
> one line change, then introducing $(CHECKER_CFLAGS), then this one.

I'm OK with any of the three too, but I actually think the last
patch is the best. ;-)

ATB,
Ramsay Jones

Re: [PATCH] Makefile: pass -Wno-vla to sparse while checking pre-process.c

[Christopher Li]

On Mon, Jul 31, 2017 at 12:21 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 31/07/17 16:51, Christopher Li wrote:
> I thought of it more as a non-override-able list of cflags, which
> are required for 'correct'/'desired' behaviour. The CFLAGS are
> supposed to be user settable from the command line. So, for example,
> if you wanted to compile c2xml and override the CFLAGS, then you must
> know to add the output of '$(PKG_CONFIG) --cflags libxml-2.0' manually.

I think CFLAGS are used by makefile implicate rules. So it has pre-existing
usage pattern there. Setting CFLAGS on command line is most likely not going
to work with most open source project out there, unless you give a very detai
 and long CFLAGS.

>
> I'm OK with any of the three too, but I actually think the last
> patch is the best. ;-)

In that  case, will the simplest one line change one a better middle
ground then?
Obvious there is only one file need to add flags here. Just keep it simple.
If more complicate flags setting required later for self checking, we can always
introduce the CHECK_CFFLAGS later. Just asking.

Chris