This is an archive of the discontinued LLVM Phabricator instance.

[Object] Change uint32_t getSymbolFlags() to Expected<uint32_t> getSymbolFlags().
ClosedPublic

Authored by Higuoxing on Apr 10 2020, 12:48 AM.

Details

Summary

This change enables getSymbolFlags() to return errors which benefit error reporting in clients.

Diff Detail

Event Timeline

Higuoxing created this revision.Apr 10 2020, 12:48 AM

Apply clang-format.

Higuoxing planned changes to this revision.Apr 10 2020, 12:54 AM

Sorry, not ready for reviewing.

Higuoxing updated this revision to Diff 256526.EditedApr 10 2020, 1:17 AM

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().

Higuoxing edited the summary of this revision. (Show Details)Apr 10 2020, 3:45 AM
Higuoxing added a reviewer: MaskRay.
Higuoxing updated this revision to Diff 256568.Apr 10 2020, 7:01 AM
Higuoxing marked an inline comment as done.

Fix one missed cantFail() situation.

grimar added inline comments.Apr 13 2020, 3:15 AM
llvm/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h
313–315

The following might be better, because:

  1. Does not use auto when the type is not obvious.
  2. Limits the scope of the SymbolFlagsOrErr
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.
Probably no need to have IsNullSymbol as it does not make the code simpler anymore:

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
211–213
uint32_t SymFlags = cantFail(Sym.getFlags());

This and below comment applies to all other places where you've added cantFail.

213

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.
Higuoxing updated this revision to Diff 257187.Apr 13 2020, 9:16 PM

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.
jhenderson added inline comments.Apr 14 2020, 1:42 AM
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.

1090

I'm okay with the variable rename, but I'd do it in a separate commit if you want to rename it.

1221–1222

Same comment as above.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1934–1935

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.

Higuoxing marked 2 inline comments as done.Apr 14 2020, 2:51 AM
Higuoxing added inline comments.
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 ... 🤣

jhenderson added inline comments.Apr 14 2020, 3:19 AM
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.

Higuoxing updated this revision to Diff 257315.Apr 14 2020, 7:10 AM
Higuoxing marked an inline comment as done.

Addressed comments.

  • Use existing methods for error reporting.
Higuoxing marked an inline comment as done.Apr 14 2020, 7:14 AM
Higuoxing added inline comments.
llvm/include/llvm/Object/ELFObjectFile.h
635

Yes.

llvm/lib/Object/ObjectFile.cpp
65

Sorry, I mean report_fatal_error, because it dumps calling stacks in debug mode, which looks like crash. I will look into it. Thanks.

jhenderson accepted this revision.Apr 17 2020, 7:37 AM

LGTM.

llvm/include/llvm/Object/ELFObjectFile.h
635

Okay. I just don't see the corresponding TODOs disappearing in that change ;)

llvm/tools/llvm-nm/llvm-nm.cpp
805

Don't forget to rebase your other change on top of this one.

This revision is now accepted and ready to land.Apr 17 2020, 7:37 AM
This revision was automatically updated to reflect the committed changes.