* [PATCH v2 0/2] Fix regressions introduced by Fortran compiler identification series @ 2022-06-07 10:16 Nils-Christian Kempke 2022-06-07 10:16 ` [PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version Nils-Christian Kempke 2022-06-07 10:16 ` [PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis Nils-Christian Kempke 0 siblings, 2 replies; 18+ messages in thread From: Nils-Christian Kempke @ 2022-06-07 10:16 UTC (permalink / raw) To: gdb-patches; +Cc: aburgess, JiniSusan.George, tdevries, 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version 2022-06-07 10:16 [PATCH v2 0/2] Fix regressions introduced by Fortran compiler identification series Nils-Christian Kempke @ 2022-06-07 10:16 ` Nils-Christian Kempke 2022-06-08 9:50 ` Andrew Burgess 2022-06-07 10:16 ` [PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis Nils-Christian Kempke 1 sibling, 1 reply; 18+ messages in thread From: Nils-Christian Kempke @ 2022-06-07 10:16 UTC (permalink / raw) To: gdb-patches; +Cc: aburgess, JiniSusan.George, tdevries, 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version 2022-06-07 10:16 ` [PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version Nils-Christian Kempke @ 2022-06-08 9:50 ` Andrew Burgess 0 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 9:50 UTC (permalink / raw) To: Nils-Christian Kempke, gdb-patches Cc: JiniSusan.George, tdevries, Nils-Christian Kempke 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis 2022-06-07 10:16 [PATCH v2 0/2] Fix regressions introduced by Fortran compiler identification series Nils-Christian Kempke 2022-06-07 10:16 ` [PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version Nils-Christian Kempke @ 2022-06-07 10:16 ` Nils-Christian Kempke 2022-06-08 14:18 ` Andrew Burgess 1 sibling, 1 reply; 18+ messages in thread From: Nils-Christian Kempke @ 2022-06-07 10:16 UTC (permalink / raw) To: gdb-patches; +Cc: aburgess, JiniSusan.George, tdevries, 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis 2022-06-07 10:16 ` [PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis Nils-Christian Kempke @ 2022-06-08 14:18 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess 0 siblings, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 14:18 UTC (permalink / raw) To: Nils-Christian Kempke, gdb-patches Cc: JiniSusan.George, tdevries, Nils-Christian Kempke 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/5] Solve caching problems with compiler_info 2022-06-08 14:18 ` Andrew Burgess @ 2022-06-08 14:22 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 1/5] gdb/testsuite: use test_compiler_info in gcc_major_version Andrew Burgess ` (5 more replies) 0 siblings, 6 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/5] gdb/testsuite: use test_compiler_info in gcc_major_version 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess @ 2022-06-08 14:22 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp Andrew Burgess ` (4 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 14:22 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess 2022-06-08 14:22 ` [PATCH 1/5] gdb/testsuite: use test_compiler_info in gcc_major_version Andrew Burgess @ 2022-06-08 14:22 ` Andrew Burgess 2022-06-09 13:39 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info Andrew Burgess ` (3 subsequent siblings) 5 siblings, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp 2022-06-08 14:22 ` [PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp Andrew Burgess @ 2022-06-09 13:39 ` Andrew Burgess 0 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-09 13:39 UTC (permalink / raw) To: gdb-patches 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} { ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess 2022-06-08 14:22 ` [PATCH 1/5] gdb/testsuite: use test_compiler_info in gcc_major_version Andrew Burgess 2022-06-08 14:22 ` [PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp Andrew Burgess @ 2022-06-08 14:22 ` Andrew Burgess 2022-06-08 16:06 ` Kempke, Nils-Christian 2022-06-08 14:22 ` [PATCH 4/5] gdb/testsuite: handle errors better in test_compiler_info Andrew Burgess ` (2 subsequent siblings) 5 siblings, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info 2022-06-08 14:22 ` [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info Andrew Burgess @ 2022-06-08 16:06 ` Kempke, Nils-Christian 2022-06-09 13:40 ` Andrew Burgess 0 siblings, 1 reply; 18+ messages in thread From: Kempke, Nils-Christian @ 2022-06-08 16:06 UTC (permalink / raw) To: Andrew Burgess, gdb-patches > -----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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info 2022-06-08 16:06 ` Kempke, Nils-Christian @ 2022-06-09 13:40 ` Andrew Burgess 0 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-09 13:40 UTC (permalink / raw) To: Kempke, Nils-Christian, gdb-patches "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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/5] gdb/testsuite: handle errors better in test_compiler_info 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess ` (2 preceding siblings ...) 2022-06-08 14:22 ` [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info Andrew Burgess @ 2022-06-08 14:22 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 5/5] gdb/testsuite: solve problems with compiler_info caching Andrew Burgess 2022-06-08 16:21 ` [PATCH 0/5] Solve caching problems with compiler_info Kempke, Nils-Christian 5 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/5] gdb/testsuite: solve problems with compiler_info caching 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess ` (3 preceding siblings ...) 2022-06-08 14:22 ` [PATCH 4/5] gdb/testsuite: handle errors better in test_compiler_info Andrew Burgess @ 2022-06-08 14:22 ` Andrew Burgess 2022-06-08 16:21 ` [PATCH 0/5] Solve caching problems with compiler_info Kempke, Nils-Christian 5 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-08 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 0/5] Solve caching problems with compiler_info 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess ` (4 preceding siblings ...) 2022-06-08 14:22 ` [PATCH 5/5] gdb/testsuite: solve problems with compiler_info caching Andrew Burgess @ 2022-06-08 16:21 ` Kempke, Nils-Christian 2022-06-09 13:42 ` Andrew Burgess 5 siblings, 1 reply; 18+ messages in thread From: Kempke, Nils-Christian @ 2022-06-08 16:21 UTC (permalink / raw) To: Andrew Burgess, gdb-patches > -----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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 0/5] Solve caching problems with compiler_info 2022-06-08 16:21 ` [PATCH 0/5] Solve caching problems with compiler_info Kempke, Nils-Christian @ 2022-06-09 13:42 ` Andrew Burgess 2022-06-09 16:25 ` Tom de Vries 0 siblings, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2022-06-09 13:42 UTC (permalink / raw) To: Kempke, Nils-Christian, gdb-patches "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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/5] Solve caching problems with compiler_info 2022-06-09 13:42 ` Andrew Burgess @ 2022-06-09 16:25 ` Tom de Vries 2022-06-10 11:03 ` Andrew Burgess 0 siblings, 1 reply; 18+ messages in thread From: Tom de Vries @ 2022-06-09 16:25 UTC (permalink / raw) To: Andrew Burgess, Kempke, Nils-Christian, gdb-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/5] Solve caching problems with compiler_info 2022-06-09 16:25 ` Tom de Vries @ 2022-06-10 11:03 ` Andrew Burgess 0 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2022-06-10 11:03 UTC (permalink / raw) To: Tom de Vries, Kempke, Nils-Christian, gdb-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-06-10 11:03 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-07 10:16 [PATCH v2 0/2] Fix regressions introduced by Fortran compiler identification series Nils-Christian Kempke 2022-06-07 10:16 ` [PATCH v2 1/2] gdb/testsuite: use test_compiler_info in gcc_major_version Nils-Christian Kempke 2022-06-08 9:50 ` Andrew Burgess 2022-06-07 10:16 ` [PATCH v2 2/2] gdb/testsuite: cache compiler_info on a per language basis Nils-Christian Kempke 2022-06-08 14:18 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 0/5] Solve caching problems with compiler_info Andrew Burgess 2022-06-08 14:22 ` [PATCH 1/5] gdb/testsuite: use test_compiler_info in gcc_major_version Andrew Burgess 2022-06-08 14:22 ` [PATCH 2/5] gdb/testsuite: remove unneeded get_compiler_info calls from gdb.exp Andrew Burgess 2022-06-09 13:39 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 3/5] gdb/testsuite: make 'c' default language for get/test compiler info Andrew Burgess 2022-06-08 16:06 ` Kempke, Nils-Christian 2022-06-09 13:40 ` Andrew Burgess 2022-06-08 14:22 ` [PATCH 4/5] gdb/testsuite: handle errors better in test_compiler_info Andrew Burgess 2022-06-08 14:22 ` [PATCH 5/5] gdb/testsuite: solve problems with compiler_info caching Andrew Burgess 2022-06-08 16:21 ` [PATCH 0/5] Solve caching problems with compiler_info Kempke, Nils-Christian 2022-06-09 13:42 ` Andrew Burgess 2022-06-09 16:25 ` Tom de Vries 2022-06-10 11:03 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).