-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[llvm-readobj] Dump callgraph section info for ELF #157499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Introduce a new flag `--call-graph-info` which outputs callgraph ELF section information to the console as a text output or as JSON output.
…toms out to llvm_unreachable.
3499c2d to
0b9c46d
Compare
jh7370
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to update the llvm-readobj and llvm-readelf CommandGuide documents under llvm/docs.
I'm keen to review this, as I've seen several things when skimming I'd like addressed, but don't have time to review it properly this week, due to other reviews and some planned time off.
| #include "llvm/Support/DataExtractor.h" | ||
| #include "llvm/Support/Debug.h" | ||
| #include "llvm/Support/Endian.h" | ||
| #include "llvm/Support/Error.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised some/most/all of these new headers need to be added? Is your IDE adding additional headers unnecessarily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the IDE is adding the corresponding header whenever I initialize a type. Cleaned up the header includes. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error.h is almost certainly not needed explicitly, given Error and Expected are already widely in use. (https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible, specifically the bit about being able to include headers implicitly via other headers).
ilovepi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is basically LGTM from me. Lets make sure we wait until @jh7370 weighs in w/ a full review to land though.
| # JSON-NEXT: "NumDirectCallees": 1, | ||
| # JSON-NEXT: "DirectCallees": [ | ||
| # JSON-NEXT: { | ||
| # JSON-NEXT: "Offset": 19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is Offset here? is it the address? offset from the start of the section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offset is pointless actually. It's the offset of the symbol for which we couldn't find corresponding relocation. I removed "offset" and turned it into a warning for this scenario.
jh7370
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in looking. The CommandGuide docs still haven't been updated. Please update them with the new option.
I've made another pass, but have run out of time to look at everything. Hopefully there's enough here to keep going on.
| @@ -0,0 +1,150 @@ | |||
| ## Tests --call-graph-info prints information from call graph section. | |||
|
|
|||
| # REQUIRES: x86-registered-target | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we convert the test input to use yaml2obj, instead of requiring x86? It looks like the file format is trivial enough that something like a YAML ELF input using ContentArray would be just as descriptive, whilst also allowing more precise control of everything.
llvm/test/tools/llvm-readobj/ELF/call-graph-info-callgraph-section.s
Outdated
Show resolved
Hide resolved
llvm/test/tools/llvm-readobj/ELF/call-graph-info-callgraph-section.s
Outdated
Show resolved
Hide resolved
| reportError(createError("No .llvm.callgraph section found."), | ||
| "Missing section"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
Same applies to other error/warning messages.
That being said, we don't tend to emit an error for other options that do similar things, if the respective section doesn't exist.
When a diagnostic message is appropriate, we usually prefer warnings, rather than errors, so that we can continue dumping other requested items, or we emit a warning then try to dump the information we can for the requested section and use placeholder values (e.g. "<?>").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely not done. There are loads of other diagnostic messages that are using a capital letter, despite the rules described. Please take another look.
|
|
||
| Expected<ArrayRef<uint8_t>> SectionBytesOrErr = | ||
| Obj.getSectionContents(*CGSection); | ||
| if (!SectionBytesOrErr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applies in several other places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have addressed all the cases. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've highlighted several cases where it's not been addressed. I'd be surprised if I caught them all.
| .str()); | ||
| }; | ||
|
|
||
| DataExtractor Data(SectionBytesOrErr.get(), Obj.isLE(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked into using the DataExtractor::Cursor class? I think that would simplify some of your parsing logic below.
🐧 Linux x64 Test Results
|
| SmallVector<uint64_t, 4> IndirectTypeIdsList = SmallVector<uint64_t, 4>( | ||
| CGInfo.IndirectTypeIDs.begin(), CGInfo.IndirectTypeIDs.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| SmallVector<uint64_t, 4> IndirectTypeIdsList = SmallVector<uint64_t, 4>( | |
| CGInfo.IndirectTypeIDs.begin(), CGInfo.IndirectTypeIDs.end()); | |
| SmallVector<uint64_t, 4> IndirectTypeIdsList(CGInfo.IndirectTypeIDs.begin(), CGInfo.IndirectTypeIDs.end()); |
Can't recall if you need .begin()/.end() to convert w/ SmallSet, or if there is an overload in ADT.
Alternatively, you can avoid the conversion altogether. Looking at printHexList(), its going to put the items in a SmallVector on its own (
| template <typename T> void printHexList(StringRef Label, const T &List) { |
ArraryRef that just forwards to printHexListImpl().
| # JSON: "CallGraph": [ | ||
| # JSON-NEXT: { | ||
| # JSON-NEXT: "Function": { | ||
| # JSON-SAME: warning: '[[FILE]]': unknown relocation at offset 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we getting this warning here? can we avoid it in this test case, and have specific test cases that should issue the diagnostic? To ensure no warnings occur, you can use the FileCheck --implicit-check-not "warning:".
| @@ -0,0 +1,22 @@ | |||
| ## Tests that --call-graph-info fails if .llvm.callgraph section has invalid | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you only issue a warning now, the file name should probably get updated.
| # RUN: llvm-readelf --elf-output-style=LLVM --call-graph-info %t 2>&1 | FileCheck %s -DFILE=%t | ||
| # RUN: llvm-readelf --elf-output-style=JSON --pretty-print --call-graph-info %t 2>&1 | FileCheck %s -DFILE=%t | ||
|
|
||
| # CHECK: warning: 'missing section': No .llvm.callgraph section found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update file name to reflect that its no longer an error.
| ## We do not support GNU format console output for --call-graph-info as it is an LLVM only info. | ||
| # CHECK-NOT: . | ||
|
|
||
| # LLVM: CallGraph [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # LLVM: CallGraph [ | |
| # LLVM: CallGraph [ |
I think this would align it w/ the one below. Seems less jarring than the current indentation. Could also unindent this more to reflect the actual nesting.
| SmallVector<std::string> getFunctionNames(uint64_t FuncAddr) { | ||
| SmallVector<uint32_t> FuncSymIndexes = | ||
| this->getSymbolIndexesForFunctionAddress(FuncAddr, std::nullopt); | ||
| SmallVector<std::string> FuncSymNames; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know the size at this point, so you can use reserve() to avoid extra allocations + copies on push_back when the vector grows.
| Twine("while reading call graph info's [" + Component + | ||
| "] for function at [0x" + FuncPC + "]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the def for reportError() in llvm-readobj, I think the Input parameter (2nd param) is typically a file name. Or, at least its getting passed to reportFileError() under the hood, and I don't see anyone passing a string like this into reportError() in the rest of the code.
Maybe the typical pattern is to wrap your error message in another error type?
| Error CGSectionErr = Error::success(); | ||
| uint8_t FormatVersionNumber = Data.getU8(&Offset, &CGSectionErr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the initialization dead, since getU8() updates it's value?
| reportError(createError("Unknown format version value [" + | ||
| std::to_string(FormatVersionNumber) + | ||
| "] in .llvm.callgraph section."), | ||
| "unknown value"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "unknown value"); | |
| FileStr); |
| reportWarning(std::move(CGSectionErr), | ||
| "while reading call graph info's Flags"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to wrap the message in an error type. you're passing it in as a filename, from what I can see.
This seems pervasive, throughout the patch.
jh7370
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes to make it clear there's still plenty of work to be done.
| #include "llvm/Support/DataExtractor.h" | ||
| #include "llvm/Support/Debug.h" | ||
| #include "llvm/Support/Endian.h" | ||
| #include "llvm/Support/Error.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error.h is almost certainly not needed explicitly, given Error and Expected are already widely in use. (https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible, specifically the bit about being able to include headers implicitly via other headers).
| #include "llvm/Support/ScopedPrinter.h" | ||
| #include "llvm/Support/SystemZ/zOSSupport.h" | ||
| #include "llvm/Support/raw_ostream.h" | ||
| #include "llvm/TargetParser/Triple.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this include?
| } // namespace callgraph | ||
|
|
||
| template <class ELFT> bool ELFDumper<ELFT>::processCallGraphSection() { | ||
| const Elf_Shdr *CGSection = findSectionByName(".llvm.callgraph"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is .llvm.callgraph a new section, or a long pre-existing section that we're just gaining functionality to dump now? If it's new, a much-preferred approach would be to give the section a specific type, as type lookup is more appropriate for ELF than name lookup (and much faster, especially in large objects).
|
|
||
| Expected<ArrayRef<uint8_t>> SectionBytesOrErr = | ||
| Obj.getSectionContents(*CGSection); | ||
| if (!SectionBytesOrErr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove braces here.
| return false; | ||
| } | ||
| if (FormatVersionNumber != 0) { | ||
| reportError(createError("Unknown format version value [" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a warning and bail out of the dumping, so that other requested llvm-readobj operations can continue.
| } | ||
|
|
||
| template <class ELFT> void LLVMELFDumper<ELFT>::printCallGraphInfo() { | ||
| if (!this->processCallGraphSection() || this->FuncCGInfos.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary braces.
| this->reportUniqueWarning(RelSymOrErr.takeError()); | ||
| return; | ||
| } | ||
| if (!RelSymOrErr->Name.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary braces.
| reportError(createError("No .llvm.callgraph section found."), | ||
| "Missing section"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely not done. There are loads of other diagnostic messages that are using a capital letter, despite the rules described. Please take another look.
|
|
||
| Expected<ArrayRef<uint8_t>> SectionBytesOrErr = | ||
| Obj.getSectionContents(*CGSection); | ||
| if (!SectionBytesOrErr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've highlighted several cases where it's not been addressed. I'd be surprised if I caught them all.
Introduce a new flag
--call-graph-infowhich outputs callgraph ELFsection information to the console as a text output or as JSON output.