Fix a bug introduced by the move of AddressRanges.h into ADT.

[clayborg]

Summary

The bug was introduced when the AddressRange class was no longer able to modify the End address directly and the entire range of the .text address range that contained the trailing empty symbol was replaced. There was no unit test for this, so it wasn't caught. I fixed the bug and added a unit test for it.

The effects of this bug are serious as the AddressOffsetSize in the header would be incorrectly calculated and an invalid GSYM would be created.

[avl]

Thank you for catching this!

llvm/include/llvm/ADT/AddressRanges.h

30 ↗

(On Diff #436976)

I would suggest to leave AddressRange to be immutable.
It might prevent from side effects if borders of range would be changed.

If you think it is still better to have setStart/setEnd then we need to add assertions:

assert(Start <= End);

llvm/lib/DebugInfo/GSYM/GsymCreator.cpp

295

without setStart/setEnd we can reconstruct Address Range:

Funcs.back().Range = {Funcs.back().Range.start(), Range->end()};

This would allow not to introduce setters.

[clayborg]

Removed accessors from AddressRange class to keep it immutable.

[avl]

LGTM

llvm/lib/DebugInfo/GSYM/GsymCreator.cpp

llvm::Error GsymCreator::finalize(llvm::raw_ostream &OS) {
   // If our last function info entry doesn't have a size and if we have valid
   // text ranges, we should set the size of the last entry since any search for
   // a high address might match our last entry. By fixing up this size, we can
   // help ensure we don't cause lookups to always return the last symbol that
   // has no size when doing lookups.
   if (!Funcs.empty() && Funcs.back().Range.size() == 0 && ValidTextRanges) {
     if (auto Range =
             ValidTextRanges->getRangeThatContains(Funcs.back().Range.start())) {
-      Funcs.back().Range = *Range;
+      Funcs.back().Range = {Funcs.back().Range.start(), Range->end()};
     }
   }
   OS << "Pruned " << NumBefore - Funcs.size() << " functions, ended with "
      << Funcs.size() << " total\n";
   return Error::success();
 }
 
 uint32_t GsymCreator::insertString(StringRef S, bool Copy) {

llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp

TEST(GSYMTest, TestDWARFTextRanges) {
   ASSERT_THAT_EXPECTED(ExpFI, Succeeded());
   ASSERT_EQ(ExpFI->Range, AddressRange(0x1000, 0x2000));
   EXPECT_FALSE(ExpFI->OptLineTable.hasValue());
   EXPECT_FALSE(ExpFI->Inline.hasValue());
   StringRef MethodName = GR->getString(ExpFI->Name);
   EXPECT_EQ(MethodName, "main");
 }
 
+TEST(GSYMTest, TestEmptySymbolEndAddressOfTextRanges) {
+  // Test that if we have valid text ranges and we have a symbol with no size
+  // as the last FunctionInfo entry that the size of the symbol gets set to the
+  // end address of the text range.
+  GsymCreator GC;
+  AddressRanges TextRanges;
+  TextRanges.insert(AddressRange(0x1000, 0x2000));
+  GC.SetValidTextRanges(TextRanges);
+  GC.addFunctionInfo(FunctionInfo(0x1500, 0, GC.insertString("symbol")));
+  auto &OS = llvm::nulls();
+  ASSERT_THAT_ERROR(GC.finalize(OS), Succeeded());
+  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 only be one function in our GSYM.
+  EXPECT_EQ(GR->getNumAddresses(), 1u);
+  auto ExpFI = GR->getFunctionInfo(0x1500);
+  ASSERT_THAT_EXPECTED(ExpFI, Succeeded());
+  ASSERT_EQ(ExpFI->Range, AddressRange(0x1500, 0x2000));
+  EXPECT_FALSE(ExpFI->OptLineTable.hasValue());
+  EXPECT_FALSE(ExpFI->Inline.hasValue());
+  StringRef MethodName = GR->getString(ExpFI->Name);
+  EXPECT_EQ(MethodName, "symbol");
+}
+
 TEST(GSYMTest, TestDWARFInlineInfo) {
   // Make sure we parse the line table and inline information correctly from
   // DWARF.
   StringRef yamldata = R"(
   debug_str:
     - ''
     - /tmp/main.c
     - main