[llvm] r224780 - DIBuilder: Similar to createPointerType, make createMemberPointerType take

[Adrian Prantl]

> On Dec 23, 2014, at 4:04 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Dec 23, 2014 at 3:55 PM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
> 
>> On Dec 23, 2014, at 3:42 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Tue, Dec 23, 2014 at 11:11 AM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>> Author: adrian
>> Date: Tue Dec 23 13:11:47 2014
>> New Revision: 224780
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=224780&view=rev <http://llvm.org/viewvc/llvm-project?rev=224780&view=rev>
>> Log:
>> DIBuilder: Similar to createPointerType, make createMemberPointerType take
>> a size and alignment. Several assertions in DwarfDebug rely on all variable
>> types to report back a size, or to be derived from a type with a size.
>> 
>> Are these reasonable assertions?
> 
> In my opinion, yes. I added them to sanity-check DW_OP_piece expressions.
> 
> Perhaps we don't need to sanity check these? I can sort of see it, but on the other hand we could jsut treat the size in the metadata as being an opaque value used to emit DWARF, not something we should necessarily rely on/care about in any semantic way. (that way the absence of the value in the metadata reflects the absence of the value in the emitted DWARF, etc - simpler model, allows greater flexibility for clients (some LLVM clients might have different languages they want to generate debug info for with different types that they want to include/omit the size from, etc))

I see this mostly as a way to sanity-check what the optimization passes (sroa, legalize, ..., hopefully more!) do to the debug info. Unless there is a specific frontend that needs the flexibility of not specifying type sizes, I’d rather use the information that we have to make the backend more robust and able to verify the integrity of the metadata. And if there is a frontend that needs to be able to omit type sizes, we could still address that by making those checks optional, while still having the improved safety for clang.

-- adrian
>  
> 
>> Does this change modify the resulting DWARF? (should we be putting a DW_AT_size on such things? I assume not, but haven't looked/compared/etc)
> 
> .. looking.. Actually it does, but that was unintentional. If want to keep the old behavior, we should extend this check to also include member function pointers.
> 
> /// constructTypeDIE - Construct derived type die from DIDerivedType.
> void DwarfUnit::constructTypeDIE(DIE &Buffer, DIDerivedType DTy) {
>   ...
>  // Add size if non-zero (derived types might be zero-sized.)
>   if (Size && Tag != dwarf::DW_TAG_pointer_type)
>     addUInt(Buffer, dwarf::DW_AT_byte_size, None, Size);
> 
> I don't hugely mind adding a special case for member pointers the same as pointer_type that's already there - but I do wonder if it's the right approach (given my reasoning above)
>  
> I’ll do that — just to be consistent with the past. Specifying the size of a pointer in DWARF seems redundant as it should be specified by the target; assuming no target we want to support has more than one pointer size. 
> 
> -- adrian
> 
>>  
>> 
>> Tested in CFE.
>> 
>> Modified:
>>     llvm/trunk/include/llvm/IR/DIBuilder.h
>>     llvm/trunk/lib/IR/DIBuilder.cpp
>> 
>> Modified: llvm/trunk/include/llvm/IR/DIBuilder.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=224780&r1=224779&r2=224780&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=224780&r1=224779&r2=224780&view=diff>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/IR/DIBuilder.h (original)
>> +++ llvm/trunk/include/llvm/IR/DIBuilder.h Tue Dec 23 13:11:47 2014
>> @@ -172,8 +172,12 @@ namespace llvm {
>> 
>>      /// \brief Create debugging information entry for a pointer to member.
>>      /// @param PointeeTy Type pointed to by this pointer.
>> +    /// @param SizeInBits  Size.
>> +    /// @param AlignInBits Alignment. (optional)
>>      /// @param Class Type for which this pointer points to members of.
>> -    DIDerivedType createMemberPointerType(DIType PointeeTy, DIType Class);
>> +    DIDerivedType createMemberPointerType(DIType PointeeTy, DIType Class,
>> +                                          uint64_t SizeInBits,
>> +                                          uint64_t AlignInBits = 0);
>> 
>>      /// createReferenceType - Create debugging information entry for a c++
>>      /// style reference or rvalue reference type.
>> 
>> Modified: llvm/trunk/lib/IR/DIBuilder.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=224780&r1=224779&r2=224780&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=224780&r1=224779&r2=224780&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
>> +++ llvm/trunk/lib/IR/DIBuilder.cpp Tue Dec 23 13:11:47 2014
>> @@ -328,14 +328,15 @@ DIBuilder::createPointerType(DIType Poin
>>    return DIDerivedType(MDNode::get(VMContext, Elts));
>>  }
>> 
>> -DIDerivedType DIBuilder::createMemberPointerType(DIType PointeeTy,
>> -                                                 DIType Base) {
>> +DIDerivedType
>> +DIBuilder::createMemberPointerType(DIType PointeeTy, DIType Base,
>> +                                   uint64_t SizeInBits, uint64_t AlignInBits) {
>>    // Pointer types are encoded in DIDerivedType format.
>>    Metadata *Elts[] = {HeaderBuilder::get(dwarf::DW_TAG_ptr_to_member_type)
>>                            .concat(StringRef())
>>                            .concat(0) // Line
>> -                          .concat(0) // Size
>> -                          .concat(0) // Align
>> +                          .concat(SizeInBits) // Size
>> +                          .concat(AlignInBits) // Align
>>                            .concat(0) // Offset
>>                            .concat(0) // Flags
>>                            .get(VMContext),
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141223/9756b682/attachment.html>