[PATCH] D27995: Add the ability for DWARFDie objects to get the parent DWARFDie.

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 20 12:49:37 PST 2016


dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:412-416
+  // Find the next DIE whose depth is the same as the Die's depth.
+  for (size_t I=getDIEIndex(Die)+1, EndIdx = DieArray.size(); I<EndIdx; ++I) {
+    if (DieArray[I].getDepth() == Depth)
+      return getDIEAtIndex(I);
+  }
----------------
Could this accidentally walk into children of another DIE?

(eg: if this is the last child of a DIE - then the next DIE at the same level could be not a sibling, but a child of the following DIE?)

  A
    A1
  B
    B1

The next DIE at the same level as A1 is B1 (unless the 'null' at the end of A's child list qualifies as the next sibling? But then, if the null is a DIE, what about its next sibling?)

If I read the test case correctly, it's testing CU{int, subprogram{param(i), param(j)}} - so it doesn't test this case where there's a following child at the same level that isn't a sibling. So if it does work, might be good to have a test case for it.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:993-1004
+  dwarfgen::DIE SubprogramDie = CUDie.addChild(DW_TAG_subprogram);
+  SubprogramDie.addAttribute(DW_AT_name, DW_FORM_strp, "test");
+  SubprogramDie.addAttribute(DW_AT_low_pc, DW_FORM_addr, 0x1000U);
+  SubprogramDie.addAttribute(DW_AT_high_pc, DW_FORM_addr, 0x1100U);
+  
+  dwarfgen::DIE Arg1Die = SubprogramDie.addChild(DW_TAG_formal_parameter);
+  Arg1Die.addAttribute(DW_AT_name, DW_FORM_strp, "i");
----------------
I'd actually be inclined to make /less/ realistic DWARF, just to make it obvious that the semantics of DWARF aren't important here. We're just interested in tags and their children (probably don't have attributes at all - use the same tag for everything (perhaps some 'unknown' tag value, if possible)).


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1072-1112
+TEST(DWARFDebugInfo, TestDWARF32Version2Addr4Relations) {
+  // Test that we can decode address values in DWARF32, version 2, with 4 byte
+  // addresses.
+  typedef uint32_t AddrType;
+  TestRelations<2, AddrType>();
+}
+
----------------
I don't think we need to test all these variations - the code seems pretty agnostic/independent of these features, such that the variations don't add a lot of value to the test coverage.


https://reviews.llvm.org/D27995





More information about the llvm-commits mailing list