<div dir="ltr">Maybe we should make all fatal functions noreturn functions, and rename fatal to check if it may return?</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 3, 2016 at 12:25 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Nice change. Do you think that fatal is an appropriate name for that function? I guess I'd probably name this "check" or something if we didn't have "fatal" function before.<div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Thu, Mar 3, 2016 at 8:21 AM, Rafael Espindola via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: rafael<br>
Date: Thu Mar 3 10:21:44 2016<br>
New Revision: 262626<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=262626&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=262626&view=rev</a><br>
Log:<br>
Simplify error handling.<br>
<br>
This makes fatal return T when there is no error. This avoids the need<br>
for quite a few temporaries.<br>
<br>
Modified:<br>
lld/trunk/ELF/Driver.cpp<br>
lld/trunk/ELF/Error.cpp<br>
lld/trunk/ELF/Error.h<br>
lld/trunk/ELF/InputFiles.cpp<br>
lld/trunk/ELF/InputSection.cpp<br>
lld/trunk/ELF/SymbolTable.cpp<br>
lld/trunk/ELF/Writer.cpp<br>
<br>
Modified: lld/trunk/ELF/Driver.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Driver.cpp?rev=262626&r1=262625&r2=262626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Driver.cpp?rev=262626&r1=262625&r2=262626&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/Driver.cpp (original)<br>
+++ lld/trunk/ELF/Driver.cpp Thu Mar 3 10:21:44 2016<br>
@@ -70,17 +70,18 @@ static std::pair<ELFKind, uint16_t> pars<br>
// Returns slices of MB by parsing MB as an archive file.<br>
// Each slice consists of a member file in the archive.<br>
static std::vector<MemoryBufferRef> getArchiveMembers(MemoryBufferRef MB) {<br>
- ErrorOr<std::unique_ptr<Archive>> FileOrErr = Archive::create(MB);<br>
- fatal(FileOrErr, "Failed to parse archive");<br>
- std::unique_ptr<Archive> File = std::move(*FileOrErr);<br>
+ std::unique_ptr<Archive> File =<br>
+ fatal(Archive::create(MB), "Failed to parse archive");<br>
<br>
std::vector<MemoryBufferRef> V;<br>
- for (const ErrorOr<Archive::Child> &C : File->children()) {<br>
- fatal(C, "Could not get the child of the archive " + File->getFileName());<br>
- ErrorOr<MemoryBufferRef> MbOrErr = C->getMemoryBufferRef();<br>
- fatal(MbOrErr, "Could not get the buffer for a child of the archive " +<br>
- File->getFileName());<br>
- V.push_back(*MbOrErr);<br>
+ for (const ErrorOr<Archive::Child> &COrErr : File->children()) {<br>
+ Archive::Child C = fatal(COrErr, "Could not get the child of the archive " +<br>
+ File->getFileName());<br>
+ MemoryBufferRef Mb =<br>
+ fatal(C.getMemoryBufferRef(),<br>
+ "Could not get the buffer for a child of the archive " +<br>
+ File->getFileName());<br>
+ V.push_back(Mb);<br>
}<br>
return V;<br>
}<br>
<br>
Modified: lld/trunk/ELF/Error.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Error.cpp?rev=262626&r1=262625&r2=262626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Error.cpp?rev=262626&r1=262625&r2=262626&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/Error.cpp (original)<br>
+++ lld/trunk/ELF/Error.cpp Thu Mar 3 10:21:44 2016<br>
@@ -50,6 +50,10 @@ void fatal(const Twine &Msg) {<br>
exit(1);<br>
}<br>
<br>
+void fatal(const Twine &Msg, const Twine &Prefix) {<br>
+ fatal(Prefix + ": " + Msg);<br>
+}<br>
+<br></blockquote></div></div><div><div><br></div><div>This new function seems to be used only once in Error.cpp. You can remove this if you update the caller to use +.</div></div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
void fatal(std::error_code EC, const Twine &Prefix) {<br>
if (EC)<br>
fatal(Prefix + ": " + EC.message());<br>
<br>
Modified: lld/trunk/ELF/Error.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Error.h?rev=262626&r1=262625&r2=262626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Error.h?rev=262626&r1=262625&r2=262626&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/Error.h (original)<br>
+++ lld/trunk/ELF/Error.h Thu Mar 3 10:21:44 2016<br>
@@ -38,14 +38,21 @@ template <typename T> bool error(const E<br>
}<br>
<br>
LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &Msg);<br>
+LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &Msg, const Twine &Prefix);<br>
void fatal(std::error_code EC, const Twine &Prefix);<br>
void fatal(std::error_code EC);<br>
<br>
-template <typename T> void fatal(const ErrorOr<T> &V, const Twine &Prefix) {<br>
- fatal(V.getError(), Prefix);<br>
+template <class T> T fatal(ErrorOr<T> EO) {<br>
+ if (EO)<br>
+ return std::move(*EO);<br>
+ fatal(EO.getError().message());<br>
}<br>
<br>
-template <typename T> void fatal(const ErrorOr<T> &V) { fatal(V.getError()); }<br>
+template <class T> T fatal(ErrorOr<T> EO, const Twine &Prefix) {<br>
+ if (EO)<br>
+ return std::move(*EO);<br>
+ fatal(EO.getError().message(), Prefix);<br>
+}<br>
<br>
} // namespace elf<br>
} // namespace lld<br>
<br>
Modified: lld/trunk/ELF/InputFiles.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.cpp?rev=262626&r1=262625&r2=262626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.cpp?rev=262626&r1=262625&r2=262626&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/InputFiles.cpp (original)<br>
+++ lld/trunk/ELF/InputFiles.cpp Thu Mar 3 10:21:44 2016<br>
@@ -73,9 +73,7 @@ uint32_t ELFFileBase<ELFT>::getSectionIn<br>
template <class ELFT> void ELFFileBase<ELFT>::initStringTable() {<br>
if (!Symtab)<br>
return;<br>
- ErrorOr<StringRef> StringTableOrErr = ELFObj.getStringTableForSymtab(*Symtab);<br>
- fatal(StringTableOrErr);<br>
- StringTable = *StringTableOrErr;<br>
+ StringTable = fatal(ELFObj.getStringTableForSymtab(*Symtab));<br>
}<br>
<br>
template <class ELFT><br>
@@ -124,26 +122,19 @@ template <class ELFT><br>
StringRef elf::ObjectFile<ELFT>::getShtGroupSignature(const Elf_Shdr &Sec) {<br>
const ELFFile<ELFT> &Obj = this->ELFObj;<br>
uint32_t SymtabdSectionIndex = Sec.sh_link;<br>
- ErrorOr<const Elf_Shdr *> SecOrErr = Obj.getSection(SymtabdSectionIndex);<br>
- fatal(SecOrErr);<br>
- const Elf_Shdr *SymtabSec = *SecOrErr;<br>
+ const Elf_Shdr *SymtabSec = fatal(Obj.getSection(SymtabdSectionIndex));<br>
uint32_t SymIndex = Sec.sh_info;<br>
const Elf_Sym *Sym = Obj.getSymbol(SymtabSec, SymIndex);<br>
- ErrorOr<StringRef> StringTableOrErr = Obj.getStringTableForSymtab(*SymtabSec);<br>
- fatal(StringTableOrErr);<br>
- ErrorOr<StringRef> SignatureOrErr = Sym->getName(*StringTableOrErr);<br>
- fatal(SignatureOrErr);<br>
- return *SignatureOrErr;<br>
+ StringRef StringTable = fatal(Obj.getStringTableForSymtab(*SymtabSec));<br>
+ return fatal(Sym->getName(StringTable));<br>
}<br>
<br>
template <class ELFT><br>
ArrayRef<typename elf::ObjectFile<ELFT>::uint32_X><br>
elf::ObjectFile<ELFT>::getShtGroupEntries(const Elf_Shdr &Sec) {<br>
const ELFFile<ELFT> &Obj = this->ELFObj;<br>
- ErrorOr<ArrayRef<uint32_X>> EntriesOrErr =<br>
- Obj.template getSectionContentsAsArray<uint32_X>(&Sec);<br>
- fatal(EntriesOrErr);<br>
- ArrayRef<uint32_X> Entries = *EntriesOrErr;<br>
+ ArrayRef<uint32_X> Entries =<br>
+ fatal(Obj.template getSectionContentsAsArray<uint32_X>(&Sec));<br>
if (Entries.empty() || Entries[0] != GRP_COMDAT)<br>
fatal("Unsupported SHT_GROUP format");<br>
return Entries.slice(1);<br>
@@ -202,12 +193,9 @@ void elf::ObjectFile<ELFT>::initializeSe<br>
case SHT_SYMTAB:<br>
this->Symtab = &Sec;<br>
break;<br>
- case SHT_SYMTAB_SHNDX: {<br>
- ErrorOr<ArrayRef<Elf_Word>> ErrorOrTable = Obj.getSHNDXTable(Sec);<br>
- fatal(ErrorOrTable);<br>
- this->SymtabSHNDX = *ErrorOrTable;<br>
+ case SHT_SYMTAB_SHNDX:<br>
+ this->SymtabSHNDX = fatal(Obj.getSHNDXTable(Sec));<br>
break;<br>
- }<br>
case SHT_STRTAB:<br>
case SHT_NULL:<br>
break;<br>
@@ -248,9 +236,7 @@ void elf::ObjectFile<ELFT>::initializeSe<br>
template <class ELFT><br>
InputSectionBase<ELFT> *<br>
elf::ObjectFile<ELFT>::createInputSection(const Elf_Shdr &Sec) {<br>
- ErrorOr<StringRef> NameOrErr = this->ELFObj.getSectionName(&Sec);<br>
- fatal(NameOrErr);<br>
- StringRef Name = *NameOrErr;<br>
+ StringRef Name = fatal(this->ELFObj.getSectionName(&Sec));<br>
<br>
// .note.GNU-stack is a marker section to control the presence of<br>
// PT_GNU_STACK segment in outputs. Since the presence of the segment<br>
@@ -300,9 +286,7 @@ elf::ObjectFile<ELFT>::getSection(const<br>
<br>
template <class ELFT><br>
SymbolBody *elf::ObjectFile<ELFT>::createSymbolBody(const Elf_Sym *Sym) {<br>
- ErrorOr<StringRef> NameOrErr = Sym->getName(this->StringTable);<br>
- fatal(NameOrErr);<br>
- StringRef Name = *NameOrErr;<br>
+ StringRef Name = fatal(Sym->getName(this->StringTable));<br>
<br>
switch (Sym->st_shndx) {<br>
case SHN_UNDEF:<br>
@@ -328,9 +312,7 @@ SymbolBody *elf::ObjectFile<ELFT>::creat<br>
}<br>
<br>
void ArchiveFile::parse() {<br>
- ErrorOr<std::unique_ptr<Archive>> FileOrErr = Archive::create(MB);<br>
- fatal(FileOrErr, "Failed to parse archive");<br>
- File = std::move(*FileOrErr);<br>
+ File = fatal(Archive::create(MB), "Failed to parse archive");<br>
<br>
// Allocate a buffer for Lazy objects.<br>
size_t NumSyms = File->getNumberOfSymbols();<br>
@@ -343,18 +325,16 @@ void ArchiveFile::parse() {<br>
<br>
// Returns a buffer pointing to a member file containing a given symbol.<br>
MemoryBufferRef ArchiveFile::getMember(const Archive::Symbol *Sym) {<br>
- ErrorOr<Archive::Child> COrErr = Sym->getMember();<br>
- fatal(COrErr, "Could not get the member for symbol " + Sym->getName());<br>
- const Archive::Child &C = *COrErr;<br>
+ Archive::Child C =<br>
+ fatal(Sym->getMember(),<br>
+ "Could not get the member for symbol " + Sym->getName());<br>
<br>
if (!Seen.insert(C.getChildOffset()).second)<br>
return MemoryBufferRef();<br>
<br>
- ErrorOr<MemoryBufferRef> RefOrErr = C.getMemoryBufferRef();<br>
- if (!RefOrErr)<br>
- fatal(RefOrErr, "Could not get the buffer for the member defining symbol " +<br>
- Sym->getName());<br>
- return *RefOrErr;<br>
+ return fatal(C.getMemoryBufferRef(),<br>
+ "Could not get the buffer for the member defining symbol " +<br>
+ Sym->getName());<br>
}<br>
<br>
template <class ELFT><br>
@@ -367,9 +347,7 @@ SharedFile<ELFT>::getSection(const Elf_S<br>
uint32_t Index = this->getSectionIndex(Sym);<br>
if (Index == 0)<br>
return nullptr;<br>
- ErrorOr<const Elf_Shdr *> Ret = this->ELFObj.getSection(Index);<br>
- fatal(Ret);<br>
- return *Ret;<br>
+ return fatal(this->ELFObj.getSection(Index));<br>
}<br>
<br>
// Partially parse the shared object file so that we can call<br>
@@ -390,13 +368,10 @@ template <class ELFT> void SharedFile<EL<br>
case SHT_DYNAMIC:<br>
DynamicSec = &Sec;<br>
break;<br>
- case SHT_SYMTAB_SHNDX: {<br>
- ErrorOr<ArrayRef<Elf_Word>> ErrorOrTable = Obj.getSHNDXTable(Sec);<br>
- fatal(ErrorOrTable);<br>
- this->SymtabSHNDX = *ErrorOrTable;<br>
+ case SHT_SYMTAB_SHNDX:<br>
+ this->SymtabSHNDX = fatal(Obj.getSHNDXTable(Sec));<br>
break;<br>
}<br>
- }<br>
}<br>
<br>
this->initStringTable();<br>
@@ -444,11 +419,8 @@ bool BitcodeFile::classof(const InputFil<br>
<br>
void BitcodeFile::parse(DenseSet<StringRef> &ComdatGroups) {<br>
LLVMContext Context;<br>
- ErrorOr<std::unique_ptr<IRObjectFile>> ObjOrErr =<br>
- IRObjectFile::create(MB, Context);<br>
- fatal(ObjOrErr);<br>
- IRObjectFile &Obj = **ObjOrErr;<br>
- const Module &M = Obj.getModule();<br>
+ std::unique_ptr<IRObjectFile> Obj = fatal(IRObjectFile::create(MB, Context));<br>
+ const Module &M = Obj->getModule();<br>
<br>
DenseSet<const Comdat *> KeptComdats;<br>
for (const auto &P : M.getComdatSymbolTable()) {<br>
@@ -457,8 +429,8 @@ void BitcodeFile::parse(DenseSet<StringR<br>
KeptComdats.insert(&P.second);<br>
}<br>
<br>
- for (const BasicSymbolRef &Sym : Obj.symbols()) {<br>
- if (const GlobalValue *GV = Obj.getSymbolGV(Sym.getRawDataRefImpl()))<br>
+ for (const BasicSymbolRef &Sym : Obj->symbols()) {<br>
+ if (const GlobalValue *GV = Obj->getSymbolGV(Sym.getRawDataRefImpl()))<br>
if (const Comdat *C = GV->getComdat())<br>
if (!KeptComdats.count(C))<br>
continue;<br>
<br>
Modified: lld/trunk/ELF/InputSection.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputSection.cpp?rev=262626&r1=262625&r2=262626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputSection.cpp?rev=262626&r1=262625&r2=262626&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/InputSection.cpp (original)<br>
+++ lld/trunk/ELF/InputSection.cpp Thu Mar 3 10:21:44 2016<br>
@@ -39,17 +39,12 @@ InputSectionBase<ELFT>::InputSectionBase<br>
}<br>
<br>
template <class ELFT> StringRef InputSectionBase<ELFT>::getSectionName() const {<br>
- ErrorOr<StringRef> Name = File->getObj().getSectionName(this->Header);<br>
- fatal(Name);<br>
- return *Name;<br>
+ return fatal(File->getObj().getSectionName(this->Header));<br>
}<br>
<br>
template <class ELFT><br>
ArrayRef<uint8_t> InputSectionBase<ELFT>::getSectionData() const {<br>
- ErrorOr<ArrayRef<uint8_t>> Ret =<br>
- this->File->getObj().getSectionContents(this->Header);<br>
- fatal(Ret);<br>
- return *Ret;<br>
+ return fatal(this->File->getObj().getSectionContents(this->Header));<br>
}<br>
<br>
template <class ELFT><br>
<br>
Modified: lld/trunk/ELF/SymbolTable.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/SymbolTable.cpp?rev=262626&r1=262625&r2=262626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/SymbolTable.cpp?rev=262626&r1=262625&r2=262626&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/SymbolTable.cpp (original)<br>
+++ lld/trunk/ELF/SymbolTable.cpp Thu Mar 3 10:21:44 2016<br>
@@ -136,11 +136,9 @@ ObjectFile<ELFT> *SymbolTable<ELFT>::cre<br>
for (const std::unique_ptr<BitcodeFile> &F : BitcodeFiles) {<br>
std::unique_ptr<MemoryBuffer> Buffer =<br>
MemoryBuffer::getMemBuffer(F->MB, false);<br>
- ErrorOr<std::unique_ptr<Module>> MOrErr =<br>
- getLazyBitcodeModule(std::move(Buffer), Context,<br>
- /*ShouldLazyLoadMetadata*/ true);<br>
- fatal(MOrErr);<br>
- std::unique_ptr<Module> &M = *MOrErr;<br>
+ std::unique_ptr<Module> M =<br>
+ fatal(getLazyBitcodeModule(std::move(Buffer), Context,<br>
+ /*ShouldLazyLoadMetadata*/ true));<br>
L.linkInModule(std::move(M));<br>
}<br>
std::unique_ptr<InputFile> F = codegen(Combined);<br>
<br>
Modified: lld/trunk/ELF/Writer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Writer.cpp?rev=262626&r1=262625&r2=262626&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Writer.cpp?rev=262626&r1=262625&r2=262626&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/Writer.cpp (original)<br>
+++ lld/trunk/ELF/Writer.cpp Thu Mar 3 10:21:44 2016<br>
@@ -564,9 +564,7 @@ template <class ELFT> void Writer<ELFT>:<br>
return;<br>
for (const std::unique_ptr<ObjectFile<ELFT>> &F : Symtab.getObjectFiles()) {<br>
for (const Elf_Sym &Sym : F->getLocalSymbols()) {<br>
- ErrorOr<StringRef> SymNameOrErr = Sym.getName(F->getStringTable());<br>
- fatal(SymNameOrErr);<br>
- StringRef SymName = *SymNameOrErr;<br>
+ StringRef SymName = fatal(Sym.getName(F->getStringTable()));<br>
if (!shouldKeepInSymtab<ELFT>(*F, SymName, Sym))<br>
continue;<br>
if (Sym.st_shndx != SHN_ABS) {<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div><br></div></div></div>
</blockquote></div><br></div>