* [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten
@ 2024-06-11 11:51 Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 1/3] binutils/testsuite: Add a helper for relative path construction Maciej W. Rozycki
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2024-06-11 11:51 UTC (permalink / raw)
To: binutils
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] binutils/testsuite: Add a helper for relative path construction
2024-06-11 11:51 [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Maciej W. Rozycki
@ 2024-06-11 11:51 ` Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 2/3] GAS/testsuite: Add a helper for paths outside the source dir Maciej W. Rozycki
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2024-06-11 11:51 UTC (permalink / raw)
To: binutils
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.
#
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] GAS/testsuite: Add a helper for paths outside the source dir
2024-06-11 11:51 [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 1/3] binutils/testsuite: Add a helper for relative path construction Maciej W. Rozycki
@ 2024-06-11 11:51 ` Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 3/3] GAS/testsuite: Make a copy of none.s before operating on it as output Maciej W. Rozycki
2024-06-14 0:19 ` [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Alan Modra
3 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2024-06-11 11:51 UTC (permalink / raw)
To: binutils
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] GAS/testsuite: Make a copy of none.s before operating on it as output
2024-06-11 11:51 [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 1/3] binutils/testsuite: Add a helper for relative path construction Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 2/3] GAS/testsuite: Add a helper for paths outside the source dir Maciej W. Rozycki
@ 2024-06-11 11:51 ` Maciej W. Rozycki
2024-06-14 0:19 ` [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Alan Modra
3 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2024-06-11 11:51 UTC (permalink / raw)
To: binutils
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten
2024-06-11 11:51 [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Maciej W. Rozycki
` (2 preceding siblings ...)
2024-06-11 11:51 ` [PATCH 3/3] GAS/testsuite: Make a copy of none.s before operating on it as output Maciej W. Rozycki
@ 2024-06-14 0:19 ` Alan Modra
2024-06-17 22:31 ` Maciej W. Rozycki
3 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2024-06-14 0:19 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten
2024-06-14 0:19 ` [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Alan Modra
@ 2024-06-17 22:31 ` Maciej W. Rozycki
0 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2024-06-17 22:31 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-17 22:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-11 11:51 [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 1/3] binutils/testsuite: Add a helper for relative path construction Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 2/3] GAS/testsuite: Add a helper for paths outside the source dir Maciej W. Rozycki
2024-06-11 11:51 ` [PATCH 3/3] GAS/testsuite: Make a copy of none.s before operating on it as output Maciej W. Rozycki
2024-06-14 0:19 ` [PATCH 0/3] GAS/testsuite: Make sure none.s is not overwritten Alan Modra
2024-06-17 22:31 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).