* Fix genmultilib reuse rule checks for large sets of option combinations
@ 2017-06-08 20:28 Joseph Myers
2017-06-14 15:09 ` Ping " Joseph Myers
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Joseph Myers @ 2017-06-08 20:28 UTC (permalink / raw)
To: gcc-patches
genmultilib computes combination_space, a list of all combinations of
options in MULTILIB_OPTIONS that might have multilibs built for them
(some of which may end up not having multilibs built for them, and
some of those may end up being mapped to other multilibs with
MULTILIB_REUSE). It is then used to validate the right hand part of
MULTILIB_REUSE rules, checking with expr that combination_space
matches a basic regular expression derived from that right hand part.
There are two problems with this approach to validation:
* It requires that right hand part to have options in the same order
as in MULTILIB_OPTIONS, in contradiction to the documentation of
MULTILIB_REUSE saying that order does not matter there.
* combination_space can be so large that the expr call fails with an
E2BIG error. I have a local ARM configuration with 40 multilibs but
3840 combinations of options from MULTILIB_OPTIONS (so 3839 listed
in combination_space, since it doesn't list the default multilib)
and 996 MULTILIB_REUSE rules. This generates a combination_space
string longer than the Linux kernel's MAX_ARG_STRLEN (PAGE_SIZE *
32, the limit on the length of a single argv string), so that expr
cannot be run.
This patch changes the validation approach to generate a much shorter
extended regular expression for any sequence of multilib options in
any order, and uses that for the validation instead.
Tested with a built for arm-none-eabi --with-multilib-list=aprofile
(as a configuration that uses MULTILIB_REUSE).
2017-06-08 Joseph Myers <joseph@codesourcery.com>
* genmultilib (combination_space): Remove variable.
Validate reuse rules against regular expression for any sequence
of multilib options in any order.
Index: gcc/genmultilib
===================================================================
--- gcc/genmultilib (revision 249028)
+++ gcc/genmultilib (working copy)
@@ -186,8 +186,7 @@
EOF
chmod +x tmpmultilib
-combination_space=`initial=/ ./tmpmultilib ${options}`
-combinations="$combination_space"
+combinations=`initial=/ ./tmpmultilib ${options}`
# If there exceptions, weed them out now
if [ -n "${exceptions}" ]; then
@@ -460,6 +459,15 @@
echo "NULL"
echo "};"
+# Generate a regular expression to validate option combinations.
+options_re=
+for set in ${options}; do
+ for opt in `echo ${set} | sed -e 's_[/|]_ _g'`; do
+ options_re="${options_re}${options_re:+|}${opt}"
+ done
+done
+options_re="^/((${options_re})/)*\$"
+
# Output rules used for multilib reuse.
echo ""
echo "static const char *const multilib_reuse_raw[] = {"
@@ -473,7 +481,7 @@
# in this variable, it means no multilib will be built for current reuse
# rule. Thus the reuse purpose specified by current rule is meaningless.
if expr "${combinations} " : ".*/${combo}/.*" > /dev/null; then
- if expr "${combination_space} " : ".*/${copts}/.*" > /dev/null; then
+ if echo "/${copts}/" | grep -E "${options_re}" > /dev/null; then
combo="/${combo}/"
dirout=`./tmpmultilib3 "${combo}" "${todirnames}" "${toosdirnames}" "${enable_multilib}"`
copts="/${copts}/"
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Ping Re: Fix genmultilib reuse rule checks for large sets of option combinations
2017-06-08 20:28 Fix genmultilib reuse rule checks for large sets of option combinations Joseph Myers
@ 2017-06-14 15:09 ` Joseph Myers
2017-06-27 15:48 ` Ping^2 " Joseph Myers
2017-06-27 16:14 ` Jeff Law
2017-06-28 8:04 ` Christophe Lyon
2 siblings, 1 reply; 8+ messages in thread
From: Joseph Myers @ 2017-06-14 15:09 UTC (permalink / raw)
To: gcc-patches
Ping. This patch
<https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00562.html> is pending
review.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Ping^2 Re: Fix genmultilib reuse rule checks for large sets of option combinations
2017-06-14 15:09 ` Ping " Joseph Myers
@ 2017-06-27 15:48 ` Joseph Myers
0 siblings, 0 replies; 8+ messages in thread
From: Joseph Myers @ 2017-06-27 15:48 UTC (permalink / raw)
To: gcc-patches
Ping^2. This patch
<https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00562.html> is still
pending review.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fix genmultilib reuse rule checks for large sets of option combinations
2017-06-08 20:28 Fix genmultilib reuse rule checks for large sets of option combinations Joseph Myers
2017-06-14 15:09 ` Ping " Joseph Myers
@ 2017-06-27 16:14 ` Jeff Law
2017-06-28 8:04 ` Christophe Lyon
2 siblings, 0 replies; 8+ messages in thread
From: Jeff Law @ 2017-06-27 16:14 UTC (permalink / raw)
To: Joseph Myers, gcc-patches
On 06/08/2017 02:28 PM, Joseph Myers wrote:
> genmultilib computes combination_space, a list of all combinations of
> options in MULTILIB_OPTIONS that might have multilibs built for them
> (some of which may end up not having multilibs built for them, and
> some of those may end up being mapped to other multilibs with
> MULTILIB_REUSE). It is then used to validate the right hand part of
> MULTILIB_REUSE rules, checking with expr that combination_space
> matches a basic regular expression derived from that right hand part.
>
> There are two problems with this approach to validation:
>
> * It requires that right hand part to have options in the same order
> as in MULTILIB_OPTIONS, in contradiction to the documentation of
> MULTILIB_REUSE saying that order does not matter there.
>
> * combination_space can be so large that the expr call fails with an
> E2BIG error. I have a local ARM configuration with 40 multilibs but
> 3840 combinations of options from MULTILIB_OPTIONS (so 3839 listed
> in combination_space, since it doesn't list the default multilib)
> and 996 MULTILIB_REUSE rules. This generates a combination_space
> string longer than the Linux kernel's MAX_ARG_STRLEN (PAGE_SIZE *
> 32, the limit on the length of a single argv string), so that expr
> cannot be run.
>
> This patch changes the validation approach to generate a much shorter
> extended regular expression for any sequence of multilib options in
> any order, and uses that for the validation instead.
>
> Tested with a built for arm-none-eabi --with-multilib-list=aprofile
> (as a configuration that uses MULTILIB_REUSE).
>
> 2017-06-08 Joseph Myers <joseph@codesourcery.com>
>
> * genmultilib (combination_space): Remove variable.
> Validate reuse rules against regular expression for any sequence
> of multilib options in any order.
Going to trust you on this :-) regexps are far from my sweet spot.
jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fix genmultilib reuse rule checks for large sets of option combinations
2017-06-08 20:28 Fix genmultilib reuse rule checks for large sets of option combinations Joseph Myers
2017-06-14 15:09 ` Ping " Joseph Myers
2017-06-27 16:14 ` Jeff Law
@ 2017-06-28 8:04 ` Christophe Lyon
2017-06-28 8:09 ` Christophe Lyon
2 siblings, 1 reply; 8+ messages in thread
From: Christophe Lyon @ 2017-06-28 8:04 UTC (permalink / raw)
To: Joseph Myers; +Cc: gcc-patches
Hi Joseph,
On 8 June 2017 at 22:28, Joseph Myers <joseph@codesourcery.com> wrote:
> genmultilib computes combination_space, a list of all combinations of
> options in MULTILIB_OPTIONS that might have multilibs built for them
> (some of which may end up not having multilibs built for them, and
> some of those may end up being mapped to other multilibs with
> MULTILIB_REUSE). It is then used to validate the right hand part of
> MULTILIB_REUSE rules, checking with expr that combination_space
> matches a basic regular expression derived from that right hand part.
>
> There are two problems with this approach to validation:
>
> * It requires that right hand part to have options in the same order
> as in MULTILIB_OPTIONS, in contradiction to the documentation of
> MULTILIB_REUSE saying that order does not matter there.
>
> * combination_space can be so large that the expr call fails with an
> E2BIG error. I have a local ARM configuration with 40 multilibs but
> 3840 combinations of options from MULTILIB_OPTIONS (so 3839 listed
> in combination_space, since it doesn't list the default multilib)
> and 996 MULTILIB_REUSE rules. This generates a combination_space
> string longer than the Linux kernel's MAX_ARG_STRLEN (PAGE_SIZE *
> 32, the limit on the length of a single argv string), so that expr
> cannot be run.
>
> This patch changes the validation approach to generate a much shorter
> extended regular expression for any sequence of multilib options in
> any order, and uses that for the validation instead.
>
> Tested with a built for arm-none-eabi --with-multilib-list=aprofile
> (as a configuration that uses MULTILIB_REUSE).
>
> 2017-06-08 Joseph Myers <joseph@codesourcery.com>
>
> * genmultilib (combination_space): Remove variable.
> Validate reuse rules against regular expression for any sequence
> of multilib options in any order.
>
> Index: gcc/genmultilib
> ===================================================================
> --- gcc/genmultilib (revision 249028)
> +++ gcc/genmultilib (working copy)
> @@ -186,8 +186,7 @@
> EOF
> chmod +x tmpmultilib
>
> -combination_space=`initial=/ ./tmpmultilib ${options}`
> -combinations="$combination_space"
> +combinations=`initial=/ ./tmpmultilib ${options}`
>
> # If there exceptions, weed them out now
> if [ -n "${exceptions}" ]; then
> @@ -460,6 +459,15 @@
> echo "NULL"
> echo "};"
>
> +# Generate a regular expression to validate option combinations.
> +options_re=
> +for set in ${options}; do
> + for opt in `echo ${set} | sed -e 's_[/|]_ _g'`; do
> + options_re="${options_re}${options_re:+|}${opt}"
> + done
> +done
> +options_re="^/((${options_re})/)*\$"
> +
> # Output rules used for multilib reuse.
> echo ""
> echo "static const char *const multilib_reuse_raw[] = {"
> @@ -473,7 +481,7 @@
> # in this variable, it means no multilib will be built for current reuse
> # rule. Thus the reuse purpose specified by current rule is meaningless.
> if expr "${combinations} " : ".*/${combo}/.*" > /dev/null; then
> - if expr "${combination_space} " : ".*/${copts}/.*" > /dev/null; then
> + if echo "/${copts}/" | grep -E "${options_re}" > /dev/null; then
> combo="/${combo}/"
> dirout=`./tmpmultilib3 "${combo}" "${todirnames}" "${toosdirnames}" "${enable_multilib}"`
> copts="/${copts}/"
>
This broke my builds, where I do not use
--with-multilib-list=aprofile, and uses the default.
I suspect it would also fail for the aprofile multilibs, though.
I think there's a problem with options that have a '+' which confuses
the regexp in options_re.
For instance, I get this error message:
The rule mthumb=mthumb/mfpu.auto/march.armv5te+fp contains an option
absent from MULTILIB_OPTIONS.
A bit of manual debugging led me to:
$ echo /mthumb/mfpu=auto/march=armv5te+fp/ | grep -E
'^/((marm|mthumb|mfpu=auto|march=armv5te+fp|march=armv7+fp|mfloat-abi=hard)/)*$'
[empty result]
Replacing the '+' in armv5te+fp with either '\+' or '.' allows the
pattern to match:
/mthumb/mfpu=auto/march=armv5te+fp/
Is it a matter of adding sed -e 's/\+/./g' when building options_re ?
Or would this break something else?
Thanks,
Christophe
> --
> Joseph S. Myers
> joseph@codesourcery.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fix genmultilib reuse rule checks for large sets of option combinations
2017-06-28 8:04 ` Christophe Lyon
@ 2017-06-28 8:09 ` Christophe Lyon
2017-06-28 8:14 ` Andreas Schwab
0 siblings, 1 reply; 8+ messages in thread
From: Christophe Lyon @ 2017-06-28 8:09 UTC (permalink / raw)
To: Joseph Myers; +Cc: gcc-patches
On 28 June 2017 at 10:03, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> Hi Joseph,
>
> On 8 June 2017 at 22:28, Joseph Myers <joseph@codesourcery.com> wrote:
>> genmultilib computes combination_space, a list of all combinations of
>> options in MULTILIB_OPTIONS that might have multilibs built for them
>> (some of which may end up not having multilibs built for them, and
>> some of those may end up being mapped to other multilibs with
>> MULTILIB_REUSE). It is then used to validate the right hand part of
>> MULTILIB_REUSE rules, checking with expr that combination_space
>> matches a basic regular expression derived from that right hand part.
>>
>> There are two problems with this approach to validation:
>>
>> * It requires that right hand part to have options in the same order
>> as in MULTILIB_OPTIONS, in contradiction to the documentation of
>> MULTILIB_REUSE saying that order does not matter there.
>>
>> * combination_space can be so large that the expr call fails with an
>> E2BIG error. I have a local ARM configuration with 40 multilibs but
>> 3840 combinations of options from MULTILIB_OPTIONS (so 3839 listed
>> in combination_space, since it doesn't list the default multilib)
>> and 996 MULTILIB_REUSE rules. This generates a combination_space
>> string longer than the Linux kernel's MAX_ARG_STRLEN (PAGE_SIZE *
>> 32, the limit on the length of a single argv string), so that expr
>> cannot be run.
>>
>> This patch changes the validation approach to generate a much shorter
>> extended regular expression for any sequence of multilib options in
>> any order, and uses that for the validation instead.
>>
>> Tested with a built for arm-none-eabi --with-multilib-list=aprofile
>> (as a configuration that uses MULTILIB_REUSE).
>>
>> 2017-06-08 Joseph Myers <joseph@codesourcery.com>
>>
>> * genmultilib (combination_space): Remove variable.
>> Validate reuse rules against regular expression for any sequence
>> of multilib options in any order.
>>
>> Index: gcc/genmultilib
>> ===================================================================
>> --- gcc/genmultilib (revision 249028)
>> +++ gcc/genmultilib (working copy)
>> @@ -186,8 +186,7 @@
>> EOF
>> chmod +x tmpmultilib
>>
>> -combination_space=`initial=/ ./tmpmultilib ${options}`
>> -combinations="$combination_space"
>> +combinations=`initial=/ ./tmpmultilib ${options}`
>>
>> # If there exceptions, weed them out now
>> if [ -n "${exceptions}" ]; then
>> @@ -460,6 +459,15 @@
>> echo "NULL"
>> echo "};"
>>
>> +# Generate a regular expression to validate option combinations.
>> +options_re=
>> +for set in ${options}; do
>> + for opt in `echo ${set} | sed -e 's_[/|]_ _g'`; do
>> + options_re="${options_re}${options_re:+|}${opt}"
>> + done
>> +done
>> +options_re="^/((${options_re})/)*\$"
>> +
>> # Output rules used for multilib reuse.
>> echo ""
>> echo "static const char *const multilib_reuse_raw[] = {"
>> @@ -473,7 +481,7 @@
>> # in this variable, it means no multilib will be built for current reuse
>> # rule. Thus the reuse purpose specified by current rule is meaningless.
>> if expr "${combinations} " : ".*/${combo}/.*" > /dev/null; then
>> - if expr "${combination_space} " : ".*/${copts}/.*" > /dev/null; then
>> + if echo "/${copts}/" | grep -E "${options_re}" > /dev/null; then
>> combo="/${combo}/"
>> dirout=`./tmpmultilib3 "${combo}" "${todirnames}" "${toosdirnames}" "${enable_multilib}"`
>> copts="/${copts}/"
>>
>
> This broke my builds, where I do not use
> --with-multilib-list=aprofile, and uses the default.
> I suspect it would also fail for the aprofile multilibs, though.
>
> I think there's a problem with options that have a '+' which confuses
> the regexp in options_re.
> For instance, I get this error message:
> The rule mthumb=mthumb/mfpu.auto/march.armv5te+fp contains an option
> absent from MULTILIB_OPTIONS.
>
> A bit of manual debugging led me to:
> $ echo /mthumb/mfpu=auto/march=armv5te+fp/ | grep -E
> '^/((marm|mthumb|mfpu=auto|march=armv5te+fp|march=armv7+fp|mfloat-abi=hard)/)*$'
> [empty result]
>
> Replacing the '+' in armv5te+fp with either '\+' or '.' allows the
> pattern to match:
> /mthumb/mfpu=auto/march=armv5te+fp/
>
> Is it a matter of adding sed -e 's/\+/./g' when building options_re ?
> Or would this break something else?
>
This small patch:
diff --git a/gcc/genmultilib b/gcc/genmultilib
index 0767e68..e65a0dd 100644
--- a/gcc/genmultilib
+++ b/gcc/genmultilib
@@ -462,7 +462,7 @@ echo "};"
# Generate a regular expression to validate option combinations.
options_re=
for set in ${options}; do
- for opt in `echo ${set} | sed -e 's_[/|]_ _g'`; do
+ for opt in `echo ${set} | sed -e 's_[/|]_ _g' | sed -e 's/\+/./g' `; do
options_re="${options_re}${options_re:+|}${opt}"
done
done
works for me. If OK, I'll commit it with a suitable ChangeLog entry.
Thanks,
> Thanks,
>
> Christophe
>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fix genmultilib reuse rule checks for large sets of option combinations
2017-06-28 8:09 ` Christophe Lyon
@ 2017-06-28 8:14 ` Andreas Schwab
2017-06-28 9:17 ` Christophe Lyon
0 siblings, 1 reply; 8+ messages in thread
From: Andreas Schwab @ 2017-06-28 8:14 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Joseph Myers, gcc-patches
On Jun 28 2017, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> diff --git a/gcc/genmultilib b/gcc/genmultilib
> index 0767e68..e65a0dd 100644
> --- a/gcc/genmultilib
> +++ b/gcc/genmultilib
> @@ -462,7 +462,7 @@ echo "};"
> # Generate a regular expression to validate option combinations.
> options_re=
> for set in ${options}; do
> - for opt in `echo ${set} | sed -e 's_[/|]_ _g'`; do
> + for opt in `echo ${set} | sed -e 's_[/|]_ _g' | sed -e 's/\+/./g' `; do
No need to run two seds, just pass -e twice. Also, + isn't special, so
no backslash.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fix genmultilib reuse rule checks for large sets of option combinations
2017-06-28 8:14 ` Andreas Schwab
@ 2017-06-28 9:17 ` Christophe Lyon
0 siblings, 0 replies; 8+ messages in thread
From: Christophe Lyon @ 2017-06-28 9:17 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Joseph Myers, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 902 bytes --]
On 28 June 2017 at 10:14, Andreas Schwab <schwab@suse.de> wrote:
> On Jun 28 2017, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>
>> diff --git a/gcc/genmultilib b/gcc/genmultilib
>> index 0767e68..e65a0dd 100644
>> --- a/gcc/genmultilib
>> +++ b/gcc/genmultilib
>> @@ -462,7 +462,7 @@ echo "};"
>> # Generate a regular expression to validate option combinations.
>> options_re=
>> for set in ${options}; do
>> - for opt in `echo ${set} | sed -e 's_[/|]_ _g'`; do
>> + for opt in `echo ${set} | sed -e 's_[/|]_ _g' | sed -e 's/\+/./g' `; do
>
> No need to run two seds, just pass -e twice. Also, + isn't special, so
> no backslash.
>
Indeed, thanks.
Here is what I have committed (r249730)
Christophe
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
[-- Attachment #2: genmultilib.chlog.txt --]
[-- Type: text/plain, Size: 123 bytes --]
2017-06-28 Christophe Lyon <christophe.lyon@linaro.org>
* genmultilib (combination_space): Accept '+' in option names.
[-- Attachment #3: genmultilib.patch.txt --]
[-- Type: text/plain, Size: 447 bytes --]
diff --git a/gcc/genmultilib b/gcc/genmultilib
index 0767e68..1da3a6e 100644
--- a/gcc/genmultilib
+++ b/gcc/genmultilib
@@ -462,7 +462,7 @@ echo "};"
# Generate a regular expression to validate option combinations.
options_re=
for set in ${options}; do
- for opt in `echo ${set} | sed -e 's_[/|]_ _g'`; do
+ for opt in `echo ${set} | sed -e 's_[/|]_ _g' -e 's/+/./g' `; do
options_re="${options_re}${options_re:+|}${opt}"
done
done
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-28 9:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-08 20:28 Fix genmultilib reuse rule checks for large sets of option combinations Joseph Myers
2017-06-14 15:09 ` Ping " Joseph Myers
2017-06-27 15:48 ` Ping^2 " Joseph Myers
2017-06-27 16:14 ` Jeff Law
2017-06-28 8:04 ` Christophe Lyon
2017-06-28 8:09 ` Christophe Lyon
2017-06-28 8:14 ` Andreas Schwab
2017-06-28 9:17 ` Christophe Lyon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).