[DWARF] - Refactoring of DWARFContextInMemory implementation.

[grimar]

Summary

This change is basically relative to D31136, where I initially wanted to
implement some relocations handling optimization which shows it can give
significant boost. Though even without any caching algorithm looks
code can have some cleanup at first.

Refactoring separates out code for taking symbol address, used in relocations
computation. That itself IMO reasonable cleanup.

[grimar]

• Removed unrelative clang-format changes (file is not clang-formated, I think we want to format it separatelly).

[aprantl]

lib/DebugInfo/DWARF/DWARFContext.cpp

626 ↗

(On Diff #94335)

///

635 ↗

(On Diff #94335)

nitpick: LLVM coding guidelines want full sentences in comments -> .

659 ↗

(On Diff #94335)

if (L && RSec != Obj.section_end())

664 ↗

(On Diff #94335)

if (uint64_t SectionLoadAddress = L->getSectionLoadAddress(*RSec))

[grimar]

• Addressed review comments.

lib/DebugInfo/DWARF/DWARFContext.cpp

626 ↗

(On Diff #94335)

Fixed.

635 ↗

(On Diff #94335)

Fixed, thanks ! (btw, it was original comment. It looks whole file needs to be reviewed, formatted and corrected to match LLVM coding style)

659 ↗

(On Diff #94335)

Fixed.

664 ↗

(On Diff #94335)

Fixed.

[echristo]

Looks like Adrian has this review, removing myself.

[aprantl]

One last comment in line 664 otherwise looks fine to me.

lib/DebugInfo/DWARF/DWARFContext.cpp

674 ↗

(On Diff #94590)

dyn_cast only makes sense if the result is checked for nullptr later on. Otherwise this should be just a cast.

[grimar]

Thanks for review, Adrian !

[dblaikie]

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

670–674

isa + cast, prefer a single dyn_cast instead:

const auto *MachObj = dyn_cast<MachOObjectFile>(&Obj);
if (!MachObj)
  return false;
...

[grimar]

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

670–674

Right. I'll fix.

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

if (Spec.FLIKind != FileLineInfoKind::None) {
         FunctionDIE.getCallerFrame(CallFile, CallLine, CallColumn);
       }
     }
     InliningInfo.addFrame(Frame);
   }
   return InliningInfo;
 }
 
+static Error createError(const Twine &Reason, llvm::Error E) {
+  return make_error<StringError>(Reason + toString(std::move(E)),
+                                 inconvertibleErrorCode());
+}
+
+/// Returns the address of symbol relocation used against. Used for futher
+/// relocations computation. Symbol's section load address is taken in account if
+/// LoadedObjectInfo interface is provided.
+static Expected<uint64_t> getSymbolAddress(const object::ObjectFile &Obj,
+                                           const RelocationRef &Reloc,
+                                           const LoadedObjectInfo *L) {
+  uint64_t Ret = 0;
+  object::section_iterator RSec = Obj.section_end();
+  object::symbol_iterator Sym = Reloc.getSymbol();
+
+  // First calculate the address of the symbol or section as it appears
+  // in the object file
+  if (Sym != Obj.symbol_end()) {
+    Expected<uint64_t> SymAddrOrErr = Sym->getAddress();
+    if (!SymAddrOrErr)
+      return createError("error: failed to compute symbol address: ",
+                         SymAddrOrErr.takeError());
+
+    // Also remember what section this symbol is in for later
+    auto SectOrErr = Sym->getSection();
+    if (!SectOrErr)
+      return createError("error: failed to get symbol section: ",
+                         SectOrErr.takeError());
+
+    RSec = *SectOrErr;
+    Ret = *SymAddrOrErr;
+  } else if (auto *MObj = dyn_cast<MachOObjectFile>(&Obj)) {
+    RSec = MObj->getRelocationSection(Reloc.getRawDataRefImpl());
+    Ret = RSec->getAddress();
+  }
+
+  // If we are given load addresses for the sections, we need to adjust:
+  // SymAddr = (Address of Symbol Or Section in File) -
+  //           (Address of Section in File) +
+  //           (Load Address of Section)
+  // RSec is now either the section being targeted or the section
+  // containing the symbol being targeted. In either case,
+  // we need to perform the same computation.
+  if (L && RSec != Obj.section_end())
+    if (uint64_t SectionLoadAddress = L->getSectionLoadAddress(*RSec))
+      Ret += SectionLoadAddress - RSec->getAddress();
+  return Ret;
+}
+
+static bool isRelocScattered(const object::ObjectFile &Obj,
+                             const RelocationRef &Reloc) {
+  if (!isa<MachOObjectFile>(&Obj))
+    return false;
+  // MachO also has relocations that point to sections and
+  // scattered relocations.
+  const MachOObjectFile *MachObj = cast<MachOObjectFile>(&Obj);
+  auto RelocInfo = MachObj->getRelocation(Reloc.getRawDataRefImpl());
+  return MachObj->isRelocationScattered(RelocInfo);
+}
+
 DWARFContextInMemory::DWARFContextInMemory(const object::ObjectFile &Obj,
     const LoadedObjectInfo *L)
     : IsLittleEndian(Obj.isLittleEndian()),
       AddressSize(Obj.getBytesInAddress()) {
   for (const SectionRef &Section : Obj.sections()) {
     StringRef name;
     Section.getName(name);
     // Skip BSS and Virtual sections, they aren't interesting.
if (!Map) {
         Map = &TypesDWOSections[*RelocatedSection].Relocs;
       else
         continue;
     }
 
     if (Section.relocation_begin() != Section.relocation_end()) {
       uint64_t SectionSize = RelocatedSection->getSize();
       for (const RelocationRef &Reloc : Section.relocations()) {
-        uint64_t Address = Reloc.getOffset();
-        uint64_t Type = Reloc.getType();
-        uint64_t SymAddr = 0;
-        uint64_t SectionLoadAddress = 0;
-        object::symbol_iterator Sym = Reloc.getSymbol();
-        object::section_iterator RSec = Obj.section_end();
-
-        // First calculate the address of the symbol or section as it appears
-        // in the objct file
-        if (Sym != Obj.symbol_end()) {
-          Expected<uint64_t> SymAddrOrErr = Sym->getAddress();
-          if (!SymAddrOrErr) {
-            std::string Buf;
-            raw_string_ostream OS(Buf);
-            logAllUnhandledErrors(SymAddrOrErr.takeError(), OS, "");
-            OS.flush();
-            errs() << "error: failed to compute symbol address: "
-                   << Buf << '\n';
-            continue;
-          }
-          SymAddr = *SymAddrOrErr;
-          // Also remember what section this symbol is in for later
-          auto SectOrErr = Sym->getSection();
-          if (!SectOrErr) {
-            std::string Buf;
-            raw_string_ostream OS(Buf);
-            logAllUnhandledErrors(SectOrErr.takeError(), OS, "");
-            OS.flush();
-            errs() << "error: failed to get symbol section: "
-                   << Buf << '\n';
-            continue;
-          }
-          RSec = *SectOrErr;
-        } else if (auto *MObj = dyn_cast<MachOObjectFile>(&Obj)) {
-          // MachO also has relocations that point to sections and
-          // scattered relocations.
-          auto RelocInfo = MObj->getRelocation(Reloc.getRawDataRefImpl());
-          if (MObj->isRelocationScattered(RelocInfo)) {
-            // FIXME: it's not clear how to correctly handle scattered
-            // relocations.
-            continue;
-          } else {
-            RSec = MObj->getRelocationSection(Reloc.getRawDataRefImpl());
-            SymAddr = RSec->getAddress();
-          }
-        }
+        if (isRelocScattered(Obj, Reloc))
 
-        // If we are given load addresses for the sections, we need to adjust:
-        // SymAddr = (Address of Symbol Or Section in File) -
-        //           (Address of Section in File) +
-        //           (Load Address of Section)
-        if (L != nullptr && RSec != Obj.section_end()) {
-          // RSec is now either the section being targeted or the section
-          // containing the symbol being targeted. In either case,
-          // we need to perform the same computation.
-          StringRef SecName;
-          RSec->getName(SecName);
-//           dbgs() << "Name: '" << SecName
-//                  << "', RSec: " << RSec->getRawDataRefImpl()
-//                  << ", Section: " << Section.getRawDataRefImpl() << "\n";
-          SectionLoadAddress = L->getSectionLoadAddress(*RSec);
-          if (SectionLoadAddress != 0)
-            SymAddr += SectionLoadAddress - RSec->getAddress();
+        object::symbol_iterator RelSym = Reloc.getSymbol();
+        Expected<uint64_t> SymAddrOrErr = getSymbolAddress(Obj, Reloc, L);
+        if (!SymAddrOrErr) {
+          errs() << toString(std::move(SymAddrOrErr.takeError())) << '\n';
+          continue;
         }
 
         object::RelocVisitor V(Obj);
-        object::RelocToApply R(V.visit(Type, Reloc, SymAddr));
+        object::RelocToApply R(V.visit(Reloc.getType(), Reloc, *SymAddrOrErr));
         if (V.error()) {
           SmallString<32> Name;
           Reloc.getTypeName(Name);
           errs() << "error: failed to compute relocation: "
                  << Name << "\n";
           continue;
         }
-
+        uint64_t Address = Reloc.getOffset();
         if (Address + R.Width > SectionSize) {
           errs() << "error: " << R.Width << "-byte relocation starting "
                  << Address << " bytes into section " << name << " which is "
                  << SectionSize << " bytes long.\n";
           continue;
         }
         if (R.Width > 8) {
           errs() << "error: can't handle a relocation of more than 8 bytes at "