| Message ID | 20210407151209.2261068-2-felix.willgerodt@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
* Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> [2021-04-07 17:12:08 +0200]: > The main goal of this patch is to get rid of a warning for the new Fortran > compiler: > > (gdb) b 9 > warning: Could not recognize version of Intel Compiler in: "Intel(R) Fortran 21.0-2087b" > Breakpoint 1 at 0x4048cf: file comp.f90, line 9. > > While trying to fix this I analyzed DW_AT_producer of all latest Intel > compilers for C, C++ and Fortran. They do no longer necessarily start with > "Intel (R)" nor do they follow the internal and external version number > scheme that the original patch for this check assumed. Some newer compilers > even contradict the "intermediate" digit in the old version scheme and have > the MINOR number as the second digit, even when having 3 or 4 digits overall. > > Therefore I rewrote the check to consider the first MAJOR.MINOR string found > as the version number. This might not be 100% correct for some older > internal compilers, but the only current user of this function is only > checking for the major version anyway. Hence this should be reliable enough > and extendable enough going forward. > > gdb/Changelog: > 2021-03-12 Felix Willgerodt <felix.willgerodt@intel.com> > > * producer.c: (producer_is_icc): Update for new version scheme. > (producer_parsing_tests): Update names and expected results. > * producer.h: (producer_is_icc): Update comment accordingly. LGTM. Thanks, Andrew > --- > gdb/producer.c | 51 ++++++++++++++++---------------------------------- > gdb/producer.h | 20 +------------------- > 2 files changed, 17 insertions(+), 54 deletions(-) > > diff --git a/gdb/producer.c b/gdb/producer.c > index fcfd7b9b807..1cda48c204a 100644 > --- a/gdb/producer.c > +++ b/gdb/producer.c > @@ -20,6 +20,7 @@ > #include "defs.h" > #include "producer.h" > #include "gdbsupport/selftest.h" > +#include <regex> > > /* See producer.h. */ > > @@ -78,50 +79,30 @@ producer_is_gcc (const char *producer, int *major, int *minor) > bool > producer_is_icc (const char *producer, int *major, int *minor) > { > - if (producer == NULL || !startswith (producer, "Intel(R)")) > + std::regex i_re ("Intel\\(R\\)"); > + std::cmatch i_m; > + if ((producer == nullptr) || !std::regex_search (producer, i_m, i_re)) > return false; > > /* Prepare the used fields. */ > int maj, min; > - if (major == NULL) > + if (major == nullptr) > major = &maj; > - if (minor == NULL) > + if (minor == nullptr) > minor = &min; > > *minor = 0; > *major = 0; > > - /* Consumes the string till a "Version" is found. */ > - const char *cs = strstr (producer, "Version"); > - if (cs != NULL) > - { > - cs = skip_to_space (cs); > - > - int intermediate = 0; > - int nof = sscanf (cs, "%d.%d.%d.%*d", major, &intermediate, minor); > - > - /* Internal versions are represented only as MAJOR.MINOR, where > - minor is usually 0. > - Public versions have 3 fields as described with the command > - above. */ > - if (nof == 3) > - return true; > - > - if (nof == 2) > - { > - *minor = intermediate; > - return true; > - } > - } > + std::regex re ("[0-9]+\\.[0-9]+"); > + std::cmatch version; > > - static bool warning_printed = false; > - /* Not recognized as Intel, let the user know. */ > - if (!warning_printed) > + if (std::regex_search (producer, version, re)) > { > - warning (_("Could not recognize version of Intel Compiler in: \"%s\""), > - producer); > - warning_printed = true; > + sscanf (version.str ().c_str (), "%d.%d", major, minor); > + return true; > } > + > return false; > } > > @@ -152,15 +133,15 @@ producer_parsing_tests () > } > > { > - static const char extern_f_14_1[] = "\ > + static const char extern_f_14_0[] = "\ > Intel(R) Fortran Intel(R) 64 Compiler XE for applications running on \ > Intel(R) 64, \ > Version 14.0.1.074 Build 20130716"; > > int major = 0, minor = 0; > - SELF_CHECK (producer_is_icc (extern_f_14_1, &major, &minor) > - && major == 14 && minor == 1); > - SELF_CHECK (!producer_is_gcc (extern_f_14_1, &major, &minor)); > + SELF_CHECK (producer_is_icc (extern_f_14_0, &major, &minor) > + && major == 14 && minor == 0); > + SELF_CHECK (!producer_is_gcc (extern_f_14_0, &major, &minor)); > } > > { > diff --git a/gdb/producer.h b/gdb/producer.h > index 15a60dc0ab5..9cfccd63371 100644 > --- a/gdb/producer.h > +++ b/gdb/producer.h > @@ -31,25 +31,7 @@ extern int producer_is_gcc_ge_4 (const char *producer); > extern int producer_is_gcc (const char *producer, int *major, int *minor); > > /* Returns true if the given PRODUCER string is Intel or false > - otherwise. Sets the MAJOR and MINOR versions when not NULL. > - > - Internal and external ICC versions have to be taken into account. > - PRODUCER strings for internal releases are slightly different than > - for public ones. Internal releases have a major release number and > - 0 as minor release. External releases have 4 fields, 3 of them are > - not 0 and only two are of interest, major and update. > - > - Examples are: > - > - Public release: > - "Intel(R) Fortran Intel(R) 64 Compiler XE for applications > - running on Intel(R) 64, Version 14.0.1.074 Build 20130716"; > - "Intel(R) C++ Intel(R) 64 Compiler XE for applications > - running on Intel(R) 64, Version 14.0.1.074 Build 20130716"; > - > - Internal releases: > - "Intel(R) C++ Intel(R) 64 Compiler for applications > - running on Intel(R) 64, Version 18.0 Beta ....". */ > + otherwise. Sets the MAJOR and MINOR versions when not NULL. */ > extern bool producer_is_icc (const char *producer, int *major, int *minor); > > /* Returns true if the given PRODUCER string is LLVM (clang/flang) or > -- > 2.25.4 > > 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 >
>>>>> ">" == Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes: >> + std::regex i_re ("Intel\\(R\\)"); The libstdc++ maintainer advised us against using std::regex. I think it would be better to use gdb_regex.h / compiled_regex instead. Tom
t65;5803;1c* Tom Tromey <tom@tromey.com> [2021-04-15 10:40:57 -0600]: > >>>>> ">" == Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes: > > >> + std::regex i_re ("Intel\\(R\\)"); > > The libstdc++ maintainer advised us against using std::regex. > I think it would be better to use gdb_regex.h / compiled_regex instead. Apologies for letting this through. How does this look? Thanks, Andrew --- commit f0d95706add8ec8e92bc2857a4279ea73fe93e1d Author: Andrew Burgess <andrew.burgess@embecosm.com> Date: Thu Apr 15 18:30:57 2021 +0100 gdb: use compiled_regex instead of std::regex In GDB we should be using compiled_regex instead of std::regex. Replace one use in producer.c. There should be no user visible changes after this commit. gdb/ChangeLog: * producer.c: Replace 'regex' include with 'gdb_regex.h'. (producer_is_icc): Replace use of std::regex with gdb's compiled_regex. diff --git a/gdb/producer.c b/gdb/producer.c index 591509fa85c..cdfd80d904c 100644 --- a/gdb/producer.c +++ b/gdb/producer.c @@ -20,7 +20,7 @@ #include "defs.h" #include "producer.h" #include "gdbsupport/selftest.h" -#include <regex> +#include "gdb_regex.h" /* See producer.h. */ @@ -91,9 +91,8 @@ producer_is_icc_ge_19 (const char *producer) bool producer_is_icc (const char *producer, int *major, int *minor) { - std::regex i_re ("Intel\\(R\\)"); - std::cmatch i_m; - if ((producer == nullptr) || !std::regex_search (producer, i_m, i_re)) + compiled_regex i_re ("Intel(R)", 0, "producer_is_icc"); + if (producer == nullptr || i_re.exec (producer, 0, nullptr, 0) != 0) return false; /* Prepare the used fields. */ @@ -106,12 +105,13 @@ producer_is_icc (const char *producer, int *major, int *minor) *minor = 0; *major = 0; - std::regex re ("[0-9]+\\.[0-9]+"); - std::cmatch version; - - if (std::regex_search (producer, version, re)) + compiled_regex re ("[0-9]+\\.[0-9]+", REG_EXTENDED, "producer_is_icc"); + regmatch_t version[1]; + if (re.exec (producer, ARRAY_SIZE (version), version, 0) == 0 + && version[0].rm_so != -1) { - sscanf (version.str ().c_str (), "%d.%d", major, minor); + const char *version_str = producer + version[0].rm_so; + sscanf (version_str, "%d.%d", major, minor); return true; }
Andrew> Apologies for letting this through. It's no biggie. Andrew> How does this look? Looks good to me, thanks. Tom
LGTM as well. Sorry for introducing this and thanks for the patch!
Can someone point me to a thread/website that has more background info? Is it because it was broken in older versions of libstdc++? Just out of curiosity, I am not suggesting any change.
Felix
-----Original Message-----
From: Gdb-patches <gdb-patches-bounces@sourceware.org> On Behalf Of Tom Tromey
Sent: Donnerstag, 15. April 2021 20:26
To: Andrew Burgess <andrew.burgess@embecosm.com>
Cc: Tom Tromey <tom@tromey.com>; Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
Subject: Re: [PATCH 1/2] gdb: Update producer check for Intel compilers.
Andrew> Apologies for letting this through.
It's no biggie.
Andrew> How does this look?
Looks good to me, thanks.
Tom
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
>> Can someone point me to a thread/website that has more background >> info? Is it because it was broken in older versions of libstdc++? >> Just out of curiosity, I am not suggesting any change. It was just a conversation on irc one day. The gist was that std::regex is not very good and my takeaway was that GDB should avoid it. I'm afraid I don't have any better details than that, I've forgotten them :( We could certainly revisit this if we felt the need. Tom
diff --git a/gdb/producer.c b/gdb/producer.c index fcfd7b9b807..1cda48c204a 100644 --- a/gdb/producer.c +++ b/gdb/producer.c @@ -20,6 +20,7 @@ #include "defs.h" #include "producer.h" #include "gdbsupport/selftest.h" +#include <regex> /* See producer.h. */ @@ -78,50 +79,30 @@ producer_is_gcc (const char *producer, int *major, int *minor) bool producer_is_icc (const char *producer, int *major, int *minor) { - if (producer == NULL || !startswith (producer, "Intel(R)")) + std::regex i_re ("Intel\\(R\\)"); + std::cmatch i_m; + if ((producer == nullptr) || !std::regex_search (producer, i_m, i_re)) return false; /* Prepare the used fields. */ int maj, min; - if (major == NULL) + if (major == nullptr) major = &maj; - if (minor == NULL) + if (minor == nullptr) minor = &min; *minor = 0; *major = 0; - /* Consumes the string till a "Version" is found. */ - const char *cs = strstr (producer, "Version"); - if (cs != NULL) - { - cs = skip_to_space (cs); - - int intermediate = 0; - int nof = sscanf (cs, "%d.%d.%d.%*d", major, &intermediate, minor); - - /* Internal versions are represented only as MAJOR.MINOR, where - minor is usually 0. - Public versions have 3 fields as described with the command - above. */ - if (nof == 3) - return true; - - if (nof == 2) - { - *minor = intermediate; - return true; - } - } + std::regex re ("[0-9]+\\.[0-9]+"); + std::cmatch version; - static bool warning_printed = false; - /* Not recognized as Intel, let the user know. */ - if (!warning_printed) + if (std::regex_search (producer, version, re)) { - warning (_("Could not recognize version of Intel Compiler in: \"%s\""), - producer); - warning_printed = true; + sscanf (version.str ().c_str (), "%d.%d", major, minor); + return true; } + return false; } @@ -152,15 +133,15 @@ producer_parsing_tests () } { - static const char extern_f_14_1[] = "\ + static const char extern_f_14_0[] = "\ Intel(R) Fortran Intel(R) 64 Compiler XE for applications running on \ Intel(R) 64, \ Version 14.0.1.074 Build 20130716"; int major = 0, minor = 0; - SELF_CHECK (producer_is_icc (extern_f_14_1, &major, &minor) - && major == 14 && minor == 1); - SELF_CHECK (!producer_is_gcc (extern_f_14_1, &major, &minor)); + SELF_CHECK (producer_is_icc (extern_f_14_0, &major, &minor) + && major == 14 && minor == 0); + SELF_CHECK (!producer_is_gcc (extern_f_14_0, &major, &minor)); } { diff --git a/gdb/producer.h b/gdb/producer.h index 15a60dc0ab5..9cfccd63371 100644 --- a/gdb/producer.h +++ b/gdb/producer.h @@ -31,25 +31,7 @@ extern int producer_is_gcc_ge_4 (const char *producer); extern int producer_is_gcc (const char *producer, int *major, int *minor); /* Returns true if the given PRODUCER string is Intel or false - otherwise. Sets the MAJOR and MINOR versions when not NULL. - - Internal and external ICC versions have to be taken into account. - PRODUCER strings for internal releases are slightly different than - for public ones. Internal releases have a major release number and - 0 as minor release. External releases have 4 fields, 3 of them are - not 0 and only two are of interest, major and update. - - Examples are: - - Public release: - "Intel(R) Fortran Intel(R) 64 Compiler XE for applications - running on Intel(R) 64, Version 14.0.1.074 Build 20130716"; - "Intel(R) C++ Intel(R) 64 Compiler XE for applications - running on Intel(R) 64, Version 14.0.1.074 Build 20130716"; - - Internal releases: - "Intel(R) C++ Intel(R) 64 Compiler for applications - running on Intel(R) 64, Version 18.0 Beta ....". */ + otherwise. Sets the MAJOR and MINOR versions when not NULL. */ extern bool producer_is_icc (const char *producer, int *major, int *minor); /* Returns true if the given PRODUCER string is LLVM (clang/flang) or
The main goal of this patch is to get rid of a warning for the new Fortran compiler: (gdb) b 9 warning: Could not recognize version of Intel Compiler in: "Intel(R) Fortran 21.0-2087b" Breakpoint 1 at 0x4048cf: file comp.f90, line 9. While trying to fix this I analyzed DW_AT_producer of all latest Intel compilers for C, C++ and Fortran. They do no longer necessarily start with "Intel (R)" nor do they follow the internal and external version number scheme that the original patch for this check assumed. Some newer compilers even contradict the "intermediate" digit in the old version scheme and have the MINOR number as the second digit, even when having 3 or 4 digits overall. Therefore I rewrote the check to consider the first MAJOR.MINOR string found as the version number. This might not be 100% correct for some older internal compilers, but the only current user of this function is only checking for the major version anyway. Hence this should be reliable enough and extendable enough going forward. gdb/Changelog: 2021-03-12 Felix Willgerodt <felix.willgerodt@intel.com> * producer.c: (producer_is_icc): Update for new version scheme. (producer_parsing_tests): Update names and expected results. * producer.h: (producer_is_icc): Update comment accordingly. --- gdb/producer.c | 51 ++++++++++++++++---------------------------------- gdb/producer.h | 20 +------------------- 2 files changed, 17 insertions(+), 54 deletions(-) -- 2.25.4 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