This change enables getSymbolFlags() to return errors which benefit error reporting in clients.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
This revision is intended to make a NFC interface change.
All the fallible paths are handled by report_fatal_error() with a comment saying we should return/report this error and test it properly.
All the infallible paths are handled by cantFail().
| llvm/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h | ||
|---|---|---|
| 313–315 | The following might be better, because:
if (Expected<uint32_t> SymbolFlagsOrErr = Symbol.getFlags()) {
if (*SymbolFlagsOrErr & object::SymbolRef::SF_Undefined)
continue;
} else {
// FIXME: Raise an error for bad symbols.
consumeError(SymbolFlagsOrErr.takeError());
continue;
} | |
| llvm/include/llvm/Object/ELFObjectFile.h | ||
| 631 | The type returned is not obvious, so do not use auto (here and in other places where applies). | |
| 634 | You do not need using else after return: if (auto SymbolsOrErr = EF.symbols(SymSec)) return Sym == SymbolsOrErr->begin(); return SymbolsOrErr.takeError(); | |
| 641 | This error is not tested I think? Hence needs a FIXME too. | |
| 647 | I think all the code above is overcomplicated now when we have to report errors instead of consuming it. if (Expected<typename ELFT::SymRange> SymtabSyms = EF.symbols(DotSymtabSec)) {
// Set the <????> flag for a null symbol.
if (ESym == SymtabSyms->begin())
Result |= SymbolRef::SF_FormatSpecific;
} else {
return SymtabSyms.takeError();
} | |
| 657 | This change doesn't seem to belong to this patch? It is unrelared to flags. | |
| llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp | ||
| 208–210 | uint32_t SymFlags = cantFail(Sym.getFlags()); This and below comment applies to all other places where you've added cantFail. | |
| 210 | Why it can't fail, btw? (needs a comment) | |
| llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp | ||
| 220 | Lets make this and other similar FIXME comments shorter to make it a single line. FIXME: return and test this error. | |
Comment Actions
Addressed comments.
- Add TODO tags on untested paths.
- Use shorter comments to fit in a single line.
- Remove unrelated changes.
- Use type Var = catFail(mayFailFunc()) style.
| llvm/include/llvm/Object/ELFObjectFile.h | ||
|---|---|---|
| 635 | This and the below TODO in this function can be removed with appropriate testing in D77864, right? | |
| 657 | There's still a blank line change here that doesn't belong. | |
| llvm/lib/ExecutionEngine/Orc/Mangling.cpp | ||
| 98–99 | Why not just return it already? (The TODO still belongs though) | |
| llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp | ||
| 131 | This might as well be ES.reportError(...), although the TODO for testing should remain. | |
| llvm/lib/ExecutionEngine/RuntimeDyld/JITSymbol.cpp | ||
| 61 | Just do return SymbolFlagsOrErr.takeError(); and adjust the TODO accordingly. | |
| llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp | ||
| 220 | This function already returns an Expected, so I'd change this to return FlagsOrErr.takeError(); | |
| 243–244 | Ditto | |
| 604–605 | This function already returns an Error, so let's avoid the unnecessary report_fatal_error and do return FlagsOrErr.takeError(); | |
| llvm/lib/Object/ObjectFile.cpp | ||
| 65 | Why consumeError here instead of report_fatal_error? | |
| llvm/tools/llvm-nm/llvm-nm.cpp | ||
| 805 | FWIW, I'd actually report the error properly here, and add testing only in D77864, but it's up to you. | |
| 1088 | I'm okay with the variable rename, but I'd do it in a separate commit if you want to rename it. | |
| 1219–1220 | Same comment as above. | |
| llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
| 1935–1936 | Same comment as llvm-nm. | |
| llvm/tools/llvm-size/llvm-size.cpp | ||
| 205–206 | Same comment as llvm-nm. | |
| llvm/tools/sancov/sancov.cpp | ||
| 663 | Let's use the existing error reporting function failIfError here. | |
| llvm/lib/Object/ObjectFile.cpp | ||
|---|---|---|
| 65 | Because this is a blocker for us to land tests. getSymbolValue() is used by getSymbolAddress() and getSymbolAddress() is used by llvm-objdump. And getSymbolAddress() is called before getSymbolFlags(). So, llvm-objdump still crashes. But, we still could tweak the calling order to check getSymbolFlags() first and report errors early to prevent crash. I'm not sure if we want to make this an unsafe API? | |
| llvm/tools/llvm-nm/llvm-nm.cpp | ||
| 805 | Oh, I was misunderstanding the meaning of "NFC", I was thinking reporting error here is a functional change ... 🤣 | |
| llvm/lib/Object/ObjectFile.cpp | ||
|---|---|---|
| 65 | Sorry, you've lost me slightly. When you say llvm-objdump crashes, do you mean due to an unchecked Error, or something else? I wouldn't consider report_fatal_error quite a crash, though it's not as desirable as a proper error, that's true. If getSymbolValue is causing problems too, maybe we should be changing its API as well, to return Expected (again, as a separate commit)? | |
| llvm/tools/llvm-nm/llvm-nm.cpp | ||
| 805 | I think you understood NFC correctly, but report_fatal_error reports errors as much as anything else. Might as well hook up the correct error function instead. | |
The following might be better, because:
if (Expected<uint32_t> SymbolFlagsOrErr = Symbol.getFlags()) { if (*SymbolFlagsOrErr & object::SymbolRef::SF_Undefined) continue; } else { // FIXME: Raise an error for bad symbols. consumeError(SymbolFlagsOrErr.takeError()); continue; }