Fix spurious errors that would be emitted when DW_TAG_subprogram DIEs had mutliple ranges in DW_AT_ranges.

[clayborg]

Summary

llvm-gsymutil would emit errors about address ranges for DW_TAG_inlined_subroutine DIEs whose address range didn't exist in the parent inline information. When a DW_TAG_subprogram DIE has more than one address range with a DW_AT_ranges attribute, we emit multiple FunctionInfo objets, one for each range of a function. When we parsed the inline information, it might have inline contribution that appear in any of the function's ranges, and if we were parsing the first range of a function, all inline entries that appeared in other valid ranges of the functions would end up emitting error messages. This patch fixes this by always passing down the full list of ranges, even if they aren't being used in the parse of the information. This eliminates reporting of errors when we shouldn't have been emitting error messages. Added a test to track this and ensure this doesn't regress.

[clayborg]

Update to not warn about inline functions not being in the parent address ranges if:

• the concrete function has more than one range and there are no inline ranges in the current range
• all top level inline functions have empty address ranges (which mean they were elided)

[kusmour]

llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp

237–238

Should we remove the curly brackets for one-line if branching?

[clayborg]

llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp

237–238

If the else statement has curly brackets, I believe we need them on the "if" too, even if it is a single line.

[dblaikie]

llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp

212

We've got a utility for checking for tombstones, perhaps that should be used here? (I guess this check is essentially for detecting tombstoned ranges?)

[clayborg]

llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp

212

The normal path as clang uses zero as the tombstone value, so that will mean the LowPC will always be zero, but it could be -1, which would omit it here, which is ok. We are just making sure we don't put invalid ranges, including empties into the returned ranges. Any tombstone values are actually removed by checking if they exist in the TextRanges that the GsymCreator contains, so if an address is tombstoned to either zero or to -1, those ranges won't fall into the actual address ranges of the sections with execute permissions. Checking if functions exist inside one of the TextRanges is the main way we remove invalid address ranges in the GSYM conversion process.

[dblaikie]

llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp

212

Checking if functions exist inside one of the TextRanges is the main way we remove invalid address ranges in the GSYM conversion process.

Not sure I follow (I don't understand a lot about GSYM's implementation) - but it'd be good to check for tombstones only in one place, rather than multiple. Could this checking here subsume/replace the checking done in TextRanges, or the other way around? (could the checking done in TextRanges be made more robust/modified in some way so as to not need this checking here)

(& the tombstoning checks in libDebugInfoDWARF only check for the -1 "official" tombstone, since 0 might be a valid address on some architectures so can't be used as a clear proof of tombstoning... so these checks are necessarily a bit non-portable, etc - all the more reason to make them in as few places as possible to make it clearer rather than having them somewhat quietly showing up in different places)

llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp

if (!CheckChildren)
     return false;
   for (DWARFDie ChildDie : Die.children()) {
     if (hasInlineInfo(ChildDie, Depth + 1))
       return true;
   }
   return false;
 }
 
+static AddressRanges
+ConvertDWARFRanges(const DWARFAddressRangesVector &DwarfRanges) {
+  AddressRanges Ranges;
+  for (const DWARFAddressRange &DwarfRange : DwarfRanges) {
+    if (DwarfRange.LowPC < DwarfRange.HighPC)
+      Ranges.insert({DwarfRange.LowPC, DwarfRange.HighPC});
+  }
+  return Ranges;
+}
+
 static void parseInlineInfo(GsymCreator &Gsym, raw_ostream *Log, CUInfo &CUI,
                             DWARFDie Die, uint32_t Depth, FunctionInfo &FI,
-                            InlineInfo &parent) {
+                            InlineInfo &Parent,
+                            const AddressRanges &AllParentRanges) {
   if (!hasInlineInfo(Die, Depth))
     return;
 
   dwarf::Tag Tag = Die.getTag();
   if (Tag == dwarf::DW_TAG_inlined_subroutine) {
     // create new InlineInfo and append to parent.children
     InlineInfo II;
+    AddressRanges AllInlineRanges;
     Expected<DWARFAddressRangesVector> RangesOrError = Die.getAddressRanges();
     if (RangesOrError) {
-      for (const DWARFAddressRange &Range : RangesOrError.get()) {
-        // Check that the inlined function is within the any of the range the
-        // parent InlineInfo. If it isn't remove it!
-        AddressRange InlineRange(Range.LowPC, Range.HighPC);
+      AllInlineRanges = ConvertDWARFRanges(RangesOrError.get());
+      for (const AddressRange &InlineRange : AllInlineRanges) {
         // Check for empty inline range in case inline function was outlined
         // or has not code
         if (!InlineRange.empty()) {
-          if (parent.Ranges.contains(InlineRange)) {
+          if (Parent.Ranges.contains(InlineRange)) {
             II.Ranges.insert(InlineRange);
           } else if (Log) {
+            // Only warn if the current inline range is not within any of all
+            // of the parent ranges. If we have a DW_TAG_subpgram with multiple
+            // ranges we will emit a FunctionInfo for each range of that
+            // function that only emits information within the current range,
+            // so we only want to emit an error if the DWARF has issues, not
+            // when a range currently just isn't in the range we are currently
+            // parsing for.
+            if (!AllParentRanges.contains(InlineRange)) {
               *Log << "error: inlined function DIE at " << HEX32(Die.getOffset())
-                 << " has a range [" << HEX64(Range.LowPC) << " - "
-                 << HEX64(Range.HighPC) << ") that isn't contained in any "
-                 << "parent address ranges, this inline range will be "
+                  << " has a range [" << HEX64(InlineRange.start()) << " - "
+                  << HEX64(InlineRange.end()) << ") that isn't contained in "
+                  << "any parent address ranges, this inline range will be "
                      "removed.\n";
             }
           }
         }
       }
+    }
     if (II.Ranges.empty())
       return;
 
     if (auto NameIndex = getQualifiedNameIndex(Die, CUI.Language, Gsym))
       II.Name = *NameIndex;
     II.CallFile = CUI.DWARFToGSYMFileIndex(
         Gsym, dwarf::toUnsigned(Die.find(dwarf::DW_AT_call_file), 0));
     II.CallLine = dwarf::toUnsigned(Die.find(dwarf::DW_AT_call_line), 0);
     // parse all children and append to parent
     for (DWARFDie ChildDie : Die.children())
-      parseInlineInfo(Gsym, Log, CUI, ChildDie, Depth + 1, FI, II);
-    parent.Children.emplace_back(std::move(II));
+      parseInlineInfo(Gsym, Log, CUI, ChildDie, Depth + 1, FI, II,
+                      AllInlineRanges);
+    Parent.Children.emplace_back(std::move(II));
     return;
   }
   if (Tag == dwarf::DW_TAG_subprogram || Tag == dwarf::DW_TAG_lexical_block) {
     // skip this Die and just recurse down
     for (DWARFDie ChildDie : Die.children())
-      parseInlineInfo(Gsym, Log, CUI, ChildDie, Depth + 1, FI, parent);
+      parseInlineInfo(Gsym, Log, CUI, ChildDie, Depth + 1, FI, Parent,
+                      AllParentRanges);
   }
 }
 
 static void convertFunctionLineTable(raw_ostream *Log, CUInfo &CUI,
                                      DWARFDie Die, GsymCreator &Gsym,
                                      FunctionInfo &FI) {
   std::vector<uint32_t> RowVector;
   const uint64_t StartAddress = FI.startAddress();
case dwarf::DW_TAG_subprogram: {
     if (!NameIndex) {
       if (OS) {
         *OS << "error: function at " << HEX64(Die.getOffset())
             << " has no name\n ";
         Die.dump(*OS, 0, DIDumpOptions::getForSingleDIE());
       }
       break;
     }
+    // All ranges for the subprogram DIE in case it has multiple. We need to
+    // pass this down into parseInlineInfo so we don't warn about inline
+    // ranges that are not in the current subrange of a function when they
+    // actually are in another subgrange. We do this because when a function
+    // has discontiguos ranges, we create multiple function entries with only
+    // the info for that range contained inside of it.
+    AddressRanges AllSubprogramRanges = ConvertDWARFRanges(Ranges);
 
     // Create a function_info for each range
     for (const DWARFAddressRange &Range : Ranges) {
       // The low PC must be less than the high PC. Many linkers don't remove
       // DWARF for functions that don't get linked into the final executable.
       // If both the high and low pc have relocations, linkers will often set
       // the address values for both to the same value to indicate the function
       // has been remove. Other linkers have been known to set the one or both
for (const DWARFAddressRange &Range : Ranges) {
           }
         }
         break;
       }
 
       FunctionInfo FI;
       FI.Range = {Range.LowPC, Range.HighPC};
       FI.Name = *NameIndex;
-      if (CUI.LineTable) {
+      if (CUI.LineTable)
         convertFunctionLineTable(OS, CUI, Die, Gsym, FI);
-      }
+
       if (hasInlineInfo(Die, 0)) {
         FI.Inline = InlineInfo();
         FI.Inline->Name = *NameIndex;
         FI.Inline->Ranges.insert(FI.Range);
-        parseInlineInfo(Gsym, OS, CUI, Die, 0, FI, *FI.Inline);
+        parseInlineInfo(Gsym, OS, CUI, Die, 0, FI, *FI.Inline,
+                        AllSubprogramRanges);
         // Make sure we at least got some valid inline info other than just
         // the top level function. If we didn't then remove the inline info
         // from the function info. We have seen cases where LTO tries to modify
         // the DWARF for functions and it messes up the address ranges for
         // the inline functions so it is no longer valid.
         //
         // By checking if there are any valid children on the top level inline
         // information object, we will know if we got anything valid from the

llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp

TEST(GSYMTest, TestFinalizeForLineTables) {
   auto ExpFI2 = GR->getFunctionInfo(0x2000);
   ASSERT_THAT_EXPECTED(ExpFI2, Succeeded());
   ASSERT_EQ(ExpFI2->Range, AddressRange(0x2000, 0x2050));
   EXPECT_TRUE(ExpFI2->OptLineTable.has_value());
   EXPECT_FALSE(ExpFI2->Inline.has_value());
   StringRef FuncName2 = GR->getString(ExpFI2->Name);
   EXPECT_EQ(FuncName2, "bar");
 }
+
+
+TEST(GSYMTest, TestRangeWarnings) {
+  // This example has a single compile unit that has a DW_TAG_subprogram that
+  // has two discontiguous ranges. We will create two FunctionInfo objects for
+  // each range in the function that only contains info for each range. We also
+  // want to verify that we only emit errors and warnings for ranges that
+  // aren't contained in any parent address ranges if this is true. Prior to
+  // this fix we would create two FunctionInfo objects and as each one was
+  // being created we would end up warning about all of the ranges that weren't
+  // in the current FunctionInfo's range even though the DWARF was well formed.
+  // Now we don't incorrectly emit errors when there are none.
+  //
+  // 0x0000000b: DW_TAG_compile_unit
+  //               DW_AT_name ("/tmp/main.cpp")
+  //               DW_AT_language (DW_LANG_C)
+  //               DW_AT_stmt_list  (0x00000000)
+  //
+  // 0x00000015:   DW_TAG_subprogram
+  //                 DW_AT_name ("foo")
+  //                 DW_AT_ranges (0x00000000
+  //                    [0x0000000000001000, 0x0000000000001050)
+  //                    [0x0000000000002000, 0x0000000000002050))
+  //
+  // 0x0000001e:     DW_TAG_inlined_subroutine
+  //                   DW_AT_name ("inline1")
+  //                   DW_AT_ranges (0x00000030
+  //                      [0x0000000000001010, 0x0000000000001040)
+  //                      [0x0000000000002010, 0x0000000000002040))
+  //                   DW_AT_call_file  ("/tmp/main.cpp")
+  //                   DW_AT_call_line  (11)
+  //
+  // 0x0000002f:       DW_TAG_inlined_subroutine
+  //                     DW_AT_name ("inline2")
+  //                     DW_AT_ranges (0x00000060
+  //                        [0x0000000000001015, 0x0000000000001020)
+  //                        [0x0000000000002015, 0x0000000000002020))
+  //                     DW_AT_call_file  ("/tmp/inline.h")
+  //                     DW_AT_call_line  (21)
+  //
+  // 0x00000040:       NULL
+  //
+  // 0x00000041:     NULL
+  //
+  // 0x00000042:   NULL
+
+  StringRef yamldata = R"(
+  debug_str:
+    - ''
+    - '/tmp/main.cpp'
+    - foo
+    - inline1
+    - inline2
+  debug_abbrev:
+    - ID:              0
+      Table:
+        - Code:            0x1
+          Tag:             DW_TAG_compile_unit
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_language
+              Form:            DW_FORM_udata
+            - Attribute:       DW_AT_stmt_list
+              Form:            DW_FORM_sec_offset
+        - Code:            0x2
+          Tag:             DW_TAG_subprogram
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_ranges
+              Form:            DW_FORM_sec_offset
+        - Code:            0x3
+          Tag:             DW_TAG_inlined_subroutine
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_ranges
+              Form:            DW_FORM_sec_offset
+            - Attribute:       DW_AT_call_file
+              Form:            DW_FORM_data4
+            - Attribute:       DW_AT_call_line
+              Form:            DW_FORM_data4
+        - Code:            0x4
+          Tag:             DW_TAG_inlined_subroutine
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_ranges
+              Form:            DW_FORM_sec_offset
+            - Attribute:       DW_AT_call_file
+              Form:            DW_FORM_data4
+            - Attribute:       DW_AT_call_line
+              Form:            DW_FORM_data4
+  debug_ranges:
+    - Offset:          0x0
+      AddrSize:        0x8
+      Entries:
+        - LowOffset:       0x1000
+          HighOffset:      0x1050
+        - LowOffset:       0x2000
+          HighOffset:      0x2050
+    - Offset:          0x30
+      AddrSize:        0x8
+      Entries:
+        - LowOffset:       0x1010
+          HighOffset:      0x1040
+        - LowOffset:       0x2010
+          HighOffset:      0x2040
+    - Offset:          0x60
+      AddrSize:        0x8
+      Entries:
+        - LowOffset:       0x1015
+          HighOffset:      0x1020
+        - LowOffset:       0x2015
+          HighOffset:      0x2020
+  debug_info:
+    - Length:          0x3F
+      Version:         4
+      AbbrevTableID:   0
+      AbbrOffset:      0x0
+      AddrSize:        8
+      Entries:
+        - AbbrCode:        0x1
+          Values:
+            - Value:           0x1
+            - Value:           0x2
+            - Value:           0x0
+        - AbbrCode:        0x2
+          Values:
+            - Value:           0xF
+            - Value:           0x0
+        - AbbrCode:        0x3
+          Values:
+            - Value:           0x13
+            - Value:           0x30
+            - Value:           0x1
+            - Value:           0xB
+        - AbbrCode:        0x4
+          Values:
+            - Value:           0x1B
+            - Value:           0x60
+            - Value:           0x2
+            - Value:           0x15
+        - AbbrCode:        0x0
+        - AbbrCode:        0x0
+        - AbbrCode:        0x0
+  debug_line:
+    - Length:          120
+      Version:         2
+      PrologueLength:  48
+      MinInstLength:   1
+      DefaultIsStmt:   1
+      LineBase:        251
+      LineRange:       14
+      OpcodeBase:      13
+      StandardOpcodeLengths: [ 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 ]
+      IncludeDirs:
+        - '/tmp'
+      Files:
+        - Name:            main.cpp
+          DirIdx:          1
+          ModTime:         0
+          Length:          0
+        - Name:            inline.h
+          DirIdx:          1
+          ModTime:         0
+          Length:          0
+      Opcodes:
+        - Opcode:          DW_LNS_extended_op
+          ExtLen:          9
+          SubOpcode:       DW_LNE_set_address
+          Data:            4096
+        - Opcode:          DW_LNS_advance_line
+          SData:           9
+          Data:            0
+        - Opcode:          DW_LNS_copy
+          Data:            0
+        - Opcode:          DW_LNS_advance_pc
+          Data:            16
+        - Opcode:          DW_LNS_set_file
+          Data:            2
+        - Opcode:          DW_LNS_advance_line
+          SData:           10
+          Data:            0
+        - Opcode:          DW_LNS_copy
+          Data:            0
+        - Opcode:          DW_LNS_advance_pc
+          Data:            16
+        - Opcode:          DW_LNS_advance_line
+          SData:           1
+          Data:            0
+        - Opcode:          DW_LNS_copy
+          Data:            0
+        - Opcode:          DW_LNS_advance_pc
+          Data:            48
+        - Opcode:          DW_LNS_set_file
+          Data:            1
+        - Opcode:          DW_LNS_advance_line
+          SData:           -10
+          Data:            0
+        - Opcode:          DW_LNS_extended_op
+          ExtLen:          1
+          SubOpcode:       DW_LNE_end_sequence
+          Data:            0
+        - Opcode:          DW_LNS_extended_op
+          ExtLen:          9
+          SubOpcode:       DW_LNE_set_address
+          Data:            8192
+        - Opcode:          DW_LNS_advance_line
+          SData:           19
+          Data:            0
+        - Opcode:          DW_LNS_copy
+          Data:            0
+        - Opcode:          DW_LNS_advance_pc
+          Data:            16
+        - Opcode:          DW_LNS_set_file
+          Data:            2
+        - Opcode:          DW_LNS_copy
+          Data:            0
+        - Opcode:          DW_LNS_advance_pc
+          Data:            16
+        - Opcode:          DW_LNS_advance_line
+          SData:           1
+          Data:            0
+        - Opcode:          DW_LNS_copy
+          Data:            0
+        - Opcode:          DW_LNS_advance_pc
+          Data:            48
+        - Opcode:          DW_LNS_set_file
+          Data:            1
+        - Opcode:          DW_LNS_extended_op
+          ExtLen:          1
+          SubOpcode:       DW_LNE_end_sequence
+          Data:            0
+  )";
+  auto ErrOrSections = DWARFYAML::emitDebugSections(yamldata);
+  ASSERT_THAT_EXPECTED(ErrOrSections, Succeeded());
+  std::unique_ptr<DWARFContext> DwarfContext =
+      DWARFContext::create(*ErrOrSections, 8);
+  ASSERT_TRUE(DwarfContext.get() != nullptr);
+  std::string errors;
+  raw_string_ostream OS(errors);
+  GsymCreator GC;
+  DwarfTransformer DT(*DwarfContext, GC);
+  const uint32_t ThreadCount = 1;
+  ASSERT_THAT_ERROR(DT.convert(ThreadCount, &OS), Succeeded());
+  ASSERT_THAT_ERROR(GC.finalize(OS), Succeeded());
+  OS.flush();
+  SmallString<512> Str;
+  raw_svector_ostream OutStrm(Str);
+  const auto ByteOrder = support::endian::system_endianness();
+  FileWriter FW(OutStrm, ByteOrder);
+  ASSERT_THAT_ERROR(GC.encode(FW), Succeeded());
+  Expected<GsymReader> GR = GsymReader::copyBuffer(OutStrm.str());
+  ASSERT_THAT_EXPECTED(GR, Succeeded());
+  // There should be two functions in our GSYM.
+  EXPECT_EQ(GR->getNumAddresses(), 2u);
+  // Verify "foo" is present and has a line table
+  auto ExpFI = GR->getFunctionInfo(0x1000);
+  ASSERT_THAT_EXPECTED(ExpFI, Succeeded());
+  ASSERT_EQ(ExpFI->Range, AddressRange(0x1000, 0x1050));
+  EXPECT_TRUE(ExpFI->OptLineTable.has_value());
+  EXPECT_TRUE(ExpFI->Inline.has_value());
+  StringRef FuncName = GR->getString(ExpFI->Name);
+  EXPECT_EQ(FuncName, "foo");
+
+  // Verify "foo" is present and has a line table
+  auto ExpFI2 = GR->getFunctionInfo(0x2000);
+  ASSERT_THAT_EXPECTED(ExpFI2, Succeeded());
+  ASSERT_EQ(ExpFI2->Range, AddressRange(0x2000, 0x2050));
+  EXPECT_TRUE(ExpFI2->OptLineTable.has_value());
+  EXPECT_TRUE(ExpFI2->Inline.has_value());
+  StringRef FuncName2 = GR->getString(ExpFI2->Name);
+  EXPECT_EQ(FuncName2, "foo");
+
+  // Make sure we don't see spurious errors in the output:
+  EXPECT_TRUE(errors.find("error:") == std::string::npos);
+}