[Object] Change ObjectFile::getSymbolValue() return type to Expected<uint64_t>

[Higuoxing]

Summary

In D77860, we have changed getSymbolFlags() return type to Expected<uint32_t>.
This change helps bubble the error further up the stack.

[Higuoxing]

Because llvm-objdump calls getSymbolAddress() first, then calls getSymbolFlags(). We have to enable error reporting in getSymbolAddress() as well.

[jhenderson]

llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp

91

Should this either a) be a cantFail or b) returned? If the latter, it should be commented with a TODO.

llvm/lib/XRay/InstrumentationMap.cpp

120–121

Whilst you're at it, could you fold these two lines together or at least rename resolved to Resolved?

[Higuoxing]

Addressed comments.

[jhenderson]

LGTM.

llvm/include/llvm/Object/ELFObjectFile.h

if ((Header->e_machine == ELF::EM_ARM || Header->e_machine == ELF::EM_MIPS) &&
     Ret &= ~1;
 
   return Ret;
 }
 
 template <class ELFT>
 Expected<uint64_t>
 ELFObjectFile<ELFT>::getSymbolAddress(DataRefImpl Symb) const {
-  uint64_t Result = getSymbolValue(Symb);
+  Expected<uint64_t> SymbolValueOrErr = getSymbolValue(Symb);
+  if (!SymbolValueOrErr)
+    // TODO: Test this error.
+    return SymbolValueOrErr.takeError();
+
+  uint64_t Result = *SymbolValueOrErr;
   const Elf_Sym *ESym = getSymbol(Symb);
   switch (ESym->st_shndx) {
   case ELF::SHN_COMMON:
   case ELF::SHN_UNDEF:
   case ELF::SHN_ABS:
     return Result;
   }
 

llvm/include/llvm/Object/ObjectFile.h

public:
 
   Expected<StringRef> getName() const;
   /// Returns the symbol virtual address (i.e. address at which it will be
   /// mapped).
   Expected<uint64_t> getAddress() const;
 
   /// Return the value of the symbol depending on the object this can be an
   /// offset or a virtual address.
-  uint64_t getValue() const;
+  Expected<uint64_t> getValue() const;
 
   /// Get the alignment of this symbol as the actual value (not log 2).
   uint32_t getAlignment() const;
   uint64_t getCommonSize() const;
   Expected<SymbolRef::Type> getType() const;
 
   /// Get section this symbol is defined in reference to. Result is
   /// end_sections() if it is undefined or is an absolute symbol.
protected:
   friend class RelocationRef;
   virtual void moveRelocationNext(DataRefImpl &Rel) const = 0;
   virtual uint64_t getRelocationOffset(DataRefImpl Rel) const = 0;
   virtual symbol_iterator getRelocationSymbol(DataRefImpl Rel) const = 0;
   virtual uint64_t getRelocationType(DataRefImpl Rel) const = 0;
   virtual void getRelocationTypeName(DataRefImpl Rel,
                                      SmallVectorImpl<char> &Result) const = 0;
 
-  uint64_t getSymbolValue(DataRefImpl Symb) const;
+  Expected<uint64_t> getSymbolValue(DataRefImpl Symb) const;
 
 public:
   ObjectFile() = delete;
   ObjectFile(const ObjectFile &other) = delete;
 
   uint64_t getCommonSymbolSize(DataRefImpl Symb) const {
     Expected<uint32_t> SymbolFlagsOrErr = getSymbolFlags(Symb);
     if (!SymbolFlagsOrErr)

 inline Expected<StringRef> SymbolRef::getName() const {
   return getObject()->getSymbolName(getRawDataRefImpl());
 }
 
 inline Expected<uint64_t> SymbolRef::getAddress() const {
   return getObject()->getSymbolAddress(getRawDataRefImpl());
 }
 
-inline uint64_t SymbolRef::getValue() const {
+inline Expected<uint64_t> SymbolRef::getValue() const {
   return getObject()->getSymbolValue(getRawDataRefImpl());
 }
 
 inline uint32_t SymbolRef::getAlignment() const {
   return getObject()->getSymbolAlignment(getRawDataRefImpl());
 }
 
 inline uint64_t SymbolRef::getCommonSize() const {

llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp

llvm::Error ObjectFileTransformer::convert(const object::ObjectFile &Obj,
   // Parse the symbol table.
   size_t NumBefore = Gsym.getNumFunctionInfos();
   for (const object::SymbolRef &Sym : Obj.symbols()) {
     Expected<SymbolRef::Type> SymType = Sym.getType();
     if (!SymType) {
       consumeError(SymType.takeError());
       continue;
     }
-    const uint64_t Addr = Sym.getValue();
+    Expected<uint64_t> AddrOrErr = Sym.getValue();
+    if (!AddrOrErr)
+      // TODO: Test this error.
+      return AddrOrErr.takeError();
+
     if (SymType.get() != SymbolRef::Type::ST_Function ||
-        !Gsym.IsValidTextAddress(Addr) || Gsym.hasFunctionInfoForAddress(Addr))
+        !Gsym.IsValidTextAddress(*AddrOrErr) ||
+        Gsym.hasFunctionInfoForAddress(*AddrOrErr))
       continue;
     // Function size for MachO files will be 0
     constexpr bool NoCopy = false;
     const uint64_t size = IsELF ? ELFSymbolRef(Sym).getSize() : 0;
     Expected<StringRef> Name = Sym.getName();
     if (!Name) {
       logAllUnhandledErrors(Name.takeError(), Log, "ObjectFileTransformer: ");
       continue;
     }
     // Remove the leading '_' character in any symbol names if there is one
     // for mach-o files.
     if (IsMachO)
       Name->consume_front("_");
-    Gsym.addFunctionInfo(FunctionInfo(Addr, size,
-                                      Gsym.insertString(*Name, NoCopy)));
+    Gsym.addFunctionInfo(
+        FunctionInfo(*AddrOrErr, size, Gsym.insertString(*Name, NoCopy)));
   }
   size_t FunctionsAddedCount = Gsym.getNumFunctionInfos() - NumBefore;
   Log << "Loaded " << FunctionsAddedCount << " functions from symbol table.\n";
   return Error::success();
 }

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldCOFF.cpp

if (auto ObjSectionToIDOrErr = loadObjectImpl(O)) {
     raw_string_ostream ErrStream(ErrorStr);
     logAllUnhandledErrors(ObjSectionToIDOrErr.takeError(), ErrStream);
     return nullptr;
   }
 }
 
 uint64_t RuntimeDyldCOFF::getSymbolOffset(const SymbolRef &Sym) {
   // The value in a relocatable COFF object is the offset.
-  return Sym.getValue();
+  return cantFail(Sym.getValue());
 }
 
 uint64_t RuntimeDyldCOFF::getDLLImportOffset(unsigned SectionID, StubMap &Stubs,
                                              StringRef Name,
                                              bool SetSectionIDMinus1) {
   LLVM_DEBUG(dbgs() << "Getting DLLImport entry for " << Name << "... ");
   assert(Name.startswith(getImportSymbolPrefix()) && "Not a DLLImport symbol?");
   RelocationValueRef Reloc;

llvm/lib/Object/COFFObjectFile.cpp

 uint32_t COFFObjectFile::getSymbolAlignment(DataRefImpl Ref) const {
   // MSVC/link.exe seems to align symbols to the next-power-of-2
   // up to 32 bytes.
   COFFSymbolRef Symb = getCOFFSymbol(Ref);
   return std::min(uint64_t(32), PowerOf2Ceil(Symb.getValue()));
 }
 
 Expected<uint64_t> COFFObjectFile::getSymbolAddress(DataRefImpl Ref) const {
-  uint64_t Result = getSymbolValue(Ref);
+  uint64_t Result = cantFail(getSymbolValue(Ref));
   COFFSymbolRef Symb = getCOFFSymbol(Ref);
   int32_t SectionNumber = Symb.getSectionNumber();
 
   if (Symb.isAnyUndefined() || Symb.isCommon() ||
       COFF::isReservedSectionNumber(SectionNumber))
     return Result;
 
   const coff_section *Section = nullptr;

llvm/lib/Object/ObjectFile.cpp

bool SectionRef::containsSymbol(SymbolRef S) const {
   if (!SymSec) {
     // TODO: Actually report errors helpfully.
     consumeError(SymSec.takeError());
     return false;
   }
   return *this == **SymSec;
 }
 
-uint64_t ObjectFile::getSymbolValue(DataRefImpl Ref) const {
+Expected<uint64_t> ObjectFile::getSymbolValue(DataRefImpl Ref) const {
   if (Expected<uint32_t> FlagsOrErr = getSymbolFlags(Ref)) {
     if (*FlagsOrErr & SymbolRef::SF_Undefined)
       return 0;
     if (*FlagsOrErr & SymbolRef::SF_Common)
       return getCommonSymbolSize(Ref);
   } else
-    // TODO: Actually report errors helpfully.
-    report_fatal_error(FlagsOrErr.takeError());
+    // TODO: Test this error.
+    return FlagsOrErr.takeError();
   return getSymbolValueImpl(Ref);
 }
 
 Error ObjectFile::printSymbolName(raw_ostream &OS, DataRefImpl Symb) const {
   Expected<StringRef> Name = getSymbolName(Symb);
   if (!Name)
     return Name.takeError();
   OS << *Name;

llvm/lib/Object/SymbolSize.cpp

llvm::object::computeSymbolSizes(const ObjectFile &O) {
   }
 
   // Collect sorted symbol addresses. Include dummy addresses for the end
   // of each section.
   std::vector<SymEntry> Addresses;
   unsigned SymNum = 0;
   for (symbol_iterator I = O.symbol_begin(), E = O.symbol_end(); I != E; ++I) {
     SymbolRef Sym = *I;
-    uint64_t Value = Sym.getValue();
-    Addresses.push_back({I, Value, SymNum, getSymbolSectionID(O, Sym)});
+    Expected<uint64_t> ValueOrErr = Sym.getValue();
+    if (!ValueOrErr)
+      // TODO: Actually report errors helpfully.
+      report_fatal_error(ValueOrErr.takeError());
+    Addresses.push_back({I, *ValueOrErr, SymNum, getSymbolSectionID(O, Sym)});
     ++SymNum;
   }
   for (SectionRef Sec : O.sections()) {
     uint64_t Address = Sec.getAddress();
     uint64_t Size = Sec.getSize();
     Addresses.push_back(
         {O.symbol_end(), Address + Size, 0, getSectionID(O, Sec)});
   }

llvm/lib/XRay/InstrumentationMap.cpp

if (ObjFile.getBinary()->isELF()) {
     std::tie(SupportsRelocation, Resolver) =
         object::getRelocationResolver(*ObjFile.getBinary());
 
     for (const object::SectionRef &Section : Sections) {
       for (const object::RelocationRef &Reloc : Section.relocations()) {
         if (SupportsRelocation && SupportsRelocation(Reloc.getType())) {
           auto AddendOrErr = object::ELFRelocationRef(Reloc).getAddend();
           auto A = AddendOrErr ? *AddendOrErr : 0;
-          uint64_t resolved = Resolver(Reloc, Reloc.getSymbol()->getValue(), A);
-          Relocs.insert({Reloc.getOffset(), resolved});
+          Expected<uint64_t> ValueOrErr = Reloc.getSymbol()->getValue();
+          if (!ValueOrErr)
+            // TODO: Test this error.
+            return ValueOrErr.takeError();
+          Relocs.insert({Reloc.getOffset(), Resolver(Reloc, *ValueOrErr, A)});
         } else if (Reloc.getType() == RelativeRelocation) {
           if (auto AddendOrErr = object::ELFRelocationRef(Reloc).getAddend())
             Relocs.insert({Reloc.getOffset(), *AddendOrErr});
         }
       }
     }
   }
 

llvm/tools/dsymutil/DebugMap.cpp

MappingTraits<dsymutil::DebugMapObject>::YamlDMO::denormalize(IO &IO) {
   } else {
     auto Object = ObjectEntry->getObject(Ctxt.BinaryTriple);
     if (!Object) {
       auto Err = Object.takeError();
       WithColor::warning() << "Unable to open " << Path << " "
                            << toString(std::move(Err)) << '\n';
     } else {
       for (const auto &Sym : Object->symbols()) {
-        uint64_t Address = Sym.getValue();
+        Expected<uint64_t> AddressOrErr = Sym.getValue();
+        if (!AddressOrErr) {
+          // TODO: Actually report errors helpfully.
+          consumeError(AddressOrErr.takeError());
+          continue;
+        }
         Expected<StringRef> Name = Sym.getName();
         Expected<uint32_t> FlagsOrErr = Sym.getFlags();
         if (!Name || !FlagsOrErr ||
             (*FlagsOrErr & (SymbolRef::SF_Absolute | SymbolRef::SF_Common))) {
           // TODO: Actually report errors helpfully.
           if (!FlagsOrErr)
             consumeError(FlagsOrErr.takeError());
           if (!Name)
             consumeError(Name.takeError());
           continue;
         }
-        SymbolAddresses[*Name] = Address;
+        SymbolAddresses[*Name] = *AddressOrErr;
       }
     }
   }
 
   dsymutil::DebugMapObject Res(Path, sys::toTimePoint(Timestamp), MachO::N_OSO);
   for (auto &Entry : Entries) {
     auto &Mapping = Entry.second;
     Optional<uint64_t> ObjAddress;

llvm/tools/dsymutil/MachODebugMapParser.cpp

 }
 
 /// Load the current object file symbols into CurrentObjectAddresses.
 void MachODebugMapParser::loadCurrentObjectFileSymbols(
     const object::MachOObjectFile &Obj) {
   CurrentObjectAddresses.clear();
 
   for (auto Sym : Obj.symbols()) {
-    uint64_t Addr = Sym.getValue();
+    uint64_t Addr = cantFail(Sym.getValue());
     Expected<StringRef> Name = Sym.getName();
     if (!Name) {
       // TODO: Actually report errors helpfully.
       consumeError(Name.takeError());
       continue;
     }
     // The value of some categories of symbols isn't meaningful. For
     // example common symbols store their size in the value field, not
for (const auto &Sym : MainBinary.symbols()) {
     if (!SectionOrErr) {
       // TODO: Actually report errors helpfully.
       consumeError(SectionOrErr.takeError());
       continue;
     }
     Section = *SectionOrErr;
     if (Section == MainBinary.section_end() || Section->isText())
       continue;
-    uint64_t Addr = Sym.getValue();
+    uint64_t Addr = cantFail(Sym.getValue());
     Expected<StringRef> NameOrErr = Sym.getName();
     if (!NameOrErr) {
       // TODO: Actually report errors helpfully.
       consumeError(NameOrErr.takeError());
       continue;
     }
     StringRef Name = *NameOrErr;
     if (Name.size() == 0 || Name[0] == '\0')

llvm/tools/llvm-objdump/MachODump.cpp

bool operator()(const SymbolRef &A, const SymbolRef &B) {
     Expected<SymbolRef::Type> ATypeOrErr = A.getType();
     if (!ATypeOrErr)
       reportError(ATypeOrErr.takeError(), A.getObject()->getFileName());
     SymbolRef::Type AType = *ATypeOrErr;
     Expected<SymbolRef::Type> BTypeOrErr = B.getType();
     if (!BTypeOrErr)
       reportError(BTypeOrErr.takeError(), B.getObject()->getFileName());
     SymbolRef::Type BType = *BTypeOrErr;
-    uint64_t AAddr = (AType != SymbolRef::ST_Function) ? 0 : A.getValue();
-    uint64_t BAddr = (BType != SymbolRef::ST_Function) ? 0 : B.getValue();
+    uint64_t AAddr =
+        (AType != SymbolRef::ST_Function) ? 0 : cantFail(A.getValue());
+    uint64_t BAddr =
+        (BType != SymbolRef::ST_Function) ? 0 : cantFail(B.getValue());
     return AAddr < BAddr;
   }
 };
 } // namespace
 
 // Types for the storted data in code table that is built before disassembly
 // and the predicate function to sort them.
 typedef std::pair<uint64_t, DiceRef> DiceTableEntry;

 static void CreateSymbolAddressMap(MachOObjectFile *O,
                                    SymbolAddressMap *AddrMap) {
   // Create a map of symbol addresses to symbol names.
   const StringRef FileName = O->getFileName();
   for (const SymbolRef &Symbol : O->symbols()) {
     SymbolRef::Type ST = unwrapOrError(Symbol.getType(), FileName);
     if (ST == SymbolRef::ST_Function || ST == SymbolRef::ST_Data ||
         ST == SymbolRef::ST_Other) {
-      uint64_t Address = Symbol.getValue();
+      uint64_t Address = cantFail(Symbol.getValue());
       StringRef SymName = unwrapOrError(Symbol.getName(), FileName);
       if (!SymName.startswith(".objc"))
         (*AddrMap)[Address] = SymName;
     }
   }
 }
 
 // GuessSymbolName is passed the address of what might be a symbol and a
if (RelocOffset == sect_offset) {
       break;
     }
   }
   // If there is an external relocation entry for a symbol in this section
   // at this section_offset then use that symbol's value for the n_value
   // and return its name.
   const char *SymbolName = nullptr;
   if (reloc_found && isExtern) {
-    n_value = Symbol.getValue();
+    n_value = cantFail(Symbol.getValue());
     StringRef Name = unwrapOrError(Symbol.getName(), info->O->getFileName());
     if (!Name.empty()) {
       SymbolName = Name.data();
       return SymbolName;
     }
   }
 
   // TODO: For fully linked images, look through the external relocation
for (const RelocationRef &Reloc : info->S.relocations()) {
       }
     }
     // If there is an external relocation entry for a symbol in a section
     // then used that symbol's value for the value of the reference.
     if (reloc_found && isExtern) {
       if (info->O->getAnyRelocationPCRel(RE)) {
         unsigned Type = info->O->getAnyRelocationType(RE);
         if (Type == MachO::X86_64_RELOC_SIGNED) {
-          ReferenceValue = Symbol.getValue();
+          ReferenceValue = cantFail(Symbol.getValue());
         }
       }
     }
   }
 
   // Look for literals such as Objective-C CFStrings refs, Selector refs,
   // Message refs and Class refs.
   bool classref, selref, msgref, cfstring;
for (unsigned SectIdx = 0; SectIdx != Sections.size(); SectIdx++) {
     // the SymbolizerSymbolLookUp() routine.
     SymbolAddressMap AddrMap;
     bool DisSymNameFound = false;
     for (const SymbolRef &Symbol : MachOOF->symbols()) {
       SymbolRef::Type ST =
           unwrapOrError(Symbol.getType(), MachOOF->getFileName());
       if (ST == SymbolRef::ST_Function || ST == SymbolRef::ST_Data ||
           ST == SymbolRef::ST_Other) {
-        uint64_t Address = Symbol.getValue();
+        uint64_t Address = cantFail(Symbol.getValue());
         StringRef SymName =
             unwrapOrError(Symbol.getName(), MachOOF->getFileName());
         AddrMap[Address] = SymName;
         if (!DisSymName.empty() && DisSymName == SymName)
           DisSymNameFound = true;
       }
     }
     if (!DisSymName.empty() && !DisSymNameFound) {
for (unsigned SymIdx = 0; SymIdx != Symbols.size(); SymIdx++) {
         continue;
 
       // If we are only disassembling one symbol see if this is that symbol.
       if (!DisSymName.empty() && DisSymName != SymName)
         continue;
 
       // Start at the address of the symbol relative to the section's address.
       uint64_t SectSize = Sections[SectIdx].getSize();
-      uint64_t Start = Symbols[SymIdx].getValue();
+      uint64_t Start = cantFail(Symbols[SymIdx].getValue());
       uint64_t SectionAddress = Sections[SectIdx].getAddress();
       Start -= SectionAddress;
 
       if (Start > SectSize) {
         outs() << "section data ends, " << SymName
                << " lies outside valid range\n";
         return;
       }
 
       // Stop disassembling either at the beginning of the next symbol or at
       // the end of the section.
       bool containsNextSym = false;
       uint64_t NextSym = 0;
       uint64_t NextSymIdx = SymIdx + 1;
       while (Symbols.size() > NextSymIdx) {
         SymbolRef::Type NextSymType = unwrapOrError(
             Symbols[NextSymIdx].getType(), MachOOF->getFileName());
         if (NextSymType == SymbolRef::ST_Function) {
           containsNextSym =
               Sections[SectIdx].containsSymbol(Symbols[NextSymIdx]);
-          NextSym = Symbols[NextSymIdx].getValue();
+          NextSym = cantFail(Symbols[NextSymIdx].getValue());
           NextSym -= SectionAddress;
           break;
         }
         ++NextSymIdx;
       }
 
       uint64_t End = containsNextSym ? std::min(NextSym, SectSize) : SectSize;
       uint64_t Size;
if (!SectOrErr) {
       // TODO: Actually report errors helpfully.
       consumeError(SectOrErr.takeError());
       continue;
     }
     section_iterator Section = *SectOrErr;
     if (Section == Obj->section_end())
       continue;
 
-    uint64_t Addr = SymRef.getValue();
+    uint64_t Addr = cantFail(SymRef.getValue());
     Symbols.insert(std::make_pair(Addr, SymRef));
   }
 
   for (const SectionRef &Section : Obj->sections()) {
     StringRef SectName;
     if (Expected<StringRef> NameOrErr = Section.getName())
       SectName = *NameOrErr;
     else