[V3] 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 add
	producer_is_icc_lt_14.
	(producer_is_icc_lt_14): New function.
	(check_producer): Add code for checking version of icc.
	(producer_is_icc): Move to dwarf2utils.
	(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.
	* producer.c (producer_is_icc, _initialize_producer):
	New function.
	* producer.h (producer_is_icc): New function.


# Conflicts:
#	gdb/producer.c
---
 gdb/dwarf2read.c |  32 ++++++++-------
 gdb/producer.c   | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/producer.h   |  23 +++++++++++
 3 files changed, 161 insertions(+), 15 deletions(-)
  

On 09/25/2017 03:51 PM, Walfred Tedeschi wrote:
> Patch adds a version checkin for workaround an icc problem.

"version check", I think.

> 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 add
> 	producer_is_icc_lt_14.
> 	(producer_is_icc_lt_14): New function.
> 	(check_producer): Add code for checking version of icc.
> 	(producer_is_icc): Move to dwarf2utils.

s/dwarf2utils/producer.c/ 

> 	(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.

Wrong file, wrong function name.

> 	(_initialize_dwarf2_read): Register the unit test.

Ditto.

> 	* producer.c (producer_is_icc, _initialize_producer):
> 	New function.
> 	* producer.h (producer_is_icc): New function.
> 
> 
> # Conflicts:
> #	gdb/producer.c

This shouldn't be here.

> ---
>  gdb/dwarf2read.c |  32 ++++++++-------
>  gdb/producer.c   | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/producer.h   |  23 +++++++++++
>  3 files changed, 161 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 4083c63..d226c67 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -603,7 +603,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
> @@ -9348,6 +9348,19 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
>  		       &objfile->objfile_obstack);
>  }
>  
> +/* 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.  */
> @@ -12853,8 +12866,8 @@ 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_icc (cu->producer, &major, &minor))
> +      cu->producer_is_icc_lt_14 = major < 14;

Still incorrect formatting.  Watch out for leading whitespaces.


>    else
>      {
>        /* For other non-GCC compilers, expect their behavior is DWARF version
> @@ -13595,17 +13608,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
> @@ -13711,7 +13713,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.  */
> diff --git a/gdb/producer.c b/gdb/producer.c
> index 5417e52..7d79c83 100644
> --- a/gdb/producer.c
> +++ b/gdb/producer.c
> @@ -76,3 +76,124 @@ producer_is_gcc (const char *producer, int *major, int *minor)
>    return 0;
>  }
>  
> +
> +/* See documentation in the producer.h file.  */
> +
> +bool
> +producer_is_icc (const char *producer, int *major, int *minor)
> +{
> +  if (producer == NULL || !startswith (producer, "Intel(R)"))
> +    return 0;

The function returns bool, so this should be return false.

> +
> +/* 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");

If "Version" isn't found in the string for reason,
CS is left NULL, and we'll crash below.  We should have
a unit test for such a string.

> +  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;
> +    }
> +

TRUE -> true.

> +  static bool warning_printed = FALSE;
> +  /* Not recognized as Intel, let user know.  */
> +  if (warning_printed == FALSE)

if (!warning_printed)

> +    {
> +      warning (_("Could not recognize version of Intel Compiler in:%s"), producer);

Line too long.  Also, I think a space after ":" would be nice.

> +      warning_printed = TRUE;
> +    }
> +  return FALSE;

FALSE -> false.


> +}
> +
> +#if defined GDB_SELF_TEST
> +namespace selftests {
> +namespace producer {
> +
> +static void
> +producer_parser ()
> +{
> +  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";
> +
> +  bool retval = producer_is_icc (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_icc (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";

Should be 

   static const char intern_c_14[] = ...

etc. for the other producer strings.

> +  major = 0;
> +  minor = 0;
> +  retval = producer_is_icc (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_icc (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_icc (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_icc (gnu_exp, &major, &minor);
> +  SELF_CHECK (retval == 0);
> +  retval = producer_is_gcc (gnu_exp, &major, &minor);
> +  SELF_CHECK (retval == 1 && major == 5 && minor == 0);

I think these test would be nicer to read if each test
was wrapped in its own scope.  Also, with that, we can
easily have independent major/minor variables in each
scope, avoiding potential accidental local reuse bugs.

> +}
> +}
> +}
> +#endif
> +
> +void
> +_initialize_dwarf2utils ()

Should have been renamed...

> +{
> +#if defined GDB_SELF_TEST
> +  selftests::register_test ("producer-parser", selftests::producer::producer_parser);

Line too long.

> +#endif
> +}
> diff --git a/gdb/producer.h b/gdb/producer.h
> index cd27d8f..06c8058 100644
> --- a/gdb/producer.h
> +++ b/gdb/producer.h
> @@ -31,4 +31,27 @@ extern int producer_is_gcc_ge_4 (const char *producer);
>     is NULL or it isn't GCC.  */
>  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 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\

Spurious \ at line end.

Since I applied the patch locally, I went ahead and did the
changes above, and pushed it in.
I clarified (or tried to clarify) the commit logs while at it.
I'll send it to the list in a moment.  Let me know if I
messed something up.

Thanks,
Pedro Alves