Re: [PATCH v2] ld/testsuite: add support for remote testing in ld-cdtest

On Wed, Jul 1, 2026 at 1:06 AM Maciej W. Rozycki <[email protected]> wrote:
>
> On Tue, 30 Jun 2026, Clément Chigot wrote:
>
> > > > This converts the existing to code to use "remote_load" allowing
> > > > execution on both native and remote targets.
> > > >
> > > > The "diff" between the output and the expected result has been
> > > > transformed to the usual regexp_diff. The previous could have been
> > > > transformed into `remote_exec build diff` but tends to be fickled with
> > > > new lines.
> > >
> > > Arf got Maciej's Redhat email rejected. Trying with [email protected].
> >
> > Gentle ping to Maciej.
>
>  Hectic time here since I got back online on Jun 22nd, after a holiday.
>
>  I can see this works correctly here with native `powerpc64le-linux-gnu'
> system as well as remote `mips-linux-gnu' cross target, enabling the tests
> in the latter case.  I have looked through the change and it seems good to
> me except for one nit across two places; see below.
>
> > > > diff --git a/ld/testsuite/ld-cdtest/cdtest.exp b/ld/testsuite/ld-cdtest/cdtest.exp
> > > > index 89fdd581abb..81547af3c9e 100644
> > > > --- a/ld/testsuite/ld-cdtest/cdtest.exp
> > > > +++ b/ld/testsuite/ld-cdtest/cdtest.exp
> > > > @@ -51,44 +47,39 @@ set expected_output "$srcdir/$subdir/cdtest.dat"
> > > >  if ![ld_link $CC_FOR_TARGET tmpdir/cdtest "$CFLAGS_FOR_TARGET tmpdir/cdtest-foo.o tmpdir/cdtest-bar.o tmpdir/cdtest-main.o"] {
> > > >      fail $test1
> > > >  } else {
> > > > -    send_log   "tmpdir/cdtest >tmpdir/cdtest.out\n"
> > > > -    verbose    "tmpdir/cdtest >tmpdir/cdtest.out"
> > > > -    catch "exec tmpdir/cdtest >tmpdir/cdtest.out" exec_output
> > > > +    set failed 0
> > > >
> > > > -    if ![string match "" $exec_output] then {
> > > > +    send_log "Running: tmpdir/cdtest\n"
> > > > +    verbose "Running: tmpdir/cdtest"
> > > > +    set state [remote_load target tmpdir/cdtest]
> > > > +    set status [lindex $state 0]
> > > > +    set exec_output [lindex $state 1]
> > > > +
> > > > +    if { $status != "pass"  } {
> > > >         send_log "$exec_output\n"
> > > >         verbose "$exec_output" 1
> > > > -
> > > > -       fail $test1
> > > > +       set failed 1
> > > >      } else {
> > > > -       send_log   "diff tmpdir/cdtest.out $expected_output\n"
> > > > -       verbose    "diff tmpdir/cdtest.out $expected_output"
> > > > -       catch "exec diff tmpdir/cdtest.out $expected_output" exec_output
> > > > -       set exec_output [prune_warnings $exec_output]
> > > > -
> > > > -       if ![string match "" $exec_output] then {
> > > > -           send_log "$exec_output\n"
> > > > -           verbose  "$exec_output" 1
> > > > +       set output_filename "tmpdir/cdtest.out"
> > > > +       set_file_contents $output_filename $exec_output
> > > >
> > > > +       send_log [file_contents $output_filename]
> > > > +       verbose [file_contents $output_filename] 2
> > > > +       if [regexp_diff $output_filename $expected_output] {
> > > >             send_log "Checking against Named Return Value optimization\n"
> > > >             verbose  "Checking against Named Return Value optimization" 1
> > > >
> > > >             set expected_output "$srcdir/$subdir/cdtest-nrv.dat"
> > > > -
> > > > -           send_log   "diff tmpdir/cdtest.out $expected_output\n"
> > > > -           verbose    "diff tmpdir/cdtest.out $expected_output"
> > > > -           catch "exec diff tmpdir/cdtest.out $expected_output" exec_output
> > > > -           set exec_output [prune_warnings $exec_output]
> > > > +           if [regexp_diff $output_filename $expected_output] {
> > > > +               set failed 1
> > > > +           }
> > > >         }
> > > > +    }
> > > >
> > > > -       if [string match "" $exec_output] then {
> > > > -           pass $test1
> > > > -       } else {
> > > > -           send_log "$exec_output\n"
> > > > -           verbose  "$exec_output" 1
> > > > -
> > > > -           fail $test1
> > > > -       }
> > > > +    if { $failed != 0 } {
>
>  I find checking a boolean value against zero superfluous and tougher to
> parse for a human being.  How about just:
>
>     if { $failed } {
>
> instead?

Applied and indeed better. Thanks for the suggestions.

> > > >      } else {
> > > > -       send_log   "tmpdir/cdtest >tmpdir/cdtest.out\n"
> > > > -       verbose    "tmpdir/cdtest >tmpdir/cdtest.out"
> > > > -       catch "exec tmpdir/cdtest >tmpdir/cdtest.out" exec_output
> > > > +       set failed 0
> > > > +
> > > > +       send_log "Running: tmpdir/cdtest\n"
> > > > +       verbose "Running: tmpdir/cdtest"
> > > > +       set state [remote_load target tmpdir/cdtest]
> > > > +       set status [lindex $state 0]
> > > > +       set exec_output [lindex $state 1]
> > > >
> > > > -       if ![string match "" $exec_output] then {
> > > > +       if { $status != "pass" } {
> > > >             send_log "$exec_output\n"
> > > > -           verbose  "$exec_output" 1
> > > > +           verbose "$exec_output" 1
> > > > +           set failed 1
> > > > +       } else {
> > > > +           set output_filename "tmpdir/cdtest.out"
> > > > +           set_file_contents $output_filename $exec_output
> > > >
> > > > +           send_log [file_contents $output_filename]
> > > > +           verbose [file_contents $output_filename] 2
> > > > +           if [regexp_diff $output_filename $expected_output] {
> > > > +               set failed 1
> > > > +           }
> > > > +       }
> > > > +
> > > > +       if { $failed != 0 } {
>
>  Likewise.
>
>  I can't approve, but I can see Alan has got that covered already and I
> suppose won't object my suggestion.  Thank you for your contribution, a
> worthwhile test robustness improvement.

Push with the "{ $failed }" modification.
Thanks both for the review !

Clément

>   Maciej