diff mbox series

genmultilib: Fix sanity check

Message ID 20221121115915.374247-1-christophe.lyon@arm.com
State Accepted
Headers
Series genmultilib: Fix sanity check |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Christophe Lyon Nov. 21, 2022, 11:59 a.m. UTC 
  My previous patch to add a sanity check to genmultilib actually
checked the number of dirnames with the number of "sets of options"
rather than the number of options, thus breaking the build on some
targets.

To avoid duplicating once more the loop that constructs the sed
patterns, this patch checks that the current dirname/osdirname is not
empty in the existing loops.

Are there targets where:
if [ "$1" != "${opt}" ]; then
is "legally" executed with an empty $1? (and thus where this patch
would incorrectly trigger an error?)

Sorry for the breakage. Tested on aarch64 by adding an option to
t-aarch64 and no corresponding dirname, and on x86_64.

OK for trunk?

gcc/ChangeLog:

	* genmultilib: Fix options and dirnames/osdirnames sanity
          check.
---
 gcc/genmultilib | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

Comments

Jakub Jelinek Nov. 21, 2022, 12:17 p.m. UTC | #1 
On Mon, Nov 21, 2022 at 12:59:15PM +0100, Christophe Lyon wrote:
> My previous patch to add a sanity check to genmultilib actually
> checked the number of dirnames with the number of "sets of options"
> rather than the number of options, thus breaking the build on some
> targets.
> 
> To avoid duplicating once more the loop that constructs the sed
> patterns, this patch checks that the current dirname/osdirname is not
> empty in the existing loops.
> 
> Are there targets where:
> if [ "$1" != "${opt}" ]; then
> is "legally" executed with an empty $1? (and thus where this patch
> would incorrectly trigger an error?)

Dunno, let's try your patch.  And if that triggers on something
valid then the next step would be just to revert the sanity checks
completely.

> 	* genmultilib: Fix options and dirnames/osdirnames sanity
>           check.

This won't get through the pre-commit hook, the second line
should be indented just by tab and nothing further.

	Jakub
Christophe Lyon Nov. 21, 2022, 12:20 p.m. UTC | #2 
On 11/21/22 13:17, Jakub Jelinek wrote:
> On Mon, Nov 21, 2022 at 12:59:15PM +0100, Christophe Lyon wrote:
>> My previous patch to add a sanity check to genmultilib actually
>> checked the number of dirnames with the number of "sets of options"
>> rather than the number of options, thus breaking the build on some
>> targets.
>>
>> To avoid duplicating once more the loop that constructs the sed
>> patterns, this patch checks that the current dirname/osdirname is not
>> empty in the existing loops.
>>
>> Are there targets where:
>> if [ "$1" != "${opt}" ]; then
>> is "legally" executed with an empty $1? (and thus where this patch
>> would incorrectly trigger an error?)
> 
> Dunno, let's try your patch.  And if that triggers on something
> valid then the next step would be just to revert the sanity checks
> completely.

Agreed.


> 
>> 	* genmultilib: Fix options and dirnames/osdirnames sanity
>>            check.
> 
> This won't get through the pre-commit hook, the second line
> should be indented just by tab and nothing further.
> 
Thanks for catching this.

Pushed.

Christophe

> 	Jakub
>
Jeff Law Nov. 21, 2022, 2:27 p.m. UTC | #3 
On 11/21/22 05:20, Christophe Lyon via Gcc-patches wrote:
>
>
> On 11/21/22 13:17, Jakub Jelinek wrote:
>> On Mon, Nov 21, 2022 at 12:59:15PM +0100, Christophe Lyon wrote:
>>> My previous patch to add a sanity check to genmultilib actually
>>> checked the number of dirnames with the number of "sets of options"
>>> rather than the number of options, thus breaking the build on some
>>> targets.
>>>
>>> To avoid duplicating once more the loop that constructs the sed
>>> patterns, this patch checks that the current dirname/osdirname is not
>>> empty in the existing loops.
>>>
>>> Are there targets where:
>>> if [ "$1" != "${opt}" ]; then
>>> is "legally" executed with an empty $1? (and thus where this patch
>>> would incorrectly trigger an error?)
>>
>> Dunno, let's try your patch.  And if that triggers on something
>> valid then the next step would be just to revert the sanity checks
>> completely.
>
> Agreed.

The first version (as we know) tripped on a few targets in my tester.  
I've got those restarted and we should know in a few hours if there's 
any major issues.


jeff
diff mbox series

Patch

diff --git a/gcc/genmultilib b/gcc/genmultilib
index b5f372c8358..c0c271eddd6 100644
--- a/gcc/genmultilib
+++ b/gcc/genmultilib
@@ -141,20 +141,6 @@  multiarch=$9
 multilib_reuse=${10}
 enable_multilib=${11}
 
-# Sanity check: make sure we have as many dirnames as options
-if [ -n "${dirnames}" ]; then
-    set x $options
-    nboptions=$#
-    set x $dirnames
-    nbdirnames=$#
-    if [ $nbdirnames -ne $nboptions ]; then
-	echo 1>&2 "Error calling $0: Number of dirnames ($nbdirnames) does not match number of options ($nboptions)"
-	echo 1>&2 "options: ${options}"
-	echo 1>&2 "dirnames: ${dirnames}"
-	exit 1
-    fi
-fi
-
 echo "static const char *const multilib_raw[] = {"
 
 mkdir tmpmultilib.$$ || exit 1
@@ -264,6 +250,10 @@  if [ -n "${dirnames}" ]; then
     for opts in `echo ${set} | sed -e 's|/| |'g`; do
       patt="/"
       for opt in `echo ${opts} | sed -e 's_|_ _'g`; do
+	if [ -z "$1" ]; then
+	  echo 1>&2 "Error calling $0: No dirname for option: $opt"
+	  exit 1
+	fi
         if [ "$1" != "${opt}" ]; then
           todirnames="${todirnames} -e s|/${opt}/|/${1}/|g"
 	  patt="${patt}${1}/"
@@ -320,6 +310,10 @@  if [ -n "${osdirnames}" ]; then
       for opts in `echo ${set} | sed -e 's|/| |'g`; do
         patt="/"
         for opt in `echo ${opts} | sed -e 's_|_ _'g`; do
+	if [ -z "$1" ]; then
+	  echo 1>&2 "Error calling $0: No osdirname for option: $opt"
+	  exit 1
+	fi
           if [ "$1" != "${opt}" ]; then
             toosdirnames="${toosdirnames} -e s|/${opt}/|/${1}/|g"
 	    patt="${patt}${1}/"