[PATCH] testsuite, lib: Re-allow mulitple function start labels.

[PATCH] testsuite, lib: Re-allow mulitple function start labels.

[Iain Sandoe]

Tested on a cross to armv8l-unknown-linux-gnueabihf where the failing
testcase is restored, and on aarch64-linux-gnu where no change is seen
on the aarch64.exp suite.  Also tested on arm64 Darwin for aarch64.exp
and aarch64-darwin.exp.

OK for trunk, or some alternative would be better?
Iain

--- 8< ---

The change applied in r14-5760-g2a46e0e7e20 changed the behaviour of
functions with assembly like:

bar:
__acle_se_bar:

Where both bar and __acle_se_bar are globals refering to the same
function body.  The old behaviour overrides 'bar' with '__acle_se_bar'
and the scan tests for that label.

The change here re-allows the override.

Case like this are not legal Mach-O (where two global symbols cannot
have the same address in the assembler output).  However, given the
constraints on the Mach-O scanning, it does not seem that it is
necessary to skip the change (any incorrect case should be easily
evident in the assembler).

gcc/testsuite/ChangeLog:

	* lib/scanasm.exp: Allow multiple function start symbols,
	taking the last as the function name.

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
---
 gcc/testsuite/lib/scanasm.exp | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
index 85ee54ff9a8..7ec3cfce02b 100644
--- a/gcc/testsuite/lib/scanasm.exp
+++ b/gcc/testsuite/lib/scanasm.exp
@@ -877,7 +877,15 @@ proc parse_function_bodies { config filename result } {
 		set in_function 0
 	    }
 	} elseif { $in_function } {
-	    if { [regexp $up_config(end) $line] } {
+	    # We allow multiple function start labels, taking the last one seen
+	    # as the function name.
+	    if { [regexp [lindex $up_config(start) 0] \
+			 $line dummy maybe_function_name] } {
+		verbose "parse_function_bodies: overriding $function_name with $maybe_function_name"
+		set function_name $maybe_function_name
+		set in_function 1
+		set function_body ""
+	    } elseif { [regexp $up_config(end) $line] } {
 		verbose "parse_function_bodies: $function_name:\n$function_body"
 		set up_result($function_name) $function_body
 		set in_function 0
-- 
2.39.2 (Apple Git-143)

Re: [PATCH] testsuite, lib: Re-allow mulitple function start labels.

[Christophe Lyon]

Hi Iain,

Thanks for dealing with this :-)

On Thu, 23 Nov 2023 at 10:58, Iain Sandoe <iains.gcc@gmail.com> wrote:
>
> Tested on a cross to armv8l-unknown-linux-gnueabihf where the failing
> testcase is restored, and on aarch64-linux-gnu where no change is seen
> on the aarch64.exp suite.  Also tested on arm64 Darwin for aarch64.exp
> and aarch64-darwin.exp.
>
> OK for trunk, or some alternative would be better?
> Iain
>
> --- 8< ---
>
> The change applied in r14-5760-g2a46e0e7e20 changed the behaviour of
> functions with assembly like:
>
> bar:
> __acle_se_bar:
>
> Where both bar and __acle_se_bar are globals refering to the same
> function body.  The old behaviour overrides 'bar' with '__acle_se_bar'
> and the scan tests for that label.
>
> The change here re-allows the override.
>
> Case like this are not legal Mach-O (where two global symbols cannot
> have the same address in the assembler output).  However, given the
> constraints on the Mach-O scanning, it does not seem that it is
> necessary to skip the change (any incorrect case should be easily
> evident in the assembler).
>
> gcc/testsuite/ChangeLog:
>
>         * lib/scanasm.exp: Allow multiple function start symbols,
>         taking the last as the function name.
>
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> ---
>  gcc/testsuite/lib/scanasm.exp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
> index 85ee54ff9a8..7ec3cfce02b 100644
> --- a/gcc/testsuite/lib/scanasm.exp
> +++ b/gcc/testsuite/lib/scanasm.exp
> @@ -877,7 +877,15 @@ proc parse_function_bodies { config filename result } {
>                 set in_function 0
>             }
>         } elseif { $in_function } {
> -           if { [regexp $up_config(end) $line] } {
> +           # We allow multiple function start labels, taking the last one seen
> +           # as the function name.
> +           if { [regexp [lindex $up_config(start) 0] \
> +                        $line dummy maybe_function_name] } {
> +               verbose "parse_function_bodies: overriding $function_name with $maybe_function_name"
> +               set function_name $maybe_function_name
> +               set in_function 1
this is not necessary, since we are already inside if ($in_function) ?

> +               set function_body ""
> +           } elseif { [regexp $up_config(end) $line] } {
>                 verbose "parse_function_bodies: $function_name:\n$function_body"
>                 set up_result($function_name) $function_body
>                 set in_function 0
> --
> 2.39.2 (Apple Git-143)
>

Thanks,

Christophe

Re: [PATCH] testsuite, lib: Re-allow mulitple function start labels.

[Iain Sandoe]

Hi 

> On 23 Nov 2023, at 16:11, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> 
> Hi Iain,
> 
> Thanks for dealing with this :-)
> 
> On Thu, 23 Nov 2023 at 10:58, Iain Sandoe <iains.gcc@gmail.com> wrote:
>> 
>> Tested on a cross to armv8l-unknown-linux-gnueabihf where the failing
>> testcase is restored, and on aarch64-linux-gnu where no change is seen
>> on the aarch64.exp suite.  Also tested on arm64 Darwin for aarch64.exp
>> and aarch64-darwin.exp.
>> 
>> OK for trunk, or some alternative would be better?
>> Iain
>> 
>> --- 8< ---
>> 
>> The change applied in r14-5760-g2a46e0e7e20 changed the behaviour of
>> functions with assembly like:
>> 
>> bar:
>> __acle_se_bar:
>> 
>> Where both bar and __acle_se_bar are globals refering to the same
>> function body.  The old behaviour overrides 'bar' with '__acle_se_bar'
>> and the scan tests for that label.
>> 
>> The change here re-allows the override.
>> 
>> Case like this are not legal Mach-O (where two global symbols cannot
>> have the same address in the assembler output).  However, given the
>> constraints on the Mach-O scanning, it does not seem that it is
>> necessary to skip the change (any incorrect case should be easily
>> evident in the assembler).
>> 
>> gcc/testsuite/ChangeLog:
>> 
>>        * lib/scanasm.exp: Allow multiple function start symbols,
>>        taking the last as the function name.
>> 
>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>> ---
>> gcc/testsuite/lib/scanasm.exp | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
>> index 85ee54ff9a8..7ec3cfce02b 100644
>> --- a/gcc/testsuite/lib/scanasm.exp
>> +++ b/gcc/testsuite/lib/scanasm.exp
>> @@ -877,7 +877,15 @@ proc parse_function_bodies { config filename result } {
>>                set in_function 0
>>            }
>>        } elseif { $in_function } {
>> -           if { [regexp $up_config(end) $line] } {
>> +           # We allow multiple function start labels, taking the last one seen
>> +           # as the function name.
>> +           if { [regexp [lindex $up_config(start) 0] \
>> +                        $line dummy maybe_function_name] } {
>> +               verbose "parse_function_bodies: overriding $function_name with $maybe_function_name"
>> +               set function_name $maybe_function_name
>> +               set in_function 1
> this is not necessary, since we are already inside if ($in_function) ?

It resets the state to “first line matched” for mutli-line function start cases.   Currently, those cases are only for Darwin and as noted in the changelog actually such code is not legal Mach-O, so in practice, at present we could remove it - but it might be better to be consistent (I am OK either way).

====

Of course, an alternate fix would be to match the first label always and change the testcase to scan for ‘bar’ - since that seems to be the only instance needing this facility  - but I’ll leave that to you folks to consider.

Iain

> 
>> +               set function_body ""
>> +           } elseif { [regexp $up_config(end) $line] } {
>>                verbose "parse_function_bodies: $function_name:\n$function_body"
>>                set up_result($function_name) $function_body
>>                set in_function 0
>> --
>> 2.39.2 (Apple Git-143)
>> 
> 
> Thanks,
> 
> Christophe

Re: [PATCH] testsuite, lib: Re-allow mulitple function start labels.

[Christophe Lyon]

On Thu, 23 Nov 2023 at 17:35, Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi
>
> > On 23 Nov 2023, at 16:11, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> >
> > Hi Iain,
> >
> > Thanks for dealing with this :-)
> >
> > On Thu, 23 Nov 2023 at 10:58, Iain Sandoe <iains.gcc@gmail.com> wrote:
> >>
> >> Tested on a cross to armv8l-unknown-linux-gnueabihf where the failing
> >> testcase is restored, and on aarch64-linux-gnu where no change is seen
> >> on the aarch64.exp suite.  Also tested on arm64 Darwin for aarch64.exp
> >> and aarch64-darwin.exp.
> >>
> >> OK for trunk, or some alternative would be better?
> >> Iain
> >>
> >> --- 8< ---
> >>
> >> The change applied in r14-5760-g2a46e0e7e20 changed the behaviour of
> >> functions with assembly like:
> >>
> >> bar:
> >> __acle_se_bar:
> >>
> >> Where both bar and __acle_se_bar are globals refering to the same
> >> function body.  The old behaviour overrides 'bar' with '__acle_se_bar'
> >> and the scan tests for that label.
> >>
> >> The change here re-allows the override.
> >>
> >> Case like this are not legal Mach-O (where two global symbols cannot
> >> have the same address in the assembler output).  However, given the
> >> constraints on the Mach-O scanning, it does not seem that it is
> >> necessary to skip the change (any incorrect case should be easily
> >> evident in the assembler).
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >>        * lib/scanasm.exp: Allow multiple function start symbols,
> >>        taking the last as the function name.
> >>
> >> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> >> ---
> >> gcc/testsuite/lib/scanasm.exp | 10 +++++++++-
> >> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
> >> index 85ee54ff9a8..7ec3cfce02b 100644
> >> --- a/gcc/testsuite/lib/scanasm.exp
> >> +++ b/gcc/testsuite/lib/scanasm.exp
> >> @@ -877,7 +877,15 @@ proc parse_function_bodies { config filename result } {
> >>                set in_function 0
> >>            }
> >>        } elseif { $in_function } {
> >> -           if { [regexp $up_config(end) $line] } {
> >> +           # We allow multiple function start labels, taking the last one seen
> >> +           # as the function name.
> >> +           if { [regexp [lindex $up_config(start) 0] \
> >> +                        $line dummy maybe_function_name] } {
> >> +               verbose "parse_function_bodies: overriding $function_name with $maybe_function_name"
> >> +               set function_name $maybe_function_name
> >> +               set in_function 1
> > this is not necessary, since we are already inside if ($in_function) ?
>
> It resets the state to “first line matched” for mutli-line function start cases.   Currently, those cases are only for Darwin and as noted in the changelog actually such code is not legal Mach-O, so in practice, at present we could remove it - but it might be better to be consistent (I am OK either way).
>

Ha, right.

Thanks

> ====
>
> Of course, an alternate fix would be to match the first label always and change the testcase to scan for ‘bar’ - since that seems to be the only instance needing this facility  - but I’ll leave that to you folks to consider.
>
> Iain
>
> >
> >> +               set function_body ""
> >> +           } elseif { [regexp $up_config(end) $line] } {
> >>                verbose "parse_function_bodies: $function_name:\n$function_body"
> >>                set up_result($function_name) $function_body
> >>                set in_function 0
> >> --
> >> 2.39.2 (Apple Git-143)
> >>
> >
> > Thanks,
> >
> > Christophe
>

Re: [PATCH] testsuite, lib: Re-allow mulitple function start labels.

[Richard Sandiford]

Iain Sandoe <iain@sandoe.co.uk> writes:
> Hi 
>
>> On 23 Nov 2023, at 16:11, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>> 
>> Hi Iain,
>> 
>> Thanks for dealing with this :-)
>> 
>> On Thu, 23 Nov 2023 at 10:58, Iain Sandoe <iains.gcc@gmail.com> wrote:
>>> 
>>> Tested on a cross to armv8l-unknown-linux-gnueabihf where the failing
>>> testcase is restored, and on aarch64-linux-gnu where no change is seen
>>> on the aarch64.exp suite.  Also tested on arm64 Darwin for aarch64.exp
>>> and aarch64-darwin.exp.
>>> 
>>> OK for trunk, or some alternative would be better?
>>> Iain
>>> 
>>> --- 8< ---
>>> 
>>> The change applied in r14-5760-g2a46e0e7e20 changed the behaviour of
>>> functions with assembly like:
>>> 
>>> bar:
>>> __acle_se_bar:
>>> 
>>> Where both bar and __acle_se_bar are globals refering to the same
>>> function body.  The old behaviour overrides 'bar' with '__acle_se_bar'
>>> and the scan tests for that label.
>>> 
>>> The change here re-allows the override.
>>> 
>>> Case like this are not legal Mach-O (where two global symbols cannot
>>> have the same address in the assembler output).  However, given the
>>> constraints on the Mach-O scanning, it does not seem that it is
>>> necessary to skip the change (any incorrect case should be easily
>>> evident in the assembler).
>>> 
>>> gcc/testsuite/ChangeLog:
>>> 
>>>        * lib/scanasm.exp: Allow multiple function start symbols,
>>>        taking the last as the function name.
>>> 
>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>> ---
>>> gcc/testsuite/lib/scanasm.exp | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
>>> index 85ee54ff9a8..7ec3cfce02b 100644
>>> --- a/gcc/testsuite/lib/scanasm.exp
>>> +++ b/gcc/testsuite/lib/scanasm.exp
>>> @@ -877,7 +877,15 @@ proc parse_function_bodies { config filename result } {
>>>                set in_function 0
>>>            }
>>>        } elseif { $in_function } {
>>> -           if { [regexp $up_config(end) $line] } {
>>> +           # We allow multiple function start labels, taking the last one seen
>>> +           # as the function name.
>>> +           if { [regexp [lindex $up_config(start) 0] \
>>> +                        $line dummy maybe_function_name] } {
>>> +               verbose "parse_function_bodies: overriding $function_name with $maybe_function_name"
>>> +               set function_name $maybe_function_name
>>> +               set in_function 1
>> this is not necessary, since we are already inside if ($in_function) ?
>
> It resets the state to “first line matched” for mutli-line function start cases.   Currently, those cases are only for Darwin and as noted in the changelog actually such code is not legal Mach-O, so in practice, at present we could remove it - but it might be better to be consistent (I am OK either way).
>
> ====
>
> Of course, an alternate fix would be to match the first label always and change the testcase to scan for ‘bar’ - since that seems to be the only instance needing this facility  - but I’ll leave that to you folks to consider.

A third option would be to collect a list of function names and record
them all.  But that won't be any simpler than the posted patch, and there
are no known use cases for multiple names.  So yeah, the patch as posted
is OK.  The:

>>> +               set function_body ""

is redundant though, so IMO it's clearer to drop it.

Thanks,
Richard

>
> Iain
>
>> 
>>> +           } elseif { [regexp $up_config(end) $line] } {
>>>                verbose "parse_function_bodies: $function_name:\n$function_body"
>>>                set up_result($function_name) $function_body
>>>                set in_function 0
>>> --
>>> 2.39.2 (Apple Git-143)
>>> 
>> 
>> Thanks,
>> 
>> Christophe