[DWARF] - Cleanup relocations proccessing.

[grimar]

Summary

There was 3 visible problem this patch trying to fix:

1. RelocAddrMap was defined at 2 difference files at once. I moved definition to DWARFRelocMap.h.
2. RelocAddrMap was a pair of <width, address>, where width is relocation size (4/8), and width field was never used in code.
3. Relocations proccessing loop had checks for width field. I don't think DWARF parser should do that. There is probably no much sense to validate relocations during proccessing them in parser.

Patch moves definition to single file, stops storing width field, stops doing additional checks during handling
of relocations, that all together simplifies the code.

[aprantl]

I'm assuming the checks are removed because they are already done elsewhere?

[grimar]

In D33194#755181, @aprantl wrote:

I'm assuming the checks are removed because they are already done elsewhere?

Let me explain in details my position. Lets take a look more closer what was checked.
There was 2 checks I removed:

1. if (Address + R.Width > SectionSize) { ...
2. if (R.Width > 8)

R.Width field is just a size of a relocation. For example for x86_64 target it can be 4 or 8 usually:

RelocToApply visitELF_X86_64_64(RelocationRef R, uint64_t Value) {
  int64_t Addend = getELFAddend(R);
  return RelocToApply(Value + Addend, 8);
}
RelocToApply visitELF_X86_64_PC32(RelocationRef R, uint64_t Value) {
  int64_t Addend = getELFAddend(R);
  uint64_t Address = R.getOffset();
  return RelocToApply(Value + Addend - Address, 4);
}

That means that first check is always true until implementation of relocation visitor is correct and
until we do not trying to parse broken object. Implementation issues should be checked in testcases,
and not in code. And broken inputs is unlikely can cause problem here. If we have broken input, it is ok to produce
broken dump. Does not seem that DWARF parsers really should check that relocations produced by compiler are correct in a hot loop ?

Second check tests that relocation size is <= 8. I do not know why it was checked, because Width is unused in code at all.
If something wrong with such relocations, then visitor should report an error "failed to compute relocation" that we still handle.
So check looks never triggers while visitor implementation is correct. Currently (RelocVisitor.h) never returns width > 8.

[grimar]

Thanks for review, Adrian !

I am planning to commit this after D33184 probably, because
we have RelocAddrEntry now:

struct RelocAddrEntry {
  uint8_t Width;
  int64_t Value;
};
typedef DenseMap<uint64_t, RelocAddrEntry> RelocAddrMap;

And if I remove Width field, like this patch do, then struct with single field
would look unreasonable. D33184 adds one more field and should look fine.

[grimar]

On the other side I would like to have this check removed because going to post another patch where this cleanup
is used heavely. So I am going to commit this.

If it is not ok to temporarily have single member in RelocAddrEntry, I'll remove this struct separatelly, though I would suggest just
to wait for D33184 which looks promising, it will use this struct anyways.

include/llvm/DebugInfo/DWARF/DWARFContext.h

 #include <memory>
 #include <utility>
 
 namespace llvm {
 
 class MemoryBuffer;
 class raw_ostream;
 
-// In place of applying the relocations to the data we've read from disk we use
-// a separate mapping table to the side and checking that at locations in the
-// dwarf where we expect relocated values. This adds a bit of complexity to the
-// dwarf parsing/extraction at the benefit of not allocating memory for the
-// entire size of the debug info sections.
-typedef DenseMap<uint64_t, std::pair<uint8_t, int64_t>> RelocAddrMap;
-
 /// Reads a value from data extractor and applies a relocation to the result if
 /// one exists for the given offset.
 uint64_t getRelocatedValue(const DataExtractor &Data, uint32_t Size,
                            uint32_t *Off, const RelocAddrMap *Relocs);
 
 /// DWARFContext
 /// This data structure is the top level entity that deals with dwarf debug
 /// information parsing. The actual data is supplied through pure virtual

include/llvm/DebugInfo/DWARF/DWARFRelocMap.h

 #define LLVM_DEBUGINFO_DWARF_DWARFRELOCMAP_H
 
 #include "llvm/ADT/DenseMap.h"
 #include <cstdint>
 #include <utility>
 
 namespace llvm {
 
-typedef DenseMap<uint64_t, std::pair<uint8_t, int64_t>> RelocAddrMap;
+// In place of applying the relocations to the data we've read from disk we use
+// a separate mapping table to the side and checking that at locations in the
+// dwarf where we expect relocated values. This adds a bit of complexity to the
+// dwarf parsing/extraction at the benefit of not allocating memory for the
+// entire size of the debug info sections.
+typedef DenseMap<uint64_t, int64_t> RelocAddrMap;
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFRELOCMAP_H

lib/DebugInfo/DWARF/DWARFContext.cpp

 
 uint64_t llvm::getRelocatedValue(const DataExtractor &Data, uint32_t Size,
                                  uint32_t *Off, const RelocAddrMap *Relocs) {
   if (!Relocs)
     return Data.getUnsigned(Off, Size);
   RelocAddrMap::const_iterator AI = Relocs->find(*Off);
   if (AI == Relocs->end())
     return Data.getUnsigned(Off, Size);
-  return Data.getUnsigned(Off, Size) + AI->second.second;
+  return Data.getUnsigned(Off, Size) + AI->second;
 }
 
 static void dumpAccelSection(raw_ostream &OS, StringRef Name,
                              const DWARFSection& Section, StringRef StringSection,
                              bool LittleEndian) {
   DataExtractor AccelSection(Section.Data, LittleEndian, 0);
   DataExtractor StrData(StringSection, LittleEndian, 0);
   OS << "\n." << Name << " contents:\n";
if (!Map) {
       if (RelSecName == "debug_types")
         Map = &TypesSections[*RelocatedSection].Relocs;
       else if (RelSecName == "debug_types.dwo")
         Map = &TypesDWOSections[*RelocatedSection].Relocs;
       else
         continue;
     }
 
+    if (Section.relocation_begin() == Section.relocation_end())
+      continue;
+
     std::map<SymbolRef, uint64_t> AddrCache;
-    if (Section.relocation_begin() != Section.relocation_end()) {
-      uint64_t SectionSize = RelocatedSection->getSize();
-      for (const RelocationRef &Reloc : Section.relocations()) {
-        // FIXME: it's not clear how to correctly handle scattered
-        // relocations.
-        if (isRelocScattered(Obj, Reloc))
-          continue;
 
-        Expected<uint64_t> SymAddrOrErr =
-            getSymbolAddress(Obj, Reloc, L, AddrCache);
-        if (!SymAddrOrErr) {
-          errs() << toString(SymAddrOrErr.takeError()) << '\n';
-          continue;
-        }
 
-        object::RelocVisitor V(Obj);
-        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 "
-                    "a time.\n";
-          continue;
-        }
-        DEBUG(dbgs() << "Writing " << format("%p", R.Value)
-                     << " at " << format("%p", Address)
-                     << " with width " << format("%d", R.Width)
-                     << "\n");
-        Map->insert(std::make_pair(Address, std::make_pair(R.Width, R.Value)));
-      }
+        errs() << "error: failed to compute relocation: " << Name << "\n";
+      Map->insert({Reloc.getOffset(), R.Value});
     }
   }
 }
 
 DWARFContextInMemory::DWARFContextInMemory(
     const StringMap<std::unique_ptr<MemoryBuffer>> &Sections, uint8_t AddrSize,
     bool isLittleEndian)
     : IsLittleEndian(isLittleEndian), AddressSize(AddrSize) {