icc: allow code path for newer versions of icc.

  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.

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.
	(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.
	* utils.c (producer_is_intel): New function added using same
	signature as producer_is_gcc.
	* 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(-)
  

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
  

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
  

Hi Pedro and Simon,


Thanks again for the review!


On 09/18/2017 06:23 PM, Simon Marchi wrote:
> On 2017-09-18 17:34, Pedro Alves wrote:
>> Hi,
>>
>> I read the patch and I found a few nits to pick.
>> See below.
>>
>> On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
>>> 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;
>>
>> ...
>>
>>>
>>> +#if defined GDB_SELF_TEST
>>> +#include "selftest.h"
>>> +
>>> +namespace selftests {
>>> +namespace gdbserver {
>>> +static void
>>> +dwarf_producer_test ()
>>
>> Simon already pointed at the gdbserver namespace.  I'd like
>> to add that it looks odd to me to define the actual
>> functionality in utils.c and define the corresponding self
>> tests in dwarf2read.c.  Why not put the tests in utils.c as well?
>>
>> BTW, I'd support moving all these producer checks to
>> a separate file (maybe producer.c), instead of putting
>> it all in the utils.c kitchen sync, though that's a
>> preexisting issue.
>
> I agree about that.  I almost wrote that this function didn't really 
> belong in utils.c, until I saw that it's where other similar functions 
> were already.
>
> Simon

I am considering sending then two patches:
1. Add things in utils.c
2. move all producers to a new producers.h/c file.

Would this be ok?

Thanks again,
/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
  

Hi,

I read the patch and I found a few nits to pick.
See below.

On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
> 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;

...

>  
> +#if defined GDB_SELF_TEST
> +#include "selftest.h"
> +
> +namespace selftests {
> +namespace gdbserver {
> +static void
> +dwarf_producer_test ()

Simon already pointed at the gdbserver namespace.  I'd like
to add that it looks odd to me to define the actual
functionality in utils.c and define the corresponding self
tests in dwarf2read.c.  Why not put the tests in utils.c as well?

BTW, I'd support moving all these producer checks to
a separate file (maybe producer.c), instead of putting
it all in the utils.c kitchen sync, though that's a
preexisting issue.

> +  if (producer == NULL || !startswith (producer, "Intel(R)"))
> +    return 0;
> +
> +/* Preparing the used fields.  */
> +  int maj, min;

Missing leading double-space in comment line.

> +  if (major == NULL)
> +    major = &maj;
> +  if (minor == NULL)
> +    minor = &min;

> +  /* Not recognized as Intel, let user know.  */
> +  warning ("Intel producer: version not recognized.");
> +  return 0;
> +}

If this ever triggers, won't the user get an annoying
flood of such warnings?  If you hack your local copy to reach
the warning, what do you see?

I also wonder whether we should print a more-friend clearer
message, "intel producer" may mean nothing to a user, and I'm
not sure what they could do with that.  Maybe at least print
the actual version string?

Thanks,
Pedro Alves
  

On 2017-09-18 17:34, Pedro Alves wrote:
> Hi,
> 
> I read the patch and I found a few nits to pick.
> See below.
> 
> On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
>> 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;
> 
> ...
> 
>> 
>> +#if defined GDB_SELF_TEST
>> +#include "selftest.h"
>> +
>> +namespace selftests {
>> +namespace gdbserver {
>> +static void
>> +dwarf_producer_test ()
> 
> Simon already pointed at the gdbserver namespace.  I'd like
> to add that it looks odd to me to define the actual
> functionality in utils.c and define the corresponding self
> tests in dwarf2read.c.  Why not put the tests in utils.c as well?
> 
> BTW, I'd support moving all these producer checks to
> a separate file (maybe producer.c), instead of putting
> it all in the utils.c kitchen sync, though that's a
> preexisting issue.

I agree about that.  I almost wrote that this function didn't really 
belong in utils.c, until I saw that it's where other similar functions 
were already.

Simon
  

On 2017-09-18 17:31, Tedeschi, Walfred wrote:
> I am considering sending then two patches:
> 1. Add things in utils.c
> 2. move all producers to a new producers.h/c file.
> 
> Would this be ok?

(I am just speaking for myself)

I usually prefer the opposite, cleanup first then add new features 
(because sometimes the cleanup is promised and never comes :)), but 
really I'm fine with both.  The fact that you are ready to do some 
cleanup is appreciated.

Simon
  

On 09/18/2017 09:13 PM, Simon Marchi wrote:
> On 2017-09-18 17:31, Tedeschi, Walfred wrote:
>> I am considering sending then two patches:
>> 1. Add things in utils.c
>> 2. move all producers to a new producers.h/c file.
>>
>> Would this be ok?
>
> (I am just speaking for myself)
>
> I usually prefer the opposite, cleanup first then add new features 
> (because sometimes the cleanup is promised and never comes :)), but 
> really I'm fine with both.  The fact that you are ready to do some 
> cleanup is appreciated.
>
> Simon
Hello Simon and Pedro,

I have prepared a cleaning patch first and another one with the icc 
version check.

The changes are in:
users/wtedesch/icc_version

Comments are welcome!

Thanks and regards,
/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
  

On 09/20/2017 03:17 PM, Tedeschi, Walfred wrote:

> Hello Simon and Pedro,
> 
> I have prepared a cleaning patch first and another one with the icc
> version check.
> 
> The changes are in:
> users/wtedesch/icc_version
> 
> Comments are welcome!

Thanks!

Speaking for myself, while a git branch is handy for trying
locally (and I tend to push changes to branches too to help
others for larger series, in addition to posting on the list),
I still find it easier to reply/review to patches sent via email,
because that allows conveniently quoting patches.

But I went ahead and took a quick look.  I have to say that
don't care for the "dwarf2utils" name much BTW.  My issue with
"utils.h|c" is that "utils" is a kitchen sync name; almost anything
can be called an "utility".  And "dwarf2utils" has almost the same
issue, in my view.  Note that I had suggested producers.h/c without
a "dwarf" qualification because the producers string is exposed
in generic code, through symtab.h:compunit_symtab etc.
(even though the producers info is currently only sourced
from DWARF info).

As for the the rest, it seems like several previous comments
haven't been addressed yet, so I'll wait for a repost.

Thanks,
Pedro Alves
  

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves

Sent: Wednesday, September 20, 2017 5:46 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>
Cc: qiyaoltc@gmail.com; gdb-patches@sourceware.org
Subject: Re: [PATCH] icc: allow code path for newer versions of icc.

On 09/20/2017 03:17 PM, Tedeschi, Walfred wrote:

> Hello Simon and Pedro,

> 

> I have prepared a cleaning patch first and another one with the icc 

> version check.

> 

> The changes are in:

> users/wtedesch/icc_version

> 

> Comments are welcome!


Thanks!

Speaking for myself, while a git branch is handy for trying locally (and I tend to push changes to branches too to help others for larger series, in addition to posting on the list), I still find it easier to reply/review to patches sent via email, because that allows conveniently quoting patches.

But I went ahead and took a quick look.  I have to say that don't care for the "dwarf2utils" name much BTW.  My issue with "utils.h|c" is that "utils" is a kitchen sync name; almost anything can be called an "utility".  And "dwarf2utils" has almost the same issue, in my view.  Note that I had suggested producers.h/c without a "dwarf" qualification because the producers string is exposed in generic code, through symtab.h:compunit_symtab etc.
(even though the producers info is currently only sourced from DWARF info).

As for the the rest, it seems like several previous comments haven't been addressed yet, so I'll wait for a repost.

Thanks,
Pedro Alves

Pedro,

Thanks for your quick answer!

I will prepare new patches.

Thanks and regards,
/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
  

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves

Sent: Wednesday, September 20, 2017 5:46 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>
Cc: qiyaoltc@gmail.com; gdb-patches@sourceware.org
Subject: Re: [PATCH] icc: allow code path for newer versions of icc.

On 09/20/2017 03:17 PM, Tedeschi, Walfred wrote:

>> Hello Simon and Pedro,

>> 

>> I have prepared a cleaning patch first and another one with the icc 

>> version check.

>> 

>> The changes are in:

>> users/wtedesch/icc_version

>> 

>> Comments are welcome!

>

>Thanks!

>

>Speaking for myself, while a git branch is handy for trying locally (and I tend to push changes to branches too to help others for larger series, in >addition to posting on the list), I still find it easier to reply/review to patches sent via email, because that allows conveniently quoting patches. >

>

>But I went ahead and took a quick look.  I have to say that don't care for the "dwarf2utils" name much BTW.  My issue with "utils.h|c" is that >"utils" is a kitchen sync name; almost anything can be called an "utility".  And "dwarf2utils" has almost the same issue, in my view.  Note that I >had suggested producers.h/c without a "dwarf" qualification because the producers string is exposed in generic code, through >symtab.h:compunit_symtab etc.

> (even though the producers info is currently only sourced from DWARF info).

>

>As for the the rest, it seems like several previous comments haven't been addressed yet, so I'll wait for a repost. 

>

>Thanks,

>Pedro Alves


Hello Pedro,

Thanks for the quick reply!

My concern with the name was to create a file that could be used for more things, but not much more.
Do you consider a better approach to have a specialized file, having only the producers on it?

Best regards,
/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
  

On 09/21/2017 12:27 PM, Tedeschi, Walfred wrote:
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves

>> But I went ahead and took a quick look.  I have to say that don't care for the "dwarf2utils" name much BTW.  My issue with "utils.h|c" is that >"utils" is a kitchen sync name; almost anything can be called an "utility".  And "dwarf2utils" has almost the same issue, in my view.  Note that I >had suggested producers.h/c without a "dwarf" qualification because the producers string is exposed in generic code, through >symtab.h:compunit_symtab etc.
>> (even though the producers info is currently only sourced from DWARF info).

> My concern with the name was to create a file that could be used for more things, but not much more.
> Do you consider a better approach to have a specialized file, having only the producers on it?

Yes.

Thanks,
Pedro Alves