The current implementation of displaying .stack_size information presumes that each entry represents a single function but this is not always the case. For example with the use of ICF multiple functions can be represented with the same code, meaning that the address found in a .stack_size entry corresponds to multiple function symbols.
This change allows multiple function names to be displayed when appropriate.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
See https://bugs.llvm.org/show_bug.cgi?id=43245 which I believe was incorrectly marked as fixed, as the behaviour described in the bug ticket is still present.
I want to note here, in the case of --elf-output-style=GNU this change should not be disruptive for the usual use case in which only one function symbol corresponds to an entry. However the --elf-output-style=LLVM output requires these symbols to be displayed as a list, meaning in the usual use case a list of size one will be output for each .stack_size entry.
This change touches printBBAddrMaps due to implementation but should see no behaviour difference.
I realise a number of other tests require updating due to the change in output, I'll fix them up once an agreement has been made regarding this change.
llvm/test/tools/llvm-readobj/ELF/stack-sizes-multiple-symbols.test | ||
---|---|---|
1 ↗ | (On Diff #358219) | It probably makes more sense to add this test case to the existing stack-sizes.test test. |
llvm/test/tools/llvm-readobj/ELF/stack-sizes.test | ||
180 | Multiple symbols with the same value is not specific to executables, so combining these test cases sounds like it risks confusion. It probably makes more sense to just take the multiple symbols case you've written in a separate file and add it to this file. | |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
65 | What's this for? | |
304–307 | Make sure to clang-format new/modified code. | |
5813 | Nit: fix all the clang-tidy warnings. | |
5824 | Use ArrayRef for passing around vectors by reference. Same elsewhere. | |
5830 | Use StringRef, not const std::string &. |
I also folded the new stack-size test into the existing one as requested.
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
65 | This is for use of std::accumulate |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
5828 | After some offline discussion around the inefficiency of the previous implementation, the use of join makes more sense here. |
LGTM, with one nit.
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
65 | I believe you can now remove this include? |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
65 | Good catch, I've removed it. |
Multiple symbols with the same value is not specific to executables, so combining these test cases sounds like it risks confusion. It probably makes more sense to just take the multiple symbols case you've written in a separate file and add it to this file.