Hi Richard,
I had a little bit more of a think about this patch, and have changed my
mind on how problematic it is to use ALWAYS_CFLAGS w.r.t. naming. It is
already used by the C++ tests. If we're happy to use that name then we
don't need to introduce a new save/restore.
Another reason I think this is better is that I noticed that elsewhere
in the testsuite TEST_ALWAYS_FLAGS seems to be specifically for holding
*flags* instead of DejaGNU options, which means that in order to stick
with existing usage we couldn't specify this -B argument is only needed
while linking.
Attaching a patch with this approach -- which is much simpler than the
previous patch.
How does that look to you?
On 1/2/25 15:58, Richard Sandiford wrote:
External email: Use caution opening links or attachments
Matthew Malcomson <mmalcom...@nvidia.com> writes:
On 1/2/25 12:08, Richard Sandiford wrote:
+ # This set in order to give libitm.c++/c++.exp a nicely named flag to set
+ # when adding C++ options.
+ set TEST_ALWAYS_FLAGS ""
This looked odd at first glance. By unconditionally writing "" to the
variable, it seems to subvert the save and restore done in c++.exp.
Yeah -- I see your point, that's not good.
How about instead copying the behaviour of asan_init and asan_finish,
so that libitm_init and libitm_finish do the save and restore? Or perhaps
a slight variation: after saving, libitm_init can set TEST_ALWAYS_FLAGS
to "" if TEST_ALWAYS_FLAGS was previously unset.
c++.exp would then not need to save and restore the flags itself, and
could still assume that TEST_ALWAYS_FLAGS is always set.
Have made the suggested change -- mentioning the extra little bit of
complexity that this introduced ...
Since libitm is a "tool" in the DejaGNU sense (while asan is not),
libitm_finish gets called twice for each libitm_init call.
The `runtest` procedure in DejaGNU's `runtest.exp` calls `${tool}_init`,
executes the c.exp or c++.exp test runner and then calls
`${tool}_finish`, while in each of the test runners we also call
`dg-finish` (as required by the dg.exp API) which calls `${tool}_finish`
directly.
This means using `libitm_finish` needs an extra bit in global state to
check whether we have already reset things.
- Has been set in libitm_init and was unset at start
=> saved_TEST_ALWAYS_FLAGS is unset.
- Has been set in libitm_init and was set at start
=> saved_TEST_ALWAYS_FLAGS is set.
- Has already been reset => some other flag.
Ick. Hadn't realised that dg-finish called *_finish while dg-init didn't
call *_init.
I suppose a way of handling this without the second variable would be to
make libitm_finish check whether TEST_ALWAYS_FLAGS is set. If it isn't,
then it must be the second call to libitm_finish.
OK that way if you agree, or OK as-is if you think it's better.
Richard
Have attached the adjusted patch to this email.
From fbce3b25e8ccad80697f1596f566b268fff71221 Mon Sep 17 00:00:00 2001
From: Matthew Malcomson <mmalcom...@nvidia.com>
Date: Wed, 11 Dec 2024 11:03:55 +0000
Subject: [PATCH] testsuite: libitm: Adjust how libitm.c++ passes link flags
For the `gcc` and `g++` tools we often pass -B/path/to/object/dir in via
`TEST_ALWAYS_FLAGS` (see e.g. asan.exp where this is set).
In libitm.c++/c++.exp we pass that -B flag via the `tool_flags` argument
to `dg-runtest`.
Passing as the `tool_flags` argument means that these flags get added to
the name of the test. This means that if one were to compare the
testsuite results between runs made in different build directories
libitm.c++ gives a reasonable amount of NA->PASS and PASS->NA
differences even though the same tests passed each time.
This patch follows the example set in other parts of the testsuite like
asan_init and passes the -B arguments to the compiler via a global
variable called `TEST_ALWAYS_FLAGS`. For this DejaGNU "tool" we had to
newly initialise that variable in libitm_init and add a check against
that variable in libitm_target_compile. I thought about adding the
relevant flags we need for C++ into `ALWAYS_CFLAGS` but decided against
it since the name didn't match what we would be using it for.
We save the global `TEST_ALWAYS_FLAGS` in `libitm_init` and ensure
it's initialised. We then reset this in `libitm_finish`. Since
`libitm_finish` gets called twice (once from `dg-finish` and once from
the `runtest` procedure) we have to manage state to tell whether
TEST_ALWAYS_FLAGS has already been reset.
Testing done to bootstrap & regtest on AArch64. Manually observed that
the testsuite diff between two different build directories no longer
exists.
N.b. since I pass the new flags in the `ldflags` option of the DejaGNU
options while the previous code always passed this -B flag, the compile
test throwdown.C no longer gets compiled with this -B flag. I believe
that is not a problem.
libitm/ChangeLog:
* testsuite/libitm.c++/c++.exp: Use TEST_ALWAYS_FLAGS instead of
passing arguments to dg-runtest.
* testsuite/lib/libitm.exp (libitm_init): Initialise
TEST_ALWAYS_FLAGS.
(libitm_finish): Reset TEST_ALWAYS_FLAGS.
(libitm_target_compile): Take flags from TEST_ALWAYS_FLAGS.
Signed-off-by: Matthew Malcomson <mmalcom...@nvidia.com>
---
libitm/testsuite/lib/libitm.exp | 42 +++++++++++++++++++++++++++++
libitm/testsuite/libitm.c++/c++.exp | 5 ++--
2 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/libitm/testsuite/lib/libitm.exp b/libitm/testsuite/lib/libitm.exp
index ac390d6d0dd..c5b9bb1127f 100644
--- a/libitm/testsuite/lib/libitm.exp
+++ b/libitm/testsuite/lib/libitm.exp
@@ -59,6 +59,7 @@ set dg-do-what-default run
#
set libitm_compile_options ""
+set libitm_initialised 0
#
# libitm_init
@@ -77,12 +78,15 @@ proc libitm_init { args } {
global blddir
global gluefile wrap_flags
global ALWAYS_CFLAGS
+ global TEST_ALWAYS_FLAGS
+ global saved_TEST_ALWAYS_FLAGS
global CFLAGS
global TOOL_EXECUTABLE TOOL_OPTIONS
global GCC_UNDER_TEST
global TESTING_IN_BUILD_TREE
global target_triplet
global always_ld_library_path
+ global libitm_initialised
set blddir [lookfor_file [get_multilibs] libitm]
@@ -145,6 +149,13 @@ proc libitm_init { args } {
}
}
+ # This set in order to give libitm.c++/c++.exp a nicely named flag to set
+ # when adding C++ options.
+ if [info exists TEST_ALWAYS_FLAGS] {
+ set saved_TEST_ALWAYS_FLAGS $TEST_ALWAYS_FLAGS
+ } else {
+ set TEST_ALWAYS_FLAGS ""
+ }
set ALWAYS_CFLAGS ""
if { $blddir != "" } {
lappend ALWAYS_CFLAGS "additional_flags=-B${blddir}/"
@@ -180,6 +191,33 @@ proc libitm_init { args } {
lappend ALWAYS_CFLAGS "additional_flags=-fgnu-tm"
lappend ALWAYS_CFLAGS "additional_flags=-fdiagnostics-color=never"
+
+ set libitm_initialised 1
+}
+
+#
+# libitm_finish
+#
+proc libitm_finish { args } {
+ global TEST_ALWAYS_FLAGS
+ global saved_TEST_ALWAYS_FLAGS
+ global libitm_initialised
+ # Need two bits of information to tell difference between:
+ # - libitm_init has set TEST_ALWAYS_FLAGS and it was originally unset.
+ # - libitm_init has set TEST_ALWAYS_FLAGS and it was originally set.
+ # - libitm_finish has already been run and cleaned up what libitm_init had
+ # set.
+ if !$libitm_initialised {
+ return
+ }
+
+ if [info exists saved_TEST_ALWAYS_FLAGS] {
+ set TEST_ALWAYS_FLAGS $saved_TEST_ALWAYS_FLAGS
+ unset saved_TEST_ALWAYS_FLAGS
+ } else {
+ unset TEST_ALWAYS_FLAGS
+ }
+ set libitm_initialised 0
}
#
@@ -191,6 +229,7 @@ proc libitm_target_compile { source dest type options } {
global libitm_compile_options
global gluefile wrap_flags
global ALWAYS_CFLAGS
+ global TEST_ALWAYS_FLAGS
global GCC_UNDER_TEST
global lang_test_file
global lang_library_path
@@ -217,6 +256,9 @@ proc libitm_target_compile { source dest type options } {
if [info exists ALWAYS_CFLAGS] {
set options [concat "$ALWAYS_CFLAGS" $options]
}
+ if [info exists TEST_ALWAYS_FLAGS] {
+ set options [concat "$TEST_ALWAYS_FLAGS" $options]
+ }
set options [dg-additional-files-options $options $source $dest $type]
diff --git a/libitm/testsuite/libitm.c++/c++.exp
b/libitm/testsuite/libitm.c++/c++.exp
index fdcfc28e1e0..7dbe75903d4 100644
--- a/libitm/testsuite/libitm.c++/c++.exp
+++ b/libitm/testsuite/libitm.c++/c++.exp
@@ -56,10 +56,9 @@ if { $lang_test_file_found } {
# Gather a list of all tests.
set tests [lsort [glob -nocomplain $srcdir/$subdir/*.C]]
- set stdcxxadder ""
if { $blddir != "" } {
set ld_library_path
"$always_ld_library_path:${blddir}/${lang_library_path}"
- set stdcxxadder "-B ${blddir}/${lang_library_path}"
+ set TEST_ALWAYS_FLAGS "$TEST_ALWAYS_FLAGS
ldflags=-B${blddir}/${lang_library_path}"
} else {
set ld_library_path "$always_ld_library_path"
}
@@ -74,7 +73,7 @@ if { $lang_test_file_found } {
}
# Main loop.
- dg-runtest $tests $stdcxxadder $libstdcxx_includes
+ dg-runtest $tests "" $libstdcxx_includes
}
# All done.
From 8ca3f74545abb178b83bece29bf9dee3f95e983e Mon Sep 17 00:00:00 2001
From: Matthew Malcomson <mmalcom...@nvidia.com>
Date: Wed, 11 Dec 2024 11:03:55 +0000
Subject: [PATCH] testsuite: libitm: Adjust how libitm.c++ passes link flags
For the `gcc` and `g++` tools we often pass -B/path/to/object/dir in
via `TEST_ALWAYS_FLAGS`, `ALWAYS_CXXFLAGS`, or `ALWAYS_CFLAGS` (see
e.g. asan.exp for the first two or libatomic.exp for examples of using
the last one). In libitm.c++/c++.exp we pass one use of the -B flag
via the `tool_flags` argument to `dg-runtest`.
Passing as the `tool_flags` argument means that these flags get added to
the name of the test. This means that if one were to compare the
testsuite results between runs made in different build directories
libitm.c++ gives a reasonable amount of NA->PASS and PASS->NA
differences even though the same tests passed each time.
This patch follows the example set in other parts of the testsuite
like libatomic_init (and for other -B flags in this same testsuite
libitm_init) and passes the -B arguments to the compiler via a global
variable called `ALWAYS_CFLAGS`. This chosen because libitm already
uses this variable, and this variable is in the format of DejaGNU
options, which means we can specify that this flag is only needed for
linking. The name is somewhat misleading for this particular
argument, given that the argument is only needed for the c++ part of
the testsuite, but I considered this naming sequence not so
problematic given that this variable is already used for the C++
testsuite.
N.b. since we are now using the DejaGNU `options` syntax we can
specify that the -B flag is only needed for linking. This means that
we no longer pass this flag when compiling throwdown.C since it's only
a compilation test.
Testing done to bootstrap & regtest on AArch64. Manually observed that
the testsuite diff between two different build directories no longer
exists.
libitm/ChangeLog:
* testsuite/libitm.c++/c++.exp: Use ALWAYS_CFLAGS instead of
passing arguments to dg-runtest.
Signed-off-by: Matthew Malcomson <mmalcom...@nvidia.com>
---
libitm/testsuite/libitm.c++/c++.exp | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/libitm/testsuite/libitm.c++/c++.exp b/libitm/testsuite/libitm.c++/c++.exp
index fdcfc28e1e0..278f50cccf1 100644
--- a/libitm/testsuite/libitm.c++/c++.exp
+++ b/libitm/testsuite/libitm.c++/c++.exp
@@ -56,10 +56,9 @@ if { $lang_test_file_found } {
# Gather a list of all tests.
set tests [lsort [glob -nocomplain $srcdir/$subdir/*.C]]
- set stdcxxadder ""
if { $blddir != "" } {
set ld_library_path "$always_ld_library_path:${blddir}/${lang_library_path}"
- set stdcxxadder "-B ${blddir}/${lang_library_path}"
+ lappend ALWAYS_CFLAGS "ldflags=-B${blddir}/${lang_library_path}"
} else {
set ld_library_path "$always_ld_library_path"
}
@@ -74,7 +73,7 @@ if { $lang_test_file_found } {
}
# Main loop.
- dg-runtest $tests $stdcxxadder $libstdcxx_includes
+ dg-runtest $tests "" $libstdcxx_includes
}
# All done.
--
2.43.0
