Re: [Xen-devel] [PATCH v7 2/4] xen: introduce a C99 headers check

2017-03-31 Thread Stefano Stabellini 
On Fri, 31 Mar 2017, Jan Beulich wrote:
> >>> On 30.03.17 at 23:04,  wrote:
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -274,6 +274,7 @@ xen/arch/*/efi/compat.c
> >  xen/arch/*/efi/efi.h
> >  xen/arch/*/efi/runtime.c
> >  xen/include/headers.chk
> > +xen/include/headers99.chk
> >  xen/include/headers++.chk
> 
> I'm sorry for not having noticed on the previous round, but please
> make this xen/include/headers*.chk just like was done in the clean
> target. Generally clean targets and .gitignore entries should match.

Sure


> > @@ -104,16 +105,21 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
> > done >$@.new
> > mv $@.new $@
> >  
> > +headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
> > +   rm -f $@.new
> > +   $(foreach i, $(filter %.h,$^), $(CC) -x c -std=c99 -Wall -Werror\
> > +   -include stdint.h $(foreach j, $($(i)-prereq), -include $(j).h) \
> > +   -S -o /dev/null $(i) || exit 1; echo $(i) >> $@.new;)
> > +   mv $@.new $@
> > +
> >  headers++.chk: $(PUBLIC_HEADERS) Makefile
> > -   if $(CXX) -v >/dev/null 2>&1; then \
> > -   for i in $(filter %.h,$^); do \
> > -   echo '#include "'$$i'"' \
> > -   | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
> > - -include stdint.h -include public/xen.h -S -o /dev/null - \
> > -   || exit 1; \
> > -   echo $$i; \
> > -   done ; \
> > -   fi >$@.new
> > +   rm -f $@.new
> > +   $(CXX) -v >/dev/null 2>&1 || exit 0
> 
> As said before, the lack of a semicolon and line continuation here
> will result in this not dong what you want: If there's no C++
> compiler available, you'll exit only the shell which was invoked to
> execute above command, signaling success to make, which will
> then continue with the next command in the rule, which will then
> fail due to there not being a C++ compiler.

I think understand now, I'll fix.


> > +   $(foreach i, $(filter %.h,$^), echo "#include "\"$(i)\"|   \
> 
> Preferably I'd like to see the | on the start of the next line, as it
> was before (the same would then go for the later ||). Alternatively
> at least have a blank between closing quote and pipeline character.

I moved | and || at the beginning of the next lines.


> > +   $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__   \
> > +   -include stdint.h -include public/xen.h\
> > +   $(foreach j, $($(i)-prereq), -include c$(j))   \
> > +   -S -o /dev/null - || exit 1; echo $(i) >> $@.new;)
> 
> Would you mind switching to "exit $$?" instead of "exit 1" here,
> to propagate the exit code from the compiler? Also in the C99
> case then. An alternative would be to use "set -e" up front.

I can do that


> I'm sorry, I again should have noticed these last two earlier.

That's OK. Thanks for the quick reviews.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7 2/4] xen: introduce a C99 headers check

2017-03-31 Thread Jan Beulich 
>>> On 30.03.17 at 23:04,  wrote:
> --- a/.gitignore
> +++ b/.gitignore
> @@ -274,6 +274,7 @@ xen/arch/*/efi/compat.c
>  xen/arch/*/efi/efi.h
>  xen/arch/*/efi/runtime.c
>  xen/include/headers.chk
> +xen/include/headers99.chk
>  xen/include/headers++.chk

I'm sorry for not having noticed on the previous round, but please
make this xen/include/headers*.chk just like was done in the clean
target. Generally clean targets and .gitignore entries should match.

> @@ -104,16 +105,21 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
>   done >$@.new
>   mv $@.new $@
>  
> +headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
> + rm -f $@.new
> + $(foreach i, $(filter %.h,$^), $(CC) -x c -std=c99 -Wall -Werror\
> + -include stdint.h $(foreach j, $($(i)-prereq), -include $(j).h) \
> + -S -o /dev/null $(i) || exit 1; echo $(i) >> $@.new;)
> + mv $@.new $@
> +
>  headers++.chk: $(PUBLIC_HEADERS) Makefile
> - if $(CXX) -v >/dev/null 2>&1; then \
> - for i in $(filter %.h,$^); do \
> - echo '#include "'$$i'"' \
> - | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
> -   -include stdint.h -include public/xen.h -S -o /dev/null - \
> - || exit 1; \
> - echo $$i; \
> - done ; \
> - fi >$@.new
> + rm -f $@.new
> + $(CXX) -v >/dev/null 2>&1 || exit 0

As said before, the lack of a semicolon and line continuation here
will result in this not dong what you want: If there's no C++
compiler available, you'll exit only the shell which was invoked to
execute above command, signaling success to make, which will
then continue with the next command in the rule, which will then
fail due to there not being a C++ compiler.

> + $(foreach i, $(filter %.h,$^), echo "#include "\"$(i)\"|   \

Preferably I'd like to see the | on the start of the next line, as it
was before (the same would then go for the later ||). Alternatively
at least have a blank between closing quote and pipeline character.

> + $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__   \
> + -include stdint.h -include public/xen.h\
> + $(foreach j, $($(i)-prereq), -include c$(j))   \
> + -S -o /dev/null - || exit 1; echo $(i) >> $@.new;)

Would you mind switching to "exit $$?" instead of "exit 1" here,
to propagate the exit code from the compiler? Also in the C99
case then. An alternative would be to use "set -e" up front.

I'm sorry, I again should have noticed these last two earlier.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v7 2/4] xen: introduce a C99 headers check

2017-03-30 Thread Stefano Stabellini 
Introduce a C99 headers check, for non-ANSI compliant headers: 9pfs.h
and pvcalls.h.

In addition to the usual -include stdint.h, also add -include string.h
to the C99 check to get the declaration of memcpy and size_t.

For the same reason, also add -include cstring to the C++ check when
necessary.

Signed-off-by: Stefano Stabellini 
CC: jbeul...@suse.com
CC: konrad.w...@oracle.com
---
 .gitignore   |  1 +
 xen/include/Makefile | 30 ++
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/.gitignore b/.gitignore
index 443b12a..a8905b1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -274,6 +274,7 @@ xen/arch/*/efi/compat.c
 xen/arch/*/efi/efi.h
 xen/arch/*/efi/runtime.c
 xen/include/headers.chk
+xen/include/headers99.chk
 xen/include/headers++.chk
 xen/include/asm
 xen/include/asm-*/asm-offsets.h
diff --git a/xen/include/Makefile b/xen/include/Makefile
index aca7f20..64fecbd 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -90,11 +90,12 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile
 
 ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
 
-all: headers.chk headers++.chk
+all: headers.chk headers99.chk headers++.chk
 
 PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard 
public/*.h public/*/*.h) $(public-y))
 
-PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% 
public/%hvm/save.h, $(PUBLIC_HEADERS))
+PUBLIC_C99_HEADERS :=
+PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% 
public/%hvm/save.h $(PUBLIC_C99_HEADERS), $(PUBLIC_HEADERS))
 
 headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
for i in $(filter %.h,$^); do \
@@ -104,16 +105,21 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
done >$@.new
mv $@.new $@
 
+headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
+   rm -f $@.new
+   $(foreach i, $(filter %.h,$^), $(CC) -x c -std=c99 -Wall -Werror\
+   -include stdint.h $(foreach j, $($(i)-prereq), -include $(j).h) \
+   -S -o /dev/null $(i) || exit 1; echo $(i) >> $@.new;)
+   mv $@.new $@
+
 headers++.chk: $(PUBLIC_HEADERS) Makefile
-   if $(CXX) -v >/dev/null 2>&1; then \
-   for i in $(filter %.h,$^); do \
-   echo '#include "'$$i'"' \
-   | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
- -include stdint.h -include public/xen.h -S -o /dev/null - \
-   || exit 1; \
-   echo $$i; \
-   done ; \
-   fi >$@.new
+   rm -f $@.new
+   $(CXX) -v >/dev/null 2>&1 || exit 0
+   $(foreach i, $(filter %.h,$^), echo "#include "\"$(i)\"|   \
+   $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__   \
+   -include stdint.h -include public/xen.h\
+   $(foreach j, $($(i)-prereq), -include c$(j))   \
+   -S -o /dev/null - || exit 1; echo $(i) >> $@.new;)
mv $@.new $@
 
 endif
@@ -128,5 +134,5 @@ all: $(BASEDIR)/include/asm-x86/cpuid-autogen.h
 endif
 
 clean::
-   rm -rf compat headers.chk headers++.chk
+   rm -rf compat headers*.chk
rm -f $(BASEDIR)/include/asm-x86/cpuid-autogen.h
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel