[llvm] r187963 - Make sure that if we're going to attempt to add a type to a DIE that

David Blaikie dblaikie at gmail.com
Sat Aug 10 13:31:29 PDT 2013


On Thu, Aug 8, 2013 at 12:40 AM, Eric Christopher <echristo at gmail.com>wrote:

> Author: echristo
> Date: Thu Aug  8 02:40:37 2013
> New Revision: 187963
>
> URL: http://llvm.org/viewvc/llvm-project?rev=187963&view=rev
> Log:
> Make sure that if we're going to attempt to add a type to a DIE that
> the type exists.
>
> Fix up cases where we weren't checking for optional types and add
> an assert to addType to make sure we catch this in the future.
>
> Fix up a testcase that was using the tag for DW_TAG_array_type
> when it meant DW_TAG_enumeration_type.
>
> Modified:
>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
>     llvm/trunk/lib/IR/DIBuilder.cpp
>     llvm/trunk/lib/IR/DebugInfo.cpp
>     llvm/trunk/test/CodeGen/ARM/debug-info-blocks.ll
>
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp?rev=187963&r1=187962&r2=187963&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp (original)
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp Thu Aug  8
> 02:40:37 2013
> @@ -776,8 +776,7 @@ DIE *CompileUnit::getOrCreateTypeDIE(con
>
>  /// addType - Add a new type attribute to the specified entity.
>  void CompileUnit::addType(DIE *Entity, DIType Ty, uint16_t Attribute) {
> -  if (!Ty.isType())
> -    return;
> +  assert(Ty && "Trying to add a type that doesn't exist?");
>
>    // Check for pre-existence.
>    DIEEntry *Entry = getDIEEntry(Ty);
> @@ -863,7 +862,8 @@ void CompileUnit::constructTypeDIE(DIE &
>
>    // Map to main type, void will not have a type.
>    DIType FromTy = DTy.getTypeDerivedFrom();
> -  addType(&Buffer, FromTy);
> +  if (FromTy)
> +    addType(&Buffer, FromTy);
>
>    // Add name if not anonymous or intermediate type.
>    if (!Name.empty())
> @@ -947,10 +947,11 @@ void CompileUnit::constructTypeDIE(DIE &
>    }
>      break;
>    case dwarf::DW_TAG_subroutine_type: {
> -    // Add return type.
> +    // Add return type. A void return won't have a type.
>      DIArray Elements = CTy.getTypeArray();
>      DIDescriptor RTy = Elements.getElement(0);
> -    addType(&Buffer, DIType(RTy));
> +    if (RTy)
> +      addType(&Buffer, DIType(RTy));
>
>      bool isPrototyped = true;
>      // Add arguments.
> @@ -1137,7 +1138,11 @@ CompileUnit::getOrCreateTemplateValuePar
>      return ParamDIE;
>
>    ParamDIE = new DIE(VP.getTag());
> -  addType(ParamDIE, VP.getType());
> +
> +  // Add the type if there is one, template template and template
> parameter
> +  // packs will not have a type.
>

Changing the test to: if (VP.getTag() == DW_TAG_template_value_parameter)
might make it more clear (between the comment above & this condition, it
would be clear which kinds of non-type template parameters we're dealing
with here)

(wonder if we could sink this further down into the if/else if chain below
- instead of `if (constant) { ... } else if (global) { ... } else if
(template template) { ... } else if (template parameter pack) { ... }` do
something like `switch (tag) { case value_parameter: addType; if (constant)
{ ... } else if (global) { ... } case template template: ... case template
parameter pack: ... }` - not sure if there are cases where we don't have a
value but we do have a type for a non-type template parameter... perhaps
there are, in which case this isn't so tidy either way)


> +  if (VP.getType())
> +    addType(ParamDIE, VP.getType());
>    if (!VP.getName().empty())
>      addString(ParamDIE, dwarf::DW_AT_name, VP.getName());
>    if (Value *Val = VP.getValue()) {
> @@ -1246,13 +1251,14 @@ DIE *CompileUnit::getOrCreateSubprogramD
>         Language == dwarf::DW_LANG_ObjC))
>      addFlag(SPDie, dwarf::DW_AT_prototyped);
>
> -  // Add Return Type.
> +  // Add Return Type. A void return type will not have a type.
>

Moving this comment down closer to where the null-test is occurring might
be helpful.


>    DICompositeType SPTy = SP.getType();
>    assert(SPTy.getTag() == dwarf::DW_TAG_subroutine_type &&
>           "the type of a subprogram should be a subroutine");
>
>    DIArray Args = SPTy.getTypeArray();
> -  addType(SPDie, DIType(Args.getElement(0)));
> +  if (Args.getElement(0))
> +    addType(SPDie, DIType(Args.getElement(0)));
>
>    unsigned VK = SP.getVirtuality();
>    if (VK) {
> @@ -1502,9 +1508,8 @@ void CompileUnit::constructArrayTypeDIE(
>    if (CTy->isVector())
>      addFlag(&Buffer, dwarf::DW_AT_GNU_vector);
>
> -  // Emit derived type.
> +  // Emit the element type.
>    addType(&Buffer, CTy->getTypeDerivedFrom());
> -  DIArray Elements = CTy->getTypeArray();
>
>    // Get an anonymous type for index type.
>    // FIXME: This type should be passed down from the front end
> @@ -1522,6 +1527,7 @@ void CompileUnit::constructArrayTypeDIE(
>    }
>
>    // Add subranges to array type.
> +  DIArray Elements = CTy->getTypeArray();
>    for (unsigned i = 0, N = Elements.getNumElements(); i < N; ++i) {
>      DIDescriptor Element = Elements.getElement(i);
>      if (Element.getTag() == dwarf::DW_TAG_subrange_type)
>
> Modified: llvm/trunk/lib/IR/DIBuilder.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=187963&r1=187962&r2=187963&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
> +++ llvm/trunk/lib/IR/DIBuilder.cpp Thu Aug  8 02:40:37 2013
> @@ -759,7 +759,6 @@ DICompositeType DIBuilder::createArrayTy
>  /// createVectorType - Create debugging information entry for a vector.
>  DICompositeType DIBuilder::createVectorType(uint64_t Size, uint64_t
> AlignInBits,
>                                              DIType Ty, DIArray
> Subscripts) {
> -
>    // A vector is an array type with the FlagVector flag applied.
>    Value *Elts[] = {
>      GetTagConstant(VMContext, dwarf::DW_TAG_array_type),
>
> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=187963&r1=187962&r2=187963&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
> +++ llvm/trunk/lib/IR/DebugInfo.cpp Thu Aug  8 02:40:37 2013
> @@ -483,6 +483,12 @@ bool DICompositeType::Verify() const {
>    if (!fieldIsMDNode(DbgNode, 12))
>      return false;
>
> +  // If this is an array type verify that we have a DIType in the derived
> type
> +  // field as that's the type of our element.
>

This seems rather narrow. Is this the exception moreso than the rule (ie:
most DICompositeTypes don't require a type they're derived from?)

And shouldn't this check be up in DIDerivedType? (is an array even a
DICompositeType? I wonder why it's not just a DIDerivedType)


> +  if (getTag() == dwarf::DW_TAG_array_type)
> +    if (!DIType(getTypeDerivedFrom()))
> +      return false;
> +
>    return DbgNode->getNumOperands() >= 10 && DbgNode->getNumOperands() <=
> 14;
>  }
>
>
> Modified: llvm/trunk/test/CodeGen/ARM/debug-info-blocks.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/debug-info-blocks.ll?rev=187963&r1=187962&r2=187963&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/debug-info-blocks.ll (original)
> +++ llvm/trunk/test/CodeGen/ARM/debug-info-blocks.ll Thu Aug  8 02:40:37
> 2013
> @@ -95,25 +95,25 @@ define hidden void @foobar_func_block_in
>  !llvm.dbg.cu = !{!0}
>
>  !0 = metadata !{i32 786449, metadata !153, i32 16, metadata !"Apple clang
> version 2.1", i1 false, metadata !"", i32 2, metadata !147, metadata !26,
> metadata !148, null, null, metadata !""} ; [ DW_TAG_compile_unit ]
> -!1 = metadata !{i32 786433, metadata !160, metadata !0, metadata !"", i32
> 248, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !3, i32 0, i32 0} ; [
> DW_TAG_enumeration_type ]
> +!1 = metadata !{i32 786436, metadata !160, metadata !0, metadata !"", i32
> 248, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !3, i32 0, i32 0} ; [
> DW_TAG_enumeration_type ]
>  !2 = metadata !{i32 786473, metadata !160} ; [ DW_TAG_file_type ]
>  !3 = metadata !{metadata !4}
>  !4 = metadata !{i32 786472, metadata !"Ver1", i64 0} ; [
> DW_TAG_enumerator ]
> -!5 = metadata !{i32 786433, metadata !160, metadata !0, metadata !"Mode",
> i32 79, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !7, i32 0, i32 0} ; [
> DW_TAG_enumeration_type ]
> +!5 = metadata !{i32 786436, metadata !160, metadata !0, metadata !"Mode",
> i32 79, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !7, i32 0, i32 0} ; [
> DW_TAG_enumeration_type ]
>  !6 = metadata !{i32 786473, metadata !161} ; [ DW_TAG_file_type ]
>  !7 = metadata !{metadata !8}
>  !8 = metadata !{i32 786472, metadata !"One", i64 0} ; [ DW_TAG_enumerator
> ]
> -!9 = metadata !{i32 786433, metadata !149, metadata !0, metadata !"", i32
> 15, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !11, i32 0, i32 0} ; [
> DW_TAG_enumeration_type ]
> +!9 = metadata !{i32 786436, metadata !149, metadata !0, metadata !"", i32
> 15, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !11, i32 0, i32 0} ; [
> DW_TAG_enumeration_type ]
>  !10 = metadata !{i32 786473, metadata !149} ; [ DW_TAG_file_type ]
>  !11 = metadata !{metadata !12, metadata !13}
>  !12 = metadata !{i32 786472, metadata !"Unknown", i64 0} ; [
> DW_TAG_enumerator ]
>  !13 = metadata !{i32 786472, metadata !"Known", i64 1} ; [
> DW_TAG_enumerator ]
> -!14 = metadata !{i32 786433, metadata !150, metadata !0, metadata !"",
> i32 20, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !16, i32 0, i32 0} ;
> [ DW_TAG_enumeration_type ]
> +!14 = metadata !{i32 786436, metadata !150, metadata !0, metadata !"",
> i32 20, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !16, i32 0, i32 0} ;
> [ DW_TAG_enumeration_type ]
>  !15 = metadata !{i32 786473, metadata !150} ; [ DW_TAG_file_type ]
>  !16 = metadata !{metadata !17, metadata !18}
>  !17 = metadata !{i32 786472, metadata !"Single", i64 0} ; [
> DW_TAG_enumerator ]
>  !18 = metadata !{i32 786472, metadata !"Double", i64 1} ; [
> DW_TAG_enumerator ]
> -!19 = metadata !{i32 786433, metadata !151, metadata !0, metadata !"",
> i32 14, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !21, i32 0, i32 0} ;
> [ DW_TAG_enumeration_type ]
> +!19 = metadata !{i32 786436, metadata !151, metadata !0, metadata !"",
> i32 14, i64 32, i64 32, i32 0, i32 0, i32 0, metadata !21, i32 0, i32 0} ;
> [ DW_TAG_enumeration_type ]
>  !20 = metadata !{i32 786473, metadata !151} ; [ DW_TAG_file_type ]
>  !21 = metadata !{metadata !22}
>  !22 = metadata !{i32 786472, metadata !"Eleven", i64 0} ; [
> DW_TAG_enumerator ]
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> 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/20130810/9f832da9/attachment.html>


More information about the llvm-commits mailing list