Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On Fri, Dec 18, 2015 at 01:43:19PM +, Peter Maydell wrote: > On 18 December 2015 at 13:41, Paolo Bonzini wrote: > > > > > > On 18/12/2015 14:35, Dr. David Alan Gilbert wrote: > >> > That would not be recursive make, but rather a completely separate > >> > Makefile to be manually invoked with -f. > >> > >> Hmm but wouldn't this Makefile also be a good place for small-fast > >> style check scripts that could be included in make check ? > > > > Yes, but there is no benefit from inclusion, since these tests do not > > depend on anything having been built already. > > In particular, if it doesn't require a preceding configure or > build I can just run it once as part of my merge testing rather > than farming it out to be run in parallel on five machines and > ten different configs :-) Ok, yeah, that makes total sense - it certainly shouldn't depend on any configure / build step. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
* Peter Maydell (peter.mayd...@linaro.org) wrote: > On 18 December 2015 at 13:42, Dr. David Alan Gilbert > wrote: > > "make check" takes under 2m on my laptop, so I take that to be reasonably > > fast, i.e. I'd be happy to add other small (few second) tests to it. > > Is that a 'make check' for a complete all-targets configure? > Those are not fast, especially on slower hosts... Ah no, that is just for x86_64-softmmu. Dave > > thanks > -- PMM -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On 18 December 2015 at 13:42, Dr. David Alan Gilbert wrote: > "make check" takes under 2m on my laptop, so I take that to be reasonably > fast, i.e. I'd be happy to add other small (few second) tests to it. Is that a 'make check' for a complete all-targets configure? Those are not fast, especially on slower hosts... thanks -- PMM
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On 18 December 2015 at 13:41, Paolo Bonzini wrote: > > > On 18/12/2015 14:35, Dr. David Alan Gilbert wrote: >> > That would not be recursive make, but rather a completely separate >> > Makefile to be manually invoked with -f. >> >> Hmm but wouldn't this Makefile also be a good place for small-fast >> style check scripts that could be included in make check ? > > Yes, but there is no benefit from inclusion, since these tests do not > depend on anything having been built already. In particular, if it doesn't require a preceding configure or build I can just run it once as part of my merge testing rather than farming it out to be run in parallel on five machines and ten different configs :-) thanks -- PMM
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
* Daniel P. Berrange (berra...@redhat.com) wrote: > On Fri, Dec 18, 2015 at 01:35:13PM +, Dr. David Alan Gilbert wrote: > > * Paolo Bonzini (pbonz...@redhat.com) wrote: > > > > > > > > > On 18/12/2015 14:05, Daniel P. Berrange wrote: > > > > > > + > > > > > > +cs-glib-syms: > > > > > > + @perl scripts/glib-syms.pl $(GLIB_SYMS_LIST) $(C_CODE_FILES) > > > > > > > > > > > > > > > Does this need to be included, or could it be a separate Makefile > > > > > invoked with e.g. make -f scripts/Makefile.style? > > > > > > > > Any particular reason to favour that over include ? I did it this > > > > way because QEMU in general seems to be biased towards includes > > > > and not recursive make > > > > > > That would not be recursive make, but rather a completely separate > > > Makefile to be manually invoked with -f. > > > > Hmm but wouldn't this Makefile also be a good place for small-fast > > style check scripts that could be included in make check ? > > Nothing about "make check" is fast, so we could probably just wire it > all into make check by default, as qtest & the block tests take some > considerable time to run. IOW people who want speed will be running > "make check-unit" or individual tests already. "make check" takes under 2m on my laptop, so I take that to be reasonably fast, i.e. I'd be happy to add other small (few second) tests to it. e.g. with your Makefile included like you had done, the scripts to check ordering of some files could go in it. Dave > > Regards, > Daniel > -- > |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On 18/12/2015 14:35, Dr. David Alan Gilbert wrote: > > That would not be recursive make, but rather a completely separate > > Makefile to be manually invoked with -f. > > Hmm but wouldn't this Makefile also be a good place for small-fast > style check scripts that could be included in make check ? Yes, but there is no benefit from inclusion, since these tests do not depend on anything having been built already. Paolo
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On Fri, Dec 18, 2015 at 01:35:13PM +, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonz...@redhat.com) wrote: > > > > > > On 18/12/2015 14:05, Daniel P. Berrange wrote: > > > > > + > > > > > +cs-glib-syms: > > > > > + @perl scripts/glib-syms.pl $(GLIB_SYMS_LIST) $(C_CODE_FILES) > > > > > > > > > > > > Does this need to be included, or could it be a separate Makefile > > > > invoked with e.g. make -f scripts/Makefile.style? > > > > > > Any particular reason to favour that over include ? I did it this > > > way because QEMU in general seems to be biased towards includes > > > and not recursive make > > > > That would not be recursive make, but rather a completely separate > > Makefile to be manually invoked with -f. > > Hmm but wouldn't this Makefile also be a good place for small-fast > style check scripts that could be included in make check ? Nothing about "make check" is fast, so we could probably just wire it all into make check by default, as qtest & the block tests take some considerable time to run. IOW people who want speed will be running "make check-unit" or individual tests already. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
* Paolo Bonzini (pbonz...@redhat.com) wrote: > > > On 18/12/2015 14:05, Daniel P. Berrange wrote: > > > > + > > > > +cs-glib-syms: > > > > + @perl scripts/glib-syms.pl $(GLIB_SYMS_LIST) $(C_CODE_FILES) > > > > > > > > > Does this need to be included, or could it be a separate Makefile > > > invoked with e.g. make -f scripts/Makefile.style? > > > > Any particular reason to favour that over include ? I did it this > > way because QEMU in general seems to be biased towards includes > > and not recursive make > > That would not be recursive make, but rather a completely separate > Makefile to be manually invoked with -f. Hmm but wouldn't this Makefile also be a good place for small-fast style check scripts that could be included in make check ? Dave > > Paolo -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On 18/12/2015 14:05, Daniel P. Berrange wrote: > > > + > > > +cs-glib-syms: > > > + @perl scripts/glib-syms.pl $(GLIB_SYMS_LIST) $(C_CODE_FILES) > > > > > > Does this need to be included, or could it be a separate Makefile > > invoked with e.g. make -f scripts/Makefile.style? > > Any particular reason to favour that over include ? I did it this > way because QEMU in general seems to be biased towards includes > and not recursive make That would not be recursive make, but rather a completely separate Makefile to be manually invoked with -f. Paolo
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On Fri, Dec 18, 2015 at 01:36:00PM +0100, Paolo Bonzini wrote: > > > On 18/12/2015 12:36, Daniel P. Berrange wrote: > > diff --git a/scripts/Makefile b/scripts/Makefile > > new file mode 100644 > > index 000..162e7e9 > > --- /dev/null > > +++ b/scripts/Makefile > > @@ -0,0 +1,24 @@ > > +# > > +# This makefile runs various style checks across the entire > > +# source tree. > > +# > > +# This is similar in concept of checkpatch.pl, but enforces > > +# rules across the entire codebase, not just new patches > > +# > > + > > +STYLE_CHECKS = \ > > + cs-glib-syms > > + > > +ALL_FILES = \ > > + $(shell git ls-tree -r HEAD . | awk '{print $$4}') > > + > > +C_CODE_FILES = $(filter %.c %.h, $(ALL_FILES)) > > + > > +check-style: $(STYLE_CHECKS) > > + > > +# Check that we only use glib symbols present in our > > +# minimum declared glib version > > +GLIB_SYMS_LIST = scripts/glib-syms.txt > > + > > +cs-glib-syms: > > + @perl scripts/glib-syms.pl $(GLIB_SYMS_LIST) $(C_CODE_FILES) > > > Does this need to be included, or could it be a separate Makefile > invoked with e.g. make -f scripts/Makefile.style? Any particular reason to favour that over include ? I did it this way because QEMU in general seems to be biased towards includes and not recursive make > > +# Symbols not present in the release that we depend > > +# on, but which have wrappers in include/glib-compat.h > > +my @compatsyms = qw( > > +g_get_monotonic_time > > + > > +g_assert_true > > +g_assert_false > > +g_assert_null > > +g_assert_nonnull > > +g_assert_cmpmem > > + > > +g_hash_table_add > > + > > +g_cond_clear > > +g_cond_init > > +g_cond_wait_until > > + > > +g_mutex_init > > +g_mutex_clear > > + > > +g_thread_new > > + > > +g_private_replace > > +G_PRIVATE_INIT > > + > > +G_TIME_SPAN_SECOND > > +); > > + > > + > > +# Functions defined inside QEMU which are using the > > +# the same "g_" function name prefix as glib, so > > +# get mis-detected as glib symbols > > +my @blacklist = qw( > > +g_to_float64 > > +g_assert_no_errno > > +g_cclosure_new_swap > > +g_free_rcu > > +g_test_trap_subprocess > > +g_poll_fixed > > +g_list_insert_sorted_merged > > +G_BYTE > > +); > > + > > +# GObject stuff used by gtk frontend > > +my @gobjectsums = qw( > > +g_object_ref > > +g_object_unref > > +g_object_set_data > > +g_signal_connect > > +G_CALLBACK > > +); > > + > > +# Functions defined by glib which are strangely > > +# missing from their docs header index > > +my @missingindex = qw( > > +g_assertion_message > > +g_assertion_message_expr > > +g_assertion_message_cmpstr > > +g_assertion_message_cmpnum > > +g_assertion_message_error > > +); > > + > > Can we "parse" #.* as comments, and put these in a glib-syms-extra.txt > file? Then we can hypothetically do the same tests in checkpatch.pl too. Yep, that works. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
On 18/12/2015 12:36, Daniel P. Berrange wrote: > diff --git a/scripts/Makefile b/scripts/Makefile > new file mode 100644 > index 000..162e7e9 > --- /dev/null > +++ b/scripts/Makefile > @@ -0,0 +1,24 @@ > +# > +# This makefile runs various style checks across the entire > +# source tree. > +# > +# This is similar in concept of checkpatch.pl, but enforces > +# rules across the entire codebase, not just new patches > +# > + > +STYLE_CHECKS = \ > + cs-glib-syms > + > +ALL_FILES = \ > + $(shell git ls-tree -r HEAD . | awk '{print $$4}') > + > +C_CODE_FILES = $(filter %.c %.h, $(ALL_FILES)) > + > +check-style: $(STYLE_CHECKS) > + > +# Check that we only use glib symbols present in our > +# minimum declared glib version > +GLIB_SYMS_LIST = scripts/glib-syms.txt > + > +cs-glib-syms: > + @perl scripts/glib-syms.pl $(GLIB_SYMS_LIST) $(C_CODE_FILES) Does this need to be included, or could it be a separate Makefile invoked with e.g. make -f scripts/Makefile.style? > +# Symbols not present in the release that we depend > +# on, but which have wrappers in include/glib-compat.h > +my @compatsyms = qw( > +g_get_monotonic_time > + > +g_assert_true > +g_assert_false > +g_assert_null > +g_assert_nonnull > +g_assert_cmpmem > + > +g_hash_table_add > + > +g_cond_clear > +g_cond_init > +g_cond_wait_until > + > +g_mutex_init > +g_mutex_clear > + > +g_thread_new > + > +g_private_replace > +G_PRIVATE_INIT > + > +G_TIME_SPAN_SECOND > +); > + > + > +# Functions defined inside QEMU which are using the > +# the same "g_" function name prefix as glib, so > +# get mis-detected as glib symbols > +my @blacklist = qw( > +g_to_float64 > +g_assert_no_errno > +g_cclosure_new_swap > +g_free_rcu > +g_test_trap_subprocess > +g_poll_fixed > +g_list_insert_sorted_merged > +G_BYTE > +); > + > +# GObject stuff used by gtk frontend > +my @gobjectsums = qw( > +g_object_ref > +g_object_unref > +g_object_set_data > +g_signal_connect > +G_CALLBACK > +); > + > +# Functions defined by glib which are strangely > +# missing from their docs header index > +my @missingindex = qw( > +g_assertion_message > +g_assertion_message_expr > +g_assertion_message_cmpstr > +g_assertion_message_cmpnum > +g_assertion_message_error > +); > + Can we "parse" #.* as comments, and put these in a glib-syms-extra.txt file? Then we can hypothetically do the same tests in checkpatch.pl too. I'm not opposing the scripts/Makefile concept, and I'm not asking you to patch checkpatch.pl; I just would like to keep the door open for that. Paolo
[Qemu-devel] [PATCH] scripts: provide a script for checking glib symbol usage
There are frequently patches submitted for merge which use symbols only defined by newer glib versions that the max version currently permitted by QEMU (ie glib==2.22). The glib 2.32 release introduced GLIB_VERSION_MIN_REQUIRED and GLIB_VERSION_MAX_ALLOWED macros to allow apps to declare what they want. Unfortunately QEMU has a glib-compat.h file which backports certain functions from newer glib which confuses the glib version checking macros. This is effectively preventing their use in QEMU. The version check macros also don't cover all versions back to 2.22 so any checks are incomplete. This patch takes a different approach, by providing a script that can validate that source files are only using glib functions & macros present in the minimum declared glib version, by doing static analysis of the source and comparing to a previously defined list of permitted symbols. The list of functions/macros was extracted from the glib 2.22 HTML docs symbol index using $ grep '' /usr/share/gtk-doc/html/glib/ix01.html | \ awk {'print $1}' | \ sed -e 's///' -e 's/,//' | \ grep -E -i '^g_' | \ sort > scripts/glib-syms.txt The script works by simply searching through the .c and .h files looking for anything that looks like a function or macro call, starting with a 'G_' or 'g_' prefix. This is not a 100% exact science, but with a few override lists it provides a practical/usable level of detection of mistakes. This patch creates a 'scripts/Makefile' file which is intended to accumulate various other style checks, similar to those done by 'checkpatch.pl', but operating against the entire source tree rather than just new patches. The check can be invoked using 'make check-style' in the top level directory. Example output when the check fails would look like: $ make check-style tests/test-io-task.c:151: 'g_thread_ref' symbol not provided in min glib version tests/test-io-task.c:172: 'g_thread_ref' symbol not provided in min glib version tests/test-io-task.c:216: 'g_thread_unref' symbol not provided in min glib version tests/test-io-task.c:217: 'g_thread_unref' symbol not provided in min glib version tests/test-io-task.c:260: 'g_thread_unref' symbol not provided in min glib version tests/test-io-task.c:261: 'g_thread_unref' symbol not provided in min glib version Signed-off-by: Daniel P. Berrange --- Makefile |1 + scripts/Makefile | 24 + scripts/glib-syms.pl | 131 + scripts/glib-syms.txt | 1513 + 4 files changed, 1669 insertions(+) create mode 100644 scripts/Makefile create mode 100644 scripts/glib-syms.pl create mode 100644 scripts/glib-syms.txt diff --git a/Makefile b/Makefile index af3e5f1..88a7649 100644 --- a/Makefile +++ b/Makefile @@ -165,6 +165,7 @@ dummy := $(call unnest-vars,, \ ifneq ($(wildcard config-host.mak),) include $(SRC_PATH)/tests/Makefile +include $(SRC_PATH)/scripts/Makefile endif all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules diff --git a/scripts/Makefile b/scripts/Makefile new file mode 100644 index 000..162e7e9 --- /dev/null +++ b/scripts/Makefile @@ -0,0 +1,24 @@ +# +# This makefile runs various style checks across the entire +# source tree. +# +# This is similar in concept of checkpatch.pl, but enforces +# rules across the entire codebase, not just new patches +# + +STYLE_CHECKS = \ + cs-glib-syms + +ALL_FILES = \ + $(shell git ls-tree -r HEAD . | awk '{print $$4}') + +C_CODE_FILES = $(filter %.c %.h, $(ALL_FILES)) + +check-style: $(STYLE_CHECKS) + +# Check that we only use glib symbols present in our +# minimum declared glib version +GLIB_SYMS_LIST = scripts/glib-syms.txt + +cs-glib-syms: + @perl scripts/glib-syms.pl $(GLIB_SYMS_LIST) $(C_CODE_FILES) diff --git a/scripts/glib-syms.pl b/scripts/glib-syms.pl new file mode 100644 index 000..e4b50c1 --- /dev/null +++ b/scripts/glib-syms.pl @@ -0,0 +1,131 @@ +#!/usr/bin/perl +# +# Copyright (C) 2015 Red Hat Inc. +# +# This work is licensed under the terms of the GNU GPL, version 2 +# or later. See the COPYING file in the top-level directory. +# +# This parses *.c and *.h files in QEMU source tree to +# extract a list of functions used from the glib +# library and compares them to the list of symbols +# present in the minimum required glib version. + +use strict; +use warnings; + +# The file containing the list of glib symbols +# Generated by from the glib 2.22 HTML docs index +# of symbols file using +# +# $ grep '' /usr/share/gtk-doc/html/glib/ix01.html | \ +#awk {'print $1}' | \ +#sed -e 's///' -e 's/,//' | \ +#grep -E -i '^g_' | \ +#sort > scripts/glib-syms.txt +# +# Newer versions might need different grep/awk/sed rules +my $syms = shift @ARGV; + + +# Symbols not present in the release that we depend +# on, but which have wrappers in include/glib-compat.h +my @compatsyms = qw( +g_get_monotonic_time + +g_assert_true +g_assert_false +g_assert_null +g_assert_n