[PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten

[PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten

[Maciej W. Rozycki]

Hi,

 Previously I have reported:

FAIL: Output file must be distinct from input

regressions, where the fallout is that gas/testsuite/gas/all/none.s gets 
overwritten causing:

FAIL: gas/all/none

regressions for any subsequent test runs using the same source tree.  

 Running the testsuite is not supposed to clobber the sources and the 
causing test is dangerous in that it specifies a file in the source tree 
both as an input source file and the output object file for GAS, relying 
on GAS refusing to overwrite its input, the sole purpose of the test, and 
allowing for GAS clobbering it, should it fail the test.  

 This mini patch series addresses the issue, by copying input from the 
source tree to the test temporary directory and operating on it there.  A 
couple of helper TCL procedures have been defined, which is what the 
introductory patches are for.

 No regressions across my usual list of targets, counting total 239 right 
now, and the intermittent regressions removed.

  Maciej

[PATCH 1/3] binutils/testsuite: Add a helper for relative path construction

[Maciej W. Rozycki]

From: Maciej W. Rozycki <macro@redhat.com>

Implement a helper to construct a relative path between two locations in 
the filesystem, for example to make a path from the source to the object 
directory for the case where a tool has been set up to look at a given 
path and there is a need to point it elsewhere, but an absolute path 
will not work.  The helper works on normalized paths internally, so the 
result is correct even in the presence of symlinks as intermediate path 
components.

So given "/path/to/src/gas/testsuite/gas/all" as the FROM argument and 
then "/path/to/obj/gas/testsuite/tmpdir/none.s" as the TO argument the 
helper will return "../../../../../obj/gas/testsuite/tmpdir/none.s" in 
the absence of symlinks.
---
 binutils/testsuite/lib/binutils-common.exp |   32 +++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

binutils-test-binutils-get-relative-path.diff
Index: binutils-gdb/binutils/testsuite/lib/binutils-common.exp
===================================================================
--- binutils-gdb.orig/binutils/testsuite/lib/binutils-common.exp
+++ binutils-gdb/binutils/testsuite/lib/binutils-common.exp
@@ -480,6 +480,38 @@ proc supports_dt_relr {} {
     return 0
 }
 
+# get_relative_path FROM TO
+#
+# Return a relative path to TO starting from FROM, which is usually
+# supposed to be a directory.  The result is optimised in that both
+# paths are normalized and any leading part shared between the two
+# discarded, and then a suitable number of "../" elements prepended
+# to the remaining part of TO to get to the point of divergence from
+# FROM.
+
+proc get_relative_path { from to } {
+    set split_from [file split [file normalize $from]]
+    set split_to [file split [file normalize [file dirname $to]]]
+    set from_len [llength $split_from]
+    set to_len [llength $split_to]
+    set len [expr { $to_len < $from_len } ? $to_len : $from_len]
+    set relative_path {}
+
+    for { set i 0 } { $i < $len } { incr i } {
+	if { ![string equal [lindex $split_from $i] [lindex $split_to $i]] } {
+	    break
+	}
+    }
+    for { set j $i } { $j < $from_len } { incr j } {
+	lappend relative_path ".."
+    }
+    for { set j $i } { $j < $to_len } { incr j } {
+	lappend relative_path [lindex $split_to $j]
+    }
+
+    return [eval file join $relative_path [file tail $to]]
+}
+
 # Compare two files line-by-line.  FILE_1 is the actual output and FILE_2
 # is the expected output.  Ignore blank lines in either file.
 #

[PATCH 2/3] GAS/testsuite: Add a helper for paths outside the source dir

[Maciej W. Rozycki]

From: Maciej W. Rozycki <macro@redhat.com>

Implement a helper to construct a relative path from $srcdir/$subdir, 
where `gas_run' operates, to an arbitrary place in the filesystem, for 
example a file in the test object directory.
---
 gas/testsuite/lib/gas-defs.exp |   13 +++++++++++++
 1 file changed, 13 insertions(+)

binutils-test-gas-srcdir-path.diff
Index: binutils-gdb/gas/testsuite/lib/gas-defs.exp
===================================================================
--- binutils-gdb.orig/gas/testsuite/lib/gas-defs.exp
+++ binutils-gdb/gas/testsuite/lib/gas-defs.exp
@@ -104,6 +104,19 @@ proc gas_host_run { cmd redir } {
     return [list [lindex $status 0] "$to_return"]
 }
 
+# gas_srcdir_path FILENAME
+#
+# Return a path to FILENAME relative to the directory we normally get
+# sources from in the current script.  For use when the actual source
+# is for example in the test output directory.
+
+proc gas_srcdir_path { filename } {
+    global srcdir
+    global subdir
+
+    return [get_relative_path $srcdir/$subdir $filename]
+}
+
 proc gas_run { prog as_opts redir } {
     global AS
     global ASFLAGS

[PATCH 3/3] GAS/testsuite: Make a copy of none.s before operating on it as output

[Maciej W. Rozycki]

From: Maciej W. Rozycki <macro@redhat.com>

The "Output file must be distinct from input" test in gas/all/gas.exp 
operates on none.s as output.  Should the test fail it may happen that 
GAS will delete the output file requested in which case none.s will be 
removed.  Since the test operates directly on the source tree it will be 
clobbered as a result.  It has actually been observed in the field in 
the form of intermittent:

FAIL: gas/all/none

regressions in a parallel run of many configurations.

Prevent this from happening by copying none.s first to the test object 
directory and operating on it instead.  It does not prevent the file 
from being removed should the test fail, but the source tree won't be 
clobbered in that case.

A nice side effect is that syntactically different paths will now be 
used in this test for the input and the output file each, so coverage 
will extend to verifying that a file is checked against itself even if 
referred to via different paths.  Previously "$srcdir/$subdir/none.s"
was used for both paths and now "tmpdir/none.s" is referred to directly 
and via a relative path from "$srcdir/$subdir" respectively.

I note that we have no previous use of the UNRESOLVED test result in the 
GAS testsuite, but it seems the correct one should copying none.s fail, 
as this is an unexpected situation that requires a human intervention 
and the test proper has not been evaluated.
---
 gas/testsuite/gas/all/gas.exp |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

binutils-test-gas-all-none-copy.diff
Index: binutils-gdb/gas/testsuite/gas/all/gas.exp
===================================================================
--- binutils-gdb.orig/gas/testsuite/gas/all/gas.exp
+++ binutils-gdb/gas/testsuite/gas/all/gas.exp
@@ -86,9 +86,18 @@ gas_test_error "equiv1.s" "" ".equiv for
 gas_test_error "equiv2.s" "" ".equiv for symbol already set to an expression"
 
 # The inode comparison used to detect identical input and output files
-# doesn't work on non-Posix hosts.
+# doesn't work on non-Posix hosts.  Make a copy of the input file and
+# operate on it so as not to clobber the source tree should this test
+# fail.
 if { ![ishost "*-*-mingw*"] } then {
-    gas_test_error "none.s" "-o $srcdir/$subdir/none.s" "Output file must be distinct from input"
+    set testname "Output file must be distinct from input"
+    set filename tmpdir/none.s
+    if [catch {file copy -force $srcdir/$subdir/none.s $filename}] {
+	perror "Could not make a copy of the input file"
+	unresolved $testname
+    } else {
+	gas_test_error [gas_srcdir_path $filename] "-o $filename" $testname
+    }
 }
 
 # .equ works differently on some targets.

Re: [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten

[Alan Modra]

On Tue, Jun 11, 2024 at 12:51:18PM +0100, Maciej W. Rozycki wrote:
> Hi,
> 
>  Previously I have reported:
> 
> FAIL: Output file must be distinct from input
> 
> regressions, where the fallout is that gas/testsuite/gas/all/none.s gets 
> overwritten causing:
> 
> FAIL: gas/all/none
> 
> regressions for any subsequent test runs using the same source tree.  
> 
>  Running the testsuite is not supposed to clobber the sources and the 
> causing test is dangerous in that it specifies a file in the source tree 
> both as an input source file and the output object file for GAS, relying 
> on GAS refusing to overwrite its input, the sole purpose of the test, and 
> allowing for GAS clobbering it, should it fail the test.  
> 
>  This mini patch series addresses the issue, by copying input from the 
> source tree to the test temporary directory and operating on it there.  A 
> couple of helper TCL procedures have been defined, which is what the 
> introductory patches are for.
> 
>  No regressions across my usual list of targets, counting total 239 right 
> now, and the intermittent regressions removed.
> 
>   Maciej

Thanks, the series looks good.  Please apply.

-- 
Alan Modra

Re: [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten

[Maciej W. Rozycki]

On Fri, 14 Jun 2024, Alan Modra wrote:

> >  This mini patch series addresses the issue, by copying input from the 
> > source tree to the test temporary directory and operating on it there.  A 
> > couple of helper TCL procedures have been defined, which is what the 
> > introductory patches are for.
[...]
> 
> Thanks, the series looks good.  Please apply.

 Now applied, thank you for your review.

  Maciej