-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[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.
| } | ||
| } | ||
|
|
||
| template <class ELFT> void GNUELFDumper<ELFT>::printCallGraphInfo() { |
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.
Do we need a GNU-style dumper? In other words, does GNU readelf support the section? If it doesn't, this function shouldn't be implemented, or at the very most, should do the same as the LLVM dumper. See various other modes for examples.
For context: llvm-readobj has three ELF output modes: JSON (which produces legal JSON output), LLVM (which is a human-readable dictionary-like structure), and GNU. GNU output is supposed to mirror GNU's readelf output, as close as possible. This is because people expect it to be a drop-in replacement in this mode. If there isn't a GNU readelf option to dump it, we shouldn't try to make up one.
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.
Dropped the GNU dumper implementation as suggested.
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.
| # RUN: llvm-readelf --elf-output-style=LLVM --call-graph-info %t 2>&1 | FileCheck %s -DFILE=%t --check-prefix=LLVM | ||
| # RUN: llvm-readelf --elf-output-style=JSON --pretty-print --call-graph-info %t 2>&1 | FileCheck %s -DFILE=%t --check-prefix=JSON | ||
|
|
||
|
|
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.
Nit: why the double blank line?
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.
|
|
||
|
|
||
| ## We do not support GNU format console output for --call-graph-info as it is an LLVM only info. | ||
| # CHECK-NOT: . |
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 isn't going to do what you think it will. This checks simply that the character "." doesn't appear anywhere in the output. What you probably wanted was {{.}} which is a regex pattern of any character. However, if the output is truly empty, the better method is to use count 0. You should find other examples elsewhere in the testsuite.
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.
Neat. Thank you. Done.
| def arch_specific : FF<"arch-specific", "Display architecture-specific information">; | ||
| def bb_addr_map : FF<"bb-addr-map", "Display the BB address map section">; | ||
| def pretty_pgo_analysis_map : FF<"pretty-pgo-analysis-map", "Display PGO analysis values with formatting rather than raw numbers">; | ||
| def call_graph_info : FF<"call-graph-info", "Display call graph information">; |
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.
The option for the call graph profile abbreviates it to cg-profile. Should this also be cg-info? I don't know much about the functionality, so I might be mistaken.
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.
These two things are unrelated. I'd like to keep the name to call_graph_info which is consistent with the flags in Clang/LLVM. Same for other similar suggestions made towards abbreviating call_graph to cg.
| virtual void printGroupSections() {} | ||
| virtual void printHashHistograms() {} | ||
| virtual void printCGProfile() {} | ||
| virtual void printCallGraphInfo() {} |
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.
Perhaps printCGInfo() to mirror printCGProfile?
| } | ||
| } | ||
| W.printNumber("NumIndirectTargetTypeIDs", CGInfo.IndirectTypeIDs.size()); | ||
| auto IndirectTypeIdsList = SmallVector<uint64_t, 4>( |
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.
| auto IndirectTypeIdsList = SmallVector<uint64_t, 4>( | |
| SmallVector<uint64_t, 4> IndirectTypeIdsList( |
This is more standard syntax in LLVM, I feel?
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.
| 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.
|
|
||
| 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.
| .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.
| if (Name == CallGraphSectionName) | ||
| return Sec; | ||
| } else | ||
| consumeError(NameOrErr.takeError()); |
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.
consumeError is generally a code smell.
Can an actual error be emitted here? If so, it should either be propagated or reported as a warning. If it can't ever fail here, cantFail is more appropriate.
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 function was not used and should've been removed in the first place. Thank you.
🐧 Linux x64 Test Results
|
Introduce a new flag
--call-graph-infowhich outputs callgraph ELFsection information to the console as a text output or as JSON output.