[XEN PATCH v2 0/4] xen: rework compat headers generation

[XEN PATCH v2 0/4] xen: rework compat headers generation

[Anthony PERARD]

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.build-system-xen-include-rework-v2

v2:
- new patch [1/4] to fix issue with command line that can be way too long
- other small changes, and reorder patches

Hi,

This patch series is about 2 improvement. First one is to use $(if_changed, )
in "include/Makefile" to make the generation of the compat headers less verbose
and to have the command line part of the decision to rebuild the headers.
Second one is to replace one slow script by a much faster one, and save time
when generating the headers.

There's some number here:
    https://lore.kernel.org/xen-devel/Yp3%2F%2Fc%2FCAcwLHCvi@perard.uk.xensource.com/

    On my machine when doing a full build [of the hypervisor], with `ccache`
    enabled, it saves about 1.17 seconds (out of ~17s), and without ccache, it
    saves about 2.0 seconds (out of ~37s).

Thanks.

Anthony PERARD (4):
  build,include: rework shell script for headers++.chk
  build: remove auto.conf prerequisite from compat/xlat.h target
  build: set PERL
  build: replace get-fields.sh by a perl script

 xen/Makefile                    |   1 +
 xen/include/Makefile            |  25 +-
 README                          |   1 +
 xen/tools/compat-xlat-header.pl | 539 ++++++++++++++++++++++++++++++++
 xen/tools/get-fields.sh         | 528 -------------------------------
 5 files changed, 557 insertions(+), 537 deletions(-)
 create mode 100755 xen/tools/compat-xlat-header.pl
 delete mode 100644 xen/tools/get-fields.sh

-- 
Anthony PERARD

[XEN PATCH v2 1/4] build,include: rework shell script for headers++.chk

[Anthony PERARD]

The command line generated for headers++.chk by make is quite long,
and in some environment it is too long. This issue have been seen in
Yocto build environment.

Error messages:
    make[9]: execvp: /bin/sh: Argument list too long
    make[9]: *** [include/Makefile:181: include/headers++.chk] Error 127

Rework so that we do the foreach loop in shell rather that make to
reduce the command line size by a lot. We also need a way to get
headers prerequisite for some public headers so we use a switch "case"
in shell to be able to do some simple pattern matching. Variables
alone in POSIX shell don't allow to work with associative array or
variables with "/".

Reported-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
Fixes: 28e13c7f43 ("build: xen/include: use if_changed")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---

Notes:
    v2:
    - fix typo in commit message
    - fix out-of-tree build
    
    v1:
    - was sent as a reply to v1 of the series

 xen/include/Makefile | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 6d9bcc19b0..49c75a78f9 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -158,13 +158,22 @@ define cmd_headerscxx_chk
 	    touch $@.new;                                                     \
 	    exit 0;                                                           \
 	fi;                                                                   \
-	$(foreach i, $(filter %.h,$^),                                        \
-	    echo "#include "\"$(i)\"                                          \
+	get_prereq() {                                                        \
+	    case $$1 in                                                       \
+	    $(foreach i, $(filter %.h,$^),                                    \
+	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
+		$(i)$(close)                                                  \
+		echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
+			-include c$(j))";;))                                  \
+	    esac;                                                             \
+	};                                                                    \
+	for i in $(filter %.h,$^); do                                         \
+	    echo "#include "\"$$i\"                                           \
 	    | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
 	      -include stdint.h -include $(srcdir)/public/xen.h               \
-	      $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include c$(j)) \
+	      `get_prereq $$i`                                                \
 	      -S -o /dev/null -                                               \
-	    || exit $$?; echo $(i) >> $@.new;) \
+	    || exit $$?; echo $$i >> $@.new; done;                            \
 	mv $@.new $@
 endef
 
-- 
Anthony PERARD

[XEN PATCH v2 2/4] build: remove auto.conf prerequisite from compat/xlat.h target

[Anthony PERARD]

Now that the command line generating "xlat.h" is check on rebuild, the
header will be regenerated whenever the list of xlat headers changes
due to change in ".config". We don't need to force a regeneration for
every changes in ".config".

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/include/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 49c75a78f9..0d3e3d66e0 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -101,7 +101,7 @@ cmd_xlat_h = \
 	cat $(filter %.h,$^) >$@.new; \
 	mv -f $@.new $@
 
-$(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) $(obj)/config/auto.conf FORCE
+$(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) FORCE
 	$(call if_changed,xlat_h)
 
 ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
-- 
Anthony PERARD

[XEN PATCH v2 3/4] build: set PERL

[Anthony PERARD]

We are going to use it in a moment.

Also update README about Perl requirement.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v2:
    - update ./README

 xen/Makefile | 1 +
 README       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/Makefile b/xen/Makefile
index 82f5310b12..a6650a2acc 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -22,6 +22,7 @@ PYTHON_INTERPRETER	:= $(word 1,$(shell which python3 python python2 2>/dev/null)
 export PYTHON		?= $(PYTHON_INTERPRETER)
 
 export CHECKPOLICY	?= checkpolicy
+export PERL		?= perl
 
 $(if $(filter __%, $(MAKECMDGOALS)), \
     $(error targets prefixed with '__' are only for internal use))
diff --git a/README b/README
index 5e55047ffd..c1c18de7e0 100644
--- a/README
+++ b/README
@@ -64,6 +64,7 @@ provided by your OS distributor:
     * iproute package (/sbin/ip)
     * GNU bison and GNU flex
     * ACPI ASL compiler (iasl)
+    * Perl
 
 In addition to the above there are a number of optional build
 prerequisites. Omitting these will cause the related features to be
-- 
Anthony PERARD

[XEN PATCH v2 4/4] build: replace get-fields.sh by a perl script

[Anthony PERARD]

The get-fields.sh which generate all the include/compat/.xlat/*.h
headers is quite slow. It takes for example nearly 3 seconds to
generate platform.h on a recent machine, or 2.3 seconds for memory.h.

Since it's only text processing, rewriting the mix of shell/sed/python
into a single perl script make the generation of those file a lot
faster.

I tried to keep a similar look for the code, to keep the code similar
between the shell and perl, and to ease review. So some code in perl
might look weird or could be written better.

No functional change, the headers generated are identical.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v2:
    - Add .pl extension to the perl script
    - remove "-w" from the shebang as it is duplicate of "use warning;"
    - Add a note in the commit message that the "headers generated are identical".

 xen/include/Makefile            |   6 +-
 xen/tools/compat-xlat-header.pl | 539 ++++++++++++++++++++++++++++++++
 xen/tools/get-fields.sh         | 528 -------------------------------
 3 files changed, 541 insertions(+), 532 deletions(-)
 create mode 100755 xen/tools/compat-xlat-header.pl
 delete mode 100644 xen/tools/get-fields.sh

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 0d3e3d66e0..31b0be14bc 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -60,9 +60,7 @@ cmd_compat_c = \
 
 quiet_cmd_xlat_headers = GEN     $@
 cmd_xlat_headers = \
-    while read what name; do \
-        $(SHELL) $(srctree)/tools/get-fields.sh "$$what" compat_$$name $< || exit $$?; \
-    done <$(patsubst $(obj)/compat/%,$(obj)/compat/.xlat/%,$(basename $<)).lst >$@.new; \
+    $(PERL) $(srctree)/tools/compat-xlat-header.pl $< $(patsubst $(obj)/compat/%,$(obj)/compat/.xlat/%,$(basename $<)).lst > $@.new; \
     mv -f $@.new $@
 
 targets += $(headers-y)
@@ -80,7 +78,7 @@ $(obj)/compat/%.c: $(src)/public/%.h $(srcdir)/xlat.lst $(srctree)/tools/compat-
 	$(call if_changed,compat_c)
 
 targets += $(patsubst compat/%, compat/.xlat/%, $(headers-y))
-$(obj)/compat/.xlat/%.h: $(obj)/compat/%.h $(obj)/compat/.xlat/%.lst $(srctree)/tools/get-fields.sh FORCE
+$(obj)/compat/.xlat/%.h: $(obj)/compat/%.h $(obj)/compat/.xlat/%.lst $(srctree)/tools/compat-xlat-header.pl FORCE
 	$(call if_changed,xlat_headers)
 
 quiet_cmd_xlat_lst = GEN     $@
diff --git a/xen/tools/compat-xlat-header.pl b/xen/tools/compat-xlat-header.pl
new file mode 100755
index 0000000000..791230591c
--- /dev/null
+++ b/xen/tools/compat-xlat-header.pl
@@ -0,0 +1,539 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+open COMPAT_LIST, "<$ARGV[1]" or die "can't open $ARGV[1], $!";
+open HEADER, "<$ARGV[0]" or die "can't open $ARGV[0], $!";
+
+my @typedefs;
+
+my @header_tokens;
+while (<HEADER>) {
+    next if m/^\s*#.*/;
+    s/([\]\[,;:{}])/ $1 /g;
+    s/^\s+//;
+    push(@header_tokens, split(/\s+/));
+}
+
+sub get_fields {
+    my ($looking_for) = @_;
+    my $level = 1;
+    my $aggr = 0;
+    my ($name, @fields);
+
+    foreach (@header_tokens) {
+        if (/^(struct|union)$/) {
+            unless ($level != 1) {
+                $aggr = 1;
+                @fields = ();
+                $name = '';
+            }
+        } elsif ($_ eq '{') {
+            $level++;
+        } elsif ($_ eq '}') {
+            $level--;
+            if ($level == 1 and $name eq $looking_for) {
+                push (@fields, $_);
+                return @fields;
+            }
+        } elsif (/^[a-zA-Z_].*/) {
+            unless ($aggr == 0 or $name ne "") {
+                $name = $_;
+            }
+        }
+        unless ($aggr == 0) {
+            push (@fields, $_);
+        }
+    }
+    return ();
+}
+
+sub get_typedefs {
+    my $level = 1;
+    my $state = 0;
+    my @typedefs;
+    foreach (@_) {
+        if ($_ eq 'typedef') {
+            unless ($level != 1) {
+                $state = 1;
+            }
+        } elsif (m/^COMPAT_HANDLE\(.*\)$/) {
+            unless ($level != 1 or $state != 1) {
+                $state = 2;
+            }
+        } elsif (m/^[\[\{]$/) {
+            $level++;
+        } elsif (m/^[\]\}]$/) {
+            $level--;
+        } elsif ($_ eq ';') {
+            unless  ($level != 1) {
+                $state = 0;
+            }
+        } elsif (m/^[a-zA-Z_].*$/) {
+            unless ($level != 1 or $state != 2) {
+                push (@typedefs, $_);
+            }
+        }
+    }
+    return @typedefs;
+}
+
+sub build_enums {
+    my ($name, @tokens) = @_;
+
+    my $level = 1;
+    my $kind = '';
+    my $named = '';
+    my (@fields, @members, $id);
+
+    foreach (@tokens) {
+        if (m/^(struct|union)$/) {
+            unless ($level != 2) {
+                @fields = ('');
+            }
+            $kind="$_;$kind";
+        } elsif ($_ eq '{') {
+            $level++;
+        } elsif ($_ eq '}') {
+            $level--;
+            if ($level == 1) {
+                my $subkind = $kind;
+                $subkind =~ s/;.*//;
+                if ($subkind eq 'union') {
+                    print "\nenum XLAT_$name {\n";
+                    foreach (@members) {
+                        print "    XLAT_${name}_$_,\n";
+                    }
+                    print "};\n";
+                }
+                return;
+            } elsif ($level == 2) {
+                $named = '?';
+            }
+        } elsif (/^[a-zA-Z_].*$/) {
+            $id = $_;
+            my $k = $kind;
+            $k =~ s/.*?;//;
+            if ($named ne '' and $k ne '') {
+                shift @fields if @fields > 0 and $fields[0] eq '';
+                build_enums("${name}_$_", @fields);
+                $named = '!';
+            }
+        } elsif ($_ eq ',') {
+            unless ($level != 2) {
+                push (@members, $id);
+            }
+        } elsif ($_ eq ';') {
+            unless ($level != 2) {
+                push (@members, $id);
+            }
+            unless ($named eq '') {
+                $kind =~ s/.*?;//;
+            }
+            $named = '';
+        }
+        unless (@fields == 0) {
+            push (@fields, $_);
+        }
+    }
+}
+
+sub handle_field {
+    my ($prefix, $name, $id, $type, @fields) = @_;
+
+    if (@fields == 0) {
+        print " \\\n";
+        if ($type eq '') {
+            print "$prefix\(_d_)->$id = (_s_)->$id;"
+        } else {
+            my $k = $id;
+            $k =~ s/\./_/g;
+            print "${prefix}XLAT_${name}_HNDL_${k}(_d_, _s_);";
+        }
+    } elsif ("@fields" !~ m/[{}]/) {
+        my $tag = "@fields";
+        $tag =~ s/\s*(struct|union)\s+(compat_)?(\w+)\s.*/$3/;
+        print " \\\n";
+        print "${prefix}XLAT_$tag(&(_d_)->$id, &(_s_)->$id);"
+    } else {
+        my $func_id = $id;
+        my @func_tokens = @fields;
+        my $kind = '';
+        my $array = "";
+        my $level = 1;
+        my $arrlvl = 1;
+        my $array_type = '';
+        my $id = '';
+        my $type = '';
+        @fields = ();
+        foreach (@func_tokens) {
+            if (/^(struct|union)$/) {
+                unless ($level != 2) {
+                    @fields = ('');
+                }
+                if ($level == 1) {
+                    $kind = $_;
+                    if ($kind eq 'union') {
+                        my $tmp = $func_id;
+                        $tmp =~ s/\./_/g;
+                        print " \\\n";
+                        print  "${prefix}switch ($tmp) {"
+                    }
+                }
+            } elsif ($_ eq '{') {
+                $level++;
+                $id = '';
+            } elsif ($_ eq '}') {
+                $level--;
+                $id = '';
+                if ($level == 1 and $kind eq 'union') {
+                    print " \\\n";
+                    print "$prefix}"
+                }
+            } elsif ($_ eq '[') {
+                if ($level != 2 or $arrlvl != 1) {
+                    # skip
+                } elsif ($array eq '') {
+                    $array = ' ';
+                } else {
+                    $array = "$array;";
+                }
+                $arrlvl++;
+            } elsif ($_ eq ']') {
+                $arrlvl--;
+            } elsif (m/^COMPAT_HANDLE\((.*)\)$/) {
+                if ($level == 2 and $id eq '') {
+                    $type = $1;
+                    $type =~ s/^compat_//;
+                }
+            } elsif ($_ eq "compat_domain_handle_t") {
+                if ($level == 2 and $id eq '') {
+                    $array_type = $_;
+                }
+            } elsif (m/^[a-zA-Z_].*$/) {
+                if ($id eq '' and $type eq '' and $array_type eq '') {
+                    foreach $id (@typedefs) {
+                        unless ($id ne $_) {
+                            $type = $id;
+                        }
+                    }
+                    if ($type eq '') {
+                        $id = $_;
+                    } else {
+                        $id = '';
+                    }
+                } else {
+                    $id = $_;
+                }
+            } elsif (m/^[,;]$/) {
+                if ($level == 2 and $id !~ /^_pad\d*$/) {
+                    if ($kind eq 'union') {
+                        my $tmp = "$func_id.$id";
+                        $tmp =~ s/\./_/g;
+                        print " \\\n";
+                        print  "${prefix}case XLAT_${name}_$tmp:";
+                        shift @fields if @fields > 0 and $fields[0] eq '';
+                        handle_field("$prefix    ", $name,  "$func_id.$id", $type, @fields);
+                    } elsif ($array eq '' and $array_type eq '') {
+                        shift @fields if @fields > 0 and $fields[0] eq '';
+                        handle_field($prefix, $name, "$func_id.$id", $type, @fields);
+                    } elsif ($array eq '') {
+                        copy_array("    ", "$func_id.$id");
+                    } else {
+                        $array =~ s/^.*?;//;
+                        shift @fields if @fields > 0 and $fields[0] eq '';
+                        handle_array($prefix, $name, "$func_id.$id", $array, $type, @fields);
+                    }
+                    unless ($_ ne ';') {
+                        @fields = ();
+                        $id = '';
+                        $type = '';
+                    }
+                    $array = '';
+                    if ($kind eq 'union') {
+                        print " \\\n";
+                        print "$prefix    break;";
+                    }
+                }
+            } else {
+                if ($array ne '') {
+                    $array = "$array $_";
+                }
+            }
+            unless (@fields == 0) {
+                push (@fields, $_);
+            }
+        }
+    }
+}
+
+sub copy_array {
+    my ($prefix, $id) = @_;
+
+    print " \\\n";
+    print  "${prefix}if ((_d_)->$id != (_s_)->$id) \\\n";
+    print  "$prefix    memcpy((_d_)->$id, (_s_)->$id, sizeof((_d_)->$id));";
+}
+
+sub handle_array {
+    my ($prefix, $name, $id, $array, $type, @fields) = @_;
+
+    my $i = $array;
+    $i =~ s/[^;]//g;
+    $i = length($i);
+    $i = "i$i";
+
+    print " \\\n";
+    print "$prefix\{ \\\n";
+    print "$prefix    unsigned int $i; \\\n";
+    my $tmp = $array;
+    $tmp =~ s/;.*$//;
+    $tmp =~ s/^\s*(.*)\s*$/$1/;
+    print "$prefix    for ($i = 0; $i < $tmp; ++$i) {";
+    if ($array !~ m/^.*?;/) {
+        handle_field("$prefix        ", $name, "$id\[$i]", $type, @fields);
+    } else {
+        handle_array("$prefix        " ,$name, "$id\[$i]", $', $type, @fields);
+    }
+    print " \\\n";
+    print "$prefix    } \\\n";
+    print "$prefix\}";
+}
+
+sub build_body {
+    my ($name, @tokens) = @_;
+    my $level = 1;
+    my $id = '';
+    my $array = '';
+    my $arrlvl = 1;
+    my $array_type = '';
+    my $type = '';
+    my @fields;
+
+    printf "\n#define XLAT_$name(_d_, _s_) do {";
+
+    foreach (@tokens) {
+        if (/^(struct|union)$/) {
+            unless ($level != 2) {
+                @fields = ('');
+            }
+        } elsif ($_ eq '{') {
+            $level++;
+            $id = '';
+        } elsif ($_ eq '}') {
+            $level--;
+            $id = '';
+        } elsif ($_ eq '[') {
+            if ($level != 2 or $arrlvl != 1) {
+                # skip
+            } elsif ($array eq '') {
+                $array = ' ';
+            } else {
+                $array = "$array;";
+            }
+            $arrlvl++;
+        } elsif ($_ eq ']') {
+            $arrlvl--;
+        } elsif (m/^COMPAT_HANDLE\((.*)\)$/) {
+            if ($level == 2 and $id eq '') {
+                $type = $1;
+                $type =~ s/^compat_//;
+            }
+        } elsif ($_ eq "compat_domain_handle_t") {
+            if ($level == 2 and $id eq '') {
+                $array_type = $_;
+            }
+        } elsif (m/^[a-zA-Z_].*$/) {
+            if ($array ne '') {
+                $array = "$array $_";
+            } elsif ($id eq '' and $type eq '' and $array_type eq '') {
+                foreach $id (@typedefs) {
+                    unless ($id eq $_) {
+                        $type = $id;
+                    }
+                }
+                if ($type eq '') {
+                    $id = $_;
+                } else {
+                    $id = '';
+                }
+            } else {
+                $id = $_;
+            }
+        } elsif (m/^[,;]$/) {
+            if ($level == 2 and $id !~ /^_pad\d*$/) {
+                if ($array eq '' and $array_type eq '') {
+                    shift @fields if @fields > 0 and $fields[0] eq '';
+                    handle_field("    ", $name, $id, $type, @fields);
+                } elsif ($array eq '') {
+                    copy_array("    ", $id);
+                } else {
+                    my $tmp = $array;
+                    $tmp =~ s/^.*?;//;
+                    shift @fields if @fields > 0 and $fields[0] eq '';
+                    handle_array("    ", $name, $id, $tmp, $type, @fields);
+                }
+                unless ($_ ne ';') {
+                    @fields = ();
+                    $id = '';
+                    $type = '';
+                }
+                $array = '';
+            }
+        } else {
+            if ($array ne '') {
+                $array = "$array $_";
+            }
+        }
+        unless (@fields == 0) {
+            push (@fields, $_);
+        }
+    }
+    print " \\\n} while (0)\n";
+}
+
+sub check_field {
+    my ($kind, $name, $field, @extrafields) = @_;
+
+    if ("@extrafields" !~ m/[{}]/) {
+        print "; \\\n";
+        if (@extrafields != 0) {
+            foreach (@extrafields) {
+                if (m/^(struct|union)$/) {
+                    # skip
+                } elsif (m/^[a-zA-Z_].*/) {
+                    s/^xen_//;
+                    print "    CHECK_$_";
+                    last;
+                } else {
+                    die "Malformed compound declaration: '$_'";
+                }
+            }
+        } elsif ($field !~ m/\./) {
+            print "    CHECK_FIELD_($kind, $name, $field)";
+        } else {
+            my $n = $field =~ s/\./, /g;
+            print  "    CHECK_SUBFIELD_${n}_($kind, $name, $field)";
+        }
+    } else {
+        my $level = 1;
+        my @fields = ();
+        my $id = '';
+
+        foreach (@extrafields) {
+            if (m/^(struct|union)$/) {
+                unless ($level != 2) {
+                    @fields = ('');
+                }
+            } elsif ($_ eq '{') {
+                $level++;
+                $id = '';
+            } elsif ($_ eq '}') {
+                $level--;
+                $id = '';
+            } elsif (/^compat_.*_t$/) {
+                if ($level == 2) {
+                    @fields = ('');
+                    s/_t$//;
+                    s/^compat_//;
+                }
+            } elsif (/^evtchn_.*_compat_t$/) {
+                if ($level == 2 and $_ ne "evtchn_port_compat_t") {
+                    @fields = ('');
+                    s/_compat_t$//;
+                }
+            } elsif (/^[a-zA-Z_].*$/) {
+                $id = $_;
+            } elsif (/^[,;]$/) {
+                if ($level == 2 and $id !~ /^_pad\d*$/) {
+                    shift @fields if @fields > 0 and $fields[0] eq '';
+                    check_field($kind, $name, "$field.$id", @fields);
+                    unless ($_ ne ";") {
+                        @fields = ();
+                        $id = '';
+                    }
+                }
+            }
+            unless (@fields == 0) {
+                push (@fields, $_);
+            }
+        }
+    }
+}
+
+sub build_check {
+    my ($name, @tokens) = @_;
+    my $level = 1;
+    my (@fields, $kind, $id);
+    my $arrlvl = 1;
+
+    print "\n";
+    print "#define CHECK_$name \\\n";
+
+    foreach (@tokens) {
+        if (/^(struct|union)$/) {
+            if ($level == 1) {
+                $kind = $_;
+                print "    CHECK_SIZE_($kind, $name)";
+            } elsif ($level == 2) {
+                @fields = ('');
+            }
+        } elsif ($_ eq '{') {
+            $level++;
+            $id = '';
+        } elsif ($_ eq '}') {
+            $level--;
+            $id = '';
+        } elsif ($_ eq '[') {
+            $arrlvl++;
+        } elsif ($_ eq ']') {
+            $arrlvl--;
+        } elsif (/^compat_.*_t$/) {
+            if ($level == 2 and $_ ne "compat_argo_port_t") {
+                @fields = ('');
+                s/_t$//;
+                s/^compat_//;
+            }
+        } elsif (/^[a-zA-Z_].*$/) {
+            unless ($level != 2 or $arrlvl != 1) {
+                $id = $_;
+            }
+        } elsif (/^[,;]$/) {
+            if ($level == 2 and $id !~ /^_pad\d*$/) {
+                shift @fields if @fields > 0 and $fields[0] eq '';
+                check_field($kind, $name, $id, @fields);
+                unless ($_ ne ";") {
+                    @fields = ();
+                    $id = '';
+                }
+            }
+        }
+
+        unless (@fields == 0) {
+            push (@fields, $_);
+        }
+    }
+    print "\n";
+}
+
+@typedefs = get_typedefs(@header_tokens);
+
+while (<COMPAT_LIST>) {
+    my ($what, $name) = split(/\s+/, $_);
+    $name =~ s/^xen//;
+
+    my @fields = get_fields("compat_$name");
+    if (@fields == 0) {
+	die "Fields of 'compat_$name' not found in '$ARGV[1]'";
+    }
+
+    if ($what eq "!") {
+        build_enums($name, @fields);
+        build_body($name, @fields);
+    } elsif ($what eq "?") {
+        build_check($name, @fields);
+    } else {
+        die "Invalid translation indicator: '$what'";
+    }
+}
diff --git a/xen/tools/get-fields.sh b/xen/tools/get-fields.sh
deleted file mode 100644
index 002db2093f..0000000000
--- a/xen/tools/get-fields.sh
+++ /dev/null
@@ -1,528 +0,0 @@
-test -n "$1" -a -n "$2" -a -n "$3"
-set -ef
-
-SED=sed
-if test -x /usr/xpg4/bin/sed; then
-	SED=/usr/xpg4/bin/sed
-fi
-if test -z ${PYTHON}; then
-	PYTHON=`/usr/bin/env python`
-fi
-if test -z ${PYTHON}; then
-	echo "Python not found"
-	exit 1
-fi
-
-get_fields ()
-{
-	local level=1 aggr=0 name= fields=
-	for token in $2
-	do
-		case "$token" in
-		struct|union)
-			test $level != 1 || aggr=1 fields= name=
-			;;
-		"{")
-			level=$(expr $level + 1)
-			;;
-		"}")
-			level=$(expr $level - 1)
-			if [ $level = 1 -a $name = $1 ]
-			then
-				echo "$fields }"
-				return 0
-			fi
-			;;
-		[a-zA-Z_]*)
-			test $aggr = 0 -o -n "$name" || name="$token"
-			;;
-		esac
-		test $aggr = 0 || fields="$fields $token"
-	done
-}
-
-get_typedefs ()
-{
-	local level=1 state=
-	for token in $1
-	do
-		case "$token" in
-		typedef)
-			test $level != 1 || state=1
-			;;
-		COMPAT_HANDLE\(*\))
-			test $level != 1 -o "$state" != 1 || state=2
-			;;
-		[\{\[])
-			level=$(expr $level + 1)
-			;;
-		[\}\]])
-			level=$(expr $level - 1)
-			;;
-		";")
-			test $level != 1 || state=
-			;;
-		[a-zA-Z_]*)
-			test $level != 1 -o "$state" != 2 || echo "$token"
-			;;
-		esac
-	done
-}
-
-build_enums ()
-{
-	local level=1 kind= fields= members= named= id= token
-	for token in $2
-	do
-		case "$token" in
-		struct|union)
-			test $level != 2 || fields=" "
-			kind="$token;$kind"
-			;;
-		"{")
-			level=$(expr $level + 1)
-			;;
-		"}")
-			level=$(expr $level - 1)
-			if [ $level = 1 ]
-			then
-				if [ "${kind%%;*}" = union ]
-				then
-					echo
-					echo "enum XLAT_$1 {"
-					for m in $members
-					do
-						echo "    XLAT_${1}_$m,"
-					done
-					echo "};"
-				fi
-				return 0
-			elif [ $level = 2 ]
-			then
-				named='?'
-			fi
-			;;
-		[a-zA-Z]*)
-			id=$token
-			if [ -n "$named" -a -n "${kind#*;}" ]
-			then
-				build_enums ${1}_$token "$fields"
-				named='!'
-			fi
-			;;
-		",")
-			test $level != 2 || members="$members $id"
-			;;
-		";")
-			test $level != 2 || members="$members $id"
-			test -z "$named" || kind=${kind#*;}
-			named=
-			;;
-		esac
-		test -z "$fields" || fields="$fields $token"
-	done
-}
-
-handle_field ()
-{
-	if [ -z "$5" ]
-	then
-		echo " \\"
-		if [ -z "$4" ]
-		then
-			printf %s "$1(_d_)->$3 = (_s_)->$3;"
-		else
-			printf %s "$1XLAT_${2}_HNDL_$(echo $3 | $SED 's,\.,_,g')(_d_, _s_);"
-		fi
-	elif [ -z "$(echo "$5" | $SED 's,[^{}],,g')" ]
-	then
-		local tag=$(echo "$5" | ${PYTHON} -c '
-import re,sys
-for line in sys.stdin.readlines():
-    sys.stdout.write(re.subn(r"\s*(struct|union)\s+(compat_)?(\w+)\s.*", r"\3", line)[0].rstrip() + "\n")
-')
-		echo " \\"
-		printf %s "${1}XLAT_$tag(&(_d_)->$3, &(_s_)->$3);"
-	else
-		local level=1 kind= fields= id= array= arrlvl=1 array_type= type= token
-		for token in $5
-		do
-			case "$token" in
-			struct|union)
-				test $level != 2 || fields=" "
-				if [ $level = 1 ]
-				then
-					kind=$token
-					if [ $kind = union ]
-					then
-						echo " \\"
-						printf %s "${1}switch ($(echo $3 | $SED 's,\.,_,g')) {"
-					fi
-				fi
-				;;
-			"{")
-				level=$(expr $level + 1) id=
-				;;
-			"}")
-				level=$(expr $level - 1) id=
-				if [ $level = 1 -a $kind = union ]
-				then
-					echo " \\"
-					printf %s "$1}"
-				fi
-				;;
-			"[")
-				if [ $level != 2 -o $arrlvl != 1 ]
-				then
-					:
-				elif [ -z "$array" ]
-				then
-					array=" "
-				else
-					array="$array;"
-				fi
-				arrlvl=$(expr $arrlvl + 1)
-				;;
-			"]")
-				arrlvl=$(expr $arrlvl - 1)
-				;;
-			COMPAT_HANDLE\(*\))
-				if [ $level = 2 -a -z "$id" ]
-				then
-					type=${token#COMPAT_HANDLE?}
-					type=${type%?}
-					type=${type#compat_}
-				fi
-				;;
-			compat_domain_handle_t)
-				if [ $level = 2 -a -z "$id" ]
-				then
-					array_type=$token
-				fi
-				;;
-			[a-zA-Z]*)
-				if [ -z "$id" -a -z "$type" -a -z "$array_type" ]
-				then
-					for id in $typedefs
-					do
-						test $id != "$token" || type=$id
-					done
-					if [ -z "$type" ]
-					then
-						id=$token
-					else
-						id=
-					fi
-				else
-					id=$token
-				fi
-				;;
-			[\,\;])
-				if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
-				then
-					if [ $kind = union ]
-					then
-						echo " \\"
-						printf %s "${1}case XLAT_${2}_$(echo $3.$id | $SED 's,\.,_,g'):"
-						handle_field "$1    " $2 $3.$id "$type" "$fields"
-					elif [ -z "$array" -a -z "$array_type" ]
-					then
-						handle_field "$1" $2 $3.$id "$type" "$fields"
-					elif [ -z "$array" ]
-					then
-						copy_array "    " $3.$id
-					else
-						handle_array "$1" $2 $3.$id "${array#*;}" "$type" "$fields"
-					fi
-					test "$token" != ";" || fields= id= type=
-					array=
-					if [ $kind = union ]
-					then
-						echo " \\"
-						printf %s "$1    break;"
-					fi
-				fi
-				;;
-			*)
-				if [ -n "$array" ]
-				then
-					array="$array $token"
-				fi
-				;;
-			esac
-			test -z "$fields" || fields="$fields $token"
-		done
-	fi
-}
-
-copy_array ()
-{
-	echo " \\"
-	echo "${1}if ((_d_)->$2 != (_s_)->$2) \\"
-	printf %s "$1    memcpy((_d_)->$2, (_s_)->$2, sizeof((_d_)->$2));"
-}
-
-handle_array ()
-{
-	local i="i$(echo $4 | $SED 's,[^;], ,g' | wc -w | $SED 's,[[:space:]]*,,g')"
-	echo " \\"
-	echo "$1{ \\"
-	echo "$1    unsigned int $i; \\"
-	printf %s "$1    for ($i = 0; $i < "${4%%;*}"; ++$i) {"
-	if [ "$4" = "${4#*;}" ]
-	then
-		handle_field "$1        " $2 $3[$i] "$5" "$6"
-	else
-		handle_array "$1        " $2 $3[$i] "${4#*;}" "$5" "$6"
-	fi
-	echo " \\"
-	echo "$1    } \\"
-	printf %s "$1}"
-}
-
-build_body ()
-{
-	echo
-	printf %s "#define XLAT_$1(_d_, _s_) do {"
-	local level=1 fields= id= array= arrlvl=1 array_type= type= token
-	for token in $2
-	do
-		case "$token" in
-		struct|union)
-			test $level != 2 || fields=" "
-			;;
-		"{")
-			level=$(expr $level + 1) id=
-			;;
-		"}")
-			level=$(expr $level - 1) id=
-			;;
-		"[")
-			if [ $level != 2 -o $arrlvl != 1 ]
-			then
-				:
-			elif [ -z "$array" ]
-			then
-				array=" "
-			else
-				array="$array;"
-			fi
-			arrlvl=$(expr $arrlvl + 1)
-			;;
-		"]")
-			arrlvl=$(expr $arrlvl - 1)
-			;;
-		COMPAT_HANDLE\(*\))
-			if [ $level = 2 -a -z "$id" ]
-			then
-				type=${token#COMPAT_HANDLE?}
-				type=${type%?}
-				type=${type#compat_}
-			fi
-			;;
-		compat_domain_handle_t)
-			if [ $level = 2 -a -z "$id" ]
-			then
-				array_type=$token
-			fi
-			;;
-		[a-zA-Z_]*)
-			if [ -n "$array" ]
-			then
-				array="$array $token"
-			elif [ -z "$id" -a -z "$type" -a -z "$array_type" ]
-			then
-				for id in $typedefs
-				do
-					test $id != "$token" || type=$id
-				done
-				if [ -z "$type" ]
-				then
-					id=$token
-				else
-					id=
-				fi
-			else
-				id=$token
-			fi
-			;;
-		[\,\;])
-			if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
-			then
-				if [ -z "$array" -a -z "$array_type" ]
-				then
-					handle_field "    " $1 $id "$type" "$fields"
-				elif [ -z "$array" ]
-				then
-					copy_array "    " $id
-				else
-					handle_array "    " $1 $id "${array#*;}" "$type" "$fields"
-				fi
-				test "$token" != ";" || fields= id= type=
-				array=
-			fi
-			;;
-		*)
-			if [ -n "$array" ]
-			then
-				array="$array $token"
-			fi
-			;;
-		esac
-		test -z "$fields" || fields="$fields $token"
-	done
-	echo " \\"
-	echo "} while (0)"
-}
-
-check_field ()
-{
-	if [ -z "$(echo "$4" | $SED 's,[^{}],,g')" ]
-	then
-		echo "; \\"
-		local n=$(echo $3 | $SED 's,[^.], ,g' | wc -w | $SED 's,[[:space:]]*,,g')
-		if [ -n "$4" ]
-		then
-			for n in $4
-			do
-				case $n in
-				struct|union)
-					;;
-				[a-zA-Z_]*)
-					printf %s "    CHECK_${n#xen_}"
-					break
-					;;
-				*)
-					echo "Malformed compound declaration: '$n'" >&2
-					exit 1
-					;;
-				esac
-			done
-		elif [ $n = 0 ]
-		then
-			printf %s "    CHECK_FIELD_($1, $2, $3)"
-		else
-			printf %s "    CHECK_SUBFIELD_${n}_($1, $2, $(echo $3 | $SED 's!\.!, !g'))"
-		fi
-	else
-		local level=1 fields= id= token
-		for token in $4
-		do
-			case "$token" in
-			struct|union)
-				test $level != 2 || fields=" "
-				;;
-			"{")
-				level=$(expr $level + 1) id=
-				;;
-			"}")
-				level=$(expr $level - 1) id=
-				;;
-			compat_*_t)
-				if [ $level = 2 ]
-				then
-					fields=" "
-					token="${token%_t}"
-					token="${token#compat_}"
-				fi
-				;;
-			evtchn_*_compat_t)
-				if [ $level = 2 -a $token != evtchn_port_compat_t ]
-				then
-					fields=" "
-					token="${token%_compat_t}"
-				fi
-				;;
-			[a-zA-Z]*)
-				id=$token
-				;;
-			[\,\;])
-				if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
-				then
-					check_field $1 $2 $3.$id "$fields"
-					test "$token" != ";" || fields= id=
-				fi
-				;;
-			esac
-			test -z "$fields" || fields="$fields $token"
-		done
-	fi
-}
-
-build_check ()
-{
-	echo
-	echo "#define CHECK_$1 \\"
-	local level=1 fields= kind= id= arrlvl=1 token
-	for token in $2
-	do
-		case "$token" in
-		struct|union)
-			if [ $level = 1 ]
-			then
-				kind=$token
-				printf %s "    CHECK_SIZE_($kind, $1)"
-			elif [ $level = 2 ]
-			then
-				fields=" "
-			fi
-			;;
-		"{")
-			level=$(expr $level + 1) id=
-			;;
-		"}")
-			level=$(expr $level - 1) id=
-			;;
-		"[")
-			arrlvl=$(expr $arrlvl + 1)
-			;;
-		"]")
-			arrlvl=$(expr $arrlvl - 1)
-			;;
-		compat_*_t)
-			if [ $level = 2 -a $token != compat_argo_port_t ]
-			then
-				fields=" "
-				token="${token%_t}"
-				token="${token#compat_}"
-			fi
-			;;
-		[a-zA-Z_]*)
-			test $level != 2 -o $arrlvl != 1 || id=$token
-			;;
-		[\,\;])
-			if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
-			then
-				check_field $kind $1 $id "$fields"
-				test "$token" != ";" || fields= id=
-			fi
-			;;
-		esac
-		test -z "$fields" || fields="$fields $token"
-	done
-	echo ""
-}
-
-list="$($SED -e 's,^[[:space:]]#.*,,' -e 's!\([]\[,;:{}]\)! \1 !g' $3)"
-fields="$(get_fields $(echo $2 | $SED 's,^compat_xen,compat_,') "$list")"
-if [ -z "$fields" ]
-then
-	echo "Fields of '$2' not found in '$3'" >&2
-	exit 1
-fi
-name=${2#compat_}
-name=${name#xen}
-case "$1" in
-"!")
-	typedefs="$(get_typedefs "$list")"
-	build_enums $name "$fields"
-	build_body $name "$fields"
-	;;
-"?")
-	build_check $name "$fields"
-	;;
-*)
-	echo "Invalid translation indicator: '$1'" >&2
-	exit 1
-	;;
-esac
-- 
Anthony PERARD

Re: [XEN PATCH v2 3/4] build: set PERL

[Jan Beulich]

On 14.06.2022 18:22, Anthony PERARD wrote:
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -22,6 +22,7 @@ PYTHON_INTERPRETER	:= $(word 1,$(shell which python3 python python2 2>/dev/null)
>  export PYTHON		?= $(PYTHON_INTERPRETER)
>  
>  export CHECKPOLICY	?= checkpolicy
> +export PERL		?= perl
>  
>  $(if $(filter __%, $(MAKECMDGOALS)), \
>      $(error targets prefixed with '__' are only for internal use))

Considering my patch yesterday that moved the exporting of CC etc, I
wonder if - at the very least for consistency - this and the neighbouring
pre-existing exports shouldn't all be moved below the inclusion of
./Config.mk as well. After all ./config might override any of those.
(Yes, the ones here don't prevent such overriding, but only as long as
there aren't any further make quirks.)

Since this may want doing in a separate patch, this one is
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN PATCH v2 1/4] build,include: rework shell script for headers++.chk

[Jan Beulich]

On 14.06.2022 18:22, Anthony PERARD wrote:
> The command line generated for headers++.chk by make is quite long,
> and in some environment it is too long. This issue have been seen in
> Yocto build environment.
> 
> Error messages:
>     make[9]: execvp: /bin/sh: Argument list too long
>     make[9]: *** [include/Makefile:181: include/headers++.chk] Error 127
> 
> Rework so that we do the foreach loop in shell rather that make to
> reduce the command line size by a lot. We also need a way to get
> headers prerequisite for some public headers so we use a switch "case"
> in shell to be able to do some simple pattern matching. Variables
> alone in POSIX shell don't allow to work with associative array or
> variables with "/".
> 
> Reported-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Fixes: 28e13c7f43 ("build: xen/include: use if_changed")
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> 
> Notes:
>     v2:
>     - fix typo in commit message
>     - fix out-of-tree build
>     
>     v1:
>     - was sent as a reply to v1 of the series
> 
>  xen/include/Makefile | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index 6d9bcc19b0..49c75a78f9 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -158,13 +158,22 @@ define cmd_headerscxx_chk
>  	    touch $@.new;                                                     \
>  	    exit 0;                                                           \
>  	fi;                                                                   \
> -	$(foreach i, $(filter %.h,$^),                                        \
> -	    echo "#include "\"$(i)\"                                          \
> +	get_prereq() {                                                        \
> +	    case $$1 in                                                       \
> +	    $(foreach i, $(filter %.h,$^),                                    \
> +	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
> +		$(i)$(close)                                                  \
> +		echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
> +			-include c$(j))";;))                                  \
> +	    esac;                                                             \

If I'm reading this right (indentation looks to be a little misleading
and hence one needs to count parentheses) the case statement could (in
theory) remain without any "body". As per the command language spec I'm
looking at this (if it works in the first place) is an extension, and
formally there's always at least one label required. Since we aim to be
portable in such regards, I'd like to request that there be a final
otherwise empty *) line.

> +	};                                                                    \
> +	for i in $(filter %.h,$^); do                                         \
> +	    echo "#include "\"$$i\"                                           \
>  	    | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
>  	      -include stdint.h -include $(srcdir)/public/xen.h               \
> -	      $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include c$(j)) \
> +	      `get_prereq $$i`                                                \

While I know we use back-ticked quoting elsewhere, I'd generally
recommend to use $() for readability. But maybe others view this
exactly the other way around ...

And a question without good context to put at: Isn't headers99.chk in
similar need of converting? It looks only slightly less involved than
the C++ one.

Jan

>  	      -S -o /dev/null -                                               \
> -	    || exit $$?; echo $(i) >> $@.new;) \
> +	    || exit $$?; echo $$i >> $@.new; done;                            \
>  	mv $@.new $@
>  endef
>  

Re: [XEN PATCH v2 1/4] build,include: rework shell script for headers++.chk

[Anthony PERARD]

On Wed, Jun 15, 2022 at 08:37:42AM +0200, Jan Beulich wrote:
> On 14.06.2022 18:22, Anthony PERARD wrote:
> > diff --git a/xen/include/Makefile b/xen/include/Makefile
> > index 6d9bcc19b0..49c75a78f9 100644
> > --- a/xen/include/Makefile
> > +++ b/xen/include/Makefile
> > @@ -158,13 +158,22 @@ define cmd_headerscxx_chk
> >  	    touch $@.new;                                                     \
> >  	    exit 0;                                                           \
> >  	fi;                                                                   \
> > -	$(foreach i, $(filter %.h,$^),                                        \
> > -	    echo "#include "\"$(i)\"                                          \
> > +	get_prereq() {                                                        \
> > +	    case $$1 in                                                       \
> > +	    $(foreach i, $(filter %.h,$^),                                    \
> > +	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
> > +		$(i)$(close)                                                  \
> > +		echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
> > +			-include c$(j))";;))                                  \
> > +	    esac;                                                             \
> 
> If I'm reading this right (indentation looks to be a little misleading
> and hence one needs to count parentheses) the case statement could (in
> theory) remain without any "body". As per the command language spec I'm
> looking at this (if it works in the first place) is an extension, and
> formally there's always at least one label required. Since we aim to be
> portable in such regards, I'd like to request that there be a final
> otherwise empty *) line.

When looking at the shell grammar at [1], an empty body seems to be
allowed. But I can add "*)" at the end for peace of mind.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_10_02

As for misleading indentation, I've got my $EDITOR to show me matching
parentheses, so I don't need to count them. But if I rewrite that
function as following, would it be easier to follow?

+	get_prereq() {                                                        \
+	case $$1 in                                                           \
+	$(foreach i, $(filter %.h,$^),                                        \
+	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
+		$(i)$(close)                                                  \
+		echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
+			-include c$(j))";;))                                  \
+	*) ;;                                                                 \
+	esac;                                                                 \
+	};                                                                    \


> > +	};                                                                    \
> > +	for i in $(filter %.h,$^); do                                         \
> > +	    echo "#include "\"$$i\"                                           \
> >  	    | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
> >  	      -include stdint.h -include $(srcdir)/public/xen.h               \
> > -	      $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include c$(j)) \
> > +	      `get_prereq $$i`                                                \
> 
> While I know we use back-ticked quoting elsewhere, I'd generally
> recommend to use $() for readability. But maybe others view this
> exactly the other way around ...

Well, in Makefile it's `` vs $$(). At least, we don't have to write
$$$(open)$(close) here :-).

I guess $$(get_prereq $$i) isn't too bad here, I'll replace the
backquote.

> And a question without good context to put at: Isn't headers99.chk in
> similar need of converting? It looks only slightly less involved than
> the C++ one.

It doesn't really need converting at the moment, because there's only
two headers to check, so the resulting command line is small. But
converting it at the same time is probably a good idea to avoid having
two different implementations of the header check.

Thanks,

-- 
Anthony PERARD

Re: [XEN PATCH v2 1/4] build,include: rework shell script for headers++.chk

[Jan Beulich]

On 15.06.2022 12:31, Anthony PERARD wrote:
> On Wed, Jun 15, 2022 at 08:37:42AM +0200, Jan Beulich wrote:
>> On 14.06.2022 18:22, Anthony PERARD wrote:
>>> diff --git a/xen/include/Makefile b/xen/include/Makefile
>>> index 6d9bcc19b0..49c75a78f9 100644
>>> --- a/xen/include/Makefile
>>> +++ b/xen/include/Makefile
>>> @@ -158,13 +158,22 @@ define cmd_headerscxx_chk
>>>  	    touch $@.new;                                                     \
>>>  	    exit 0;                                                           \
>>>  	fi;                                                                   \
>>> -	$(foreach i, $(filter %.h,$^),                                        \
>>> -	    echo "#include "\"$(i)\"                                          \
>>> +	get_prereq() {                                                        \
>>> +	    case $$1 in                                                       \
>>> +	    $(foreach i, $(filter %.h,$^),                                    \
>>> +	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
>>> +		$(i)$(close)                                                  \
>>> +		echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
>>> +			-include c$(j))";;))                                  \
>>> +	    esac;                                                             \
>>
>> If I'm reading this right (indentation looks to be a little misleading
>> and hence one needs to count parentheses) the case statement could (in
>> theory) remain without any "body". As per the command language spec I'm
>> looking at this (if it works in the first place) is an extension, and
>> formally there's always at least one label required. Since we aim to be
>> portable in such regards, I'd like to request that there be a final
>> otherwise empty *) line.
> 
> When looking at the shell grammar at [1], an empty body seems to be
> allowed. But I can add "*)" at the end for peace of mind.
> 
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_10_02

Hmm, indeed. As opposed to

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_04

> As for misleading indentation, I've got my $EDITOR to show me matching
> parentheses, so I don't need to count them. But if I rewrite that
> function as following, would it be easier to follow?
> 
> +	get_prereq() {                                                        \
> +	case $$1 in                                                           \
> +	$(foreach i, $(filter %.h,$^),                                        \
> +	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
> +		$(i)$(close)                                                  \
> +		echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
> +			-include c$(j))";;))                                  \
> +	*) ;;                                                                 \
> +	esac;                                                                 \
> +	};                                                                    \

Hmm, not really, no (and it may be more obvious looking at the reply
context above): My primary concern is the use of hard tabs beyond the
leading one (which is uniform across all lines and hence doesn't alter
apparent alignment even with the + or > prefixed).

Jan

Re: [XEN PATCH v2 3/4] build: set PERL

[Anthony PERARD]

On Wed, Jun 15, 2022 at 08:11:10AM +0200, Jan Beulich wrote:
> On 14.06.2022 18:22, Anthony PERARD wrote:
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -22,6 +22,7 @@ PYTHON_INTERPRETER	:= $(word 1,$(shell which python3 python python2 2>/dev/null)
> >  export PYTHON		?= $(PYTHON_INTERPRETER)
> >  
> >  export CHECKPOLICY	?= checkpolicy
> > +export PERL		?= perl
> >  
> >  $(if $(filter __%, $(MAKECMDGOALS)), \
> >      $(error targets prefixed with '__' are only for internal use))
> 
> Considering my patch yesterday that moved the exporting of CC etc, I
> wonder if - at the very least for consistency - this and the neighbouring
> pre-existing exports shouldn't all be moved below the inclusion of
> ./Config.mk as well. After all ./config might override any of those.
> (Yes, the ones here don't prevent such overriding, but only as long as
> there aren't any further make quirks.)

Sounds good, we can move CHECKPOLICY and PERL, but there's a problem
with PYTHON. Config.mk define a different value for PYTHON, so the one
in xen/Makefile needs to be set before including ./Config.mk.

Thanks,

-- 
Anthony PERARD

[XEN PATCH v2.1 1/4] build,include: rework shell script for headers++.chk

[Anthony PERARD]

The command line generated for headers++.chk by make is quite long,
and in some environment it is too long. This issue have been seen in
Yocto build environment.

Error messages:
    make[9]: execvp: /bin/sh: Argument list too long
    make[9]: *** [include/Makefile:181: include/headers++.chk] Error 127

Rework so that we do the foreach loop in shell rather that make to
reduce the command line size by a lot. We also need a way to get
headers prerequisite for some public headers so we use a switch "case"
in shell to be able to do some simple pattern matching. Variables
alone in POSIX shell don't allow to work with associative array or
variables with "/".

Also rework headers99.chk as it has a similar implementation, even if
with only two headers to check the command line isn't too long at the
moment.

Reported-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
Fixes: 28e13c7f43 ("build: xen/include: use if_changed")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---

Notes:
    v3:
    - add one more pattern to avoid a possible empty body for "case"
    - use $() instead of `` to execute get_prereq()
    - also convert headers99_chk
    - convert some 'tab' to 'space', have only 1 tab at start of line
    
    v2:
    - fix typo in commit message
    - fix out-of-tree build
    
    v1:
    - was sent as a reply to v1 of the series

 xen/include/Makefile | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 617599df7e..510f65c92a 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -141,13 +141,24 @@ cmd_header_chk = \
 quiet_cmd_headers99_chk = CHK     $@
 define cmd_headers99_chk
 	rm -f $@.new; \
-	$(foreach i, $(filter %.h,$^),                                        \
-	    echo "#include "\"$(i)\"                                          \
+	get_prereq() {                                                        \
+	    case $$1 in                                                       \
+	    $(foreach i, $(filter %.h,$^),                                    \
+	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
+	        $(i)$(close)                                                  \
+	        echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
+	                -include $(j).h)";;))                                 \
+	    *) ;;                                                             \
+	    esac;                                                             \
+	};                                                                    \
+	for i in $(filter %.h,$^); do                                         \
+	    echo "#include "\"$$i\"                                           \
 	    | $(CC) -x c -std=c99 -Wall -Werror                               \
 	      -include stdint.h                                               \
-	      $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include $(j).h) \
+	      $$(get_prereq $$i)                                              \
 	      -S -o /dev/null -                                               \
-	    || exit $$?; echo $(i) >> $@.new;) \
+	    || exit $$?; echo $$i >> $@.new;                                  \
+	done;                                                                 \
 	mv $@.new $@
 endef
 
@@ -158,13 +169,23 @@ define cmd_headerscxx_chk
 	    touch $@.new;                                                     \
 	    exit 0;                                                           \
 	fi;                                                                   \
-	$(foreach i, $(filter %.h,$^),                                        \
-	    echo "#include "\"$(i)\"                                          \
+	get_prereq() {                                                        \
+	    case $$1 in                                                       \
+	    $(foreach i, $(filter %.h,$^),                                    \
+	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
+	        $(i)$(close)                                                  \
+	        echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
+	                -include c$(j))";;))                                  \
+	    *) ;;                                                             \
+	    esac;                                                             \
+	};                                                                    \
+	for i in $(filter %.h,$^); do                                         \
+	    echo "#include "\"$$i\"                                           \
 	    | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
 	      -include stdint.h -include $(srcdir)/public/xen.h               \
-	      $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include c$(j)) \
+	      $$(get_prereq $$i)                                              \
 	      -S -o /dev/null -                                               \
-	    || exit $$?; echo $(i) >> $@.new;) \
+	    || exit $$?; echo $$i >> $@.new; done;                            \
 	mv $@.new $@
 endef
 
-- 
Anthony PERARD

Re: [XEN PATCH v2.1 1/4] build,include: rework shell script for headers++.chk

[Michal Orzel]

Hi Anthony,

On 21.06.2022 12:11, Anthony PERARD wrote:
> The command line generated for headers++.chk by make is quite long,
> and in some environment it is too long. This issue have been seen in
> Yocto build environment.
> 
> Error messages:
>     make[9]: execvp: /bin/sh: Argument list too long
>     make[9]: *** [include/Makefile:181: include/headers++.chk] Error 127
> 
> Rework so that we do the foreach loop in shell rather that make to
> reduce the command line size by a lot. We also need a way to get
> headers prerequisite for some public headers so we use a switch "case"
> in shell to be able to do some simple pattern matching. Variables
> alone in POSIX shell don't allow to work with associative array or
> variables with "/".
> 
> Also rework headers99.chk as it has a similar implementation, even if
> with only two headers to check the command line isn't too long at the
> moment.
> 
> Reported-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Fixes: 28e13c7f43 ("build: xen/include: use if_changed")
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Tested-by: Michal Orzel <michal.orzel@arm.com>

Cheers,
Michal

Re: [XEN PATCH v2.1 1/4] build,include: rework shell script for headers++.chk

[Jan Beulich]

On 22.06.2022 14:35, Michal Orzel wrote:
> Hi Anthony,
> 
> On 21.06.2022 12:11, Anthony PERARD wrote:
>> The command line generated for headers++.chk by make is quite long,
>> and in some environment it is too long. This issue have been seen in
>> Yocto build environment.
>>
>> Error messages:
>>     make[9]: execvp: /bin/sh: Argument list too long
>>     make[9]: *** [include/Makefile:181: include/headers++.chk] Error 127
>>
>> Rework so that we do the foreach loop in shell rather that make to
>> reduce the command line size by a lot. We also need a way to get
>> headers prerequisite for some public headers so we use a switch "case"
>> in shell to be able to do some simple pattern matching. Variables
>> alone in POSIX shell don't allow to work with associative array or
>> variables with "/".
>>
>> Also rework headers99.chk as it has a similar implementation, even if
>> with only two headers to check the command line isn't too long at the
>> moment.
>>
>> Reported-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
>> Fixes: 28e13c7f43 ("build: xen/include: use if_changed")
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Reviewed-by: Michal Orzel <michal.orzel@arm.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

> Tested-by: Michal Orzel <michal.orzel@arm.com>
> 
> Cheers,
> Michal

Re: [XEN PATCH v2.1 1/4] build,include: rework shell script for headers++.chk

[Jan Beulich]

On 21.06.2022 12:11, Anthony PERARD wrote:
> The command line generated for headers++.chk by make is quite long,
> and in some environment it is too long. This issue have been seen in
> Yocto build environment.
> 
> Error messages:
>     make[9]: execvp: /bin/sh: Argument list too long
>     make[9]: *** [include/Makefile:181: include/headers++.chk] Error 127
> 
> Rework so that we do the foreach loop in shell rather that make to
> reduce the command line size by a lot. We also need a way to get
> headers prerequisite for some public headers so we use a switch "case"
> in shell to be able to do some simple pattern matching. Variables
> alone in POSIX shell don't allow to work with associative array or
> variables with "/".
> 
> Also rework headers99.chk as it has a similar implementation, even if
> with only two headers to check the command line isn't too long at the
> moment.
> 
> Reported-by: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Fixes: 28e13c7f43 ("build: xen/include: use if_changed")
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

I have committed this, but strictly speaking imo the R-b should have
been dropped because ...

> ---
> 
> Notes:
>     v3:
>     - add one more pattern to avoid a possible empty body for "case"
>     - use $() instead of `` to execute get_prereq()
>     - also convert headers99_chk
>     - convert some 'tab' to 'space', have only 1 tab at start of line

... at least the added headers99_chk conversion was not a purely
mechanical change.

Jan

>     v2:
>     - fix typo in commit message
>     - fix out-of-tree build
>     
>     v1:
>     - was sent as a reply to v1 of the series
> 
>  xen/include/Makefile | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index 617599df7e..510f65c92a 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -141,13 +141,24 @@ cmd_header_chk = \
>  quiet_cmd_headers99_chk = CHK     $@
>  define cmd_headers99_chk
>  	rm -f $@.new; \
> -	$(foreach i, $(filter %.h,$^),                                        \
> -	    echo "#include "\"$(i)\"                                          \
> +	get_prereq() {                                                        \
> +	    case $$1 in                                                       \
> +	    $(foreach i, $(filter %.h,$^),                                    \
> +	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
> +	        $(i)$(close)                                                  \
> +	        echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
> +	                -include $(j).h)";;))                                 \
> +	    *) ;;                                                             \
> +	    esac;                                                             \
> +	};                                                                    \
> +	for i in $(filter %.h,$^); do                                         \
> +	    echo "#include "\"$$i\"                                           \
>  	    | $(CC) -x c -std=c99 -Wall -Werror                               \
>  	      -include stdint.h                                               \
> -	      $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include $(j).h) \
> +	      $$(get_prereq $$i)                                              \
>  	      -S -o /dev/null -                                               \
> -	    || exit $$?; echo $(i) >> $@.new;) \
> +	    || exit $$?; echo $$i >> $@.new;                                  \
> +	done;                                                                 \
>  	mv $@.new $@
>  endef
>  
> @@ -158,13 +169,23 @@ define cmd_headerscxx_chk
>  	    touch $@.new;                                                     \
>  	    exit 0;                                                           \
>  	fi;                                                                   \
> -	$(foreach i, $(filter %.h,$^),                                        \
> -	    echo "#include "\"$(i)\"                                          \
> +	get_prereq() {                                                        \
> +	    case $$1 in                                                       \
> +	    $(foreach i, $(filter %.h,$^),                                    \
> +	    $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
> +	        $(i)$(close)                                                  \
> +	        echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
> +	                -include c$(j))";;))                                  \
> +	    *) ;;                                                             \
> +	    esac;                                                             \
> +	};                                                                    \
> +	for i in $(filter %.h,$^); do                                         \
> +	    echo "#include "\"$$i\"                                           \
>  	    | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
>  	      -include stdint.h -include $(srcdir)/public/xen.h               \
> -	      $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include c$(j)) \
> +	      $$(get_prereq $$i)                                              \
>  	      -S -o /dev/null -                                               \
> -	    || exit $$?; echo $(i) >> $@.new;) \
> +	    || exit $$?; echo $$i >> $@.new; done;                            \
>  	mv $@.new $@
>  endef
>  

Re: [XEN PATCH v2 4/4] build: replace get-fields.sh by a perl script

[Anthony PERARD]

On Tue, Jun 14, 2022 at 05:22:48PM +0100, Anthony PERARD wrote:
> The get-fields.sh which generate all the include/compat/.xlat/*.h
> headers is quite slow. It takes for example nearly 3 seconds to
> generate platform.h on a recent machine, or 2.3 seconds for memory.h.
> 
> Since it's only text processing, rewriting the mix of shell/sed/python
> into a single perl script make the generation of those file a lot
> faster.
> 
> I tried to keep a similar look for the code, to keep the code similar
> between the shell and perl, and to ease review. So some code in perl
> might look weird or could be written better.
> 
> No functional change, the headers generated are identical.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Hi,

This patch have been on the mailing list for a little while now, could
it be acked then committed along with the previous one now?

Thanks,

-- 
Anthony PERARD

Re: [XEN PATCH v2 4/4] build: replace get-fields.sh by a perl script

[Jan Beulich]

On 03.08.2022 15:27, Anthony PERARD wrote:
> On Tue, Jun 14, 2022 at 05:22:48PM +0100, Anthony PERARD wrote:
>> The get-fields.sh which generate all the include/compat/.xlat/*.h
>> headers is quite slow. It takes for example nearly 3 seconds to
>> generate platform.h on a recent machine, or 2.3 seconds for memory.h.
>>
>> Since it's only text processing, rewriting the mix of shell/sed/python
>> into a single perl script make the generation of those file a lot
>> faster.
>>
>> I tried to keep a similar look for the code, to keep the code similar
>> between the shell and perl, and to ease review. So some code in perl
>> might look weird or could be written better.
>>
>> No functional change, the headers generated are identical.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> This patch have been on the mailing list for a little while now, could
> it be acked then committed along with the previous one now?

Well, I would certainly be happy to ack it, but only on the basis of a
suitable R-b by somebody else. My Perl is close to non-existing, so I
can't sensibly review this myself.

Jan