[PATCH] Fix check for ICC incomplete struct types

[PATCH] Fix check for ICC incomplete struct types

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

GDB contains code in read_structure_type() which is supposed
to check for incorrect DWARF generated by ICC for an incomplete
structure type.  The code is incomplete, in that it doesn't
check for length == 0, and it doesn't set the STUB flag.

This patch adds the test and sets the flag.

gdb:
2014-01-07  Michael Eager <eager@eagercon.com>

    * dwarf2read.c (read_structure_type): Set stub if ICC & length == 0.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

[-- Attachment #2: icc_stub.patch --]
[-- Type: text/x-patch, Size: 929 bytes --]

From 38d324503619d60646d8a5194f80b71b91bcb713 Mon Sep 17 00:00:00 2001
From: Michael Eager <eager@eagercon.com>
Date: Tue, 7 Jan 2014 09:15:48 -0800
Subject: [PATCH] 2014-01-07  Michael Eager <eager@eagercon.com>

   * dwarf2read.c (read_structure_type): Set stub if ICC & length == 0.
---
 gdb/dwarf2read.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 23bcfe0..cbec171 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -12830,10 +12830,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
       TYPE_LENGTH (type) = 0;
     }
 
-  if (producer_is_icc (cu))
+  if (producer_is_icc (cu) && (TYPE_LENGTH (type) == 0))
     {
       /* ICC does not output the required DW_AT_declaration
 	 on incomplete types, but gives them a size of zero.  */
+      TYPE_STUB (type) = 1;
     }
   else
     TYPE_STUB_SUPPORTED (type) = 1;
-- 
1.8.1.4

Re: [PATCH] Fix check for ICC incomplete struct types

[Tom Tromey]

>>>>> "Michael" == Michael Eager <eager@eagerm.com> writes:

Michael> GDB contains code in read_structure_type() which is supposed
Michael> to check for incorrect DWARF generated by ICC for an incomplete
Michael> structure type.  The code is incomplete, in that it doesn't
Michael> check for length == 0, and it doesn't set the STUB flag.

Thanks.

Michael> -  if (producer_is_icc (cu))
Michael> +  if (producer_is_icc (cu) && (TYPE_LENGTH (type) == 0))
Michael>      {
Michael>        /* ICC does not output the required DW_AT_declaration
Michael>  	 on incomplete types, but gives them a size of zero.  */
Michael> +      TYPE_STUB (type) = 1;
Michael>      }
Michael>    else
Michael>      TYPE_STUB_SUPPORTED (type) = 1;

It seems to me that TYPE_STUB_SUPPORTED should be set unconditionally
(and, btw, eww, what a big hack TYPE_STUB_SUPPORTED is).  Without the
TYPE_STUB_SUPPORTED setting, TYPE_IS_OPAQUE does not honor TYPE_STUB.

Then the icc check moved to the next stanza, say like:

  TYPE_STUB_SUPPORTED (type) = 1;
  if (die_is_declaration (die, cu))
    TYPE_STUB (type) = 1;
  else if (producer_is_icc (cu) && TYPE_LENGTH (type) == 0)
    {
      /* ICC does not output the required DW_AT_declaration
	 on incomplete types, but gives them a size of zero.  */
      TYPE_STUB (type) = 1;
    }
  else if (attr == NULL && die->child == NULL
	   && producer_is_realview (cu->producer))
    /* RealView does not output the required DW_AT_declaration
       on incomplete types.  */
    TYPE_STUB (type) = 1;

What do you think of this?


Tom

Re: [PATCH] Fix check for ICC incomplete struct types

[Michael Eager]

On 01/07/14 10:43, Tom Tromey wrote:
>>>>>> "Michael" == Michael Eager <eager@eagerm.com> writes:
>
> Michael> GDB contains code in read_structure_type() which is supposed
> Michael> to check for incorrect DWARF generated by ICC for an incomplete
> Michael> structure type.  The code is incomplete, in that it doesn't
> Michael> check for length == 0, and it doesn't set the STUB flag.
>
> Thanks.
>
> Michael> -  if (producer_is_icc (cu))
> Michael> +  if (producer_is_icc (cu) && (TYPE_LENGTH (type) == 0))
> Michael>      {
> Michael>        /* ICC does not output the required DW_AT_declaration
> Michael>  	 on incomplete types, but gives them a size of zero.  */
> Michael> +      TYPE_STUB (type) = 1;
> Michael>      }
> Michael>    else
> Michael>      TYPE_STUB_SUPPORTED (type) = 1;
>
> It seems to me that TYPE_STUB_SUPPORTED should be set unconditionally
> (and, btw, eww, what a big hack TYPE_STUB_SUPPORTED is).  Without the
> TYPE_STUB_SUPPORTED setting, TYPE_IS_OPAQUE does not honor TYPE_STUB.
>
> Then the icc check moved to the next stanza, say like:
>
>    TYPE_STUB_SUPPORTED (type) = 1;
>    if (die_is_declaration (die, cu))
>      TYPE_STUB (type) = 1;
>    else if (producer_is_icc (cu) && TYPE_LENGTH (type) == 0)
>      {
>        /* ICC does not output the required DW_AT_declaration
> 	 on incomplete types, but gives them a size of zero.  */
>        TYPE_STUB (type) = 1;
>      }
>    else if (attr == NULL && die->child == NULL
> 	   && producer_is_realview (cu->producer))
>      /* RealView does not output the required DW_AT_declaration
>         on incomplete types.  */
>      TYPE_STUB (type) = 1;
>
> What do you think of this?

TYPE_IS_OPAQUE does honor TYPE_STUB and checks it before TYPE_STUB_SUPPORTED.
Which is absolutely screwy, since TYPE_STUB_SUPPORTED is set for all non-ICC
producers.

#define TYPE_IS_OPAQUE(thistype) \
   (((TYPE_CODE (thistype) == TYPE_CODE_STRUCT) \
     || (TYPE_CODE (thistype) == TYPE_CODE_UNION)) \
    && (TYPE_NFIELDS (thistype) == 0) \
    && (!HAVE_CPLUS_STRUCT (thistype) \
        || TYPE_NFN_FIELDS (thistype) == 0) \
    && (TYPE_STUB (thistype) || !TYPE_STUB_SUPPORTED (thistype)))

So the last expression for non-ICC is equivalent to
    (TYPE_STUB (thistype) || 0))

The only other place that TYPE_STUB is set in dwarf2read.c:
read_structure_type() is for TYPE_CODE_ENUM, which is excluded from
TYPE_IS_OPAQUE.  (Not that I understand why.)  It is also set in gdbtypes.c:
allocate_stub_method() with TYPE_CODE_METHOD, which is excluded from the test.
I don't understand why TYPE_CODE_CLASS is excluded from TYPE_IS_OPAQUE
either, since everything else is the same for structs and classes.

A test with ICC (after the patch) doesn't show any difference
whether TYPE_STUB_SUPPORTED is set or not, as expected.

As far as I can see, TYPE_STUB_SUPPORTED does nothing.

I'm happy to remove it and all the associated dreck and
reorder the test as you suggest.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [PATCH] Fix check for ICC incomplete struct types

[Tom Tromey]

>>>>> "Michael" == Michael Eager <eager@eagerm.com> writes:

Michael> TYPE_IS_OPAQUE does honor TYPE_STUB and checks it before
Michael> TYPE_STUB_SUPPORTED.  Which is absolutely screwy, since
Michael> TYPE_STUB_SUPPORTED is set for all non-ICC producers.

Right you are.
I think TYPE_STUB_SUPPORTED is there to handle just the case where
TYPE_STUB is zero.  I guess the older symbol readers didn't set
TYPE_STUB, so this hack was introduced.  If TYPE_STUB is set, then it's
safe to assume that the symbol reader knew what it was doing.

Michael> I'm happy to remove it and all the associated dreck and
Michael> reorder the test as you suggest.

I think my variant is mildly better because it consolidates all the
TYPE_STUB bits into a single paragraph.  If you agree, please go ahead.

Tom