[PATCH v2 0/2] Fix regressions introduced by Fortran compiler identification series

[PATCH v2 0/2] Fix regressions introduced by Fortran compiler identification series

[Nils-Christian Kempke]

This is v2 of the series found here:

  https://sourceware.org/pipermail/gdb-patches/2022-June/189778.html

Changes since v1:
	* Patch 2/2: There was a mistake in the naming of the c++
	compiler_info variable.  Inside of compiler.cc it was named
	cc_compiler_info while in gdb.exp cpp_compiler_info was used in
	some places.  It is now called cc_compiler_info everywhere.
	Additionally, I added a section to get_compiler_info, which, if
	the compiler identification failed for a specific language (so if
	get_compiler_info would return 'unknown' for this call) caches
	the result in the respective compiler_info variable.  The first
	version of the patch had missed to do this.  Earlier setting the
	variable to 'unknown' because of some unknown compiler
	diagnostics was not properly cached.  Similarly, if the
	preprocessing of the compiler.** files did not set the
	respective **_compiler_info variable 'unknown' was returned but 
	never cached.

Patch 1/2 did not change.


Best,

Nils

Nils-Christian Kempke (2):
  gdb/testsuite: use test_compiler_info in gcc_major_version
  gdb/testsuite: cache compiler_info on a per language basis

 gdb/testsuite/lib/compiler.F90 | 14 +++---
 gdb/testsuite/lib/compiler.c   | 18 ++++----
 gdb/testsuite/lib/compiler.cc  | 18 ++++----
 gdb/testsuite/lib/gdb.exp      | 79 ++++++++++++++++++++++++++++------
 4 files changed, 91 insertions(+), 38 deletions(-)

-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version

[Nils-Christian Kempke]

The procedure gcc_major_version was earlier using the global variable
compiler_info to retrieve gcc's major version.  This is discouraged and
(as can be read in a comment in compiler.c) compiler_info should be
local to get_compiler_info and test_compiler_info.

The preferred way of getting the compiler string is via calling
test_compiler_info without arguments.  Gcc_major_version was changed to
do that.
---
 gdb/testsuite/lib/gdb.exp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 402450152a..4ee7c1fb0a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4218,12 +4218,11 @@ proc test_compiler_info { {compiler ""} {language ""} } {
 # For gcc 7.5.0, the major version 7.
 
 proc gcc_major_version { } {
-    global compiler_info
     global decimal
     if { ![test_compiler_info "gcc-*"] } {
 	return -1
     }
-    set res [regexp gcc-($decimal)-($decimal)- $compiler_info \
+    set res [regexp gcc-($decimal)-($decimal)- [test_compiler_info] \
 		 dummy_var major minor]
     if { $res != 1 } {
 	return -1
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version

[Andrew Burgess]

Nils-Christian Kempke <nils-christian.kempke@intel.com> writes:

> The procedure gcc_major_version was earlier using the global variable
> compiler_info to retrieve gcc's major version.  This is discouraged and
> (as can be read in a comment in compiler.c) compiler_info should be
> local to get_compiler_info and test_compiler_info.
>
> The preferred way of getting the compiler string is via calling
> test_compiler_info without arguments.  Gcc_major_version was changed to

Lower cast 'g' in "Gcc_major_version was changed ..."

OK with this change.

Thanks,
Andrew

> do that.
> ---
>  gdb/testsuite/lib/gdb.exp | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 402450152a..4ee7c1fb0a 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4218,12 +4218,11 @@ proc test_compiler_info { {compiler ""} {language ""} } {
>  # For gcc 7.5.0, the major version 7.
>  
>  proc gcc_major_version { } {
> -    global compiler_info
>      global decimal
>      if { ![test_compiler_info "gcc-*"] } {
>  	return -1
>      }
> -    set res [regexp gcc-($decimal)-($decimal)- $compiler_info \
> +    set res [regexp gcc-($decimal)-($decimal)- [test_compiler_info] \
>  		 dummy_var major minor]
>      if { $res != 1 } {
>  	return -1
> -- 
> 2.25.1
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis

[Nils-Christian Kempke]

An earlier patch series caused some regressions when running the
whole testsuite as noted here:

  https://sourceware.org/pipermail/gdb-patches/2022-May/189673.html

The reason for these regressions was the way get_compiler_info currenlty
caches the compiler identification string.  Every time get_compiler_info
is called, it checks wether the global variable compiler_info has been
set and, if not, the function actually attempts to identify the
requested compiler (C/Cpp/Fortran) and sets compiler_info. For every
subsequent call to get_compiler_info, it will find the compiler_info
variable as set, and it will, instead of identifying the compiler anew,
return the cached value in compiler_info.

This caching mechanism lead to troubles when running the whole testsuite
and entering the gdb.fortran part of the testsuite run.  As the first
call to get_compiler_info was done without any arguments (using the C
compiler identification) the compiler_info variable had been set
to whatever c compiler is being used, e.g. 'gcc-...'.  The fortran
testsuite now tried to match the string it got from test_compiler_info
against the Fortran compiler names (flang, ifx, gfortran ...), none of
which could be matched against the stored C compiler name.  Thus,
Fortran specific procedures like fortran_main, fortran_runto_main, and
the bunch of Fortran compiler dependent intrinsic typename procedures
(like fortran_int4 ...) failed to match against any known Fortran
compiler.  This lead to many test failing completely, some only
partially.  In addition, compiler dependent KFAILs and output checks in
the Fortran testsuite could not longer be matched.

This patch makes the compiler_info caching language specific.  Each
language that can be handled by get_/test_compiler_info gets its own
global variable c_/cpp_/f_compiler_info.  The respective variables are
checked whenever the get_/test_compiler_info are called for a specific
language (no language defaulting to C) and, if the language compiler has
been identified already, the cached value is returned.  Else, the
compiler identification for the requested language is triggered.

The three compiler identification files have been changed to no longer
set compiler_info, but instead set the language dependent compiler
information variable.

This eliminates the regressions introduced with the aforementioned
series.
---
 gdb/testsuite/lib/compiler.F90 | 14 +++----
 gdb/testsuite/lib/compiler.c   | 18 ++++----
 gdb/testsuite/lib/compiler.cc  | 18 ++++----
 gdb/testsuite/lib/gdb.exp      | 76 +++++++++++++++++++++++++++++-----
 4 files changed, 90 insertions(+), 36 deletions(-)

diff --git a/gdb/testsuite/lib/compiler.F90 b/gdb/testsuite/lib/compiler.F90
index 71cf3d2c2b..314dbd5c02 100644
--- a/gdb/testsuite/lib/compiler.F90
+++ b/gdb/testsuite/lib/compiler.F90
@@ -13,15 +13,15 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-set compiler_info "unknown"
+set f_compiler_info "unknown"
 
 #if defined (__GFORTRAN__)
-set compiler_info [join {gfortran __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
+set f_compiler_info [join {gfortran __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
 #endif
 
 /* ARM seems to not define a patch version.  */
 #if defined (__ARM_LINUX_COMPILER__)
-set compiler_info [join {armflang __armclang_major__ __armclang_minor__ 0} -]
+set f_compiler_info [join {armflang __armclang_major__ __armclang_minor__ 0} -]
 #endif
 
 /* Classic flang and LLVM flang emit their respective macros differently.  */
@@ -31,12 +31,12 @@ set compiler_info [join {armflang __armclang_major__ __armclang_minor__ 0} -]
 set major __flang_major__
 set minor __flang_minor__
 set patch __flang_patchlevel__
-set compiler_info [join "flang-llvm $major $minor $patch" -]
+set f_compiler_info [join "flang-llvm $major $minor $patch" -]
 #endif
 
 /* Classic Flang.  */
 #if defined (__FLANG)
-set compiler_info [join {flang-classic __FLANG_MAJOR__ __FLANG_MINOR__ __FLANG_PATCHLEVEL__} -]
+set f_compiler_info [join {flang-classic __FLANG_MAJOR__ __FLANG_MINOR__ __FLANG_PATCHLEVEL__} -]
 #endif
 
 /* Intel LLVM emits a string like 20220100 with version 2021.2.0 and higher.  */
@@ -44,7 +44,7 @@ set compiler_info [join {flang-classic __FLANG_MAJOR__ __FLANG_MINOR__ __FLANG_P
 set major [string range __INTEL_LLVM_COMPILER 0 3]
 set minor [string range __INTEL_LLVM_COMPILER 4 5]
 set patch [string range __INTEL_LLVM_COMPILER 6 7]
-set compiler_info [join "ifx $major $minor $patch" -]
+set f_compiler_info [join "ifx $major $minor $patch" -]
 #elif defined (__INTEL_COMPILER)
 /* Starting with 2021 the ifort versioning scheme changed.  Before, Intel ifort
    would define its version as e.g. 19.0.0 or rather __INTEL_COMPILER would be
@@ -65,5 +65,5 @@ set major __INTEL_COMPILER
 set minor __INTEL_COMPILER_UPDATE
 set patch 0
 #endif
-set compiler_info [join "ifort $major $minor $patch" -]
+set f_compiler_info [join "ifort $major $minor $patch" -]
 #endif
diff --git a/gdb/testsuite/lib/compiler.c b/gdb/testsuite/lib/compiler.c
index 86140f8c0e..817da77914 100644
--- a/gdb/testsuite/lib/compiler.c
+++ b/gdb/testsuite/lib/compiler.c
@@ -32,41 +32,41 @@
 
    */
 
-set compiler_info "unknown"
+set c_compiler_info "unknown"
 
 #if defined (__GNUC__)
 #if defined (__GNUC_PATCHLEVEL__)
 /* Only GCC versions >= 3.0 define the __GNUC_PATCHLEVEL__ macro.  */
-set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
+set c_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
 #else
-set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
+set c_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
 #endif
 #endif
 
 #if defined (__xlc__)
 /* IBM'x xlc compiler. NOTE:  __xlc__ expands to a double quoted string of four
    numbers separated by '.'s: currently "7.0.0.0" */
-set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - compiler_info]
+set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - c_compiler_info]
 #endif
 
 #if defined (__ARMCC_VERSION)
-set compiler_info [join {armcc __ARMCC_VERSION} -]
+set c_compiler_info [join {armcc __ARMCC_VERSION} -]
 #endif
 
 #if defined (__clang__)
-set compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
+set c_compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
 #endif
 
 #if defined (__ICC)
 set icc_major [string range __ICC 0 1]
 set icc_minor [format "%d" [string range __ICC 2 [expr {[string length __ICC] -1}]]]
 set icc_update __INTEL_COMPILER_UPDATE
-set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
+set c_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
 #elif defined (__ICL)
 set icc_major [string range __ICL 0 1]
 set icc_minor [format "%d" [string range __ICL 2 [expr {[string length __ICL] -1}]]]
 set icc_update __INTEL_COMPILER_UPDATE
-set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
+set c_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
 #elif defined(__INTEL_LLVM_COMPILER) && defined(__clang_version__)
 /* Intel Next Gen compiler defines preprocessor __INTEL_LLVM_COMPILER and
    provides version info in __clang_version__ e.g. value:
@@ -75,5 +75,5 @@ set total_length [string length __clang_version__]
 set version_start_index [string last "(" __clang_version__]
 set version_string [string range __clang_version__ $version_start_index+5 $total_length-2]
 set version_updated_string [string map {. -} $version_string]
-set compiler_info "icx-$version_updated_string"
+set c_compiler_info "icx-$version_updated_string"
 #endif
diff --git a/gdb/testsuite/lib/compiler.cc b/gdb/testsuite/lib/compiler.cc
index a52e81c2e3..3036e887ed 100755
--- a/gdb/testsuite/lib/compiler.cc
+++ b/gdb/testsuite/lib/compiler.cc
@@ -20,41 +20,41 @@
 /* This file is exactly like compiler.c.  I could just use compiler.c if
    I could be sure that every C++ compiler accepted extensions of ".c".  */
 
-set compiler_info "unknown"
+set cc_compiler_info "unknown"
 
 #if defined (__GNUC__)
 #if defined (__GNUC_PATCHLEVEL__)
 /* Only GCC versions >= 3.0 define the __GNUC_PATCHLEVEL__ macro.  */
-set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
+set cc_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
 #else
-set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
+set cc_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
 #endif
 #endif
 
 #if defined (__xlc__)
 /* IBM'x xlc compiler. NOTE:  __xlc__ expands to a double quoted string of four
    numbers separated by '.'s: currently "7.0.0.0" */
-set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - compiler_info]
+set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - cc_compiler_info]
 #endif
 
 #if defined (__ARMCC_VERSION)
-set compiler_info [join {armcc __ARMCC_VERSION} -]
+set cc_compiler_info [join {armcc __ARMCC_VERSION} -]
 #endif
 
 #if defined (__clang__)
-set compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
+set cc_compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
 #endif
 
 #if defined (__ICC)
 set icc_major [string range __ICC 0 1]
 set icc_minor [format "%d" [string range __ICC 2 [expr {[string length __ICC] -1}]]]
 set icc_update __INTEL_COMPILER_UPDATE
-set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
+set cc_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
 #elif defined (__ICL)
 set icc_major [string range __ICL 0 1]
 set icc_minor [format "%d" [string range __ICL 2 [expr {[string length __ICL] -1}]]]
 set icc_update __INTEL_COMPILER_UPDATE
-set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
+set cc_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
 #elif defined(__INTEL_LLVM_COMPILER) && defined(__clang_version__)
 /* Intel Next Gen compiler defines preprocessor __INTEL_LLVM_COMPILER and
    provides version info in __clang_version__ e.g. value:
@@ -63,5 +63,5 @@ set total_length [string length __clang_version__]
 set version_start_index [string last "(" __clang_version__]
 set version_string [string range __clang_version__ $version_start_index+5 $total_length-2]
 set version_updated_string [string map {. -} $version_string]
-set compiler_info "icx-$version_updated_string"
+set cc_compiler_info "icx-$version_updated_string"
 #endif
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 4ee7c1fb0a..72553a5a52 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4113,25 +4113,49 @@ proc get_compiler_info {{arg ""}} {
     global outdir
     global tool
 
-    # These come from compiler.c, compiler.cc or compiler.F90.
+    # These come from compiler.c, compiler.cc or compiler.F90.  They are local
+    # to get_compiler_info and should usually not be used outside.  They are
+    # used for caching the language dependent compiler info.
+    global c_compiler_info
+    global cc_compiler_info
+    global f_compiler_info
+
+    # Local to get_compiler_info and test_compiler_info.  Used for
+    # communicating the compiler info from get_compiler_info to
+    # test_compiler_info (as well as to avoid some ifs over all supported
+    # languages).
     global compiler_info
 
     # Legacy global data symbols.
     global gcc_compiled
 
-    if [info exists compiler_info] {
-	# Already computed.
-	return 0
-    }
-
-    # Choose which file to preprocess.
-    set ifile "${srcdir}/lib/compiler.c"
+    # Check for each language whether we already cached the requested
+    # compiler info.  If not, choose which file to preprocess.
     if { $arg == "c++" } {
+	if { [info exists cc_compiler_info] } {
+	    set compiler_info $cc_compiler_info
+	    return 0
+	}
 	set ifile "${srcdir}/lib/compiler.cc"
     } elseif { $arg == "f90" } {
+	if { [info exists f_compiler_info] } {
+	    set compiler_info $f_compiler_info
+	    return 0
+	}
 	set ifile "${srcdir}/lib/compiler.F90"
+    } else {
+	# Default to C compiler identification.
+	if { [info exists c_compiler_info] } {
+	    set compiler_info $c_compiler_info
+	    return 0
+	}
+	set ifile "${srcdir}/lib/compiler.c"
     }
 
+    # We'll attempt to identify a compiler for a language we have not yet
+    # processed.  For now the compiler_info is unknown.
+    set compiler_info "unknown"
+
     # Run $ifile through the right preprocessor.
     # Toggle gdb.log to keep the compiler output out of the log.
     set saved_log [log_file -info]
@@ -4171,10 +4195,27 @@ proc get_compiler_info {{arg ""}} {
 	}
     }
 
-    # Set to unknown if for some reason compiler_info didn't get defined.
-    if ![info exists compiler_info] {
+    # Compiler_info was set to unknown earlier, before starting the compiler
+    # identification.  If for some reason c/cpp/f_compiler_info didn't get
+    # set we'll leave it at that and cache 'unknown' as the language compiler
+    # info further down.  Else, update compiler_info to c/cpp/f_compiler_info.
+    if { $arg == "c++" } {
+	if { [info exists cc_compiler_info] } {
+	    set compiler_info $cc_compiler_info
+	}
+    } elseif { $arg == "f90" } {
+	if { [info exists f_compiler_info] } {
+	    set compiler_info $f_compiler_info
+	}
+    } else {
+	# Default to C compiler identification.
+	if { [info exists c_compiler_info] } {
+	    set compiler_info $c_compiler_info
+	}
+    }
+
+    if { $compiler_info == "unknown" } {
 	verbose -log "get_compiler_info: compiler_info not provided"
-	set compiler_info "unknown"
     }
     # Also set to unknown compiler if any diagnostics happened.
     if { $unknown } {
@@ -4182,6 +4223,19 @@ proc get_compiler_info {{arg ""}} {
 	set compiler_info "unknown"
     }
 
+    # If compiler_info is set to 'unknown' because of an unexpected diagnostic
+    # or because compiler identification failed to set the language compiler
+    # info variable we cache 'unknown' as the result here.
+    if { $compiler_info == "unknown" } {
+	if { $arg == "c++" } {
+	    set cc_compiler_info $compiler_info
+	} elseif { $arg = "f90" } {
+	    set f_compiler_info $compiler_info
+	} else {
+	    set c_compiler_info $compiler_info
+	}
+    }
+
     # Set the legacy symbols.
     set gcc_compiled 0
     regexp "^gcc-(\[0-9\]+)-" "$compiler_info" matchall gcc_compiled
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis

[Andrew Burgess]

Nils-Christian Kempke <nils-christian.kempke@intel.com> writes:

> An earlier patch series caused some regressions when running the
> whole testsuite as noted here:
>
>   https://sourceware.org/pipermail/gdb-patches/2022-May/189673.html
>
> The reason for these regressions was the way get_compiler_info currenlty
> caches the compiler identification string.  Every time get_compiler_info
> is called, it checks wether the global variable compiler_info has been
> set and, if not, the function actually attempts to identify the
> requested compiler (C/Cpp/Fortran) and sets compiler_info. For every
> subsequent call to get_compiler_info, it will find the compiler_info
> variable as set, and it will, instead of identifying the compiler anew,
> return the cached value in compiler_info.
>
> This caching mechanism lead to troubles when running the whole testsuite
> and entering the gdb.fortran part of the testsuite run.  As the first
> call to get_compiler_info was done without any arguments (using the C
> compiler identification) the compiler_info variable had been set
> to whatever c compiler is being used, e.g. 'gcc-...'.  The fortran
> testsuite now tried to match the string it got from test_compiler_info
> against the Fortran compiler names (flang, ifx, gfortran ...), none of
> which could be matched against the stored C compiler name.  Thus,
> Fortran specific procedures like fortran_main, fortran_runto_main, and
> the bunch of Fortran compiler dependent intrinsic typename procedures
> (like fortran_int4 ...) failed to match against any known Fortran
> compiler.  This lead to many test failing completely, some only
> partially.  In addition, compiler dependent KFAILs and output checks in
> the Fortran testsuite could not longer be matched.

This description of the problem unfortunately, only covers part of the
story.

When I reviewed your original series I considered this very problem and
did some experiments, and convinced myself there was no issue.

But I agree that the failures can be reproduced, so what's going on?

The first thing to understand is that, in general, the compiler_info is
NOT cached between separate test runs.  It probably should, and we'll
address that later.  But this non-caching behaviour is why I thought
your original patch would be fine[1].

The reason that the compiler_info is not normally cached is due to the
lib/gdb.exp procs gdb_setup_known_globals and gdb_cleanup_globals.
These procs aim to ensure we don't leak globals between test scripts.
Any global not defined before we start a test script is automatically
deleted when the test script ends.

As the calls to get_compiler_info are (mostly) done directly from test
scripts, then the compiler_info global does not exist before the test
script starts, and so is deleted once the test script has ended.

However, there's a problem.  The 'load_lib' proc will only load each
library file once, including between test scripts.  This means that any
globals defined by a lib would be lost once the first test script that
includes a lib has finished.

To solve this problem we override load_lib in lib/gdb.exp.  Any new
globals defined by a lib are added to the list of globals to preserve.

And so, here's what happens for me, we initially run a bunch of test
scripts, for each script we compute compiler_info, and then delete this
global at the end of the test script.

Eventually, we reach a test script that includes 'trace-support.exp'.
This library has some code at file scope that sets up the globals fpreg,
spreg, and pcreg.  Setting up these globals causes us to cal
get_compiler_info, which sets the compiler_info global.

Because this global is defined from a library our load_lib proc adds
this global to the list of preserved globals.  And now, we no longer
delete compiler_info once the test script has finished.

For me the test gdb.arch/ftrace-insn-reloc.exp is the first script that
loads 'trace-support.exp'.  After this point we will no longer recompute
compiler_info for each test script.

My mistake when checking your original patch is that I ran just two
tests, a C test and a Fortran test, I checked that compiler_info was
correctly recomputed for each test script.  Obviously, my two random
tests didn't include a library that caused compiler_info to be defined,
so I never saw this caching problem.

I think all this has a couple of implications for your patch, which I'll
discuss inline...

[1] Actually, it was probably never OK as we might have need to ask
about different compilers Fortran/C/C++ within a single test script, in
which case we would have gone wrong anyway .... but we're going to fix
that now, so no worries.

>
> This patch makes the compiler_info caching language specific.  Each
> language that can be handled by get_/test_compiler_info gets its own
> global variable c_/cpp_/f_compiler_info.  The respective variables are
> checked whenever the get_/test_compiler_info are called for a specific
> language (no language defaulting to C) and, if the language compiler has
> been identified already, the cached value is returned.  Else, the
> compiler identification for the requested language is triggered.
>
> The three compiler identification files have been changed to no longer
> set compiler_info, but instead set the language dependent compiler
> information variable.
>
> This eliminates the regressions introduced with the aforementioned
> series.
> ---
>  gdb/testsuite/lib/compiler.F90 | 14 +++----
>  gdb/testsuite/lib/compiler.c   | 18 ++++----
>  gdb/testsuite/lib/compiler.cc  | 18 ++++----
>  gdb/testsuite/lib/gdb.exp      | 76 +++++++++++++++++++++++++++++-----
>  4 files changed, 90 insertions(+), 36 deletions(-)
>
> diff --git a/gdb/testsuite/lib/compiler.F90 b/gdb/testsuite/lib/compiler.F90
> index 71cf3d2c2b..314dbd5c02 100644
> --- a/gdb/testsuite/lib/compiler.F90
> +++ b/gdb/testsuite/lib/compiler.F90
> @@ -13,15 +13,15 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> -set compiler_info "unknown"
> +set f_compiler_info "unknown"
>  
>  #if defined (__GFORTRAN__)
> -set compiler_info [join {gfortran __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
> +set f_compiler_info [join {gfortran __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
>  #endif
>  
>  /* ARM seems to not define a patch version.  */
>  #if defined (__ARM_LINUX_COMPILER__)
> -set compiler_info [join {armflang __armclang_major__ __armclang_minor__ 0} -]
> +set f_compiler_info [join {armflang __armclang_major__ __armclang_minor__ 0} -]
>  #endif
>  
>  /* Classic flang and LLVM flang emit their respective macros differently.  */
> @@ -31,12 +31,12 @@ set compiler_info [join {armflang __armclang_major__ __armclang_minor__ 0} -]
>  set major __flang_major__
>  set minor __flang_minor__
>  set patch __flang_patchlevel__
> -set compiler_info [join "flang-llvm $major $minor $patch" -]
> +set f_compiler_info [join "flang-llvm $major $minor $patch" -]
>  #endif
>  
>  /* Classic Flang.  */
>  #if defined (__FLANG)
> -set compiler_info [join {flang-classic __FLANG_MAJOR__ __FLANG_MINOR__ __FLANG_PATCHLEVEL__} -]
> +set f_compiler_info [join {flang-classic __FLANG_MAJOR__ __FLANG_MINOR__ __FLANG_PATCHLEVEL__} -]
>  #endif
>  
>  /* Intel LLVM emits a string like 20220100 with version 2021.2.0 and higher.  */
> @@ -44,7 +44,7 @@ set compiler_info [join {flang-classic __FLANG_MAJOR__ __FLANG_MINOR__ __FLANG_P
>  set major [string range __INTEL_LLVM_COMPILER 0 3]
>  set minor [string range __INTEL_LLVM_COMPILER 4 5]
>  set patch [string range __INTEL_LLVM_COMPILER 6 7]
> -set compiler_info [join "ifx $major $minor $patch" -]
> +set f_compiler_info [join "ifx $major $minor $patch" -]
>  #elif defined (__INTEL_COMPILER)
>  /* Starting with 2021 the ifort versioning scheme changed.  Before, Intel ifort
>     would define its version as e.g. 19.0.0 or rather __INTEL_COMPILER would be
> @@ -65,5 +65,5 @@ set major __INTEL_COMPILER
>  set minor __INTEL_COMPILER_UPDATE
>  set patch 0
>  #endif
> -set compiler_info [join "ifort $major $minor $patch" -]
> +set f_compiler_info [join "ifort $major $minor $patch" -]
>  #endif
> diff --git a/gdb/testsuite/lib/compiler.c b/gdb/testsuite/lib/compiler.c
> index 86140f8c0e..817da77914 100644
> --- a/gdb/testsuite/lib/compiler.c
> +++ b/gdb/testsuite/lib/compiler.c
> @@ -32,41 +32,41 @@
>  
>     */
>  
> -set compiler_info "unknown"
> +set c_compiler_info "unknown"
>  
>  #if defined (__GNUC__)
>  #if defined (__GNUC_PATCHLEVEL__)
>  /* Only GCC versions >= 3.0 define the __GNUC_PATCHLEVEL__ macro.  */
> -set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
> +set c_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
>  #else
> -set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
> +set c_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
>  #endif
>  #endif
>  
>  #if defined (__xlc__)
>  /* IBM'x xlc compiler. NOTE:  __xlc__ expands to a double quoted string of four
>     numbers separated by '.'s: currently "7.0.0.0" */
> -set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - compiler_info]
> +set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - c_compiler_info]
>  #endif
>  
>  #if defined (__ARMCC_VERSION)
> -set compiler_info [join {armcc __ARMCC_VERSION} -]
> +set c_compiler_info [join {armcc __ARMCC_VERSION} -]
>  #endif
>  
>  #if defined (__clang__)
> -set compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
> +set c_compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
>  #endif
>  
>  #if defined (__ICC)
>  set icc_major [string range __ICC 0 1]
>  set icc_minor [format "%d" [string range __ICC 2 [expr {[string length __ICC] -1}]]]
>  set icc_update __INTEL_COMPILER_UPDATE
> -set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
> +set c_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
>  #elif defined (__ICL)
>  set icc_major [string range __ICL 0 1]
>  set icc_minor [format "%d" [string range __ICL 2 [expr {[string length __ICL] -1}]]]
>  set icc_update __INTEL_COMPILER_UPDATE
> -set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
> +set c_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
>  #elif defined(__INTEL_LLVM_COMPILER) && defined(__clang_version__)
>  /* Intel Next Gen compiler defines preprocessor __INTEL_LLVM_COMPILER and
>     provides version info in __clang_version__ e.g. value:
> @@ -75,5 +75,5 @@ set total_length [string length __clang_version__]
>  set version_start_index [string last "(" __clang_version__]
>  set version_string [string range __clang_version__ $version_start_index+5 $total_length-2]
>  set version_updated_string [string map {. -} $version_string]
> -set compiler_info "icx-$version_updated_string"
> +set c_compiler_info "icx-$version_updated_string"
>  #endif
> diff --git a/gdb/testsuite/lib/compiler.cc b/gdb/testsuite/lib/compiler.cc
> index a52e81c2e3..3036e887ed 100755
> --- a/gdb/testsuite/lib/compiler.cc
> +++ b/gdb/testsuite/lib/compiler.cc
> @@ -20,41 +20,41 @@
>  /* This file is exactly like compiler.c.  I could just use compiler.c if
>     I could be sure that every C++ compiler accepted extensions of ".c".  */
>  
> -set compiler_info "unknown"
> +set cc_compiler_info "unknown"
>  
>  #if defined (__GNUC__)
>  #if defined (__GNUC_PATCHLEVEL__)
>  /* Only GCC versions >= 3.0 define the __GNUC_PATCHLEVEL__ macro.  */
> -set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
> +set cc_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ __GNUC_PATCHLEVEL__} -]
>  #else
> -set compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
> +set cc_compiler_info [join {gcc __GNUC__ __GNUC_MINOR__ "unknown"} -]
>  #endif
>  #endif
>  
>  #if defined (__xlc__)
>  /* IBM'x xlc compiler. NOTE:  __xlc__ expands to a double quoted string of four
>     numbers separated by '.'s: currently "7.0.0.0" */
> -set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - compiler_info]
> +set need_a_set [regsub -all {\.} [join {xlc __xlc__} -] - cc_compiler_info]
>  #endif
>  
>  #if defined (__ARMCC_VERSION)
> -set compiler_info [join {armcc __ARMCC_VERSION} -]
> +set cc_compiler_info [join {armcc __ARMCC_VERSION} -]
>  #endif
>  
>  #if defined (__clang__)
> -set compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
> +set cc_compiler_info [join {clang __clang_major__ __clang_minor__ __clang_patchlevel__} -]
>  #endif
>  
>  #if defined (__ICC)
>  set icc_major [string range __ICC 0 1]
>  set icc_minor [format "%d" [string range __ICC 2 [expr {[string length __ICC] -1}]]]
>  set icc_update __INTEL_COMPILER_UPDATE
> -set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
> +set cc_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
>  #elif defined (__ICL)
>  set icc_major [string range __ICL 0 1]
>  set icc_minor [format "%d" [string range __ICL 2 [expr {[string length __ICL] -1}]]]
>  set icc_update __INTEL_COMPILER_UPDATE
> -set compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
> +set cc_compiler_info [join "icc $icc_major $icc_minor $icc_update" -]
>  #elif defined(__INTEL_LLVM_COMPILER) && defined(__clang_version__)
>  /* Intel Next Gen compiler defines preprocessor __INTEL_LLVM_COMPILER and
>     provides version info in __clang_version__ e.g. value:
> @@ -63,5 +63,5 @@ set total_length [string length __clang_version__]
>  set version_start_index [string last "(" __clang_version__]
>  set version_string [string range __clang_version__ $version_start_index+5 $total_length-2]
>  set version_updated_string [string map {. -} $version_string]
> -set compiler_info "icx-$version_updated_string"
> +set cc_compiler_info "icx-$version_updated_string"
>  #endif
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 4ee7c1fb0a..72553a5a52 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4113,25 +4113,49 @@ proc get_compiler_info {{arg ""}} {
>      global outdir
>      global tool
>  
> -    # These come from compiler.c, compiler.cc or compiler.F90.
> +    # These come from compiler.c, compiler.cc or compiler.F90.  They are local
> +    # to get_compiler_info and should usually not be used outside.  They are
> +    # used for caching the language dependent compiler info.
> +    global c_compiler_info
> +    global cc_compiler_info
> +    global f_compiler_info

I think rather than adding all of these separate globals we could just
use an associative array with the language as a key.  We may, one day,
want to support additional languages, and an array will scale better
than just adding more and more globals.

Further, if we declare the new associative array like this:

    gdb_persistent_global compiler_info_cache

then we will automatically add compiler_info_cache to the list of
varibles to be preserved between test scripts, which is what we really
want.

> +
> +    # Local to get_compiler_info and test_compiler_info.  Used for
> +    # communicating the compiler info from get_compiler_info to
> +    # test_compiler_info (as well as to avoid some ifs over all supported
> +    # languages).
>      global compiler_info

By switching to an array the motivation for keeping this global
disappears, which I think is a good thing.

>  
>      # Legacy global data symbols.
>      global gcc_compiled

Given we're going to "fix" the caching here, I think we need to give
more thought to this legacy variable.

Ideally we should just remove this, but until we do, we should make sure
we don't break it I think... more below...

>  
> -    if [info exists compiler_info] {
> -	# Already computed.
> -	return 0
> -    }
> -
> -    # Choose which file to preprocess.
> -    set ifile "${srcdir}/lib/compiler.c"
> +    # Check for each language whether we already cached the requested
> +    # compiler info.  If not, choose which file to preprocess.
>      if { $arg == "c++" } {
> +	if { [info exists cc_compiler_info] } {
> +	    set compiler_info $cc_compiler_info
> +	    return 0
> +	}
>  	set ifile "${srcdir}/lib/compiler.cc"
>      } elseif { $arg == "f90" } {
> +	if { [info exists f_compiler_info] } {
> +	    set compiler_info $f_compiler_info
> +	    return 0
> +	}
>  	set ifile "${srcdir}/lib/compiler.F90"

It's worth noting that we always supported C and C++ as separate
languages here, so the problem we hit with the Fortran compiler_info was
always present, just for the C/C++ languages.  Unlike the Fortran case
though, I guess no test really cares that much if the compiler info is
mixed up between C/C++ (in most cases these compilers probably share a
version number).

> +    } else {
> +	# Default to C compiler identification.
> +	if { [info exists c_compiler_info] } {
> +	    set compiler_info $c_compiler_info
> +	    return 0
> +	}
> +	set ifile "${srcdir}/lib/compiler.c"
>      }
>  
> +    # We'll attempt to identify a compiler for a language we have not yet
> +    # processed.  For now the compiler_info is unknown.
> +    set compiler_info "unknown"
> +
>      # Run $ifile through the right preprocessor.
>      # Toggle gdb.log to keep the compiler output out of the log.
>      set saved_log [log_file -info]
> @@ -4171,10 +4195,27 @@ proc get_compiler_info {{arg ""}} {
>  	}
>      }
>  
> -    # Set to unknown if for some reason compiler_info didn't get defined.
> -    if ![info exists compiler_info] {
> +    # Compiler_info was set to unknown earlier, before starting the compiler
> +    # identification.  If for some reason c/cpp/f_compiler_info didn't get
> +    # set we'll leave it at that and cache 'unknown' as the language compiler
> +    # info further down.  Else, update compiler_info to c/cpp/f_compiler_info.
> +    if { $arg == "c++" } {
> +	if { [info exists cc_compiler_info] } {
> +	    set compiler_info $cc_compiler_info
> +	}
> +    } elseif { $arg == "f90" } {
> +	if { [info exists f_compiler_info] } {
> +	    set compiler_info $f_compiler_info
> +	}
> +    } else {
> +	# Default to C compiler identification.
> +	if { [info exists c_compiler_info] } {
> +	    set compiler_info $c_compiler_info
> +	}
> +    }
> +
> +    if { $compiler_info == "unknown" } {
>  	verbose -log "get_compiler_info: compiler_info not provided"
> -	set compiler_info "unknown"
>      }
>      # Also set to unknown compiler if any diagnostics happened.
>      if { $unknown } {
> @@ -4182,6 +4223,19 @@ proc get_compiler_info {{arg ""}} {
>  	set compiler_info "unknown"
>      }
>  
> +    # If compiler_info is set to 'unknown' because of an unexpected diagnostic
> +    # or because compiler identification failed to set the language compiler
> +    # info variable we cache 'unknown' as the result here.
> +    if { $compiler_info == "unknown" } {
> +	if { $arg == "c++" } {
> +	    set cc_compiler_info $compiler_info
> +	} elseif { $arg = "f90" } {
> +	    set f_compiler_info $compiler_info
> +	} else {
> +	    set c_compiler_info $compiler_info
> +	}
> +    }
> +
>      # Set the legacy symbols.
>      set gcc_compiled 0
>      regexp "^gcc-(\[0-9\]+)-" "$compiler_info" matchall gcc_compiled

Now we will only recompute the compiler version once for each language
we need to make sure that gcc_compiled is only updated for (I think) the
C language.  Right now, if the last call to get_compiler_info was for
the Fortran language then gcc_compiled would be left as 0, right?  Even
if the C compiler was GCC, and I don't think this is what we want.

Obviously, the real solution long term is to get rid of all uses of
gcc_compiled, and then rip this bit of code out.

Anyway.  I took a pass at writing an updated patch, which turned into a
series of patches, based on this work.  I'll follow up to this message
with my series for your consideration.

Thanks,
Andrew

> -- 
> 2.25.1
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 0/5] Solve caching problems with compiler_info

[Andrew Burgess]

This series is an alternative proposal to solve the caching problems
with compiler_info.

Patch #1 is taken unmodified from Nils' v2 series.

Patches #2, #3, #4, and #5 are new work, though the final patch
addresses the same problem as the second patch in Nils' v2 series.

Thanks,
Andrew

---

Andrew Burgess (4):
  gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp
  gdb/testsuite: make 'c' default language for get/test compiler info
  gdb/testsuite: handle errors better in test_compiler_info
  gdb/testsuite: solve problems with compiler_info caching

Nils-Christian Kempke (1):
  gdb/testsuite: use test_compiler_info in gcc_major_version

 gdb/testsuite/lib/gdb.exp | 87 +++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 44 deletions(-)

-- 
2.25.4

[PATCH 1/5] gdb/testsuite: use test_compiler_info in gcc_major_version

[Andrew Burgess]

From: Nils-Christian Kempke <nils-christian.kempke@intel.com>

The procedure gcc_major_version was earlier using the global variable
compiler_info to retrieve gcc's major version.  This is discouraged and
(as can be read in a comment in compiler.c) compiler_info should be
local to get_compiler_info and test_compiler_info.

The preferred way of getting the compiler string is via calling
test_compiler_info without arguments.  Gcc_major_version was changed to
do that.
---
 gdb/testsuite/lib/gdb.exp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 402450152ac..4ee7c1fb0ae 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4218,12 +4218,11 @@ proc test_compiler_info { {compiler ""} {language ""} } {
 # For gcc 7.5.0, the major version 7.
 
 proc gcc_major_version { } {
-    global compiler_info
     global decimal
     if { ![test_compiler_info "gcc-*"] } {
 	return -1
     }
-    set res [regexp gcc-($decimal)-($decimal)- $compiler_info \
+    set res [regexp gcc-($decimal)-($decimal)- [test_compiler_info] \
 		 dummy_var major minor]
     if { $res != 1 } {
 	return -1
-- 
2.25.4

[PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp

[Andrew Burgess]

We don't need to call get_compiler_info before calling
test_compiler_info; test_compiler_info includes a call to
get_compiler_info.

This commit cleans up gdb.exp a little by removing some unneeded
calls.  We could do the same cleanup throughout the testsuite, but I'm
leaving that for another day.

There should be no change in the test results after this commit.
---
 gdb/testsuite/lib/gdb.exp | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 4ee7c1fb0ae..b779bff81e6 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3238,10 +3238,6 @@ gdb_caching_proc skip_altivec_tests {
     }
 
     # Make sure we have a compiler that understands altivec.
-    if [get_compiler_info] {
-       warning "Could not get compiler info"
-       return 1
-    }
     if [test_compiler_info gcc*] {
         set compile_flags "additional_flags=-maltivec"
     } elseif [test_compiler_info xlc*] {
@@ -3357,10 +3353,6 @@ gdb_caching_proc skip_vsx_tests {
     }
 
     # Make sure we have a compiler that understands altivec.
-    if [get_compiler_info] {
-       warning "Could not get compiler info"
-       return 1
-    }
     if [test_compiler_info gcc*] {
         set compile_flags "additional_flags=-mvsx"
     } elseif [test_compiler_info xlc*] {
@@ -4781,11 +4773,8 @@ proc gdb_compile_shlib_1 {sources dest options} {
     } elseif { [lsearch -exact $options "f90"] >= 0 } {
 	set info_options "f90"
     }
-    if [get_compiler_info ${info_options}] {
-       return -1
-    }
 
-    switch -glob [test_compiler_info] {
+    switch -glob [test_compiler_info ${info_options}] {
         "xlc-*" {
             lappend obj_options "additional_flags=-qpic"
         }
@@ -6964,16 +6953,6 @@ proc build_executable_from_specs {testname executable options args} {
 
     set binfile [standard_output_file $executable]
 
-    set info_options ""
-    if { [lsearch -exact $options "c++"] >= 0 } {
-	set info_options "c++"
-    } elseif { [lsearch -exact $options "f90"] >= 0 } {
-	set info_options "f90"
-    }
-    if [get_compiler_info ${info_options}] {
-        return -1
-    }
-
     set func gdb_compile
     set func_index [lsearch -regexp $options {^(pthreads|shlib|shlib_pthreads|openmp)$}]
     if {$func_index != -1} {
-- 
2.25.4

Re: [PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> We don't need to call get_compiler_info before calling
> test_compiler_info; test_compiler_info includes a call to
> get_compiler_info.
>
> This commit cleans up gdb.exp a little by removing some unneeded
> calls.  We could do the same cleanup throughout the testsuite, but I'm
> leaving that for another day.
>
> There should be no change in the test results after this commit.

Nils pointed out some issues with this patch in this message:

  https://sourceware.org/pipermail/gdb-patches/2022-June/189938.html

Below is an updated version that addresses these issues.

Thanks,
Andrew

---

commit 13f333e5880e6310f0a673f0e8b12b6983eb5acd
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Jun 8 14:25:00 2022 +0100

    gdb/testsuite: remove get_compiler_info calls from gdb.exp and dwarf.exp
    
    We don't need to call get_compiler_info before calling
    test_compiler_info; test_compiler_info includes a call to
    get_compiler_info.
    
    This commit cleans up lib/gdb.exp and lib/dwarf.exp a little by
    removing some unneeded calls to get_compiler_info.  We could do the
    same cleanup throughout the testsuite, but I'm leaving that for
    another day.
    
    There should be no change in the test results after this commit.

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index b05fdd58f66..3d0ea83aee7 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -110,14 +110,6 @@ proc build_executable_and_dwo_files { testname executable options args } {
 	set binfile $executable
     }
 
-    set info_options ""
-    if { [lsearch -exact $options "c++"] >= 0 } {
-	set info_options "c++"
-    }
-    if [get_compiler_info ${info_options}] {
-        return -1
-    }
-
     set func gdb_compile
     if {[lsearch -regexp $options \
 	     {^(pthreads|shlib|shlib_pthreads|openmp)$}] != -1} {
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7b11dab870e..c9a30d88b2f 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3238,10 +3238,6 @@ gdb_caching_proc skip_altivec_tests {
     }
 
     # Make sure we have a compiler that understands altivec.
-    if [get_compiler_info] {
-       warning "Could not get compiler info"
-       return 1
-    }
     if [test_compiler_info gcc*] {
         set compile_flags "additional_flags=-maltivec"
     } elseif [test_compiler_info xlc*] {
@@ -3357,10 +3353,6 @@ gdb_caching_proc skip_vsx_tests {
     }
 
     # Make sure we have a compiler that understands altivec.
-    if [get_compiler_info] {
-       warning "Could not get compiler info"
-       return 1
-    }
     if [test_compiler_info gcc*] {
         set compile_flags "additional_flags=-mvsx"
     } elseif [test_compiler_info xlc*] {
@@ -4781,11 +4773,8 @@ proc gdb_compile_shlib_1 {sources dest options} {
     } elseif { [lsearch -exact $options "f90"] >= 0 } {
 	set info_options "f90"
     }
-    if [get_compiler_info ${info_options}] {
-       return -1
-    }
 
-    switch -glob [test_compiler_info] {
+    switch -glob [test_compiler_info "" ${info_options}] {
         "xlc-*" {
             lappend obj_options "additional_flags=-qpic"
         }
@@ -6964,16 +6953,6 @@ proc build_executable_from_specs {testname executable options args} {
 
     set binfile [standard_output_file $executable]
 
-    set info_options ""
-    if { [lsearch -exact $options "c++"] >= 0 } {
-	set info_options "c++"
-    } elseif { [lsearch -exact $options "f90"] >= 0 } {
-	set info_options "f90"
-    }
-    if [get_compiler_info ${info_options}] {
-        return -1
-    }
-
     set func gdb_compile
     set func_index [lsearch -regexp $options {^(pthreads|shlib|shlib_pthreads|openmp)$}]
     if {$func_index != -1} {

[PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info

[Andrew Burgess]

This commit is a minor cleanup for the two functions (in gdb.exp)
get_compiler_info and test_compiler_info.

Instead of using the empty string as the default language, and just
"knowing" that this means the C language.  Make this explicit.  The
language argument now defaults to "c" if not specified, and the if
chain in get_compiler_info that checks the language not explicitly
handles "c" and gives an error for unknown languages.

This is a good thing, now that the API appears to take a language, if
somebody does:

  test_compiler_info "xxxx" "rust"

to check the version of the rust compiler then we will now give an
error rather than just using the C compiler and leaving the user
having to figure out why they are not getting the results they
expect.

After a little grepping, I think the only place we were explicitly
passing the empty string to either get_compiler_info or
test_compiler_info was in gdb_compile_shlib_1, this is now changed to
pass "c" as the default language.

There should be no changes to the test results after this commit.
---
 gdb/testsuite/lib/gdb.exp | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index b779bff81e6..140da05b35e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4097,7 +4097,7 @@ set gcc_compiled		0
 #
 # -- chastain 2004-01-06
 
-proc get_compiler_info {{arg ""}} {
+proc get_compiler_info {{language "c"}} {
     # For compiler.c, compiler.cc and compiler.F90.
     global srcdir
 
@@ -4117,11 +4117,15 @@ proc get_compiler_info {{arg ""}} {
     }
 
     # Choose which file to preprocess.
-    set ifile "${srcdir}/lib/compiler.c"
-    if { $arg == "c++" } {
+    if { $language == "c++" } {
 	set ifile "${srcdir}/lib/compiler.cc"
-    } elseif { $arg == "f90" } {
+    } elseif { $language == "f90" } {
 	set ifile "${srcdir}/lib/compiler.F90"
+    } elseif { $language == "c" } {
+	set ifile "${srcdir}/lib/compiler.c"
+    } else {
+	perror "Unable to fetch compiler version for language: $language"
+	return -1
     }
 
     # Run $ifile through the right preprocessor.
@@ -4132,12 +4136,12 @@ proc get_compiler_info {{arg ""}} {
 	# We have to use -E and -o together, despite the comments
 	# above, because of how DejaGnu handles remote host testing.
 	set ppout "$outdir/compiler.i"
-	gdb_compile "${ifile}" "$ppout" preprocess [list "$arg" quiet getting_compiler_info]
+	gdb_compile "${ifile}" "$ppout" preprocess [list "$language" quiet getting_compiler_info]
 	set file [open $ppout r]
 	set cppout [read $file]
 	close $file
     } else {
-	set cppout [ gdb_compile "${ifile}" "" preprocess [list "$arg" quiet getting_compiler_info] ]
+	set cppout [ gdb_compile "${ifile}" "" preprocess [list "$language" quiet getting_compiler_info] ]
     }
     eval log_file $saved_log
 
@@ -4193,7 +4197,7 @@ proc get_compiler_info {{arg ""}} {
 # Otherwise the argument is a glob-style expression to match against
 # compiler_info.
 
-proc test_compiler_info { {compiler ""} {language ""} } {
+proc test_compiler_info { {compiler ""} {language "c"} } {
     global compiler_info
     get_compiler_info $language
 
@@ -4767,11 +4771,12 @@ proc gdb_compile_shlib_1 {sources dest options} {
 	set ada 1
     }
 
-    set info_options ""
     if { [lsearch -exact $options "c++"] >= 0 } {
 	set info_options "c++"
     } elseif { [lsearch -exact $options "f90"] >= 0 } {
 	set info_options "f90"
+    } else {
+	set info_options "c"
     }
 
     switch -glob [test_compiler_info ${info_options}] {
-- 
2.25.4

RE: [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info

[Kempke, Nils-Christian]

> -----Original Message-----
> From: Gdb-patches <gdb-patches-bounces+nils-
> christian.kempke=intel.com@sourceware.org> On Behalf Of Andrew
> Burgess via Gdb-patches
> Sent: Wednesday, June 8, 2022 4:22 PM
> To: gdb-patches@sourceware.org
> Cc: Andrew Burgess <aburgess@redhat.com>
> Subject: [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test
> compiler info
> 
> This commit is a minor cleanup for the two functions (in gdb.exp)
> get_compiler_info and test_compiler_info.
> 
> Instead of using the empty string as the default language, and just
> "knowing" that this means the C language.  Make this explicit.  The
> language argument now defaults to "c" if not specified, and the if
> chain in get_compiler_info that checks the language not explicitly
> handles "c" and gives an error for unknown languages.
> 
> This is a good thing, now that the API appears to take a language, if
> somebody does:
> 
>   test_compiler_info "xxxx" "rust"
> 
> to check the version of the rust compiler then we will now give an
> error rather than just using the C compiler and leaving the user
> having to figure out why they are not getting the results they
> expect.
> 
> After a little grepping, I think the only place we were explicitly
> passing the empty string to either get_compiler_info or
> test_compiler_info was in gdb_compile_shlib_1, this is now changed to
> pass "c" as the default language.

I think there is one more: in build_executable_and_dwo_files in dwarf.exp
we also call get_compiler_info with the $language variable which is set to
"" when the language is not given as "c++" in the procedure's options.

> 
> There should be no changes to the test results after this commit.
> ---
>  gdb/testsuite/lib/gdb.exp | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index b779bff81e6..140da05b35e 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4097,7 +4097,7 @@ set gcc_compiled		0
>  #
>  # -- chastain 2004-01-06
> 
> -proc get_compiler_info {{arg ""}} {
> +proc get_compiler_info {{language "c"}} {
>      # For compiler.c, compiler.cc and compiler.F90.
>      global srcdir
> 
> @@ -4117,11 +4117,15 @@ proc get_compiler_info {{arg ""}} {
>      }
> 
>      # Choose which file to preprocess.
> -    set ifile "${srcdir}/lib/compiler.c"
> -    if { $arg == "c++" } {
> +    if { $language == "c++" } {
>  	set ifile "${srcdir}/lib/compiler.cc"
> -    } elseif { $arg == "f90" } {
> +    } elseif { $language == "f90" } {
>  	set ifile "${srcdir}/lib/compiler.F90"
> +    } elseif { $language == "c" } {
> +	set ifile "${srcdir}/lib/compiler.c"
> +    } else {
> +	perror "Unable to fetch compiler version for language: $language"
> +	return -1
>      }
> 
>      # Run $ifile through the right preprocessor.
> @@ -4132,12 +4136,12 @@ proc get_compiler_info {{arg ""}} {
>  	# We have to use -E and -o together, despite the comments
>  	# above, because of how DejaGnu handles remote host testing.
>  	set ppout "$outdir/compiler.i"
> -	gdb_compile "${ifile}" "$ppout" preprocess [list "$arg" quiet
> getting_compiler_info]
> +	gdb_compile "${ifile}" "$ppout" preprocess [list "$language" quiet
> getting_compiler_info]
>  	set file [open $ppout r]
>  	set cppout [read $file]
>  	close $file
>      } else {
> -	set cppout [ gdb_compile "${ifile}" "" preprocess [list "$arg" quiet
> getting_compiler_info] ]
> +	set cppout [ gdb_compile "${ifile}" "" preprocess [list "$language"
> quiet getting_compiler_info] ]
>      }
>      eval log_file $saved_log
> 
> @@ -4193,7 +4197,7 @@ proc get_compiler_info {{arg ""}} {
>  # Otherwise the argument is a glob-style expression to match against
>  # compiler_info.
> 
> -proc test_compiler_info { {compiler ""} {language ""} } {
> +proc test_compiler_info { {compiler ""} {language "c"} } {
>      global compiler_info
>      get_compiler_info $language
> 
> @@ -4767,11 +4771,12 @@ proc gdb_compile_shlib_1 {sources dest options}
> {
>  	set ada 1
>      }
> 
> -    set info_options ""
>      if { [lsearch -exact $options "c++"] >= 0 } {
>  	set info_options "c++"
>      } elseif { [lsearch -exact $options "f90"] >= 0 } {
>  	set info_options "f90"
> +    } else {
> +	set info_options "c"
>      }
> 
>      switch -glob [test_compiler_info ${info_options}] {

I am wondering whether these lines ever did what was intended. The proc
test_compiler_info takes 2 arguments, the string to match against defaulted
to "" and the language defaulted to "c".  So by writing test_compiler_info ${info_options},
would we not call test_compiler_info "${info_options}" "c", specializing the first but not
the second argument?
I think this line tires to use the "Return the compiler_info string if no arg is provided" version
of test_compiler_info, which would be 
 	test_compiler_info "" ${info_options}
here. As far as me grepping the testsuite goes this is the only call like this..

Thanks,
Nils
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info

[Andrew Burgess]

"Kempke, Nils-Christian via Gdb-patches" <gdb-patches@sourceware.org>
writes:

>> -----Original Message-----
>> From: Gdb-patches <gdb-patches-bounces+nils-
>> christian.kempke=intel.com@sourceware.org> On Behalf Of Andrew
>> Burgess via Gdb-patches
>> Sent: Wednesday, June 8, 2022 4:22 PM
>> To: gdb-patches@sourceware.org
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Subject: [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test
>> compiler info
>> 
>> This commit is a minor cleanup for the two functions (in gdb.exp)
>> get_compiler_info and test_compiler_info.
>> 
>> Instead of using the empty string as the default language, and just
>> "knowing" that this means the C language.  Make this explicit.  The
>> language argument now defaults to "c" if not specified, and the if
>> chain in get_compiler_info that checks the language not explicitly
>> handles "c" and gives an error for unknown languages.
>> 
>> This is a good thing, now that the API appears to take a language, if
>> somebody does:
>> 
>>   test_compiler_info "xxxx" "rust"
>> 
>> to check the version of the rust compiler then we will now give an
>> error rather than just using the C compiler and leaving the user
>> having to figure out why they are not getting the results they
>> expect.
>> 
>> After a little grepping, I think the only place we were explicitly
>> passing the empty string to either get_compiler_info or
>> test_compiler_info was in gdb_compile_shlib_1, this is now changed to
>> pass "c" as the default language.
>
> I think there is one more: in build_executable_and_dwo_files in dwarf.exp
> we also call get_compiler_info with the $language variable which is set to
> "" when the language is not given as "c++" in the procedure's options.
>
>> 
>> There should be no changes to the test results after this commit.
>> ---
>>  gdb/testsuite/lib/gdb.exp | 21 +++++++++++++--------
>>  1 file changed, 13 insertions(+), 8 deletions(-)
>> 
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index b779bff81e6..140da05b35e 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -4097,7 +4097,7 @@ set gcc_compiled		0
>>  #
>>  # -- chastain 2004-01-06
>> 
>> -proc get_compiler_info {{arg ""}} {
>> +proc get_compiler_info {{language "c"}} {
>>      # For compiler.c, compiler.cc and compiler.F90.
>>      global srcdir
>> 
>> @@ -4117,11 +4117,15 @@ proc get_compiler_info {{arg ""}} {
>>      }
>> 
>>      # Choose which file to preprocess.
>> -    set ifile "${srcdir}/lib/compiler.c"
>> -    if { $arg == "c++" } {
>> +    if { $language == "c++" } {
>>  	set ifile "${srcdir}/lib/compiler.cc"
>> -    } elseif { $arg == "f90" } {
>> +    } elseif { $language == "f90" } {
>>  	set ifile "${srcdir}/lib/compiler.F90"
>> +    } elseif { $language == "c" } {
>> +	set ifile "${srcdir}/lib/compiler.c"
>> +    } else {
>> +	perror "Unable to fetch compiler version for language: $language"
>> +	return -1
>>      }
>> 
>>      # Run $ifile through the right preprocessor.
>> @@ -4132,12 +4136,12 @@ proc get_compiler_info {{arg ""}} {
>>  	# We have to use -E and -o together, despite the comments
>>  	# above, because of how DejaGnu handles remote host testing.
>>  	set ppout "$outdir/compiler.i"
>> -	gdb_compile "${ifile}" "$ppout" preprocess [list "$arg" quiet
>> getting_compiler_info]
>> +	gdb_compile "${ifile}" "$ppout" preprocess [list "$language" quiet
>> getting_compiler_info]
>>  	set file [open $ppout r]
>>  	set cppout [read $file]
>>  	close $file
>>      } else {
>> -	set cppout [ gdb_compile "${ifile}" "" preprocess [list "$arg" quiet
>> getting_compiler_info] ]
>> +	set cppout [ gdb_compile "${ifile}" "" preprocess [list "$language"
>> quiet getting_compiler_info] ]
>>      }
>>      eval log_file $saved_log
>> 
>> @@ -4193,7 +4197,7 @@ proc get_compiler_info {{arg ""}} {
>>  # Otherwise the argument is a glob-style expression to match against
>>  # compiler_info.
>> 
>> -proc test_compiler_info { {compiler ""} {language ""} } {
>> +proc test_compiler_info { {compiler ""} {language "c"} } {
>>      global compiler_info
>>      get_compiler_info $language
>> 
>> @@ -4767,11 +4771,12 @@ proc gdb_compile_shlib_1 {sources dest options}
>> {
>>  	set ada 1
>>      }
>> 
>> -    set info_options ""
>>      if { [lsearch -exact $options "c++"] >= 0 } {
>>  	set info_options "c++"
>>      } elseif { [lsearch -exact $options "f90"] >= 0 } {
>>  	set info_options "f90"
>> +    } else {
>> +	set info_options "c"
>>      }
>> 
>>      switch -glob [test_compiler_info ${info_options}] {
>
> I am wondering whether these lines ever did what was intended. The proc
> test_compiler_info takes 2 arguments, the string to match against defaulted
> to "" and the language defaulted to "c".  So by writing test_compiler_info ${info_options},
> would we not call test_compiler_info "${info_options}" "c", specializing the first but not
> the second argument?
> I think this line tires to use the "Return the compiler_info string if no arg is provided" version
> of test_compiler_info, which would be 
>  	test_compiler_info "" ${info_options}
> here. As far as me grepping the testsuite goes this is the only call like this..

Thanks for spotting both these issues.  The fixes for these better fit
in patch #2.  I've posted an updated version of that patch that should
resolve these two issues.

Thanks,
Andrew

[PATCH 4/5] gdb/testsuite: handle errors better in test_compiler_info

[Andrew Burgess]

Now that get_compiler_info might actually fail (if the language is not
handled), then we should try to handle this failure better in
test_compiler_info.

After this commit, if get_compiler_info fails then we will return a
suitable result depending on how the user called test_compiler_info.

If the user does something like:

  set version [test_compiler_info "" "unknown-language"]

Then test_compiler_info will return an empty string.  My assumption is
that the user will be trying to match 'version' against something, and
the empty string hopefully will not match.

If the user does something like:

  if { [test_compiler_info "some_pattern" "unknown-language"] } {
    ....
  }

Then test_compiler_info will return false which seems the obvious
choice.

There should be no change in the test results after this commit.
---
 gdb/testsuite/lib/gdb.exp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 140da05b35e..9b4f3a180ef 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4199,7 +4199,17 @@ proc get_compiler_info {{language "c"}} {
 
 proc test_compiler_info { {compiler ""} {language "c"} } {
     global compiler_info
-    get_compiler_info $language
+
+    if [get_compiler_info $language] {
+	# An error will already have been printed in this case.  Just
+	# return a suitable result depending on how the user called
+	# this function.
+	if [string match "" $compiler] {
+	    return ""
+	} else {
+	    return false
+	}
+    }
 
     # If no arg, return the compiler_info string.
     if [string match "" $compiler] {
-- 
2.25.4

[PATCH 5/5] gdb/testsuite: solve problems with compiler_info caching

[Andrew Burgess]

After this commit:

  commit 44d469c5f85a4243462b8966722dafa62b602bf5
  Date:   Tue May 31 16:43:44 2022 +0200

      gdb/testsuite: add Fortran compiler identification to GDB

Some regressions were noticed:

  https://sourceware.org/pipermail/gdb-patches/2022-May/189673.html

The problem is associated with how compiler_info variable is cached
between calls to get_compiler_info.

Even before the above commit, get_compiler_info supported two
language, C and C++.  Calling get_compiler_info would set the global
compiler_info based on the language passed as an argument to
get_compiler_info, and, in theory, compiler_info would not be updated
for the rest of the dejagnu run.

This obviously is slightly broken behaviour.  If the first call to
get_compiler_info was for the C++ language then compiler_info would be
set based on the C++ compiler in use, while if the first call to
get_compiler_info was for the C language then compiler_info would be
set based on the C compiler.

This probably wasn't very noticable, assuming a GCC based test
environment then in most cases the C and C++ compiler would be the
same version.

However, if the user starting playing with CC_FOR_TARGET or
CXX_FOR_TARGET, then they might not get the behaviour they expect.

Except, to make matters worse, most of the time, the user probably
would get the behaviour they expected .... except when they didn't!
I'll explain:

In gdb.exp we try to avoid global variables leaking between test
scripts, this is done with the help of the two procs
gdb_setup_known_globals and gdb_cleanup_globals.  All known globals
are recorded before a test script starts, and then, when the test
script ends, any new globals are deleted.

Normally, compiler_info is only set as a result of a test script
calling get_compiler_info or test_compiler_info.  This means that the
compiler_info global will not exist when the test script starts, but
will exist when the test script end, and so, the compiler_info
variable is deleted at the end of each test.

This means that, in reality, the compiler_info is recalculated once
for each test script, hence, if a test script just checks on the C
compiler, or just checks on the C++ compiler, then compiler_info will
be correct and the user will get the behaviour they expect.

However, if a single test script tries to check both the C and C++
compiler versions then this will not work (even before the above
commit).

The situation is made worse be the behaviour or the load_lib proc.
This proc (provided by dejagnu) will only load each library once.
This means that if a library defines a global, then this global would
normally be deleted at the end of the first test script that includes
the library.

As future attempts to load the library will not actually reload it,
then the global will not be redefined and would be missing for later
test scripts that also tried to load that library.

To work around this issue we override load_lib in gdb.exp, this new
version adds all globals from the newly loaded library to the list of
globals that should be preserved (not deleted).

And this is where things get interesting for us.  The library
trace-support.exp includes calls, at the file scope, to things like
is_amd64_regs_target, which cause get_compiler_info to be called.
This means that after loading the library the compiler_info global is
defined.

Our override of load_lib then decides that this new global has to be
preserved, and adds it to the gdb_persistent_globals array.

From that point on compiler_info will never be recomputed!

This commit addresses all the caching problems by doing the following:

Change the compiler_info global into compiler_info_cache global.  This
new global is an array, the keys of this array will be each of the
supported languages, and the values will be the compiler version for
that language.

Now, when we call get_compiler_info, if the compiler information for
the specific language has not been computed, then we do that, and add
it to the cache.

Next, compiler_info_cache is defined by calling
gdb_persistent_global.  This automatically adds the global to the list
of persistent globals.  Now the cache will not be deleted at the end
of each test script.

This means that, for a single test run, we will compute the compiler
version just once for each language, this result will then be cached
between test scripts.

Finally, the legacy 'gcc_compiled' flag is now only set when we call
get_compiler_info with the language 'c'.  Without making this change
the value of 'gcc_compiled' would change each time a new language is
passed to get_compiler_info.  If the last language was e.g. Fortran,
then gcc_compiled might be left false.
---
 gdb/testsuite/lib/gdb.exp | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9b4f3a180ef..a9c792d27e2 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4098,6 +4098,7 @@ set gcc_compiled		0
 # -- chastain 2004-01-06
 
 proc get_compiler_info {{language "c"}} {
+
     # For compiler.c, compiler.cc and compiler.F90.
     global srcdir
 
@@ -4106,12 +4107,9 @@ proc get_compiler_info {{language "c"}} {
     global tool
 
     # These come from compiler.c, compiler.cc or compiler.F90.
-    global compiler_info
-
-    # Legacy global data symbols.
-    global gcc_compiled
+    gdb_persistent_global compiler_info_cache
 
-    if [info exists compiler_info] {
+    if [info exists compiler_info_cache($language)] {
 	# Already computed.
 	return 0
     }
@@ -4178,9 +4176,17 @@ proc get_compiler_info {{language "c"}} {
 	set compiler_info "unknown"
     }
 
-    # Set the legacy symbols.
-    set gcc_compiled 0
-    regexp "^gcc-(\[0-9\]+)-" "$compiler_info" matchall gcc_compiled
+    set compiler_info_cache($language) $compiler_info
+
+    # Set the legacy symbol gcc_compiled.
+    if { $language == "c" } {
+	# Legacy global data symbols.
+	gdb_persistent_global gcc_compiled
+
+	set gcc_compiled 0
+
+	regexp "^gcc-(\[0-9\]+)-" "$compiler_info" matchall gcc_compiled
+    }
 
     # Log what happened.
     verbose -log "get_compiler_info: $compiler_info"
@@ -4198,7 +4204,7 @@ proc get_compiler_info {{language "c"}} {
 # compiler_info.
 
 proc test_compiler_info { {compiler ""} {language "c"} } {
-    global compiler_info
+    gdb_persistent_global compiler_info_cache
 
     if [get_compiler_info $language] {
 	# An error will already have been printed in this case.  Just
@@ -4213,10 +4219,10 @@ proc test_compiler_info { {compiler ""} {language "c"} } {
 
     # If no arg, return the compiler_info string.
     if [string match "" $compiler] {
-	return $compiler_info
+	return $compiler_info_cache($language)
     }
 
-    return [string match $compiler $compiler_info]
+    return [string match $compiler $compiler_info_cache($language)]
 }
 
 # Return the gcc major version, or -1.
-- 
2.25.4

RE: [PATCH 0/5] Solve caching problems with compiler_info

[Kempke, Nils-Christian]

> -----Original Message-----
> From: Gdb-patches <gdb-patches-bounces+nils-
> christian.kempke=intel.com@sourceware.org> On Behalf Of Andrew
> Burgess via Gdb-patches
> Sent: Wednesday, June 8, 2022 4:22 PM
> To: gdb-patches@sourceware.org
> Cc: Andrew Burgess <aburgess@redhat.com>
> Subject: [PATCH 0/5] Solve caching problems with compiler_info
> 
> This series is an alternative proposal to solve the caching problems
> with compiler_info.
> 
> Patch #1 is taken unmodified from Nils' v2 series.
> 
> Patches #2, #3, #4, and #5 are new work, though the final patch
> addresses the same problem as the second patch in Nils' v2 series.
> 
> Thanks,
> Andrew


Hi Andrew,

Thanks for this - I only had two minor comments really. The rest of this
series is nice.  I do prefer it over my initially submitted patches. And thanks
for showing how the associative array works !

Thanks,

Nils

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 0/5] Solve caching problems with compiler_info

[Andrew Burgess]

"Kempke, Nils-Christian via Gdb-patches" <gdb-patches@sourceware.org>
writes:

>> -----Original Message-----
>> From: Gdb-patches <gdb-patches-bounces+nils-
>> christian.kempke=intel.com@sourceware.org> On Behalf Of Andrew
>> Burgess via Gdb-patches
>> Sent: Wednesday, June 8, 2022 4:22 PM
>> To: gdb-patches@sourceware.org
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Subject: [PATCH 0/5] Solve caching problems with compiler_info
>> 
>> This series is an alternative proposal to solve the caching problems
>> with compiler_info.
>> 
>> Patch #1 is taken unmodified from Nils' v2 series.
>> 
>> Patches #2, #3, #4, and #5 are new work, though the final patch
>> addresses the same problem as the second patch in Nils' v2 series.
>> 
>> Thanks,
>> Andrew
>
>
> Hi Andrew,
>
> Thanks for this - I only had two minor comments really. The rest of this
> series is nice.  I do prefer it over my initially submitted patches. And thanks
> for showing how the associative array works !

Thanks for the feedback.

As this issue was causing significant test failures when not running
tests in parallel, I've gone ahead and pushed this (with the updated
patch #2).

I'm happy to address any feedback that comes up later.

Thanks,
Andrew

Re: [PATCH 0/5] Solve caching problems with compiler_info

[Tom de Vries]

On 6/9/22 15:42, Andrew Burgess via Gdb-patches wrote:
> "Kempke, Nils-Christian via Gdb-patches" <gdb-patches@sourceware.org>
> writes:
> 
>>> -----Original Message-----
>>> From: Gdb-patches <gdb-patches-bounces+nils-
>>> christian.kempke=intel.com@sourceware.org> On Behalf Of Andrew
>>> Burgess via Gdb-patches
>>> Sent: Wednesday, June 8, 2022 4:22 PM
>>> To: gdb-patches@sourceware.org
>>> Cc: Andrew Burgess <aburgess@redhat.com>
>>> Subject: [PATCH 0/5] Solve caching problems with compiler_info
>>>
>>> This series is an alternative proposal to solve the caching problems
>>> with compiler_info.
>>>
>>> Patch #1 is taken unmodified from Nils' v2 series.
>>>
>>> Patches #2, #3, #4, and #5 are new work, though the final patch
>>> addresses the same problem as the second patch in Nils' v2 series.
>>>
>>> Thanks,
>>> Andrew
>>
>>
>> Hi Andrew,
>>
>> Thanks for this - I only had two minor comments really. The rest of this
>> series is nice.  I do prefer it over my initially submitted patches. And thanks
>> for showing how the associative array works !
> 
> Thanks for the feedback.
> 
> As this issue was causing significant test failures when not running
> tests in parallel, I've gone ahead and pushed this (with the updated
> patch #2).
> 
> I'm happy to address any feedback that comes up later.

I'm running into:
...
ERROR: tcl error sourcing 
/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base/all-bin.exp.
ERROR: can't read "false": no such variable
     while executing
"gdb_test "print v_int <= v_short" " = $false" \
     "print value of v_int<=v_short""
...
which doesn't occur if I run the test-case in isolation.

Currently trying out:
...
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 67e5838a7a7..37a28822311 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4193,8 +4193,9 @@ proc get_compiler_info {{language "c"}} {

      # Most compilers will evaluate comparisons and other boolean
      # operations to 0 or 1.
-    uplevel \#0 { set true 1 }
-    uplevel \#0 { set false 0 }
+    gdb_persistent_global true false
+    set true 1
+    set false 0

      return 0
  }
...
which got past that test-case without running into the error.

Thanks,
- Tom

Re: [PATCH 0/5] Solve caching problems with compiler_info

[Andrew Burgess]

Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 6/9/22 15:42, Andrew Burgess via Gdb-patches wrote:
>> "Kempke, Nils-Christian via Gdb-patches" <gdb-patches@sourceware.org>
>> writes:
>> 
>>>> -----Original Message-----
>>>> From: Gdb-patches <gdb-patches-bounces+nils-
>>>> christian.kempke=intel.com@sourceware.org> On Behalf Of Andrew
>>>> Burgess via Gdb-patches
>>>> Sent: Wednesday, June 8, 2022 4:22 PM
>>>> To: gdb-patches@sourceware.org
>>>> Cc: Andrew Burgess <aburgess@redhat.com>
>>>> Subject: [PATCH 0/5] Solve caching problems with compiler_info
>>>>
>>>> This series is an alternative proposal to solve the caching problems
>>>> with compiler_info.
>>>>
>>>> Patch #1 is taken unmodified from Nils' v2 series.
>>>>
>>>> Patches #2, #3, #4, and #5 are new work, though the final patch
>>>> addresses the same problem as the second patch in Nils' v2 series.
>>>>
>>>> Thanks,
>>>> Andrew
>>>
>>>
>>> Hi Andrew,
>>>
>>> Thanks for this - I only had two minor comments really. The rest of this
>>> series is nice.  I do prefer it over my initially submitted patches. And thanks
>>> for showing how the associative array works !
>> 
>> Thanks for the feedback.
>> 
>> As this issue was causing significant test failures when not running
>> tests in parallel, I've gone ahead and pushed this (with the updated
>> patch #2).
>> 
>> I'm happy to address any feedback that comes up later.
>
> I'm running into:
> ...
> ERROR: tcl error sourcing 
> /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base/all-bin.exp.
> ERROR: can't read "false": no such variable
>      while executing
> "gdb_test "print v_int <= v_short" " = $false" \
>      "print value of v_int<=v_short""
> ...
> which doesn't occur if I run the test-case in isolation.
>
> Currently trying out:
> ...
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 67e5838a7a7..37a28822311 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4193,8 +4193,9 @@ proc get_compiler_info {{language "c"}} {
>
>       # Most compilers will evaluate comparisons and other boolean
>       # operations to 0 or 1.
> -    uplevel \#0 { set true 1 }
> -    uplevel \#0 { set false 0 }
> +    gdb_persistent_global true false
> +    set true 1
> +    set false 0
>
>       return 0
>   }
> ...
> which got past that test-case without running into the error.

Sorry for that.  I pushed the fix from:

  https://sourceware.org/pipermail/gdb-patches/2022-June/189985.html

after your review.

Thanks,
Andrew