[patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

ICC does not set DW_AT_declaration on opaque structure declarations, so 
gdb will show such structures as "<no data fields>" unless executing 
code within the compilation unit which contains the complete declaration.

However, ICC does set DW_AT_byte_size to zero on opaque declarations.

This patch adds a check for DW_AT_byte_size == 0 with producer == ICC to 
allow gdb to resolve opaque structures in binaries which were built with 
ICC.

See http://sourceware.org/bugzilla/show_bug.cgi?id=13277 for more 
details, including an example.

Changelog:

2011-10-16  John Steele Scott <toojays@toojays.net>

     PR symtab/13277: Resolving opaque structures in ICC generated binaries.
     * symtab.c (producer_is_icc): New function.
     * symtab.h (producer_is_icc): Declare.
     * dwarf2read.c (read_structure_type): Set TYPE_STUB on structures
     with a byte size of zero, if the binary was produced by ICC.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -7636,6 +7636,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
      /* RealView does not output the required DW_AT_declaration
         on incomplete types.  */
      TYPE_STUB (type) = 1;
+  else if (attr != NULL&&  die->child == NULL&&  TYPE_LENGTH (type) == 0
+	&&  producer_is_icc (cu->producer))
+    /* ICC does not output the required DW_AT_declaration
+       on incomplete types, but gives them a size of zero.  */
+    TYPE_STUB (type) = 1;

    /* We need to add the type field to the die immediately so we don't
       infinitely recurse when dealing with pointers to the structure
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 9447bd9..ffaa035 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4806,6 +4806,20 @@ producer_is_realview (const char *producer)
    return 0;
  }

+int
+producer_is_icc (const char *producer)
+{
+  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";
+
+  if (producer == NULL)
+    return 0;
+
+  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
+    return 1;
+
+  return 0;
+}
+
  void
  _initialize_symtab (void)
  {
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 90a6fe4..987f199 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1302,6 +1302,10 @@ extern struct symtabs_and_lines expand_line_sal (struct symtab_and_line sal);
     compiler (armcc).  */
  int producer_is_realview (const char *producer);

+/* Return 1 if the supplied producer string matches the Intel C/C++
+   compiler (icc).  */
+int producer_is_icc (const char *producer);
+
  void fixup_section (struct general_symbol_info *ginfo,
  		    CORE_ADDR addr, struct objfile *objfile);

[patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

ICC does not set DW_AT_declaration on opaque structure declarations, so
gdb will show such structures as "<no data fields>" unless executing code
within the compilation unit which contains the complete declaration.

However, ICC does set DW_AT_byte_size to zero on opaque declarations.

This patch adds a check for DW_AT_byte_size == 0 with producer == ICC to
allow gdb to resolve opaque structures in binaries which were built with
ICC.

See http://sourceware.org/bugzilla/show_bug.cgi?id=13277 for more
details, including an example.

Changelog:

2011-10-16  John Steele Scott

     PR symtab/13277: Resolving opaque structures in ICC generated binaries.
     * symtab.c (producer_is_icc): New function.
     * symtab.h (producer_is_icc): Declare.
     * dwarf2read.c (read_structure_type): Set TYPE_STUB on structures
     with a byte size of zero, if the binary was produced by ICC.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -7636,6 +7636,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
      /* RealView does not output the required DW_AT_declaration
         on incomplete types.  */
      TYPE_STUB (type) = 1;
+  else if (attr != NULL&&  die->child == NULL&&  TYPE_LENGTH (type) == 0
+	&&  producer_is_icc (cu->producer))
+    /* ICC does not output the required DW_AT_declaration
+       on incomplete types, but gives them a size of zero.  */
+    TYPE_STUB (type) = 1;

    /* We need to add the type field to the die immediately so we don't
       infinitely recurse when dealing with pointers to the structure
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 9447bd9..ffaa035 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4806,6 +4806,20 @@ producer_is_realview (const char *producer)
    return 0;
  }

+int
+producer_is_icc (const char *producer)
+{
+  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";
+
+  if (producer == NULL)
+    return 0;
+
+  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
+    return 1;
+
+  return 0;
+}
+
  void
  _initialize_symtab (void)
  {
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 90a6fe4..987f199 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1302,6 +1302,10 @@ extern struct symtabs_and_lines expand_line_sal (struct symtab_and_line sal);
     compiler (armcc).  */
  int producer_is_realview (const char *producer);

+/* Return 1 if the supplied producer string matches the Intel C/C++
+   compiler (icc).  */
+int producer_is_icc (const char *producer);
+
  void fixup_section (struct general_symbol_info *ginfo,
  		    CORE_ADDR addr, struct objfile *objfile);

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Jan Kratochvil]

On Sun, 16 Oct 2011 07:44:28 +0200, John Steele Scott wrote:
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -7636,6 +7636,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
>      /* RealView does not output the required DW_AT_declaration
>         on incomplete types.  */
>      TYPE_STUB (type) = 1;
> +  else if (attr != NULL&&  die->child == NULL&&  TYPE_LENGTH (type) == 0
> +	&&  producer_is_icc (cu->producer))

the formatting should be:
  else if (attr != NULL && die->child == NULL && TYPE_LENGTH (type) == 0
	   && producer_is_icc (cu->producer))

> +    /* ICC does not output the required DW_AT_declaration
> +       on incomplete types, but gives them a size of zero.  */
> +    TYPE_STUB (type) = 1;
> 
>    /* We need to add the type field to the die immediately so we don't
>       infinitely recurse when dealing with pointers to the structure
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 9447bd9..ffaa035 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c

/* Return non-zero if PRODUCER is Intel C Compiler.  */

> +int
> +producer_is_icc (const char *producer)

This function should have some comment.  And it should be static in
dwarf2read.c itself as it is not used from any other source file now.

OK with those changes.


One could make a testcase by hand based for example on
gdb.dwarf2/dw2-struct-optimized-out.* .  I am not much fond of the `gcc -S'
output testcases.  But neither is required for a check-in.


Thanks,
Jan

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Jan Kratochvil]

On Wed, 19 Oct 2011 10:40:11 +0200, Jan Kratochvil wrote:
> OK with those changes.

I guess this change is not "legally significant" as you do not have completed
FSF copyright assignment.  I can check it in upon your reply.


Thanks,
Jan

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

On 19/10/11 19:31, Jan Kratochvil wrote:
> On Wed, 19 Oct 2011 10:40:11 +0200, Jan Kratochvil wrote:
>> OK with those changes.
> 
> I guess this change is not "legally significant" as you do not have completed
> FSF copyright assignment.  I can check it in upon your reply.

I emailed Tom Tromey off-list about papers last week, but haven't heard back yet. I'd like to find the time to add test cases for this at some stage, so it would probably be a good idea to start on the paperwork stuff.

cheers,

John

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Joel Brobecker]

> I emailed Tom Tromey off-list about papers last week, but haven't
> heard back yet. I'd like to find the time to add test cases for this
> at some stage, so it would probably be a good idea to start on the
> paperwork stuff.

No problem, I will email you the form now.

-- 
Joel

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Tom Tromey]

John> I emailed Tom Tromey off-list about papers last week, but haven't
John> heard back yet. I'd like to find the time to add test cases for this
John> at some stage, so it would probably be a good idea to start on the
John> paperwork stuff.

Oops, sorry about this.
I'm usually more on top of things.

Tom

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

Jan, 

Thanks for your review.

Unfortunately I have found that I needed to extend this patch, and probably
further work is required before this can be committed. While the previous patch
worked for my simple test case, it was failing with a larger program: from some
contexts the opaque structure type could still not be resolved.

It turns out that the bogus stubs which ICC creates were getting into the
psymbol table: one entry per compilation unit which referenced the type, in
addition to the psymbol from the compilation unit where the complete structure
was defined. basic_lookup_transparent_type will search through all symbol tables
which contain a matching type, but I didn't see a way to iterate through all
psymbols---so basic_lookup_transparent_type_quick will only examine a single
psymbol. This means that the opaque type will only be guaranteed to be resolved
if the transparent type is already in the symbol table. If the transparent type
is only in psymbols, it may or may not be resolved depending on order.

This updated patch tries harder to make sure that the stubs don't end up in the
psymbol table. This is done by adding a check in read_partial_die. Unfortunately
this breaks the dw2-ada-ffffffff testcase: the byte size attribute of ffffffff
is translated to zero in read_attribute_value, then the new check I've added in
read_partial_die translates this to "structure is a stub", so it never gets into
the psymbol table, thus the test breaks. A producer check would get around that,
but cu->producer is NULL in read_partial_die.

A different approach would be to do something like Ada's remove_extra_symbols,
which discards stubs which have the same name as a transparent type. That seems
like a more far-reaching change than what I'm trying to do here. I much prefer
to get these ICC-produced symbols looking like the GCC ones as early as
possible.

I appreciate your comments as to how this could be made suitable for commit.

cheers,

John

Patch follows:

    ICC does not set DW_AT_declaration on opaque structure types, but does set their
    DW_AT_byte_size to zero. This patch adds checks for this, allowing gdb to
    resolve opaque structures in binaries which were built with ICC.
    
    read_structure_type now contains a special case to recognize such structures and
    mark them as TYPE_STUB.
    
    The logic used in process_structure_scope to determine whether to add a
    structure to the symbol table has been extracted out into a new function
    die_is_incomplete_type. This new function includes the ICC/zero-byte-size check.
    
    Some fixup code is added to read_partial_die() to avoid adding bogus structure
    stubs to the partial symbol table. This was causing problems when looking up a
    type which was only in the psymbols, because lookup_symbol_aux_psymtabs (called
    via basic_lookup_transparent_type_quick) only returns the first psymbol found,
    which would not always be the complete type.
    
    Changelog:
    2011-10-23  John Steele Scott  <toojays@toojays.net>
    
    	PR symtab/13277: Resolving opaque structures in ICC generated binaries.
    	* dwarf2read.c (die_is_incomplete_type): New forward declaration.
    	(producer_is_icc): New function.
    	(read_structure_type): Set TYPE_STUB on structures
    	with a byte size of zero, if the binary was produced by ICC.
    	(process_structure_scope): Extract "external reference" check into
    	die_is_incomplete_type.
    	(die_is_incomplete_type): New function.
    	(read_partial_die): New variable byte_size, set to the value of a
    	DW_AT_byte_size attribute if we find one. If a structure has a
    	byte_size of zero, clear part_die->has_byte_size and set
    	part_die->is_declaration.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 99f67d9..fcd18b0 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1014,6 +1014,9 @@ static struct attribute *dwarf2_attr_no_follow (struct die_info *,
 static int dwarf2_flag_true_p (struct die_info *die, unsigned name,
                                struct dwarf2_cu *cu);
 
+static int die_is_incomplete_type (struct die_info *die,
+				   struct dwarf2_cu *cu);
+
 static int die_is_declaration (struct die_info *, struct dwarf2_cu *cu);
 
 static struct die_info *die_specification (struct die_info *die,
@@ -7526,6 +7529,23 @@ quirk_gcc_member_function_pointer (struct type *type, struct objfile *objfile)
   smash_to_methodptr_type (type, new_type);
 }
 
+/* Return non-zero if the supplied PRODUCER string matches the Intel C/C++
+   compiler (icc).  */
+
+static int
+producer_is_icc (const char *producer)
+{
+  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";
+
+  if (producer == NULL)
+    return 0;
+
+  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
+    return 1;
+
+  return 0;
+}
+
 /* 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
    the type's name and general properties; the members will not be
@@ -7636,6 +7656,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
     /* RealView does not output the required DW_AT_declaration
        on incomplete types.  */
     TYPE_STUB (type) = 1;
+  else if (attr != NULL && die->child == NULL && TYPE_LENGTH (type) == 0
+	   && producer_is_icc (cu->producer))
+    /* ICC does not output the required DW_AT_declaration
+       on incomplete types, but gives them a size of zero.  */
+    TYPE_STUB (type) = 1;
 
   /* We need to add the type field to the die immediately so we don't
      infinitely recurse when dealing with pointers to the structure
@@ -7852,11 +7877,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
       child_die = sibling_die (child_die);
     }
 
-  /* Do not consider external references.  According to the DWARF standard,
-     these DIEs are identified by the fact that they have no byte_size
-     attribute, and a declaration attribute.  */
-  if (dwarf2_attr (die, DW_AT_byte_size, cu) != NULL
-      || !die_is_declaration (die, cu))
+  if (!die_is_incomplete_type (die, cu))
     new_symbol (die, type, cu);
 }
 
@@ -9703,6 +9724,7 @@ read_partial_die (struct partial_die_info *part_die,
   struct attribute attr;
   int has_low_pc_attr = 0;
   int has_high_pc_attr = 0;
+  ULONGEST byte_size = 0;
 
   memset (part_die, 0, sizeof (struct partial_die_info));
 
@@ -9802,8 +9824,9 @@ read_partial_die (struct partial_die_info *part_die,
 	    part_die->sibling = buffer + dwarf2_get_ref_die_offset (&attr);
 	  break;
         case DW_AT_byte_size:
-          part_die->has_byte_size = 1;
-          break;
+	  part_die->has_byte_size = 1;
+	  byte_size = DW_UNSND (&attr);
+	  break;
 	case DW_AT_calling_convention:
 	  /* DWARF doesn't provide a way to identify a program's source-level
 	     entry point.  DW_AT_calling_convention attributes are only meant
@@ -9870,6 +9893,19 @@ read_partial_die (struct partial_die_info *part_die,
 	part_die->has_pc_info = 1;
     }
 
+  /* ICC ddoes not output DW_AT_declaration on incomplete types, instead giving
+     them a size of zero. Fix that up so that we treat this as an incomplete
+     type. We can't check the producer string here, since it may not be in the
+     cu yet. Ideally we would do this in fixup_partial_die(), but that would
+     mean re-reading the DW_AT_byte_size attribute.  */
+  if (part_die->has_byte_size && byte_size == 0
+      && part_die->tag == DW_TAG_structure_type)
+    {
+      /* TODO: Check if this is also required for union and class declarations. */
+      part_die->has_byte_size = 0;
+      part_die->is_declaration = 1;
+    }
+
   return info_ptr;
 }
 
@@ -10733,6 +10769,28 @@ die_is_declaration (struct die_info *die, struct dwarf2_cu *cu)
 	  && dwarf2_attr (die, DW_AT_specification, cu) == NULL);
 }
 
+/* Return non-zero if the DIE from the compilation unit CU is an incomplete
+   type.  "An incomplete structure, union or class type is represented by a
+   structure, union or class entry that does not have a byte size attribute and
+   that has a DW_AT_declaration attribute."  */
+static int
+die_is_incomplete_type (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu);
+
+  if (dwarf2_flag_true_p (die, DW_AT_declaration, cu) && attr == NULL)
+    return 1;
+  else if (die->tag == DW_TAG_structure_type && die->child == NULL
+	   && attr != NULL && DW_UNSND (attr) == 0
+	   && producer_is_icc (cu->producer))
+    /* ICC does not output the required DW_AT_declaration
+       on incomplete structure types, but gives them a size of zero.  */
+    /* TODO: Check if this is also required for union and class declarations. */
+    return 1;
+
+  return 0;
+}
+
 /* Return the die giving the specification for DIE, if there is
    one.  *SPEC_CU is the CU containing DIE on input, and the CU
    containing the return value on output.  If there is no

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Jan Kratochvil]

On Sun, 23 Oct 2011 12:16:53 +0200, John Steele Scott wrote:
> It turns out that the bogus stubs which ICC creates were getting into the
> psymbol table:

I see this was a very bad review from me, thanks for catching it.


> This updated patch tries harder to make sure that the stubs don't end up in the
> psymbol table. This is done by adding a check in read_partial_die. Unfortunately
> this breaks the dw2-ada-ffffffff testcase:

This work around should be checking DW_AT_producer.

There is one problem that -gdwarf-4 (-fdebug-types-section) .debug_types units
do not contain DW_AT_producer and GDB currently does not try to inherit it
from the referencing .debug_info sections.

But latest icc still does not support DW_AT_producer and I am not sure if it
would use the declaration form inside .debug_types anyway.

The dw2-ada-ffffffff compiler bug work around should be also checking
DW_AT_producer but at the place where it is now implemented it is not easy to
do.


> A producer check would get around that, but cu->producer is NULL in
> read_partial_die.

So this can be changed.  I find the cost negligible, not sure if anyone
disagrees.  The cu->producer initialization could be also just duplicated in
process_psymtab_comp_unit which would save the calls in load_partial_comp_unit
and read_signatured_type.


It would be really good to have a testcase for this ICC case.


> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1014,6 +1014,9 @@ static struct attribute *dwarf2_attr_no_follow (struct die_info *,
>  static int dwarf2_flag_true_p (struct die_info *die, unsigned name,
>                                 struct dwarf2_cu *cu);
>  
> +static int die_is_incomplete_type (struct die_info *die,
> +				   struct dwarf2_cu *cu);
> +

Rather move the function definition before its first use, it should be
possible in this case.


> @@ -9703,6 +9724,7 @@ read_partial_die (struct partial_die_info *part_die,
>    struct attribute attr;
>    int has_low_pc_attr = 0;
>    int has_high_pc_attr = 0;
> +  ULONGEST byte_size = 0;
>  
>    memset (part_die, 0, sizeof (struct partial_die_info));
>  
> @@ -9802,8 +9824,9 @@ read_partial_die (struct partial_die_info *part_die,
>  	    part_die->sibling = buffer + dwarf2_get_ref_die_offset (&attr);
>  	  break;
>          case DW_AT_byte_size:
> -          part_die->has_byte_size = 1;
> -          break;
> +	  part_die->has_byte_size = 1;
> +	  byte_size = DW_UNSND (&attr);
> +	  break;
>  	case DW_AT_calling_convention:
>  	  /* DWARF doesn't provide a way to identify a program's source-level
>  	     entry point.  DW_AT_calling_convention attributes are only meant
> @@ -9870,6 +9893,19 @@ read_partial_die (struct partial_die_info *part_die,
>  	part_die->has_pc_info = 1;
>      }
>  
> +  /* ICC ddoes not output DW_AT_declaration on incomplete types, instead giving
> +     them a size of zero. Fix that up so that we treat this as an incomplete
> +     type. We can't check the producer string here, since it may not be in the
> +     cu yet. Ideally we would do this in fixup_partial_die(), but that would
> +     mean re-reading the DW_AT_byte_size attribute.  */
> +  if (part_die->has_byte_size && byte_size == 0
> +      && part_die->tag == DW_TAG_structure_type)
> +    {
> +      /* TODO: Check if this is also required for union and class declarations. */
> +      part_die->has_byte_size = 0;
> +      part_die->is_declaration = 1;
> +    }
> +

I think it can be moved into the <case DW_AT_byte_size> code above, cannot it?

And with the patch of mine below here should be a check for cu->producer now.


Thanks,
Jan


gdb/
2011-10-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* dwarf2read.c (load_full_comp_unit): Move cu->producer initialization
	to ...
	(prepare_one_comp_unit): ... here.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -4726,16 +4726,10 @@ load_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
 
   /* We try not to read any attributes in this function, because not
      all objfiles needed for references have been loaded yet, and symbol
-     table processing isn't initialized.  But we have to set the CU language,
-     or we won't be able to build types correctly.  */
+     table processing isn't initialized.  But we have to set the CU language
+     and DW_AT_producer, or we won't be able to build types correctly.  */
   prepare_one_comp_unit (cu, cu->dies);
 
-  /* Similarly, if we do not read the producer, we can not apply
-     producer-specific interpretation.  */
-  attr = dwarf2_attr (cu->dies, DW_AT_producer, cu);
-  if (attr)
-    cu->producer = DW_STRING (attr);
-
   if (read_cu)
     {
       do_cleanups (free_abbrevs_cleanup);
@@ -15901,6 +15895,12 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die)
       cu->language = language_minimal;
       cu->language_defn = language_def (cu->language);
     }
+
+  /* Similarly, if we do not read the producer, we can not apply
+     producer-specific interpretation.  */
+  attr = dwarf2_attr (comp_unit_die, DW_AT_producer, cu);
+  if (attr)
+    cu->producer = DW_STRING (attr);
 }
 
 /* Release one cached compilation unit, CU.  We unlink it from the tree

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

On 27/10/11 08:40, Jan Kratochvil wrote:
> On Sun, 23 Oct 2011 12:16:53 +0200, John Steele Scott wrote:
>> It turns out that the bogus stubs which ICC creates were getting into the
>> psymbol table:
> 
> I see this was a very bad review from me, thanks for catching it.
> 
> 
>> This updated patch tries harder to make sure that the stubs don't end up in the
>> psymbol table. This is done by adding a check in read_partial_die. Unfortunately
>> this breaks the dw2-ada-ffffffff testcase:
> 
> This work around should be checking DW_AT_producer.

Okay, now we always check DW_AT_producer when applying this workaround. This
depends on your patch which reads the producer in prepare_one_comp_unit(), which
was included in your review but has not yet been committed.
 
> There is one problem that -gdwarf-4 (-fdebug-types-section) .debug_types units
> do not contain DW_AT_producer and GDB currently does not try to inherit it
> from the referencing .debug_info sections.
> 
> But latest icc still does not support DW_AT_producer and I am not sure if it
> would use the declaration form inside .debug_types anyway.

I don't follow this paragraph. Did you mean something other than icc? I'm
using ICC 12.0.4, and it's definitely setting DW_AT_producer. I don't see it
outputting any .debug_types section though, it seems to only emit dwarf2.

I have submitted a testcase for this in a fork of this thread. Below is the
updated GDB patch which addresses your previous comments. As noted in the other
mail, I'm still waiting for my employer to produce my disclaimer paperwork.

Cheers,

John

commit 6c3b19b3cc118b9a5195c4013cd80cfdce137f9b
Author: John Steele Scott <toojays@toojays.net>
Date:   Mon Oct 17 08:27:18 2011

    ICC does not set DW_AT_declaration on opaque structure types, but does set their
    DW_AT_byte_size to zero. This patch adds checks for this, allowing gdb to
    resolve opaque structures in binaries which were built with ICC.
    
    read_structure_type now contains a special case to recognize such structures and
    mark them as TYPE_STUB.
    
    The logic used in process_structure_scope to determine whether to add a
    structure to the symbol table has been extracted out into a new function
    die_is_incomplete_type. This new function includes the ICC/zero-byte-size check.
    
    Some fixup code is added to read_partial_die() to avoid adding bogus structure
    stubs to the partial symbol table. This was causing problems when looking up a
    type which was only in the psymbols, because lookup_symbol_aux_psymtabs (called
    via basic_lookup_transparent_type_quick) only returns the first psymbol found,
    which would not always be the complete type.
    
    Union and class types are similarly affected and so have the same workarounds
    applied to them.
    
    gdb/Changelog:
    2011-11-13  John Steele Scott  <toojays@toojays.net>
    
    	PR symtab/13277: Resolving opaque structures in ICC generated binaries.
    	* dwarf2read.c (producer_is_icc): New function.
    	(read_structure_type): Set TYPE_STUB on structures/unions/classes
    	with a byte size of zero, if they were produced by ICC.
    	(process_structure_scope): Extract "external reference" check into
    	die_is_incomplete_type.
    	(die_is_incomplete_type): New function.
    	(read_partial_die): If a structure/union/class has a byte_size of zero,
    	and it was produced by ICC, set part_die->is_declaration instead of
    	part_die->has_byte_size.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index c8227c9..d2b7313 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -7585,6 +7585,23 @@ quirk_gcc_member_function_pointer (struct type *type, struct objfile *objfile)
   smash_to_methodptr_type (type, new_type);
 }
 
+/* Return non-zero if the supplied PRODUCER string matches the Intel C/C++
+   compiler (icc).  */
+
+static int
+producer_is_icc (const char *producer)
+{
+  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";
+
+  if (producer == NULL)
+    return 0;
+
+  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
+    return 1;
+
+  return 0;
+}
+
 /* 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
    the type's name and general properties; the members will not be
@@ -7695,6 +7712,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
     /* RealView does not output the required DW_AT_declaration
        on incomplete types.  */
     TYPE_STUB (type) = 1;
+  else if (attr != NULL && die->child == NULL && TYPE_LENGTH (type) == 0
+	   && producer_is_icc (cu->producer))
+    /* ICC does not output the required DW_AT_declaration
+       on incomplete types, but gives them a size of zero.  */
+    TYPE_STUB (type) = 1;
 
   /* We need to add the type field to the die immediately so we don't
      infinitely recurse when dealing with pointers to the structure
@@ -7707,6 +7729,30 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
   return type;
 }
 
+/* Return non-zero if the DIE from the compilation unit CU is an incomplete
+   type.  "An incomplete structure, union or class type is represented by a
+   structure, union or class entry that does not have a byte size attribute and
+   that has a DW_AT_declaration attribute."  */
+
+static int
+die_is_incomplete_type (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu);
+
+  if (dwarf2_flag_true_p (die, DW_AT_declaration, cu) && attr == NULL)
+    return 1;
+  else if (producer_is_icc (cu->producer)
+	   && attr != NULL && DW_UNSND (attr) == 0 && die->child == NULL
+	   && (die->tag == DW_TAG_structure_type
+	       || die->tag == DW_TAG_class_type
+	       || die->tag == DW_TAG_union_type))
+    /* ICC does not output the required DW_AT_declaration on incomplete
+       structure, union and class types, but gives them a size of zero.  */
+    return 1;
+
+  return 0;
+}
+
 /* Finish creating a structure or union type, including filling in
    its members and creating a symbol for it.  */
 
@@ -7911,11 +7957,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
       child_die = sibling_die (child_die);
     }
 
-  /* Do not consider external references.  According to the DWARF standard,
-     these DIEs are identified by the fact that they have no byte_size
-     attribute, and a declaration attribute.  */
-  if (dwarf2_attr (die, DW_AT_byte_size, cu) != NULL
-      || !die_is_declaration (die, cu))
+  if (!die_is_incomplete_type (die, cu))
     new_symbol (die, type, cu);
 }
 
@@ -9861,8 +9903,23 @@ read_partial_die (struct partial_die_info *part_die,
 	    part_die->sibling = buffer + dwarf2_get_ref_die_offset (&attr);
 	  break;
         case DW_AT_byte_size:
-          part_die->has_byte_size = 1;
-          break;
+	  if (DW_UNSND (&attr) == 0 && producer_is_icc (cu->producer)
+	      && (part_die->tag == DW_TAG_structure_type
+		  || part_die->tag == DW_TAG_union_type
+		  || part_die->tag == DW_TAG_class_type))
+	    {
+	      /* ICC ddoes not output DW_AT_declaration on incomplete types,
+		 instead giving them a size of zero. Fix that up so that we
+		 treat this as an incomplete type. Ideally we would do this in
+		 fixup_partial_die(), but that would mean re-reading the
+		 DW_AT_byte_size attribute.  */
+	      part_die->is_declaration = 1;
+	    }
+	  else
+	    {
+	      part_die->has_byte_size = 1;
+	    }
+	  break;
 	case DW_AT_calling_convention:
 	  /* DWARF doesn't provide a way to identify a program's source-level
 	     entry point.  DW_AT_calling_convention attributes are only meant

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Tom Tromey]

>>>>> "John" == John Steele Scott <toojays@toojays.net> writes:

Jan> There is one problem that -gdwarf-4 (-fdebug-types-section)
Jan> .debug_types units do not contain DW_AT_producer and GDB currently
Jan> does not try to inherit it from the referencing .debug_info
Jan> sections.

Jan> But latest icc still does not support DW_AT_producer and I am not
Jan> sure if it would use the declaration form inside .debug_types
Jan> anyway.

John> I don't follow this paragraph. Did you mean something other than
John> icc? I'm using ICC 12.0.4, and it's definitely setting
John> DW_AT_producer. I don't see it outputting any .debug_types section
John> though, it seems to only emit dwarf2.

I think maybe he meant .debug_types in that second paragraph where he
wrote DW_AT_producer.  Or maybe I just don't understand as well :)

John>     2011-11-13  John Steele Scott  <toojays@toojays.net>
    
John>     	PR symtab/13277: Resolving opaque structures in ICC generated binaries.
John>     	* dwarf2read.c (producer_is_icc): New function.
John>     	(read_structure_type): Set TYPE_STUB on structures/unions/classes
John>     	with a byte size of zero, if they were produced by ICC.
John>     	(process_structure_scope): Extract "external reference" check into
John>     	die_is_incomplete_type.
John>     	(die_is_incomplete_type): New function.
John>     	(read_partial_die): If a structure/union/class has a byte_size of zero,
John>     	and it was produced by ICC, set part_die->is_declaration instead of

This patch is ok.  You didn't say, but I assume it passed all regression
tests?

Tom

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Jan Kratochvil]

On Tue, 15 Nov 2011 18:18:49 +0100, Tom Tromey wrote:
> >>>>> "John" == John Steele Scott <toojays@toojays.net> writes:
> 
> Jan> There is one problem that -gdwarf-4 (-fdebug-types-section)
> Jan> .debug_types units do not contain DW_AT_producer and GDB currently
> Jan> does not try to inherit it from the referencing .debug_info
> Jan> sections.
> 
> Jan> But latest icc still does not support DW_AT_producer and I am not
> Jan> sure if it would use the declaration form inside .debug_types
> Jan> anyway.
> 
> John> I don't follow this paragraph. Did you mean something other than
> John> icc? I'm using ICC 12.0.4, and it's definitely setting
> John> DW_AT_producer. I don't see it outputting any .debug_types section
> John> though, it seems to only emit dwarf2.
> 
> I think maybe he meant .debug_types in that second paragraph where he
> wrote DW_AT_producer.  Or maybe I just don't understand as well :)

Sorry, there should have been "latest icc still does not support -gdwarf-4"
(and therefore .debug_types and therefore this whole problem I was talking
about).


Jan

P.S. I am mostly offline these two weeks, please check in the patch of mine if
needed.

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

On 16/11/11 03:48, Tom Tromey wrote:
> John>     2011-11-13  John Steele Scott  <toojays@toojays.net>
>     
> John>     	PR symtab/13277: Resolving opaque structures in ICC generated binaries.
> John>     	* dwarf2read.c (producer_is_icc): New function.
> John>     	(read_structure_type): Set TYPE_STUB on structures/unions/classes
> John>     	with a byte size of zero, if they were produced by ICC.
> John>     	(process_structure_scope): Extract "external reference" check into
> John>     	die_is_incomplete_type.
> John>     	(die_is_incomplete_type): New function.
> John>     	(read_partial_die): If a structure/union/class has a byte_size of zero,
> John>     	and it was produced by ICC, set part_die->is_declaration instead of
> 
> This patch is ok.  You didn't say, but I assume it passed all regression
> tests?

I'm resending this old patch (rebased to current trunk) since I finally
got word that my paperwork has been processed. I ran the dwarf2 and base tests
and saw no new failures. Please apply.

I'll resend the tests in a separate mail.

Thanks,

John

gdb/Changelog:
2012-05-05  John Steele Scott  <toojays@toojays.net>

	PR symtab/13277: Resolving opaque structures in ICC generated binaries.
	* dwarf2read.c (producer_is_icc): New function.
	(read_structure_type): Set TYPE_STUB on structures/unions/classes
	with a byte size of zero, if they were produced by ICC.
	(process_structure_scope): Extract "external reference" check into
	die_is_incomplete_type.
	(die_is_incomplete_type): New function.
	(read_partial_die): If a structure/union/class has a byte_size of zero,
	and it was produced by ICC, set part_die->is_declaration instead of
	part_die->has_byte_size.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 412fe5b..ea29ad4 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8727,6 +8727,23 @@ quirk_gcc_member_function_pointer (struct type *type, struct objfile *objfile)
   smash_to_methodptr_type (type, new_type);
 }
 
+/* Return non-zero if the supplied PRODUCER string matches the Intel C/C++
+   compiler (icc).  */
+
+static int
+producer_is_icc (const char *producer)
+{
+  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";
+
+  if (producer == NULL)
+    return 0;
+
+  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
+    return 1;
+
+  return 0;
+}
+
 /* 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
    the type's name and general properties; the members will not be
@@ -8837,6 +8854,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
     /* RealView does not output the required DW_AT_declaration
        on incomplete types.  */
     TYPE_STUB (type) = 1;
+  else if (attr != NULL && die->child == NULL && TYPE_LENGTH (type) == 0
+	   && producer_is_icc (cu->producer))
+    /* ICC does not output the required DW_AT_declaration
+       on incomplete types, but gives them a size of zero.  */
+    TYPE_STUB (type) = 1;
 
   /* We need to add the type field to the die immediately so we don't
      infinitely recurse when dealing with pointers to the structure
@@ -8849,6 +8871,30 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
   return type;
 }
 
+/* Return non-zero if the DIE from the compilation unit CU is an incomplete
+   type.  "An incomplete structure, union or class type is represented by a
+   structure, union or class entry that does not have a byte size attribute and
+   that has a DW_AT_declaration attribute."  */
+
+static int
+die_is_incomplete_type (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu);
+
+  if (dwarf2_flag_true_p (die, DW_AT_declaration, cu) && attr == NULL)
+    return 1;
+  else if (producer_is_icc (cu->producer)
+	   && attr != NULL && DW_UNSND (attr) == 0 && die->child == NULL
+	   && (die->tag == DW_TAG_structure_type
+	       || die->tag == DW_TAG_class_type
+	       || die->tag == DW_TAG_union_type))
+    /* ICC does not output the required DW_AT_declaration on incomplete
+       structure, union and class types, but gives them a size of zero.  */
+    return 1;
+
+  return 0;
+}
+
 /* Finish creating a structure or union type, including filling in
    its members and creating a symbol for it.  */
 
@@ -9053,11 +9099,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
       child_die = sibling_die (child_die);
     }
 
-  /* Do not consider external references.  According to the DWARF standard,
-     these DIEs are identified by the fact that they have no byte_size
-     attribute, and a declaration attribute.  */
-  if (dwarf2_attr (die, DW_AT_byte_size, cu) != NULL
-      || !die_is_declaration (die, cu))
+  if (!die_is_incomplete_type (die, cu))
     new_symbol (die, type, cu);
 }
 
@@ -10991,8 +11033,23 @@ read_partial_die (const struct die_reader_specs *reader,
 	    part_die->sibling = buffer + dwarf2_get_ref_die_offset (&attr).sect_off;
 	  break;
         case DW_AT_byte_size:
-          part_die->has_byte_size = 1;
-          break;
+	  if (DW_UNSND (&attr) == 0 && producer_is_icc (cu->producer)
+	      && (part_die->tag == DW_TAG_structure_type
+		  || part_die->tag == DW_TAG_union_type
+		  || part_die->tag == DW_TAG_class_type))
+	    {
+	      /* ICC ddoes not output DW_AT_declaration on incomplete types,
+		 instead giving them a size of zero. Fix that up so that we
+		 treat this as an incomplete type. Ideally we would do this in
+		 fixup_partial_die(), but that would mean re-reading the
+		 DW_AT_byte_size attribute.  */
+	      part_die->is_declaration = 1;
+	    }
+	  else
+	    {
+	      part_die->has_byte_size = 1;
+	    }
+	  break;
 	case DW_AT_calling_convention:
 	  /* DWARF doesn't provide a way to identify a program's source-level
 	     entry point.  DW_AT_calling_convention attributes are only meant

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Jan Kratochvil]

On Sat, 05 May 2012 04:32:14 +0200, John Steele Scott wrote:
> I ran the dwarf2 and base tests and saw no new failures. Please apply.

BTW did you run the testsuite with icc or gcc or both?


> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -8727,6 +8727,23 @@ quirk_gcc_member_function_pointer (struct type *type, struct objfile *objfile)
>    smash_to_methodptr_type (type, new_type);
>  }
>  
> +/* Return non-zero if the supplied PRODUCER string matches the Intel C/C++
> +   compiler (icc).  */
> +
> +static int
> +producer_is_icc (const char *producer)
> +{
> +  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";

I believe the same problem applies to 32-bit compiler while this string will
not match it.  Also isn't the same problem for example with some Intel Fortran
compiler?  That maybe some "Intel(R)" match would be enough.


> +
> +  if (producer == NULL)
> +    return 0;
> +
> +  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
> +    return 1;
> +
> +  return 0;
> +}
> +
>  /* 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
>     the type's name and general properties; the members will not be
> @@ -8837,6 +8854,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
>      /* RealView does not output the required DW_AT_declaration
>         on incomplete types.  */
>      TYPE_STUB (type) = 1;
> +  else if (attr != NULL && die->child == NULL && TYPE_LENGTH (type) == 0
> +	   && producer_is_icc (cu->producer))
> +    /* ICC does not output the required DW_AT_declaration
> +       on incomplete types, but gives them a size of zero.  */
> +    TYPE_STUB (type) = 1;

braces; it is just said so in gdb/doc/gdbint.texinfo:
Any two or more lines in code should be wrapped in braces, even if
they are comments, as they look like separate statements:

    {
      /* ICC does not output the required DW_AT_declaration
	 on incomplete types, but gives them a size of zero.  */
      TYPE_STUB (type) = 1;
    }


>  
>    /* We need to add the type field to the die immediately so we don't
>       infinitely recurse when dealing with pointers to the structure
> @@ -8849,6 +8871,30 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
>    return type;
>  }
>  
> +/* Return non-zero if the DIE from the compilation unit CU is an incomplete
> +   type.  "An incomplete structure, union or class type is represented by a
> +   structure, union or class entry that does not have a byte size attribute and
> +   that has a DW_AT_declaration attribute."  */
> +
> +static int
> +die_is_incomplete_type (struct die_info *die, struct dwarf2_cu *cu)
> +{
> +  struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu);
> +
> +  if (dwarf2_flag_true_p (die, DW_AT_declaration, cu) && attr == NULL)

This conditional should use die_is_declaration like before, as you no longer
check DW_AT_specification == NULL, as die_is_declaration does check.

But in general I do not understand why did not you put the ICC exception to
existing die_is_declaration instead.  Even with your patch still various parts
of code call die_is_declaration which may be a problem for ICC.  I do not have
a countercase at hand.  I would try to patch die_is_declaration and see if it
will make any difference with icc on the testsuite, I do not have icc ready
now to make this simple test.


> +    return 1;
> +  else if (producer_is_icc (cu->producer)
> +	   && attr != NULL && DW_UNSND (attr) == 0 && die->child == NULL
> +	   && (die->tag == DW_TAG_structure_type
> +	       || die->tag == DW_TAG_class_type
> +	       || die->tag == DW_TAG_union_type))
> +    /* ICC does not output the required DW_AT_declaration on incomplete
> +       structure, union and class types, but gives them a size of zero.  */
> +    return 1;

braces:
    {
      /* ICC does not output the required DW_AT_declaration on incomplete
	 structure, union and class types, but gives them a size of zero.  */
      return 1;
    }

> +
> +  return 0;
> +}
> +
>  /* Finish creating a structure or union type, including filling in
>     its members and creating a symbol for it.  */
>  
> @@ -9053,11 +9099,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>        child_die = sibling_die (child_die);
>      }
>  
> -  /* Do not consider external references.  According to the DWARF standard,
> -     these DIEs are identified by the fact that they have no byte_size
> -     attribute, and a declaration attribute.  */
> -  if (dwarf2_attr (die, DW_AT_byte_size, cu) != NULL
> -      || !die_is_declaration (die, cu))
> +  if (!die_is_incomplete_type (die, cu))
>      new_symbol (die, type, cu);
>  }
>  
> @@ -10991,8 +11033,23 @@ read_partial_die (const struct die_reader_specs *reader,
>  	    part_die->sibling = buffer + dwarf2_get_ref_die_offset (&attr).sect_off;
>  	  break;
>          case DW_AT_byte_size:
> -          part_die->has_byte_size = 1;
> -          break;
> +	  if (DW_UNSND (&attr) == 0 && producer_is_icc (cu->producer)
> +	      && (part_die->tag == DW_TAG_structure_type
> +		  || part_die->tag == DW_TAG_union_type
> +		  || part_die->tag == DW_TAG_class_type))
> +	    {
> +	      /* ICC ddoes not output DW_AT_declaration on incomplete types,
> +		 instead giving them a size of zero. Fix that up so that we

Two spaces after dot.

> +		 treat this as an incomplete type. Ideally we would do this in

Again.

> +		 fixup_partial_die(), but that would mean re-reading the

Remove those "()" for GNU Coding Standards compliance.

> +		 DW_AT_byte_size attribute.  */
> +	      part_die->is_declaration = 1;
> +	    }
> +	  else
> +	    {
> +	      part_die->has_byte_size = 1;
> +	    }

Needless braces but I do not mind.

> +	  break;
>  	case DW_AT_calling_convention:
>  	  /* DWARF doesn't provide a way to identify a program's source-level
>  	     entry point.  DW_AT_calling_convention attributes are only meant


Thanks,
Jan

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

Jan,

Thanks for your review. I have a few questions before I resubmit.

On 13/05/12 04:07, Jan Kratochvil wrote:
> On Sat, 05 May 2012 04:32:14 +0200, John Steele Scott wrote:
>> I ran the dwarf2 and base tests and saw no new failures. Please apply.
> 
> BTW did you run the testsuite with icc or gcc or both?

So far I have only run the testsuite with gcc. Until now I've only been occasionally accessing icc on our build machine, which is not setup for gdb development.

I may be able to get ICC setup on my local machine and try to build/test with that. Are the existing tests already known to work with ICC?

>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -8727,6 +8727,23 @@ quirk_gcc_member_function_pointer (struct type *type, struct objfile *objfile)
>>    smash_to_methodptr_type (type, new_type);
>>  }
>>  
>> +/* Return non-zero if the supplied PRODUCER string matches the Intel C/C++
>> +   compiler (icc).  */
>> +
>> +static int
>> +producer_is_icc (const char *producer)
>> +{
>> +  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";
> 
> I believe the same problem applies to 32-bit compiler while this string will
> not match it.  Also isn't the same problem for example with some Intel Fortran
> compiler?  That maybe some "Intel(R)" match would be enough.

You are correct about the 32-bit compiler. I don't know about the Fortran, we don't have that here. Just matching "Intel(R)" seems a little loose; wouldn't it be safer to try against each producer string in turn?

I have added a check for "Intel(R) C Compiler XE" which is what the 32-bit version emits. If you know the Fortran producer string I can add that, or if you really prefer we can just scan for "Intel(R)" like you said before.

>> +
>> +  if (producer == NULL)
>> +    return 0;
>> +
>> +  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
>> +    return 1;
>> +
>> +  return 0;
>> +}
>> +
>>  /* 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
>>     the type's name and general properties; the members will not be
>> @@ -8837,6 +8854,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
>>      /* RealView does not output the required DW_AT_declaration
>>         on incomplete types.  */
>>      TYPE_STUB (type) = 1;
>> +  else if (attr != NULL && die->child == NULL && TYPE_LENGTH (type) == 0
>> +	   && producer_is_icc (cu->producer))
>> +    /* ICC does not output the required DW_AT_declaration
>> +       on incomplete types, but gives them a size of zero.  */
>> +    TYPE_STUB (type) = 1;
> 
> braces; it is just said so in gdb/doc/gdbint.texinfo:
> Any two or more lines in code should be wrapped in braces, even if
> they are comments, as they look like separate statements:
> 
>     {
>       /* ICC does not output the required DW_AT_declaration
> 	 on incomplete types, but gives them a size of zero.  */
>       TYPE_STUB (type) = 1;
>     }
> 
> 
>>  
>>    /* We need to add the type field to the die immediately so we don't
>>       infinitely recurse when dealing with pointers to the structure
>> @@ -8849,6 +8871,30 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
>>    return type;
>>  }
>>  
>> +/* Return non-zero if the DIE from the compilation unit CU is an incomplete
>> +   type.  "An incomplete structure, union or class type is represented by a
>> +   structure, union or class entry that does not have a byte size attribute and
>> +   that has a DW_AT_declaration attribute."  */
>> +
>> +static int
>> +die_is_incomplete_type (struct die_info *die, struct dwarf2_cu *cu)
>> +{
>> +  struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu);
>> +
>> +  if (dwarf2_flag_true_p (die, DW_AT_declaration, cu) && attr == NULL)
> 
> This conditional should use die_is_declaration like before, as you no longer
> check DW_AT_specification == NULL, as die_is_declaration does check.
> 
> But in general I do not understand why did not you put the ICC exception to
> existing die_is_declaration instead.  Even with your patch still various parts
> of code call die_is_declaration which may be a problem for ICC.  I do not have
> a countercase at hand.  I would try to patch die_is_declaration and see if it
> will make any difference with icc on the testsuite, I do not have icc ready
> now to make this simple test.

Yes, since I don't have a deep understanding of either the dwarf standard nor gdb internals, I was being conservative and making the minimum change required for my test. I can merge this die_is_incomplete_type logic into die_is_declaration, but then it means there is no way to distinguish between a "declaration", and an "incomplete type". It seems okay (dwarf2 tests pass at least), as long as there no obscure corner of dwarf where this distinction matters to us.

> 
>> +    return 1;
>> +  else if (producer_is_icc (cu->producer)
>> +	   && attr != NULL && DW_UNSND (attr) == 0 && die->child == NULL
>> +	   && (die->tag == DW_TAG_structure_type
>> +	       || die->tag == DW_TAG_class_type
>> +	       || die->tag == DW_TAG_union_type))
>> +    /* ICC does not output the required DW_AT_declaration on incomplete
>> +       structure, union and class types, but gives them a size of zero.  */
>> +    return 1;
> 
> braces:
>     {
>       /* ICC does not output the required DW_AT_declaration on incomplete
> 	 structure, union and class types, but gives them a size of zero.  */
>       return 1;
>     }

In this case, should I also add braces to the "else if" block just above, while I'm there? Or do you prefer to avoid the churn?

If I move this into die_is_declaration I will move the above realview check there too, then I will add the braces anyway in that case.

>> +
>> +  return 0;
>> +}
>> +
>>  /* Finish creating a structure or union type, including filling in
>>     its members and creating a symbol for it.  */
>>  
>> @@ -9053,11 +9099,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>>        child_die = sibling_die (child_die);
>>      }
>>  
>> -  /* Do not consider external references.  According to the DWARF standard,
>> -     these DIEs are identified by the fact that they have no byte_size
>> -     attribute, and a declaration attribute.  */
>> -  if (dwarf2_attr (die, DW_AT_byte_size, cu) != NULL
>> -      || !die_is_declaration (die, cu))
>> +  if (!die_is_incomplete_type (die, cu))
>>      new_symbol (die, type, cu);

So if I push the die_is_incomplete_type logic into die_is_declaration, the original check for the DW_AT_byte_size attribute above must be removed, otherwise my bug is not fixed.

>>  }
>>  
>> @@ -10991,8 +11033,23 @@ read_partial_die (const struct die_reader_specs *reader,
>>  	    part_die->sibling = buffer + dwarf2_get_ref_die_offset (&attr).sect_off;
>>  	  break;
>>          case DW_AT_byte_size:
>> -          part_die->has_byte_size = 1;
>> -          break;
>> +	  if (DW_UNSND (&attr) == 0 && producer_is_icc (cu->producer)
>> +	      && (part_die->tag == DW_TAG_structure_type
>> +		  || part_die->tag == DW_TAG_union_type
>> +		  || part_die->tag == DW_TAG_class_type))
>> +	    {
>> +	      /* ICC ddoes not output DW_AT_declaration on incomplete types,
>> +		 instead giving them a size of zero. Fix that up so that we
> 
> Two spaces after dot.
> 
>> +		 treat this as an incomplete type. Ideally we would do this in
> 
> Again.
> 
>> +		 fixup_partial_die(), but that would mean re-reading the
> 
> Remove those "()" for GNU Coding Standards compliance.
> 
>> +		 DW_AT_byte_size attribute.  */
>> +	      part_die->is_declaration = 1;
>> +	    }
>> +	  else
>> +	    {
>> +	      part_die->has_byte_size = 1;
>> +	    }
> 
> Needless braces but I do not mind.
> 
>> +	  break;
>>  	case DW_AT_calling_convention:
>>  	  /* DWARF doesn't provide a way to identify a program's source-level
>>  	     entry point.  DW_AT_calling_convention attributes are only meant

cheers,

John

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

Jan,

Thank you for continuing to work with me on this.

On 19/05/12 00:16, Jan Kratochvil wrote:
> Hi John,
>
> On Mon, 14 May 2012 15:51:20 +0200, John Steele Scott wrote:
>> Are the existing tests already known to work with ICC?
> I do not remember trying it.  I assume many tests will FAIL.  But we do not
> want to diff gcc results vs. icc results but only icc results before the patch
> vs. icc results after the patch.

I bit the bullet and installed icc on my local machine. It's quite painful due to (I think) a round-trip to the licensing server for each compilation unit. Anyway, I was able to run the testsuite. I run "make check" with  RUNTESTFLAGS="CC_FOR_TARGET=icc CFLAGS_FOR_TARGET='-debug extended' CXX_FOR_TARGET=icpc CXXFLAGS_FOR_TARGET='-debug extended'".

I saw later your wiki page at http://sourceware.org/gdb/wiki/Running%20the%20test%20suite%20with%20a%20non-GCC%20compiler. It seems icc does now understand --print-multi-lib.

> With my icc test there is difference between the types for v and w:
> 	struct s;
> 	extern struct s v;
> 	struct w {} w;
>
> DW_AT_producer    : Intel(R) C Intel(R) 64 Compiler XE for applications running on Intel(R) 64, Version 12.1.4.319 Build 20120410 Fixes SameLinkageName MemberPointers
>
>  <1><ed>: Abbrev Number: 4 (DW_TAG_variable)
>     <f2>   DW_AT_name        : v
>     <f4>   DW_AT_type        : <0xfa>
> [shortened]
>  <1><fa>: Abbrev Number: 5 (DW_TAG_structure_type)
>     <fb>   DW_AT_decl_line   : 1
>     <fc>   DW_AT_decl_column : 8
>     <fd>   DW_AT_decl_file   : 1
>     <fe>   DW_AT_accessibility: 1       (public)
>     <ff>   DW_AT_byte_size   : 0
>     <100>   DW_AT_name        : s
>  <1><102>: Abbrev Number: 6 (DW_TAG_variable)
>     <107>   DW_AT_name        : w
>     <109>   DW_AT_type        : <0x118>
> [shortened]
>  <1><118>: Abbrev Number: 5 (DW_TAG_structure_type)
>     <119>   DW_AT_decl_line   : 4
>     <11a>   DW_AT_decl_column : 8
>     <11b>   DW_AT_decl_file   : 1
>     <11c>   DW_AT_accessibility: 1      (public)
>     <11d>   DW_AT_byte_size   : 0
>     <11e>   DW_AT_name        : w
>
> This means icc does must not use TYPE_STUB_SUPPORTED.  Just one needs to fix
> producer_is_icc so that it gets processed only once and cached via
> 'checked_producer' as otherwise it may needlessly slow down reading DWARF.

If I understand you correctly, the problem with my previous approach is that it essentially ignores empty structs. And indeed now that I run the testsuite with ICC, it shows failures in gdb.base/nofield.exp. The approach you suggested passes that test.

However, this brings us back to the problem I wrote about last October (see http://article.gmane.org/gmane.comp.gdb.patches/69651): the zero-size structures are cluttering up the psymbols and basic_lookup_transparent_type will only do one psymbol->symbol expansion per call. For a frequently referenced opaque type in a non-trivial program, initially "ptype" will show it as "no data fields". But if I repeatedly invoke ptype, eventually it resolves the type correctly. Of course this does not happen with my trivial testcase, and I'm unsure how I would expand that to expose this issue.

Can you suggest a way to fix this, without ignoring zero-size structs?

If I have to chose between showing empty structs, and correctly resolving non-empty structs, I would prefer to resolve the non-empty ones, they are much more interesting. :)

On the question of caching the producer info in the dwarf2_cu; I propose to extract out the second half of producer_is_gxx_lt_4_6 into a new check_producer function which will set cu->producer_is_gxx_lt_4_6 and cu->producer_is_icc as appropriate (and then set cu->checked_producer). producer_is_gxx_lt_4_6 and producer_is_icc will call check_producer if cu->checked_producer is not set. Sound okay?

Thanks,

John

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Jan Kratochvil]

[ Cc to gdb-patches has been lost.  ]
On Sun, 20 May 2012 15:09:19 +0200, Jan Kratochvil wrote:
Hi John,

On Sun, 20 May 2012 14:34:05 +0200, John Steele Scott wrote:
> basic_lookup_transparent_type will only do one psymbol->symbol expansion per
> call. For a frequently referenced opaque type in a non-trivial program,
> initially "ptype" will show it as "no data fields". But if I repeatedly
> invoke ptype, eventually it resolves the type correctly.

sorry I did not write a proper reproducer + testcase for it but I believe the
attached patch should fix it.  It is probably a regression since introduction
of quick_symbol_functions.


> On the question of caching the producer info in the dwarf2_cu; I propose to
> extract out the second half of producer_is_gxx_lt_4_6 into a new
> check_producer function which will set cu->producer_is_gxx_lt_4_6 and
> cu->producer_is_icc as appropriate (and then set cu->checked_producer).
> producer_is_gxx_lt_4_6 and producer_is_icc will call check_producer if
> cu->checked_producer is not set. Sound okay?

Yes, I find such approach appropriate.


Thanks,
Jan


gdb/
2012-05-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* psymtab.c (lookup_symbol_aux_psymtabs): New variable stab_best.  Use
	it as a fallback for TYPE_IS_OPAQUE.

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index e463fff..647368c 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -510,6 +510,7 @@ lookup_symbol_aux_psymtabs (struct objfile *objfile,
 {
   struct partial_symtab *ps;
   const int psymtab_index = (block_index == GLOBAL_BLOCK ? 1 : 0);
+  struct symtab *stab_best = NULL;
 
   ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, ps)
   {
@@ -530,13 +531,18 @@ lookup_symbol_aux_psymtabs (struct objfile *objfile,
 	  }
 
 	if (sym && strcmp_iw (SYMBOL_SEARCH_NAME (sym), name) == 0)
-	  return stab;
+	  {
+	    if (!TYPE_IS_OPAQUE (SYMBOL_TYPE (sym)))
+	      return stab;
+
+	    stab_best = stab;
+	  }
 
 	/* Keep looking through other psymtabs.  */
       }
   }
 
-  return NULL;
+  return stab_best;
 }
 
 /* Look in PST for a symbol in DOMAIN whose name matches NAME.  Search

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

On 20/05/12 22:47, Jan Kratochvil wrote:
> [ Cc to gdb-patches has been lost.  ]
> On Sun, 20 May 2012 15:09:19 +0200, Jan Kratochvil wrote:
> Hi John,
>
> On Sun, 20 May 2012 14:34:05 +0200, John Steele Scott wrote:
>> basic_lookup_transparent_type will only do one psymbol->symbol expansion per
>> call. For a frequently referenced opaque type in a non-trivial program,
>> initially "ptype" will show it as "no data fields". But if I repeatedly
>> invoke ptype, eventually it resolves the type correctly.
> sorry I did not write a proper reproducer + testcase for it but I believe the
> attached patch should fix it.  It is probably a regression since introduction
> of quick_symbol_functions.
>

Yes, this looks to have fixed it.

Cheers,

John

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[John Steele Scott]

Hi Jan,

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> [ Cc to gdb-patches has been lost.  ]
> On Sun, 20 May 2012 15:09:19 +0200, Jan Kratochvil wrote:
> Hi John,
>
> On Sun, 20 May 2012 14:34:05 +0200, John Steele Scott wrote:
>> basic_lookup_transparent_type will only do one psymbol->symbol expansion per
>> call. For a frequently referenced opaque type in a non-trivial program,
>> initially "ptype" will show it as "no data fields". But if I repeatedly
>> invoke ptype, eventually it resolves the type correctly.
>
> sorry I did not write a proper reproducer + testcase for it but I believe the
> attached patch should fix it.  It is probably a regression since introduction
> of quick_symbol_functions.
>
>
>> On the question of caching the producer info in the dwarf2_cu; I propose to
>> extract out the second half of producer_is_gxx_lt_4_6 into a new
>> check_producer function which will set cu->producer_is_gxx_lt_4_6 and
>> cu->producer_is_icc as appropriate (and then set cu->checked_producer).
>> producer_is_gxx_lt_4_6 and producer_is_icc will call check_producer if
>> cu->checked_producer is not set. Sound okay?
>
> Yes, I find such approach appropriate.

The below patch implements this.

I ran the full testsuite on amd64 with gcc (Ubuntu/Linaro 4.4.4-14ubuntu5.1)
4.4.5; it passes my testcase from
http://sourceware.org/ml/gdb-patches/2012-05/msg00763.html, and there are no
new failures.

I also ran the full testsuite with icc, as:
make -j10 check RUNTESTFLAGS="CC_FOR_TARGET=icc CFLAGS_FOR_TARGET='-debug
extended' CXX_FOR_TARGET=icpc CXXFLAGS_FOR_TARGET='-debug extended'". In
addition to fixing my testcase, this fixes the four similar broken cases in
gdb.base/opaque, now that set of tests all pass.

Two tests in gdb.base/type-opaque effectively changed polarity: previously
"opaque {struct,union} type resolving" failed, now those pass and "empty
{struct,union} type resolving" fail. As previously discussed, this is the
lesser of the two evils as far as I'm concerned.

The patch at http://sourceware.org/ml/gdb-patches/2012-05/msg00739.html is
still required to fix my non-trivial case, I hope that (or something like it)
can be committed soon.

thanks,

John

2012-05-24  John Steele Scott  <toojays@toojays.net>

	PR symtab/13277: Resolving opaque structures in ICC generated binaries.
	* dwarf2read.c (struct dwarf2_cu) <producer_is_icc>: New field.
	(producer_is_gxx_lt_4_6): Move the checking and caching to...
	(check_producer): ... this new function, which also checks for ICC
	and caches the result.
	(producer_is_icc): New function.
	(read_structure_type): Don't set TYPE_STUB_SUPPORTED if the
	producer was ICC.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b590134..2afefaf 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -429,12 +429,13 @@ struct dwarf2_cu
      unoptimized code.  For a future better test see GCC PR other/32998.  */
   unsigned int has_loclist : 1;
 
-  /* These cache the results of producer_is_gxx_lt_4_6.
-     CHECKED_PRODUCER is set if PRODUCER_IS_GXX_LT_4_6 is valid.  This
-     information is cached because profiling CU expansion showed
-     excessive time spent in producer_is_gxx_lt_4_6.  */
+  /* These cache the results for producer_is_gxx_lt_4_6 and producer_is_icc.
+     CHECKED_PRODUCER is set if both PRODUCER_IS_GXX_LT_4_6 and PRODUCER_IS_ICC
+     are valid.  This information is cached because profiling CU expansion
+     showed excessive time spent in producer_is_gxx_lt_4_6.  */
   unsigned int checked_producer : 1;
   unsigned int producer_is_gxx_lt_4_6 : 1;
+  unsigned int producer_is_icc : 1;
 
   /* Non-zero if DW_AT_addr_base was found.
      Used when processing DWO files.  */
@@ -8271,16 +8272,14 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
     }
 }
 
-/* Check for GCC PR debug/45124 fix which is not present in any G++ version up
-   to 4.5.any while it is present already in G++ 4.6.0 - the PR has been fixed
-   during 4.6.0 experimental.  */
+/* Check whether the producer field indicates either of GCC < 4.6, or the
+   Intel C/C++ compiler, and cache the result in CU.  */
 
-static int
-producer_is_gxx_lt_4_6 (struct dwarf2_cu *cu)
+static void
+check_producer (struct dwarf2_cu *cu)
 {
   const char *cs;
   int major, minor, release;
-  int result = 0;
 
   if (cu->producer == NULL)
     {
@@ -8292,22 +8291,11 @@ producer_is_gxx_lt_4_6 (struct dwarf2_cu *cu)
 	 for their space efficiency GDB cannot workaround gcc-4.5.x -gdwarf-4
 	 combination.  gcc-4.5.x -gdwarf-4 binaries have DW_AT_accessibility
 	 interpreted incorrectly by GDB now - GCC PR debug/48229.  */
-
-      return 0;
-    }
-
-  if (cu->checked_producer)
-    return cu->producer_is_gxx_lt_4_6;
-
-  /* Skip any identifier after "GNU " - such as "C++" or "Java".  */
-
-  if (strncmp (cu->producer, "GNU ", strlen ("GNU ")) != 0)
-    {
-      /* For non-GCC compilers expect their behavior is DWARF version
-	 compliant.  */
     }
-  else
+  else if (strncmp (cu->producer, "GNU ", strlen ("GNU ")) == 0)
     {
+      /* Skip any identifier after "GNU " - such as "C++" or "Java".  */
+
       cs = &cu->producer[strlen ("GNU ")];
       while (*cs && !isdigit (*cs))
 	cs++;
@@ -8316,13 +8304,30 @@ producer_is_gxx_lt_4_6 (struct dwarf2_cu *cu)
 	  /* Not recognized as GCC.  */
 	}
       else
-	result = major < 4 || (major == 4 && minor < 6);
+	cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
+    }
+  else if (strncmp (cu->producer, "Intel(R) C", strlen ("Intel(R) C")) == 0)
+    cu->producer_is_icc = 1;
+  else
+    {
+      /* For other non-GCC compilers, expect their behavior is DWARF version
+	 compliant.  */
     }
 
   cu->checked_producer = 1;
-  cu->producer_is_gxx_lt_4_6 = result;
+}
 
-  return result;
+/* Check for GCC PR debug/45124 fix which is not present in any G++ version up
+   to 4.5.any while it is present already in G++ 4.6.0 - the PR has been fixed
+   during 4.6.0 experimental.  */
+
+static int
+producer_is_gxx_lt_4_6 (struct dwarf2_cu *cu)
+{
+  if (!cu->checked_producer)
+    check_producer(cu);
+
+  return cu->producer_is_gxx_lt_4_6;
 }
 
 /* Return the default accessibility type if it is not overriden by
@@ -9005,6 +9010,18 @@ 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
    the type's name and general properties; the members will not be
@@ -9107,7 +9124,14 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
       TYPE_LENGTH (type) = 0;
     }
 
-  TYPE_STUB_SUPPORTED (type) = 1;
+  if (producer_is_icc (cu))
+    {
+      /* ICC does not output the required DW_AT_declaration
+	 on incomplete types, but gives them a size of zero.  */
+    }
+  else
+    TYPE_STUB_SUPPORTED (type) = 1;
+
   if (die_is_declaration (die, cu))
     TYPE_STUB (type) = 1;
   else if (attr == NULL && die->child == NULL

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Pedro Alves]

On 05/24/2012 12:28 AM, John Steele Scott wrote:

> -  /* These cache the results of producer_is_gxx_lt_4_6.
> -     CHECKED_PRODUCER is set if PRODUCER_IS_GXX_LT_4_6 is valid.  This
> -     information is cached because profiling CU expansion showed
> -     excessive time spent in producer_is_gxx_lt_4_6.  */
> +  /* These cache the results for producer_is_gxx_lt_4_6 and producer_is_icc.
> +     CHECKED_PRODUCER is set if both PRODUCER_IS_GXX_LT_4_6 and PRODUCER_IS_ICC
> +     are valid.  This information is cached because profiling CU expansion
> +     showed excessive time spent in producer_is_gxx_lt_4_6.  */
>    unsigned int checked_producer : 1;
>    unsigned int producer_is_gxx_lt_4_6 : 1;
> +  unsigned int producer_is_icc : 1;


Doesn't matter much at this point, since you'd need two bits anyway,
but I'll note that producer_is_gxx_lt_4_6 and producer_is_icc are mutually
exclusive, so an enum (ENUM_BITFIELD) would be a better match.

-- 
Pedro Alves

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Doug Evans]

On Sun, May 20, 2012 at 5:34 AM, John Steele Scott <toojays@toojays.net> wrote:
> On the question of caching the producer info in the dwarf2_cu; I propose to extract out the second half of producer_is_gxx_lt_4_6 into a new check_producer function which will set cu->producer_is_gxx_lt_4_6 and cu->producer_is_icc as appropriate (and then set cu->checked_producer). producer_is_gxx_lt_4_6 and producer_is_icc will call check_producer if cu->checked_producer is not set. Sound okay?

I think something along those line is needed.
Thanks.

Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries.

[Jan Kratochvil]

[ Cc to gdb-patches has been lost.  ]
On Fri, 18 May 2012 16:46:42 +0200, Jan Kratochvil wrote:
Hi John,

On Mon, 14 May 2012 15:51:20 +0200, John Steele Scott wrote:
> Are the existing tests already known to work with ICC?

I do not remember trying it.  I assume many tests will FAIL.  But we do not
want to diff gcc results vs. icc results but only icc results before the patch
vs. icc results after the patch.


> I have added a check for "Intel(R) C Compiler XE" which is what the 32-bit
> version emits. If you know the Fortran producer string I can add that, or if
> you really prefer we can just scan for "Intel(R)" like you said before.

OK, I am fine with that 32-bit C compiler DW_AT_producer.

Also check for C++ DW_AT_producer - C++ has the same problem for stub structs;
just for the real definition of 'struct {} var' type C++ has DW_AT_byte_size==1.

FYI I was unable to create stub struct with ifort.


> I can merge this die_is_incomplete_type logic into
> die_is_declaration, but then it means there is no way to distinguish between
> a "declaration", and an "incomplete type". It seems okay (dwarf2 tests pass
> at least), as long as there no obscure corner of dwarf where this
> distinction matters to us.

With my icc test there is difference between the types for v and w:
	struct s;
	extern struct s v;
	struct w {} w;

DW_AT_producer    : Intel(R) C Intel(R) 64 Compiler XE for applications running on Intel(R) 64, Version 12.1.4.319 Build 20120410 Fixes SameLinkageName MemberPointers

 <1><ed>: Abbrev Number: 4 (DW_TAG_variable)
    <f2>   DW_AT_name        : v
    <f4>   DW_AT_type        : <0xfa>
[shortened]
 <1><fa>: Abbrev Number: 5 (DW_TAG_structure_type)
    <fb>   DW_AT_decl_line   : 1
    <fc>   DW_AT_decl_column : 8
    <fd>   DW_AT_decl_file   : 1
    <fe>   DW_AT_accessibility: 1       (public)
    <ff>   DW_AT_byte_size   : 0
    <100>   DW_AT_name        : s
 <1><102>: Abbrev Number: 6 (DW_TAG_variable)
    <107>   DW_AT_name        : w
    <109>   DW_AT_type        : <0x118>
[shortened]
 <1><118>: Abbrev Number: 5 (DW_TAG_structure_type)
    <119>   DW_AT_decl_line   : 4
    <11a>   DW_AT_decl_column : 8
    <11b>   DW_AT_decl_file   : 1
    <11c>   DW_AT_accessibility: 1      (public)
    <11d>   DW_AT_byte_size   : 0
    <11e>   DW_AT_name        : w

This means icc does must not use TYPE_STUB_SUPPORTED.  Just one needs to fix
producer_is_icc so that it gets processed only once and cached via
'checked_producer' as otherwise it may needlessly slow down reading DWARF.



> >> +    return 1;
> >> +  else if (producer_is_icc (cu->producer)
> >> +	   && attr != NULL && DW_UNSND (attr) == 0 && die->child == NULL
> >> +	   && (die->tag == DW_TAG_structure_type
> >> +	       || die->tag == DW_TAG_class_type
> >> +	       || die->tag == DW_TAG_union_type))
> >> +    /* ICC does not output the required DW_AT_declaration on incomplete
> >> +       structure, union and class types, but gives them a size of zero.  */
> >> +    return 1;
> > 
> > braces:
> >     {
> >       /* ICC does not output the required DW_AT_declaration on incomplete
> > 	 structure, union and class types, but gives them a size of zero.  */
> >       return 1;
> >     }
> 
> In this case, should I also add braces to the "else if" block just above,
> while I'm there? Or do you prefer to avoid the churn?

There is only single line of code so the braces should not be at the block
above.  (I am sorry as the rule comes from me.)

This patch has not been regression tested (neither against icc) in any way.


Thanks,
Jan


diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index fc1a864..32f0603 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8977,6 +8977,23 @@ quirk_gcc_member_function_pointer (struct type *type, struct objfile *objfile)
   smash_to_methodptr_type (type, new_type);
 }
 
+/* Return non-zero if the supplied PRODUCER string matches the Intel C/C++
+   compiler (icc).  */
+
+static int
+producer_is_icc (const char *producer)
+{
+  static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE";
+
+  if (producer == NULL)
+    return 0;
+
+  if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0)
+    return 1;
+
+  return 0;
+}
+
 /* 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
    the type's name and general properties; the members will not be
@@ -9079,7 +9096,14 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
       TYPE_LENGTH (type) = 0;
     }
 
-  TYPE_STUB_SUPPORTED (type) = 1;
+  if (producer_is_icc (cu->producer))
+    {
+      /* ICC does not output the required DW_AT_declaration
+	 on incomplete types, but gives them a size of zero.  */
+    }
+  else
+    TYPE_STUB_SUPPORTED (type) = 1;
+
   if (die_is_declaration (die, cu))
     TYPE_STUB (type) = 1;
   else if (attr == NULL && die->child == NULL
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-icc-opaque.S b/gdb/testsuite/gdb.dwarf2/dw2-icc-opaque.S
new file mode 100644
index 0000000..a12c8cf
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-icc-opaque.S
@@ -0,0 +1,192 @@
+/* Copyright (C) 2011-2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.file "opaque-pointer.c"
+	.data
+	.comm p_struct,8,8
+	.global p_struct#
+
+	.section .debug_info
+	.align 1
+debug_info_seg1:
+	.4byte debug_info_seg1_end - 1f	/* Length of compilation unit info. */
+1:
+	.2byte 0x0002			/* DWARF version number. */
+	.4byte .debug_abbrev_seg1	/* Points to abbrev section for this unit. */
+	.byte 0x04			/* Target address size. */
+
+	.byte 0x01			/* DIE 1: DW_TAG_compile_unit. */
+	.byte 0x01			/* DW_AT_language = DW_LANG_C89. */
+	.ascii "Intel(R) C Intel(R) 64 Compiler XE "	/* DW_AT_producer. */
+	.ascii "for applications running on Intel(R) 64, "
+	.ascii "Version 12.0.4.191 Build 20110427\n "
+	.asciz "Fixes SameLinkageName MemberPointers"
+
+	.byte 0x02			/* DIE 2: DW_TAG_variable. */
+	.byte 0x01			/* DW_AT_accessibility. */
+	.asciz "p_struct"		/* DW_AT_name. */
+	.4byte 3f - debug_info_seg1	/* DW_AT_type. */
+	.2byte 0x0305			/* DW_AT_location: 5 bytes, DW_OP_addr */
+	.4byte p_struct			/* followed by the address of p_struct. */
+	.byte 0x01			/* DW_AT_external. */
+
+3:
+	.byte 0x03			/* DIE 3: DW_TAG_pointer_type. */
+	.4byte 4f - debug_info_seg1	/* DW_AT_type. */
+
+4:
+	.byte 0x04			/* DIE 4: DW_TAG_structure_type. */
+	.byte 0x01			/* DW_AT_accessibility. */
+	.byte 0x00			/* DW_AT_byte_size. */
+	.asciz "opaque_struct_t"	/* DW_AT_name. */
+	.byte 0x00
+	.byte 0x00
+	.byte 0x00
+	.byte 0x00
+debug_info_seg1_end:
+
+	.section .debug_abbrev
+.debug_abbrev_seg1:
+	.align 1
+	.byte 0x01	/* Abbrev 1. */
+	.byte 0x11	/* DW_TAG_compile_unit. */
+	.byte 0x01	/* DW_CHILDREN_yes. */
+	.byte 0x13	/* DW_AT_language. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x25	/* DW_AT_producer. */
+	.byte 0x08	/* DW_AT_string. */
+	.2byte 0x0000	/* End abbrev. */
+
+	.byte 0x02	/* Abbrev 2. */
+	.byte 0x34	/* DW_TAG_variable. */
+	.byte 0x00	/* DW_CHILDREN_no. */
+	.byte 0x32	/* DW_AT_accessibility. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x03	/* DW_AT_name. */
+	.byte 0x08	/* DW_FORM_string. */
+	.byte 0x49	/* DW_AT_type. */
+	.byte 0x13	/* DW_FORM_ref4. */
+	.byte 0x02	/* DW_AT_location. */
+	.byte 0x0a	/* DW_FORM_block1. */
+	.byte 0x3f	/* DW_AT_external. */
+	.byte 0x0c	/* DW_FORM_flag. */
+	.2byte 0x0000	/* End abbrev. */
+
+	.byte 0x03	/* Abbrev 3. */
+	.byte 0x0f	/* DW_TAG_pointer_type. */
+	.byte 0x00	/* DW_CHILDREN_no. */
+	.byte 0x49	/* DW_AT_type. */
+	.byte 0x13	/* DW_FORM_ref4. */
+	.2byte 0x0000	/* End abbrev. */
+
+	.byte 0x04	/* Abbrev 4. */
+	.byte 0x13	/* DW_TAG_structure_type. */
+	.byte 0x00	/* DW_CHILDREN_no. */
+	.byte 0x32	/* DW_AT_accessibility. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x0b	/* DW_AT_byte_size. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x03	/* DW_AT_name. */
+	.byte 0x08	/* DW_FORM_string. */
+	.2byte 0x0000	/* End abbrev. */
+	.byte 0x00	/* End abbrev table. */
+
+
+	.file "opaque-struct.c"
+	.section .debug_info
+	.align 1
+debug_info_seg2:
+	.4byte debug_info_seg2_end - 1f	/* Length of compilation unit info. */
+1:
+	.2byte 0x0002			/* DWARF version number. */
+	.4byte .debug_abbrev_seg2	/* Points to abbrev section for this unit. */
+	.byte 0x04			/* Target address size. */
+
+	.byte 0x01			/* DIE 1: DW_TAG_compile_unit. */
+	.byte 0x01			/* DW_AT_language = DW_LANG_C89. */
+	.ascii "Intel(R) C Intel(R) 64 Compiler XE "	/* DW_AT_producer. */
+	.ascii "for applications running on Intel(R) 64, "
+	.ascii "Version 12.0.4.191 Build 20110427\n "
+	.asciz "Fixes SameLinkageName MemberPointers"
+
+	.byte 0x02			/* DIE 2: DW_TAG_structure_type. */
+	.byte 0x01			/* DW_AT_accessibility. */
+	.byte 0x04			/* DW_AT_byte_size. */
+	.asciz "opaque_struct_t"	/* DW_AT_name. */
+
+
+	.byte 0x03			/* DIE 3: DW_TAG_member. */
+	.2byte 0x2302			/* DW_AT_data_member_location, 2 bytes, */
+	.byte 0x00			/* DW_OP_plus_uconst followed by zero. */
+	.asciz "wrapped_value"		/* DW_AT_name. */
+	.4byte 4f - debug_info_seg2	/* DW_AT_type. */
+	.byte 0x00
+
+4:
+	.byte 0x04			/* DIE 4: DW_TAG_base_type. */
+	.byte 0x04			/* DW_AT_byte_size. */
+	.byte 0x05			/* DW_AT_encoding. */
+	.asciz "int"			/* DW_AT_name. */
+	.byte 0x00
+	.byte 0x00
+	.byte 0x00
+	.byte 0x00
+debug_info_seg2_end:
+
+	.section .debug_abbrev
+.debug_abbrev_seg2:
+	.align 1
+	.byte 0x01	/* Abbrev 1. */
+	.byte 0x11	/* DW_TAG_compile_unit. */
+	.byte 0x01	/* DW_CHILDREN_yes. */
+	.byte 0x13	/* DW_AT_language. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x25	/* DW_AT_producer. */
+	.byte 0x08	/* DW_FORM_string. */
+	.2byte 0x0000	/* End abbrev. */
+
+	.byte 0x02	/* Abbrev 2. */
+	.byte 0x13	/* DW_TAG_structure_type. */
+	.byte 0x01	/* DW_CHILDREN_yes. */
+	.byte 0x32	/* DW_AT_accessibility. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x0b	/* DW_AT_byte_size. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x03	/* DW_AT_name. */
+	.byte 0x08	/* DW_FORM_string. */
+	.2byte 0x0000	/* End abbrev. */
+
+	.byte 0x03	/* Abbrev 3. */
+	.byte 0x0d	/* DW_TAG_member. */
+	.byte 0x00	/* DW_CHILDREN_no. */
+	.byte 0x38	/* DW_AT_data_member_location. */
+	.byte 0x0a	/* DW_FORM_block1. */
+	.byte 0x03	/* DW_AT_name. */
+	.byte 0x08	/* DW_FORM_string. */
+	.byte 0x49	/* DW_AT_type. */
+	.byte 0x13	/* DW_FORM_ref4. */
+	.2byte 0x0000	/* End abbrev. */
+
+	.byte 0x04	/* Abbrev 4. */
+	.byte 0x24	/* DW_TAG_base_type. */
+	.byte 0x00	/* DW_CHILDREN_no. */
+	.byte 0x0b	/* DW_AT_byte_size. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x3e	/* DW_AT_encoding. */
+	.byte 0x0b	/* DW_FORM_data1. */
+	.byte 0x03	/* DW_AT_name. */
+	.byte 0x08	/* DW_FORM_string. */
+	.2byte 0x0000	/* End abbrev. */
+	.byte 0x00	/* End abbrev table. */
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-icc-opaque.exp b/gdb/testsuite/gdb.dwarf2/dw2-icc-opaque.exp
new file mode 100644
index 0000000..53db400
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-icc-opaque.exp
@@ -0,0 +1,37 @@
+# Copyright (C) 2011-2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+set testfile "dw2-icc-opaque"
+set srcfile ${testfile}.S
+set executable ${testfile}.x
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" object {}] != "" } {
+    return -1
+}
+
+clean_restart $executable
+
+# Before PR 13277 was fixed, this would output:
+# type = struct opaque_struct_t {
+#     <no data fields>
+# } *
+gdb_test "ptype p_struct" "type = struct opaque_struct_t {\r\n *int wrapped_value;\r\n} \\*"