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

[Michael Eager <eager@eagerm.com>]

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