Re: [PATCH] icc: allow code path for newer versions of icc.

Tedeschi, Walfred
2017-09-18T14:17:20Z

Hello Simon,

Thanks for your review!

On 09/16/2017 03:49 PM, Simon Marchi wrote:
> 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).
I will change, thanks!
>
>> (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.
I will double check, sometimes there is also problems with the e-mail.
>
>> * 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.
>
Ok! I will also change it!
>> +
>> +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";
>
Thank you! I will also do it!
>> + 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.
>
Accepted, Thanks!
>> +
>> + 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.
>
Ok! I think i have copied from the GCC code. Nevertheless, your
reasoning makes sense.
>> +{
>> + 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.
>

Sorry!
>> +
>> + while (*cs && !isspace (*cs))
>> + cs++;
>
> I suggest using skip_to_space (from common-utils.c).
>
I will change it!
>> + 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.
>
Yes, I will try that! Thank you!
>> +
>> + 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.
>
Indeed!

>
>> +
>> extern int myread (int, char *, int);
>>
>> /* Ensure that V is aligned to an N byte boundary (B's assumed to be a
>
> Thanks,
>
> Simon

Thank you!

Fred

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928