Fix genmultilib reuse rule checks for large sets of option combinations

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:

works for me. If OK, I'll commit it with a suitable ChangeLog entry.

Thanks,

> Thanks,
>
> Christophe
>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com

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.