On 28 June 2017 at 10:03, Christophe Lyon <christophe.l...@linaro.org> wrote:
> Hi Joseph,
>
> On 8 June 2017 at 22:28, Joseph Myers <jos...@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 <jos...@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
>> jos...@codesourcery.com
