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

[John Steele Scott <toojays at toojays dot net>]

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