[PATCH] icc: allow code path for newer versions of icc.
Simon Marchi
simon.marchi@polymtl.ca
Sat Sep 16 13:49:00 GMT 2017
On 2017-09-06 10:46, Walfred Tedeschi wrote:
> Patch adds a version checkin for workaround an icc problem.
> Icc problem was fixed in version 14, and gdb code has to
> reflect the fix.
> This patch contains a parser for the icc string version and conditional
> workaround execution. Adds also gdb self tests for the dwarf
> producers.
Hi Walfred,
Thanks for the patch, a few comments below.
>
> 2017-06-28 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> gdb/ChangeLog:
> * dwarf2read.c (dwarf2_cu): Remove field producer_is_icc and added
> producer_is_icc_lt_14.
> (producer_is_icc_lt_14): New function.
> (check_producer): Added code for checking version of icc.
> (producer_is_icc): Removed.
Use the present tense (Added -> Add, Removed -> Remove).
> (read_structure_type): Add a check for the later version of icc
> where the issue was still not fixed.
> (dwarf_producer_test): Add new unit test.
> (_initialize_dwarf2_read): Register the unit test.
The spaces should be replaced with a tab in that last line.
> * utils.c (producer_is_intel): New function added using same
> signature as producer_is_gcc.
You can just say "New function.".
> * utils.h (producer_is_intel): Declaration of a new function.
>
> ---
> gdb/dwarf2read.c | 103
> +++++++++++++++++++++++++++++++++++++++++++++++--------
> gdb/utils.c | 66 +++++++++++++++++++++++++++++++++++
> gdb/utils.h | 3 ++
> 3 files changed, 157 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 6678b33..4aa3b3e 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -604,7 +604,7 @@ struct dwarf2_cu
> unsigned int checked_producer : 1;
> unsigned int producer_is_gxx_lt_4_6 : 1;
> unsigned int producer_is_gcc_lt_4_3 : 1;
> - unsigned int producer_is_icc : 1;
> + unsigned int producer_is_icc_lt_14 : 1;
>
> /* When set, the file that we're processing is known to have
> debugging info for C++ namespaces. GCC 3.3.x did not produce
> @@ -9331,6 +9331,19 @@ read_import_statement (struct die_info *die,
> struct dwarf2_cu *cu)
> do_cleanups (cleanups);
> }
>
> +/* For versions older than 14 ICC did not output the required
> DW_AT_declaration
> + on incomplete types, but gives them a size of zero. Starting on
> version 14
> + ICC is compatible with GCC. */
> +
> +static int
> +producer_is_icc_lt_14 (struct dwarf2_cu *cu)
> +{
> + if (!cu->checked_producer)
> + check_producer (cu);
> +
> + return cu->producer_is_icc_lt_14;
> +}
> +
> /* Check for possibly missing DW_AT_comp_dir with relative .debug_line
> directory paths. GCC SVN r127613 (new option -fdebug-prefix-map)
> fixed
> this, it was first present in GCC release 4.3.0. */
> @@ -12818,6 +12831,71 @@ dwarf2_record_block_ranges (struct die_info
> *die, struct block *block,
> }
> }
>
> +#if defined GDB_SELF_TEST
> +#include "selftest.h"
You can put this at the top of the file with the other includes.
> +
> +namespace selftests {
> +namespace gdbserver {
Change that namespace name. namespace dwarf2read would make sense I
think.
Thanks for the test btw!
> +static void
> +dwarf_producer_test ()
> +{
> + int major = 0, minor = 0;
> +
> + const char *extern_f_14_1 = "Intel(R) Fortran Intel(R) 64 Compiler \
> + XE for applications running on Intel(R) 64, Version \
> + 14.0.1.074 Build 20130716";
Watch out, the spaces you use to indent the last two lines will be
included in the string, which is probably not what you want. Try this
instead (hopefully this doesn't get wrapped by my email client):
const char *extern_f_14_1 = "Intel(R) Fortran Intel(R) 64 Compiler XE
for \
applications running on Intel(R) 64, Version 14.0.1.074 Build 20130716";
> + int retval = producer_is_intel (extern_f_14_1, &major, &minor);
> + SELF_CHECK (retval == 1 && major == 14 && minor == 1);
> + retval = producer_is_gcc (extern_f_14_1, &major, &minor);
> + SELF_CHECK (retval == 0);
> +
> + const char *intern_f_14 = "Intel(R) Fortran Intel(R) 64 Compiler \
> + XE for applications running on Intel(R) 64, Version \
> + 14.0";
> + major = 0;
> + minor = 0;
> + retval = producer_is_intel (intern_f_14, &major, &minor);
> + SELF_CHECK (retval == 1 && major == 14 && minor == 0);
> + retval = producer_is_gcc (intern_f_14, &major, &minor);
> + SELF_CHECK (retval == 0);
> +
> + const char *intern_c_14 = "Intel(R) C++ Intel(R) 64 Compiler \
> + XE for applications running on Intel(R) 64, Version \
> + 14.0";
> + major = 0;
> + minor = 0;
> + retval = producer_is_intel (intern_c_14, &major, &minor);
> + SELF_CHECK (retval == 1 && major == 14 && minor == 0);
> + retval = producer_is_gcc (intern_c_14, &major, &minor);
> + SELF_CHECK (retval == 0);
> +
> + const char *intern_c_18 = "Intel(R) C++ Intel(R) 64 Compiler \
> + for applications running on Intel(R) 64, Version 18.0 Beta";
> + major = 0;
> + minor = 0;
> + retval = producer_is_intel (intern_c_18, &major, &minor);
> + SELF_CHECK (retval == 1 && major == 18 && minor == 0);
> +
> + const char *gnu = "GNU C 4.7.2";
> + major = 0;
> + minor = 0;
> + retval = producer_is_intel (gnu, &major, &minor);
> + SELF_CHECK (retval == 0);
> + retval = producer_is_gcc (gnu, &major, &minor);
> + SELF_CHECK (retval == 1 && major == 4 && minor == 7);
> +
> + const char *gnu_exp ="GNU C++14 5.0.0 20150123 (experimental)";
> + major = 0;
> + minor = 0;
> + retval = producer_is_intel (gnu_exp, &major, &minor);
> + SELF_CHECK (retval == 0);
> + retval = producer_is_gcc (gnu_exp, &major, &minor);
> + SELF_CHECK (retval == 1 && major == 5 && minor == 0);
> +}
> +}
> +}
> +#endif
> +
> /* Check whether the producer field indicates either of GCC < 4.6, or
> the
> Intel C/C++ compiler, and cache the result in CU. */
>
> @@ -12842,8 +12920,10 @@ check_producer (struct dwarf2_cu *cu)
> cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor <
> 6);
> cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor <
> 3);
> }
> - else if (startswith (cu->producer, "Intel(R) C"))
> - cu->producer_is_icc = 1;
> + else if (producer_is_intel (cu->producer, &major, &minor))
> + {
> + cu->producer_is_icc_lt_14 = major < 14 ;
> + }
Remove the curly braces and the space before the semi-colon.
> else
> {
> /* For other non-GCC compilers, expect their behavior is DWARF
> version
> @@ -13584,17 +13664,6 @@ quirk_gcc_member_function_pointer (struct
> type *type, struct objfile *objfile)
> smash_to_methodptr_type (type, new_type);
> }
>
> -/* Return non-zero if the CU's PRODUCER string matches the Intel C/C++
> compiler
> - (icc). */
> -
> -static int
> -producer_is_icc (struct dwarf2_cu *cu)
> -{
> - if (!cu->checked_producer)
> - check_producer (cu);
> -
> - return cu->producer_is_icc;
> -}
>
> /* Called when we find the DIE that starts a structure or union scope
> (definition) to create a type for the structure or union. Fill in
> @@ -13700,7 +13769,7 @@ read_structure_type (struct die_info *die,
> struct dwarf2_cu *cu)
> TYPE_LENGTH (type) = 0;
> }
>
> - if (producer_is_icc (cu) && (TYPE_LENGTH (type) == 0))
> + if (producer_is_icc_lt_14 (cu) && (TYPE_LENGTH (type) == 0))
> {
> /* ICC does not output the required DW_AT_declaration
> on incomplete types, but gives them a size of zero. */
> @@ -24207,4 +24276,8 @@ Usage: save gdb-index DIRECTORY"),
> &dwarf2_block_frame_base_locexpr_funcs);
> dwarf2_loclist_block_index = register_symbol_block_impl (LOC_BLOCK,
> &dwarf2_block_frame_base_loclist_funcs);
> +
> +#if defined GDB_SELF_TEST
> + register_self_test (selftests::gdbserver::dwarf_producer_test);
> +#endif
Just a heads up, you'll have to modify this to give the test a name. I
suggest "dwarf-producer".
> }
> diff --git a/gdb/utils.c b/gdb/utils.c
> index af50cf0..4432318 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3036,6 +3036,72 @@ producer_is_gcc (const char *producer, int
> *major, int *minor)
> return 0;
> }
>
> +/* Returns nonzero if the given PRODUCER string is Intel or zero
> + otherwise. Sets the MAJOR and MINOR versions when not NULL.
> +
> + Internal and external versions have to be taken into account.
> + Before a public release string for the PRODUCER is slightly
> + different than the public one. Internal releases have mainly
> + 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.
The syntax is a bit strange. Suggested wording:
Internal and external 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 ....". */
> +
> +int
> +producer_is_intel (const char *producer, int *major, int *minor)
The return value should be bool, and the code should use true/false
instead of 1/0. The comment above should also be updated.
> +{
> + if (producer == NULL || !startswith (producer, "Intel(R)"))
> + return 0;
> +
> +/* Preparing the used fields. */
> + int maj, min;
> + if (major == NULL)
> + major = &maj;
> + if (minor == NULL)
> + minor = &min;
> +
> + *minor = 0;
> + *major = 0;
> +
> + /* Consumes the string till a "Version" is found. */
> + const char *cs = strstr(producer, "Version");
Missing space after strstr.
> +
> + while (*cs && !isspace (*cs))
> + cs++;
I suggest using skip_to_space (from common-utils.c).
> + if (*cs && isspace (*cs))
> + cs++;
That could be skip_spaces (from common-utils.c as well).
Actually, instead of doing this, you could also include "Version " in
the sscanf format string below. You wouldn't need to skip anything.
> +
> + int intermediate = 0;
> + int nof = sscanf (cs, "%d.%d.%d.%*d", major, &intermediate, minor);
> +
> + /* Internal versions are represented only as MAJOR.MINOR, whereas
Do you mean "where" instead of "whereas"?
> + minor is usually 0.
> + Public versions have 4 fields as described with the command
> above. */
> + if (nof == 3)
> + return 1;
> +
> + if (nof == 2)
> + {
> + *minor = intermediate;
> + return 1;
> + }
> +
> + /* Not recognized as Intel, let user know. */
> + warning ("Intel producer: version not recognized.");
> + return 0;
> +}
> +
> /* Helper for make_cleanup_free_char_ptr_vec. */
>
> static void
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 3ceefc1..1d8caae 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -453,6 +453,9 @@ extern pid_t wait_to_die_with_timeout (pid_t pid,
> int *status, int timeout);
> extern int producer_is_gcc_ge_4 (const char *producer);
> extern int producer_is_gcc (const char *producer, int *major, int
> *minor);
>
> +/* See documentation in the utils.c file. */
> +extern int producer_is_intel (const char *producer, int *major, int
> *minor);
The documentation of the function should be done the other way, the
actual doc in the header file, and
/* See utils.h. */
in utils.c.
I think it would make more sense to call it "producer_is_icc", since we
talk about the compiler and not the company.
> +
> extern int myread (int, char *, int);
>
> /* Ensure that V is aligned to an N byte boundary (B's assumed to be a
Thanks,
Simon
More information about the Gdb-patches
mailing list