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 PERARDOn 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
>
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
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
© 2016 - 2026 Red Hat, Inc.