Hi Iain,
On Mon, 6 Nov 2023 at 11:58, Richard Sandiford
<[email protected]> wrote:
>
> Iain Sandoe <[email protected]> writes:
> > Hi Richard,
> >
> >> On 5 Nov 2023, at 12:11, Richard Sandiford <[email protected]>
> >> wrote:
> >>
> >> Iain Sandoe <[email protected]> writes:
> >
> >>>> On 26 Oct 2023, at 21:00, Iain Sandoe <[email protected]> wrote:
> >>>
> >>>>> On 26 Oct 2023, at 20:49, Richard Sandiford <[email protected]>
> >>> wrote:
> >>>>>
> >>>>> Iain Sandoe <[email protected]> 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
