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

["Tedeschi, Walfred" <walfred.tedeschi@intel.com>]

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