diff mbox series

testsuite, Darwin: Add support for Mach-O function body scans.

Message ID 20231026192301.58329-1-iain@sandoe.co.uk
State Unresolved
Headers
Series testsuite, Darwin: Add support for Mach-O function body scans. |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Iain Sandoe Oct. 26, 2023, 7:23 p.m. UTC 
  This was written before Thomas' modification to the ELF-handling to allow
a config-based change for target details.  I did consider updating this
to try and use that scheme, but I think that it would sit a little
awkwardly, since there are some differences in the start-up scanning for
Mach-O.  I would say that in all probability we could improve things but
I'd like to put this forward as a well-tested initial implementation.

It has been in use for more than 2 years on the aarch64 Darwin devt.
branch. Tested also in configurations on x86_64-linux and
aarch64-linux.  Obviously, this is not aarch64-specific but that is
out initial use-case.  OK for trunk?
thanks
Iain

--- 8< ---

We need to process the source slightly differently from ELF, especially
in that we have __USER_LABEL_PREFIX__ and there are no function start
and end assembler directives.  This means we cannot delineate functions
when frame output is switched off.

TODO: consider adding -mtest-markers or something similar to inject
assembler comments that can be scanned for.

gcc/testsuite/ChangeLog:

	* lib/scanasm.exp: Initial handling for Mach-O function body scans.

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

Comments

Richard Sandiford Oct. 26, 2023, 7:49 p.m. UTC | #1 
Iain Sandoe <iains.gcc@gmail.com> writes:
> This was written before Thomas' modification to the ELF-handling to allow
> a config-based change for target details.  I did consider updating this
> to try and use that scheme, but I think that it would sit a little
> awkwardly, since there are some differences in the start-up scanning for
> Mach-O.  I would say that in all probability we could improve things but
> I'd like to put this forward as a well-tested initial implementation.

Sorry, I would prefer to extend the existing function instead.
E.g. there's already some divergence between the Mach-O version
and the default version, in that the Mach-O version doesn't print
verbose messages.  I also don't think that the current default code
is so watertight that it'll never need to be updated in future.

Some questions/suggestions below.

> It has been in use for more than 2 years on the aarch64 Darwin devt.
> branch. Tested also in configurations on x86_64-linux and
> aarch64-linux.  Obviously, this is not aarch64-specific but that is
> out initial use-case.  OK for trunk?
> thanks
> Iain
>
> --- 8< ---
>
> We need to process the source slightly differently from ELF, especially
> in that we have __USER_LABEL_PREFIX__ and there are no function start
> and end assembler directives.  This means we cannot delineate functions
> when frame output is switched off.
>
> TODO: consider adding -mtest-markers or something similar to inject
> assembler comments that can be scanned for.
>
> gcc/testsuite/ChangeLog:
>
> 	* lib/scanasm.exp: Initial handling for Mach-O function body scans.
>
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> ---
>  gcc/testsuite/lib/scanasm.exp | 55 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
> index 5df80325dff..fab4b0669ec 100644
> --- a/gcc/testsuite/lib/scanasm.exp
> +++ b/gcc/testsuite/lib/scanasm.exp
> @@ -851,6 +851,44 @@ proc parse_function_bodies { config filename result } {
>      close $fd
>  }
>  
> +proc parse_MACHO_function_bodies { filename result } {
> +    upvar $result up_result
> +
> +    # Regexp for the start of a function definition (name in \1).
> +    set label {^(_[a-zA-Z_]\S+):$}
> +    set start {^LFB[0-9]+}

Do you need to single out the start label like this?  Couldn't we just
skip LFB labels, by extending the fluff regexp?

> +
> +    # Regexp for the end of a function definition.
> +    set terminator {^LFE[0-9]+}
> +
> +    # Regexp for lines that aren't interesting.
> +    set fluff {^\s*(?:\.|//|@)}
> +    set fluff3 {^L[0-9ACESV]}

Is there any need for two regexps here?  It looks like we could combine
them as:

  {^\s*(?:\.|//|@)|^L[0-9ACESV]}

Or, with the above suggestion too:

  {^\s*(?:\.|//|@)|^L[0-9ACESV]|^LFB[0-9]}

If that works, it seems to fit within the existing config scheme.

Thanks,
Richard

> +
> +    set fd [open $filename r]
> +    set in_function 0
> +    while { [gets $fd line] >= 0 } {
> +	if { !$in_function && [regexp $label $line dummy function_name] } {
> +	    set in_function 1
> +	    set function_body ""
> +	} elseif { $in_function == 1 } {
> +	  if { [regexp $start $line] } {
> +	    set in_function 2
> +	  } else {
> +	    set in_function 0
> +	  }
> +	} elseif { $in_function == 2 } {
> +	    if { [regexp $terminator $line] } {
> +		set up_result($function_name) $function_body
> +		set in_function 0
> +	    } elseif { ![regexp $fluff $line]  && ![regexp $fluff3 $line] } {
> +		append function_body $line "\n"
> +	    }
> +	}
> +    }
> +    close $fd
> +}
> +
>  # FUNCTIONS is an array that maps function names to function bodies.
>  # Return true if it contains a definition of function NAME and if
>  # that definition matches BODY_REGEXP.
> @@ -883,6 +921,14 @@ proc check-function-bodies { args } {
>  	error "too many arguments to check-function-bodies"
>      }
>  
> +    set isELF 1
> +    # some targets have a __USER_LABEL_PREFIX__
> +    set needsULP 0
> +    if { [istarget *-*-darwin*] } {
> +      set isELF 0
> +      set needsULP 1
> +    }
> +
>      if { [llength $args] >= 3 } {
>  	set required_flags [lindex $args 2]
>  
> @@ -944,7 +990,11 @@ proc check-function-bodies { args } {
>  	remote_upload host "$filename"
>      }
>      if { [file exists $output_filename] } {
> -	parse_function_bodies config $output_filename functions
> +        if { $isELF } {
> +	    parse_function_bodies config $output_filename functions
> +	} else {
> +	    parse_MACHO_function_bodies $output_filename functions
> +	}
>  	set have_bodies 1
>      } else {
>  	verbose -log "$testcase: output file does not exist"
> @@ -988,6 +1038,9 @@ proc check-function-bodies { args } {
>  		if { $xfail_all || [string equal $selector "F"] } {
>  		    setup_xfail "*-*-*"
>  		}
> +		if { $needsULP } {
> +		    set function_name "_$function_name"
> +		}
>  		set testname "$testcase check-function-bodies $function_name"
>  		if { !$have_bodies } {
>  		    unresolved $testname
Iain Sandoe Oct. 26, 2023, 8 p.m. UTC | #2 
Hi Richard,

> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com> wrote:
> 
> Iain Sandoe <iains.gcc@gmail.com> writes:
>> This was written before Thomas' modification to the ELF-handling to allow
>> a config-based change for target details.  I did consider updating this
>> to try and use that scheme, but I think that it would sit a little
>> awkwardly, since there are some differences in the start-up scanning for
>> Mach-O.  I would say that in all probability we could improve things but
>> I'd like to put this forward as a well-tested initial implementation.
> 
> Sorry, I would prefer to extend the existing function instead.
> E.g. there's already some divergence between the Mach-O version
> and the default version, in that the Mach-O version doesn't print
> verbose messages.  I also don't think that the current default code
> is so watertight that it'll never need to be updated in future.

Fair enough, will explore what can be done (as I recall last I looked the
primary difference was in the initial start-up scan).

> Some questions/suggestions below.
> 
>> It has been in use for more than 2 years on the aarch64 Darwin devt.
>> branch. Tested also in configurations on x86_64-linux and
>> aarch64-linux.  Obviously, this is not aarch64-specific but that is
>> out initial use-case.  OK for trunk?
>> thanks
>> Iain
>> 
>> --- 8< ---
>> 
>> We need to process the source slightly differently from ELF, especially
>> in that we have __USER_LABEL_PREFIX__ and there are no function start
>> and end assembler directives.  This means we cannot delineate functions
>> when frame output is switched off.
>> 
>> TODO: consider adding -mtest-markers or something similar to inject
>> assembler comments that can be scanned for.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 	* lib/scanasm.exp: Initial handling for Mach-O function body scans.
>> 
>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>> ---
>> gcc/testsuite/lib/scanasm.exp | 55 ++++++++++++++++++++++++++++++++++-
>> 1 file changed, 54 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
>> index 5df80325dff..fab4b0669ec 100644
>> --- a/gcc/testsuite/lib/scanasm.exp
>> +++ b/gcc/testsuite/lib/scanasm.exp
>> @@ -851,6 +851,44 @@ proc parse_function_bodies { config filename result } {
>>     close $fd
>> }
>> 
>> +proc parse_MACHO_function_bodies { filename result } {
>> +    upvar $result up_result
>> +
>> +    # Regexp for the start of a function definition (name in \1).
>> +    set label {^(_[a-zA-Z_]\S+):$}
>> +    set start {^LFB[0-9]+}
> 
> Do you need to single out the start label like this?

Yes, as of now we do;
Mach-O has not .function or .size markers to delineate the code.
So, without modifying the asm output to emit something new that we can detect,
and given that unwind frames are the default on Darwin, the LFB/LFE markers
are the only reliable mechanism at present


>  Couldn't we just skip LFB labels, by extending the fluff regexp?

in this instance, they are not fluff.

The idea I had recently was to emit some well-defined markers into to asm
comments (say in response to -memit-test-markers or similar) so that we could
scan for those and avoid depending on the LFB/E.  However, that means modifying
the port output code as well.

>> +
>> +    # Regexp for the end of a function definition.
>> +    set terminator {^LFE[0-9]+}
>> +
>> +    # Regexp for lines that aren't interesting.
>> +    set fluff {^\s*(?:\.|//|@)}
>> +    set fluff3 {^L[0-9ACESV]}
> 
> Is there any need for two regexps here?  It looks like we could combine
> them as:
> 
>  {^\s*(?:\.|//|@)|^L[0-9ACESV]}

Will examine that again (I think I only inherited two matches from the original).

> Or, with the above suggestion too:
> 
>  {^\s*(?:\.|//|@)|^L[0-9ACESV]|^LFB[0-9]}
> 
> If that works, it seems to fit within the existing config scheme.

OK - will see how far I get.
thanks
Iain

> Thanks,
> Richard
> 
>> +
>> +    set fd [open $filename r]
>> +    set in_function 0
>> +    while { [gets $fd line] >= 0 } {
>> +	if { !$in_function && [regexp $label $line dummy function_name] } {
>> +	    set in_function 1
>> +	    set function_body ""
>> +	} elseif { $in_function == 1 } {
>> +	  if { [regexp $start $line] } {
>> +	    set in_function 2
>> +	  } else {
>> +	    set in_function 0
>> +	  }
>> +	} elseif { $in_function == 2 } {
>> +	    if { [regexp $terminator $line] } {
>> +		set up_result($function_name) $function_body
>> +		set in_function 0
>> +	    } elseif { ![regexp $fluff $line]  && ![regexp $fluff3 $line] } {
>> +		append function_body $line "\n"
>> +	    }
>> +	}
>> +    }
>> +    close $fd
>> +}
>> +
>> # FUNCTIONS is an array that maps function names to function bodies.
>> # Return true if it contains a definition of function NAME and if
>> # that definition matches BODY_REGEXP.
>> @@ -883,6 +921,14 @@ proc check-function-bodies { args } {
>> 	error "too many arguments to check-function-bodies"
>>     }
>> 
>> +    set isELF 1
>> +    # some targets have a __USER_LABEL_PREFIX__
>> +    set needsULP 0
>> +    if { [istarget *-*-darwin*] } {
>> +      set isELF 0
>> +      set needsULP 1
>> +    }
>> +
>>     if { [llength $args] >= 3 } {
>> 	set required_flags [lindex $args 2]
>> 
>> @@ -944,7 +990,11 @@ proc check-function-bodies { args } {
>> 	remote_upload host "$filename"
>>     }
>>     if { [file exists $output_filename] } {
>> -	parse_function_bodies config $output_filename functions
>> +        if { $isELF } {
>> +	    parse_function_bodies config $output_filename functions
>> +	} else {
>> +	    parse_MACHO_function_bodies $output_filename functions
>> +	}
>> 	set have_bodies 1
>>     } else {
>> 	verbose -log "$testcase: output file does not exist"
>> @@ -988,6 +1038,9 @@ proc check-function-bodies { args } {
>> 		if { $xfail_all || [string equal $selector "F"] } {
>> 		    setup_xfail "*-*-*"
>> 		}
>> +		if { $needsULP } {
>> +		    set function_name "_$function_name"
>> +		}
>> 		set testname "$testcase check-function-bodies $function_name"
>> 		if { !$have_bodies } {
>> 		    unresolved $testname
Iain Sandoe Oct. 27, 2023, 11 a.m. UTC | #3 
Hi Richard,

> On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:

>> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com> wrote:
>> 
>> Iain Sandoe <iains.gcc@gmail.com> writes:
>>> This was written before Thomas' modification to the ELF-handling to allow
>>> a config-based change for target details.  I did consider updating this
>>> to try and use that scheme, but I think that it would sit a little
>>> awkwardly, since there are some differences in the start-up scanning for
>>> Mach-O.  I would say that in all probability we could improve things but
>>> I'd like to put this forward as a well-tested initial implementation.
>> 
>> Sorry, I would prefer to extend the existing function instead.
>> E.g. there's already some divergence between the Mach-O version
>> and the default version, in that the Mach-O version doesn't print
>> verbose messages.  I also don't think that the current default code
>> is so watertight that it'll never need to be updated in future.
> 
> Fair enough, will explore what can be done (as I recall last I looked the
> primary difference was in the initial start-up scan).

I’ve done this as attached.

For the record, when doing it, it gave rise to the same misgivings that led
to the separate implementation before.

 * as we add formats and uncover asm oddities, they all need to be handled
   in one set of code, IMO it could be come quite convoluted.

 * now making a change to the MACH-O code, means I have to check I did not
   inadvertently break ELF (and likewise, in theory, an ELF change should check
   MACH-O, but many folks do/can not do that).

Maybe there’s some half-way-house where code can usefully be shared without
those down-sides.

Anyway, to make progress, is the revised version OK for trunk? (tested on
aarch64-linux and aarch64-darwin).
thanks
Iain
Andrew Pinski Oct. 27, 2023, 10:44 p.m. UTC | #4 
On Fri, Oct 27, 2023 at 4:00 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi Richard,
>
> > On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> >> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com> wrote:
> >>
> >> Iain Sandoe <iains.gcc@gmail.com> writes:
> >>> This was written before Thomas' modification to the ELF-handling to allow
> >>> a config-based change for target details.  I did consider updating this
> >>> to try and use that scheme, but I think that it would sit a little
> >>> awkwardly, since there are some differences in the start-up scanning for
> >>> Mach-O.  I would say that in all probability we could improve things but
> >>> I'd like to put this forward as a well-tested initial implementation.
> >>
> >> Sorry, I would prefer to extend the existing function instead.
> >> E.g. there's already some divergence between the Mach-O version
> >> and the default version, in that the Mach-O version doesn't print
> >> verbose messages.  I also don't think that the current default code
> >> is so watertight that it'll never need to be updated in future.
> >
> > Fair enough, will explore what can be done (as I recall last I looked the
> > primary difference was in the initial start-up scan).
>
> I’ve done this as attached.
>
> For the record, when doing it, it gave rise to the same misgivings that led
> to the separate implementation before.
>
>  * as we add formats and uncover asm oddities, they all need to be handled
>    in one set of code, IMO it could be come quite convoluted.
>
>  * now making a change to the MACH-O code, means I have to check I did not
>    inadvertently break ELF (and likewise, in theory, an ELF change should check
>    MACH-O, but many folks do/can not do that).
>
> Maybe there’s some half-way-house where code can usefully be shared without
> those down-sides.

There is already gcc.test-framework which seems like a good place to
put a test for both formats so when someone changes the function, they
could run that testsuite to make sure it is still working for the
other format.
(Note I am not saying you should add it as part of this patch but it
seems like that would be the perfect place for it.)

Thanks,
Andrew

>
> Anyway, to make progress, is the revised version OK for trunk? (tested on
> aarch64-linux and aarch64-darwin).
> thanks
> Iain
>
>
>
Richard Sandiford Nov. 5, 2023, 10:11 p.m. UTC | #5 
Iain Sandoe <iain@sandoe.co.uk> writes:
> Hi Richard,
>
>> On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
>
>>> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com>
> wrote:
>>>
>>> Iain Sandoe <iains.gcc@gmail.com> writes:
>>>> This was written before Thomas' modification to the ELF-handling to allow
>>>> a config-based change for target details.  I did consider updating this
>>>> to try and use that scheme, but I think that it would sit a little
>>>> awkwardly, since there are some differences in the start-up scanning for
>>>> Mach-O.  I would say that in all probability we could improve things but
>>>> I'd like to put this forward as a well-tested initial implementation.
>>>
>>> Sorry, I would prefer to extend the existing function instead.
>>> E.g. there's already some divergence between the Mach-O version
>>> and the default version, in that the Mach-O version doesn't print
>>> verbose messages.  I also don't think that the current default code
>>> is so watertight that it'll never need to be updated in future.
>>
>> Fair enough, will explore what can be done (as I recall last I looked the
>> primary difference was in the initial start-up scan).
>
> I’ve done this as attached.
>
> For the record, when doing it, it gave rise to the same misgivings that led
> to the separate implementation before.
>
>  * as we add formats and uncover asm oddities, they all need to be handled
>    in one set of code, IMO it could be come quite convoluted.
>
>  * now making a change to the MACH-O code, means I have to check I did not
>    inadvertently break ELF (and likewise, in theory, an ELF change should check
>    MACH-O, but many folks do/can not do that).
>
> Maybe there’s some half-way-house where code can usefully be shared without
> those down-sides.
>
> Anyway, to make progress, is the revised version OK for trunk? (tested on
> aarch64-linux and aarch64-darwin).

Sorry for the slow reply.  I was hoping we'd be able to share a bit more
code than that, and avoid an isMACHO toggle.  Does something like the
attached adaption of your patch work?  Only spot-checked on
aarch64-linux-gnu so far.

(The patch tries to avoid capturing the user label prefix, hopefully
avoiding the needsULP thing.)

Thanks,
Richard


diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
index 5df80325dff..2434550f0c3 100644
--- a/gcc/testsuite/lib/scanasm.exp
+++ b/gcc/testsuite/lib/scanasm.exp
@@ -785,23 +785,34 @@ proc configure_check-function-bodies { config } {
 
     # Regexp for the start of a function definition (name in \1).
     if { [istarget nvptx*-*-*] } {
-	set up_config(start) {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
+	set up_config(start) {
+	    {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
+	}
+    } elseif { [istarget *-*-darwin*] } {
+	set up_config(start) {
+	    {^_([a-zA-Z_]\S+):$}
+	    {^LFB[0-9]+:}
+	}
     } else {
-	set up_config(start) {^([a-zA-Z_]\S+):$}
+	set up_config(start) {{^([a-zA-Z_]\S+):$}}
     }
 
     # Regexp for the end of a function definition.
     if { [istarget nvptx*-*-*] } {
 	set up_config(end) {^\}$}
+    } elseif { [istarget *-*-darwin*] } {
+	set up_config(end) {^LFE[0-9]+}
     } else {
 	set up_config(end) {^\s*\.size}
     }
- 
+
     # Regexp for lines that aren't interesting.
     if { [istarget nvptx*-*-*] } {
 	# Skip lines beginning with '//' comments ('-fverbose-asm', for
 	# example).
 	set up_config(fluff) {^\s*(?://)}
+    } elseif { [istarget *-*-darwin*] } {
+	set up_config(fluff) {^\s*(?:\.|//|@)|^L[0-9ACESV]}
     } else {
 	# Skip lines beginning with labels ('.L[...]:') or other directives
 	# ('.align', '.cfi_startproc', '.quad [...]', '.text', etc.), '//' or
@@ -833,9 +844,19 @@ proc parse_function_bodies { config filename result } {
     set fd [open $filename r]
     set in_function 0
     while { [gets $fd line] >= 0 } {
-	if { [regexp $up_config(start) $line dummy function_name] } {
-	    set in_function 1
-	    set function_body ""
+	if { $in_function == 0 } {
+	    if { [regexp [lindex $up_config(start) 0] \
+			 $line dummy function_name] } {
+		set in_function 1
+		set function_body ""
+	    }
+	} elseif { $in_function < [llength $up_config(start)] } {
+	    if { [regexp [lindex $up_config(start) $in_function] $line] } {
+		incr in_function
+	    } else {
+		verbose "parse_function_bodies: skipped $function_name"
+		set in_function 0
+	    }
 	} elseif { $in_function } {
 	    if { [regexp $up_config(end) $line] } {
 		verbose "parse_function_bodies: $function_name:\n$function_body"
Iain Sandoe Nov. 6, 2023, 7:59 a.m. UTC | #6 
Hi Richard,

> On 5 Nov 2023, at 12:11, Richard Sandiford <richard.sandiford@arm.com> wrote:
> 
> Iain Sandoe <iain@sandoe.co.uk> writes:

>>> On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>>>> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com>
>> wrote:
>>>> 
>>>> Iain Sandoe <iains.gcc@gmail.com> writes:
>>>>> This was written before Thomas' modification to the ELF-handling to allow
>>>>> a config-based change for target details.  I did consider updating this
>>>>> to try and use that scheme, but I think that it would sit a little
>>>>> awkwardly, since there are some differences in the start-up scanning for
>>>>> Mach-O.  I would say that in all probability we could improve things but
>>>>> I'd like to put this forward as a well-tested initial implementation.
>>>> 
>>>> Sorry, I would prefer to extend the existing function instead.
>>>> E.g. there's already some divergence between the Mach-O version
>>>> and the default version, in that the Mach-O version doesn't print
>>>> verbose messages.  I also don't think that the current default code
>>>> is so watertight that it'll never need to be updated in future.
>>> 
>>> Fair enough, will explore what can be done (as I recall last I looked the
>>> primary difference was in the initial start-up scan).
>> 
>> I’ve done this as attached.
>> 
>> For the record, when doing it, it gave rise to the same misgivings that led
>> to the separate implementation before.
>> 
>> * as we add formats and uncover asm oddities, they all need to be handled
>>   in one set of code, IMO it could be come quite convoluted.
>> 
>> * now making a change to the MACH-O code, means I have to check I did not
>>   inadvertently break ELF (and likewise, in theory, an ELF change should check
>>   MACH-O, but many folks do/can not do that).
>> 
>> Maybe there’s some half-way-house where code can usefully be shared without
>> those down-sides.
>> 
>> Anyway, to make progress, is the revised version OK for trunk? (tested on
>> aarch64-linux and aarch64-darwin).
> 
> Sorry for the slow reply.  I was hoping we'd be able to share a bit more
> code than that, and avoid an isMACHO toggle.  Does something like the
> attached adaption of your patch work?  Only spot-checked on
> aarch64-linux-gnu so far.
> 
> (The patch tries to avoid capturing the user label prefix, hopefully
> avoiding the needsULP thing.)

Yes, this works for me too for Arm64 Darwin (and probably is fine for other
Darwin archs in case we implement body tests there).  If we decide to emit
some comment-based markers to delineat functions without unwind data,
we can just amend the start and end.

thanks,
Iain
(doing some wider testing, but for now the only mach-o cases are in the
 arm64 code, so the fact that those passed so far is pretty good indication).

-----

As an aside what’s the intention for cases like this?

	.data
foo:
	.xxxx ….
	.size foo, .-foo



> 
> Thanks,
> Richard
> 
> 
> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
> index 5df80325dff..2434550f0c3 100644
> --- a/gcc/testsuite/lib/scanasm.exp
> +++ b/gcc/testsuite/lib/scanasm.exp
> @@ -785,23 +785,34 @@ proc configure_check-function-bodies { config } {
> 
>     # Regexp for the start of a function definition (name in \1).
>     if { [istarget nvptx*-*-*] } {
> -	set up_config(start) {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
> +	set up_config(start) {
> +	    {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
> +	}
> +    } elseif { [istarget *-*-darwin*] } {
> +	set up_config(start) {
> +	    {^_([a-zA-Z_]\S+):$}
> +	    {^LFB[0-9]+:}
> +	}
>     } else {
> -	set up_config(start) {^([a-zA-Z_]\S+):$}
> +	set up_config(start) {{^([a-zA-Z_]\S+):$}}
>     }
> 
>     # Regexp for the end of a function definition.
>     if { [istarget nvptx*-*-*] } {
> 	set up_config(end) {^\}$}
> +    } elseif { [istarget *-*-darwin*] } {
> +	set up_config(end) {^LFE[0-9]+}
>     } else {
> 	set up_config(end) {^\s*\.size}
>     }
> - 
> +
>     # Regexp for lines that aren't interesting.
>     if { [istarget nvptx*-*-*] } {
> 	# Skip lines beginning with '//' comments ('-fverbose-asm', for
> 	# example).
> 	set up_config(fluff) {^\s*(?://)}
> +    } elseif { [istarget *-*-darwin*] } {
> +	set up_config(fluff) {^\s*(?:\.|//|@)|^L[0-9ACESV]}
>     } else {
> 	# Skip lines beginning with labels ('.L[...]:') or other directives
> 	# ('.align', '.cfi_startproc', '.quad [...]', '.text', etc.), '//' or
> @@ -833,9 +844,19 @@ proc parse_function_bodies { config filename result } {
>     set fd [open $filename r]
>     set in_function 0
>     while { [gets $fd line] >= 0 } {
> -	if { [regexp $up_config(start) $line dummy function_name] } {
> -	    set in_function 1
> -	    set function_body ""
> +	if { $in_function == 0 } {
> +	    if { [regexp [lindex $up_config(start) 0] \
> +			 $line dummy function_name] } {
> +		set in_function 1
> +		set function_body ""
> +	    }
> +	} elseif { $in_function < [llength $up_config(start)] } {
> +	    if { [regexp [lindex $up_config(start) $in_function] $line] } {
> +		incr in_function
> +	    } else {
> +		verbose "parse_function_bodies: skipped $function_name"
> +		set in_function 0
> +	    }
> 	} elseif { $in_function } {
> 	    if { [regexp $up_config(end) $line] } {
> 		verbose "parse_function_bodies: $function_name:\n$function_body"
> -- 
> 2.25.1
Richard Sandiford Nov. 6, 2023, 10:57 a.m. UTC | #7 
Iain Sandoe <iain@sandoe.co.uk> writes:
> Hi Richard,
>
>> On 5 Nov 2023, at 12:11, Richard Sandiford <richard.sandiford@arm.com> wrote:
>> 
>> Iain Sandoe <iain@sandoe.co.uk> writes:
>
>>>> On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>> 
>>>>> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com>
>>> wrote:
>>>>> 
>>>>> Iain Sandoe <iains.gcc@gmail.com> writes:
>>>>>> This was written before Thomas' modification to the ELF-handling to allow
>>>>>> a config-based change for target details.  I did consider updating this
>>>>>> to try and use that scheme, but I think that it would sit a little
>>>>>> awkwardly, since there are some differences in the start-up scanning for
>>>>>> Mach-O.  I would say that in all probability we could improve things but
>>>>>> I'd like to put this forward as a well-tested initial implementation.
>>>>> 
>>>>> Sorry, I would prefer to extend the existing function instead.
>>>>> E.g. there's already some divergence between the Mach-O version
>>>>> and the default version, in that the Mach-O version doesn't print
>>>>> verbose messages.  I also don't think that the current default code
>>>>> is so watertight that it'll never need to be updated in future.
>>>> 
>>>> Fair enough, will explore what can be done (as I recall last I looked the
>>>> primary difference was in the initial start-up scan).
>>> 
>>> I’ve done this as attached.
>>> 
>>> For the record, when doing it, it gave rise to the same misgivings that led
>>> to the separate implementation before.
>>> 
>>> * as we add formats and uncover asm oddities, they all need to be handled
>>>   in one set of code, IMO it could be come quite convoluted.
>>> 
>>> * now making a change to the MACH-O code, means I have to check I did not
>>>   inadvertently break ELF (and likewise, in theory, an ELF change should check
>>>   MACH-O, but many folks do/can not do that).
>>> 
>>> Maybe there’s some half-way-house where code can usefully be shared without
>>> those down-sides.
>>> 
>>> Anyway, to make progress, is the revised version OK for trunk? (tested on
>>> aarch64-linux and aarch64-darwin).
>> 
>> Sorry for the slow reply.  I was hoping we'd be able to share a bit more
>> code than that, and avoid an isMACHO toggle.  Does something like the
>> attached adaption of your patch work?  Only spot-checked on
>> aarch64-linux-gnu so far.
>> 
>> (The patch tries to avoid capturing the user label prefix, hopefully
>> avoiding the needsULP thing.)
>
> Yes, this works for me too for Arm64 Darwin (and probably is fine for other
> Darwin archs in case we implement body tests there).  If we decide to emit
> some comment-based markers to delineat functions without unwind data,
> we can just amend the start and end.
>
> thanks,
> Iain
> (doing some wider testing, but for now the only mach-o cases are in the
>  arm64 code, so the fact that those passed so far is pretty good indication).

OK, great.  It passed testing for me too, so please go ahead and commit
if it does for you.

> -----
>
> As an aside what’s the intention for cases like this?
>
> 	.data
> foo:
> 	.xxxx ….
> 	.size foo, .-foo

ATM there's no way for the test to say that specific pseudo-ops are
interesting to it.  Same for labels.  It might be useful to add
support for that though.

Thanks,
Richard

>
>
>
>> 
>> Thanks,
>> Richard
>> 
>> 
>> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
>> index 5df80325dff..2434550f0c3 100644
>> --- a/gcc/testsuite/lib/scanasm.exp
>> +++ b/gcc/testsuite/lib/scanasm.exp
>> @@ -785,23 +785,34 @@ proc configure_check-function-bodies { config } {
>> 
>>     # Regexp for the start of a function definition (name in \1).
>>     if { [istarget nvptx*-*-*] } {
>> -	set up_config(start) {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
>> +	set up_config(start) {
>> +	    {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
>> +	}
>> +    } elseif { [istarget *-*-darwin*] } {
>> +	set up_config(start) {
>> +	    {^_([a-zA-Z_]\S+):$}
>> +	    {^LFB[0-9]+:}
>> +	}
>>     } else {
>> -	set up_config(start) {^([a-zA-Z_]\S+):$}
>> +	set up_config(start) {{^([a-zA-Z_]\S+):$}}
>>     }
>> 
>>     # Regexp for the end of a function definition.
>>     if { [istarget nvptx*-*-*] } {
>> 	set up_config(end) {^\}$}
>> +    } elseif { [istarget *-*-darwin*] } {
>> +	set up_config(end) {^LFE[0-9]+}
>>     } else {
>> 	set up_config(end) {^\s*\.size}
>>     }
>> - 
>> +
>>     # Regexp for lines that aren't interesting.
>>     if { [istarget nvptx*-*-*] } {
>> 	# Skip lines beginning with '//' comments ('-fverbose-asm', for
>> 	# example).
>> 	set up_config(fluff) {^\s*(?://)}
>> +    } elseif { [istarget *-*-darwin*] } {
>> +	set up_config(fluff) {^\s*(?:\.|//|@)|^L[0-9ACESV]}
>>     } else {
>> 	# Skip lines beginning with labels ('.L[...]:') or other directives
>> 	# ('.align', '.cfi_startproc', '.quad [...]', '.text', etc.), '//' or
>> @@ -833,9 +844,19 @@ proc parse_function_bodies { config filename result } {
>>     set fd [open $filename r]
>>     set in_function 0
>>     while { [gets $fd line] >= 0 } {
>> -	if { [regexp $up_config(start) $line dummy function_name] } {
>> -	    set in_function 1
>> -	    set function_body ""
>> +	if { $in_function == 0 } {
>> +	    if { [regexp [lindex $up_config(start) 0] \
>> +			 $line dummy function_name] } {
>> +		set in_function 1
>> +		set function_body ""
>> +	    }
>> +	} elseif { $in_function < [llength $up_config(start)] } {
>> +	    if { [regexp [lindex $up_config(start) $in_function] $line] } {
>> +		incr in_function
>> +	    } else {
>> +		verbose "parse_function_bodies: skipped $function_name"
>> +		set in_function 0
>> +	    }
>> 	} elseif { $in_function } {
>> 	    if { [regexp $up_config(end) $line] } {
>> 		verbose "parse_function_bodies: $function_name:\n$function_body"
>> -- 
>> 2.25.1
Christophe Lyon Nov. 23, 2023, 9:02 a.m. UTC | #8 
Hi Iain,

On Mon, 6 Nov 2023 at 11:58, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Iain Sandoe <iain@sandoe.co.uk> writes:
> > Hi Richard,
> >
> >> On 5 Nov 2023, at 12:11, Richard Sandiford <richard.sandiford@arm.com> wrote:
> >>
> >> Iain Sandoe <iain@sandoe.co.uk> writes:
> >
> >>>> On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>>
> >>>>> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com>
> >>> wrote:
> >>>>>
> >>>>> Iain Sandoe <iains.gcc@gmail.com> writes:
> >>>>>> This was written before Thomas' modification to the ELF-handling to allow
> >>>>>> a config-based change for target details.  I did consider updating this
> >>>>>> to try and use that scheme, but I think that it would sit a little
> >>>>>> awkwardly, since there are some differences in the start-up scanning for
> >>>>>> Mach-O.  I would say that in all probability we could improve things but
> >>>>>> I'd like to put this forward as a well-tested initial implementation.
> >>>>>
> >>>>> Sorry, I would prefer to extend the existing function instead.
> >>>>> E.g. there's already some divergence between the Mach-O version
> >>>>> and the default version, in that the Mach-O version doesn't print
> >>>>> verbose messages.  I also don't think that the current default code
> >>>>> is so watertight that it'll never need to be updated in future.
> >>>>
> >>>> Fair enough, will explore what can be done (as I recall last I looked the
> >>>> primary difference was in the initial start-up scan).
> >>>
> >>> I’ve done this as attached.
> >>>
> >>> For the record, when doing it, it gave rise to the same misgivings that led
> >>> to the separate implementation before.
> >>>
> >>> * as we add formats and uncover asm oddities, they all need to be handled
> >>>   in one set of code, IMO it could be come quite convoluted.
> >>>
> >>> * now making a change to the MACH-O code, means I have to check I did not
> >>>   inadvertently break ELF (and likewise, in theory, an ELF change should check
> >>>   MACH-O, but many folks do/can not do that).
> >>>
> >>> Maybe there’s some half-way-house where code can usefully be shared without
> >>> those down-sides.
> >>>
> >>> Anyway, to make progress, is the revised version OK for trunk? (tested on
> >>> aarch64-linux and aarch64-darwin).
> >>
> >> Sorry for the slow reply.  I was hoping we'd be able to share a bit more
> >> code than that, and avoid an isMACHO toggle.  Does something like the
> >> attached adaption of your patch work?  Only spot-checked on
> >> aarch64-linux-gnu so far.
> >>
> >> (The patch tries to avoid capturing the user label prefix, hopefully
> >> avoiding the needsULP thing.)
> >
> > Yes, this works for me too for Arm64 Darwin (and probably is fine for other
> > Darwin archs in case we implement body tests there).  If we decide to emit
> > some comment-based markers to delineat functions without unwind data,
> > we can just amend the start and end.
> >
> > thanks,
> > Iain
> > (doing some wider testing, but for now the only mach-o cases are in the
> >  arm64 code, so the fact that those passed so far is pretty good indication).
>
> OK, great.  It passed testing for me too, so please go ahead and commit
> if it does for you.
>
> > -----
> >
> > As an aside what’s the intention for cases like this?
> >
> >       .data
> > foo:
> >       .xxxx ….
> >       .size foo, .-foo
>
> ATM there's no way for the test to say that specific pseudo-ops are
> interesting to it.  Same for labels.  It might be useful to add
> support for that though.
>
> Thanks,
> Richard
>

As you have probably already noticed with the notification from our
CI, this patch causes
FAIL: gcc.target/arm/pr95646.c check-function-bodies __acle_se_bar
At quick glance it's not obvious to me why check_function_body
does not print "body" and "against" debug traces, so there's not hint in gcc.log

I guess running the testsuite with -verbose or -v would help?

Can you have a look?

Thanks,

Christophe

> >
> >
> >
> >>
> >> Thanks,
> >> Richard
> >>
> >>
> >> diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
> >> index 5df80325dff..2434550f0c3 100644
> >> --- a/gcc/testsuite/lib/scanasm.exp
> >> +++ b/gcc/testsuite/lib/scanasm.exp
> >> @@ -785,23 +785,34 @@ proc configure_check-function-bodies { config } {
> >>
> >>     # Regexp for the start of a function definition (name in \1).
> >>     if { [istarget nvptx*-*-*] } {
> >> -    set up_config(start) {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
> >> +    set up_config(start) {
> >> +        {^// BEGIN(?: GLOBAL|) FUNCTION DEF: ([a-zA-Z_]\S+)$}
> >> +    }
> >> +    } elseif { [istarget *-*-darwin*] } {
> >> +    set up_config(start) {
> >> +        {^_([a-zA-Z_]\S+):$}
> >> +        {^LFB[0-9]+:}
> >> +    }
> >>     } else {
> >> -    set up_config(start) {^([a-zA-Z_]\S+):$}
> >> +    set up_config(start) {{^([a-zA-Z_]\S+):$}}
> >>     }
> >>
> >>     # Regexp for the end of a function definition.
> >>     if { [istarget nvptx*-*-*] } {
> >>      set up_config(end) {^\}$}
> >> +    } elseif { [istarget *-*-darwin*] } {
> >> +    set up_config(end) {^LFE[0-9]+}
> >>     } else {
> >>      set up_config(end) {^\s*\.size}
> >>     }
> >> -
> >> +
> >>     # Regexp for lines that aren't interesting.
> >>     if { [istarget nvptx*-*-*] } {
> >>      # Skip lines beginning with '//' comments ('-fverbose-asm', for
> >>      # example).
> >>      set up_config(fluff) {^\s*(?://)}
> >> +    } elseif { [istarget *-*-darwin*] } {
> >> +    set up_config(fluff) {^\s*(?:\.|//|@)|^L[0-9ACESV]}
> >>     } else {
> >>      # Skip lines beginning with labels ('.L[...]:') or other directives
> >>      # ('.align', '.cfi_startproc', '.quad [...]', '.text', etc.), '//' or
> >> @@ -833,9 +844,19 @@ proc parse_function_bodies { config filename result } {
> >>     set fd [open $filename r]
> >>     set in_function 0
> >>     while { [gets $fd line] >= 0 } {
> >> -    if { [regexp $up_config(start) $line dummy function_name] } {
> >> -        set in_function 1
> >> -        set function_body ""
> >> +    if { $in_function == 0 } {
> >> +        if { [regexp [lindex $up_config(start) 0] \
> >> +                     $line dummy function_name] } {
> >> +            set in_function 1
> >> +            set function_body ""
> >> +        }
> >> +    } elseif { $in_function < [llength $up_config(start)] } {
> >> +        if { [regexp [lindex $up_config(start) $in_function] $line] } {
> >> +            incr in_function
> >> +        } else {
> >> +            verbose "parse_function_bodies: skipped $function_name"
> >> +            set in_function 0
> >> +        }
> >>      } elseif { $in_function } {
> >>          if { [regexp $up_config(end) $line] } {
> >>              verbose "parse_function_bodies: $function_name:\n$function_body"
> >> --
> >> 2.25.1
Iain Sandoe Nov. 23, 2023, 9:09 a.m. UTC | #9 
Hi Christophe.

> On 23 Nov 2023, at 09:02, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> 
> Hi Iain,
> 
> On Mon, 6 Nov 2023 at 11:58, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>> 
>> Iain Sandoe <iain@sandoe.co.uk> writes:
>>> Hi Richard,
>>> 
>>>> On 5 Nov 2023, at 12:11, Richard Sandiford <richard.sandiford@arm.com> wrote:
>>>> 
>>>> Iain Sandoe <iain@sandoe.co.uk> writes:
>>> 
>>>>>> On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>>> 
>>>>>>> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com>
>>>>> wrote:
>>>>>>> 
>>>>>>> Iain Sandoe <iains.gcc@gmail.com> writes:
>>>>>>>> This was written before Thomas' modification to the ELF-handling to allow
>>>>>>>> a config-based change for target details.  I did consider updating this
>>>>>>>> to try and use that scheme, but I think that it would sit a little
>>>>>>>> awkwardly, since there are some differences in the start-up scanning for
>>>>>>>> Mach-O.  I would say that in all probability we could improve things but
>>>>>>>> I'd like to put this forward as a well-tested initial implementation.
>>>>>>> 
>>>>>>> Sorry, I would prefer to extend the existing function instead.
>>>>>>> E.g. there's already some divergence between the Mach-O version
>>>>>>> and the default version, in that the Mach-O version doesn't print
>>>>>>> verbose messages.  I also don't think that the current default code
>>>>>>> is so watertight that it'll never need to be updated in future.
>>>>>> 
>>>>>> Fair enough, will explore what can be done (as I recall last I looked the
>>>>>> primary difference was in the initial start-up scan).
>>>>> 
>>>>> I’ve done this as attached.
>>>>> 
>>>>> For the record, when doing it, it gave rise to the same misgivings that led
>>>>> to the separate implementation before.
>>>>> 
>>>>> * as we add formats and uncover asm oddities, they all need to be handled
>>>>>  in one set of code, IMO it could be come quite convoluted.
>>>>> 
>>>>> * now making a change to the MACH-O code, means I have to check I did not
>>>>>  inadvertently break ELF (and likewise, in theory, an ELF change should check
>>>>>  MACH-O, but many folks do/can not do that).
>>>>> 
>>>>> Maybe there’s some half-way-house where code can usefully be shared without
>>>>> those down-sides.
>>>>> 
>>>>> Anyway, to make progress, is the revised version OK for trunk? (tested on
>>>>> aarch64-linux and aarch64-darwin).
>>>> 
>>>> Sorry for the slow reply.  I was hoping we'd be able to share a bit more
>>>> code than that, and avoid an isMACHO toggle.  Does something like the
>>>> attached adaption of your patch work?  Only spot-checked on
>>>> aarch64-linux-gnu so far.
>>>> 
>>>> (The patch tries to avoid capturing the user label prefix, hopefully
>>>> avoiding the needsULP thing.)
>>> 
>>> Yes, this works for me too for Arm64 Darwin (and probably is fine for other
>>> Darwin archs in case we implement body tests there).  If we decide to emit
>>> some comment-based markers to delineat functions without unwind data,
>>> we can just amend the start and end.
>>> 
>>> thanks,
>>> Iain
>>> (doing some wider testing, but for now the only mach-o cases are in the
>>> arm64 code, so the fact that those passed so far is pretty good indication).
>> 
>> OK, great.  It passed testing for me too, so please go ahead and commit
>> if it does for you.
>> 
>>> -----
>>> 
>>> As an aside what’s the intention for cases like this?
>>> 
>>>      .data
>>> foo:
>>>      .xxxx ….
>>>      .size foo, .-foo
>> 
>> ATM there's no way for the test to say that specific pseudo-ops are
>> interesting to it.  Same for labels.  It might be useful to add
>> support for that though.
>> 
>> Thanks,
>> Richard
>> 
> 
> As you have probably already noticed with the notification from our
> CI, this patch causes
> FAIL: gcc.target/arm/pr95646.c check-function-bodies __acle_se_bar
> At quick glance it's not obvious to me why check_function_body
> does not print "body" and "against" debug traces, so there's not hint in gcc.log

Yeah, I’ve reproduced this (it did not show on either Richard’s nor my aarch64 testing)
... and have a potential fix.

the problem is this:

	.global bar
 …
	. global __acle_se_bar

foo:
__acle_se_bar:
  …

=====

The change in code prevernt the second label overriding the first (but the scan checks for the second).

Actually, that’s not legal Mach-O (two global labels cannot have the same address).

I have a fix that re-allows the override (thinking if I should assume Mach-O will never do this or skip the change for mach-o)

——
  

Iain
Christophe Lyon Nov. 23, 2023, 9:22 a.m. UTC | #10 
On Thu, 23 Nov 2023 at 10:09, Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi Christophe.
>
> > On 23 Nov 2023, at 09:02, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> >
> > Hi Iain,
> >
> > On Mon, 6 Nov 2023 at 11:58, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Iain Sandoe <iain@sandoe.co.uk> writes:
> >>> Hi Richard,
> >>>
> >>>> On 5 Nov 2023, at 12:11, Richard Sandiford <richard.sandiford@arm.com> wrote:
> >>>>
> >>>> Iain Sandoe <iain@sandoe.co.uk> writes:
> >>>
> >>>>>> On 26 Oct 2023, at 21:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>>>>
> >>>>>>> On 26 Oct 2023, at 20:49, Richard Sandiford <richard.sandiford@arm.com>
> >>>>> wrote:
> >>>>>>>
> >>>>>>> Iain Sandoe <iains.gcc@gmail.com> writes:
> >>>>>>>> This was written before Thomas' modification to the ELF-handling to allow
> >>>>>>>> a config-based change for target details.  I did consider updating this
> >>>>>>>> to try and use that scheme, but I think that it would sit a little
> >>>>>>>> awkwardly, since there are some differences in the start-up scanning for
> >>>>>>>> Mach-O.  I would say that in all probability we could improve things but
> >>>>>>>> I'd like to put this forward as a well-tested initial implementation.
> >>>>>>>
> >>>>>>> Sorry, I would prefer to extend the existing function instead.
> >>>>>>> E.g. there's already some divergence between the Mach-O version
> >>>>>>> and the default version, in that the Mach-O version doesn't print
> >>>>>>> verbose messages.  I also don't think that the current default code
> >>>>>>> is so watertight that it'll never need to be updated in future.
> >>>>>>
> >>>>>> Fair enough, will explore what can be done (as I recall last I looked the
> >>>>>> primary difference was in the initial start-up scan).
> >>>>>
> >>>>> I’ve done this as attached.
> >>>>>
> >>>>> For the record, when doing it, it gave rise to the same misgivings that led
> >>>>> to the separate implementation before.
> >>>>>
> >>>>> * as we add formats and uncover asm oddities, they all need to be handled
> >>>>>  in one set of code, IMO it could be come quite convoluted.
> >>>>>
> >>>>> * now making a change to the MACH-O code, means I have to check I did not
> >>>>>  inadvertently break ELF (and likewise, in theory, an ELF change should check
> >>>>>  MACH-O, but many folks do/can not do that).
> >>>>>
> >>>>> Maybe there’s some half-way-house where code can usefully be shared without
> >>>>> those down-sides.
> >>>>>
> >>>>> Anyway, to make progress, is the revised version OK for trunk? (tested on
> >>>>> aarch64-linux and aarch64-darwin).
> >>>>
> >>>> Sorry for the slow reply.  I was hoping we'd be able to share a bit more
> >>>> code than that, and avoid an isMACHO toggle.  Does something like the
> >>>> attached adaption of your patch work?  Only spot-checked on
> >>>> aarch64-linux-gnu so far.
> >>>>
> >>>> (The patch tries to avoid capturing the user label prefix, hopefully
> >>>> avoiding the needsULP thing.)
> >>>
> >>> Yes, this works for me too for Arm64 Darwin (and probably is fine for other
> >>> Darwin archs in case we implement body tests there).  If we decide to emit
> >>> some comment-based markers to delineat functions without unwind data,
> >>> we can just amend the start and end.
> >>>
> >>> thanks,
> >>> Iain
> >>> (doing some wider testing, but for now the only mach-o cases are in the
> >>> arm64 code, so the fact that those passed so far is pretty good indication).
> >>
> >> OK, great.  It passed testing for me too, so please go ahead and commit
> >> if it does for you.
> >>
> >>> -----
> >>>
> >>> As an aside what’s the intention for cases like this?
> >>>
> >>>      .data
> >>> foo:
> >>>      .xxxx ….
> >>>      .size foo, .-foo
> >>
> >> ATM there's no way for the test to say that specific pseudo-ops are
> >> interesting to it.  Same for labels.  It might be useful to add
> >> support for that though.
> >>
> >> Thanks,
> >> Richard
> >>
> >
> > As you have probably already noticed with the notification from our
> > CI, this patch causes
> > FAIL: gcc.target/arm/pr95646.c check-function-bodies __acle_se_bar
> > At quick glance it's not obvious to me why check_function_body
> > does not print "body" and "against" debug traces, so there's not hint in gcc.log
>
> Yeah, I’ve reproduced this (it did not show on either Richard’s nor my aarch64 testing)
> ... and have a potential fix.
>

It makes sense, aarch64 and arm are different targets.

> the problem is this:
>
>         .global bar
>  …
>         . global __acle_se_bar
>
> foo:
> __acle_se_bar:
>   …
>
> =====
>
> The change in code prevernt the second label overriding the first (but the scan checks for the second).
>
> Actually, that’s not legal Mach-O (two global labels cannot have the same address).
>
> I have a fix that re-allows the override (thinking if I should assume Mach-O will never do this or skip the change for mach-o)
>
Good news, thanks!

Christophe

> ——
>
>
> Iain
>
diff mbox series

Patch

diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
index 5df80325dff..fab4b0669ec 100644
--- a/gcc/testsuite/lib/scanasm.exp
+++ b/gcc/testsuite/lib/scanasm.exp
@@ -851,6 +851,44 @@  proc parse_function_bodies { config filename result } {
     close $fd
 }
 
+proc parse_MACHO_function_bodies { filename result } {
+    upvar $result up_result
+
+    # Regexp for the start of a function definition (name in \1).
+    set label {^(_[a-zA-Z_]\S+):$}
+    set start {^LFB[0-9]+}
+
+    # Regexp for the end of a function definition.
+    set terminator {^LFE[0-9]+}
+
+    # Regexp for lines that aren't interesting.
+    set fluff {^\s*(?:\.|//|@)}
+    set fluff3 {^L[0-9ACESV]}
+
+    set fd [open $filename r]
+    set in_function 0
+    while { [gets $fd line] >= 0 } {
+	if { !$in_function && [regexp $label $line dummy function_name] } {
+	    set in_function 1
+	    set function_body ""
+	} elseif { $in_function == 1 } {
+	  if { [regexp $start $line] } {
+	    set in_function 2
+	  } else {
+	    set in_function 0
+	  }
+	} elseif { $in_function == 2 } {
+	    if { [regexp $terminator $line] } {
+		set up_result($function_name) $function_body
+		set in_function 0
+	    } elseif { ![regexp $fluff $line]  && ![regexp $fluff3 $line] } {
+		append function_body $line "\n"
+	    }
+	}
+    }
+    close $fd
+}
+
 # FUNCTIONS is an array that maps function names to function bodies.
 # Return true if it contains a definition of function NAME and if
 # that definition matches BODY_REGEXP.
@@ -883,6 +921,14 @@  proc check-function-bodies { args } {
 	error "too many arguments to check-function-bodies"
     }
 
+    set isELF 1
+    # some targets have a __USER_LABEL_PREFIX__
+    set needsULP 0
+    if { [istarget *-*-darwin*] } {
+      set isELF 0
+      set needsULP 1
+    }
+
     if { [llength $args] >= 3 } {
 	set required_flags [lindex $args 2]
 
@@ -944,7 +990,11 @@  proc check-function-bodies { args } {
 	remote_upload host "$filename"
     }
     if { [file exists $output_filename] } {
-	parse_function_bodies config $output_filename functions
+        if { $isELF } {
+	    parse_function_bodies config $output_filename functions
+	} else {
+	    parse_MACHO_function_bodies $output_filename functions
+	}
 	set have_bodies 1
     } else {
 	verbose -log "$testcase: output file does not exist"
@@ -988,6 +1038,9 @@  proc check-function-bodies { args } {
 		if { $xfail_all || [string equal $selector "F"] } {
 		    setup_xfail "*-*-*"
 		}
+		if { $needsULP } {
+		    set function_name "_$function_name"
+		}
 		set testname "$testcase check-function-bodies $function_name"
 		if { !$have_bodies } {
 		    unresolved $testname