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.
