Add an option to disable sorting sections with call graph profile
Details
Diff Detail
- Repository
- rLLD LLVM Linker
Event Timeline
I really like adding the option to disable it, but I'm not sure about disabling it by default. Is there any situation where the profile is present where a user wouldn't want to perform layout? The algorithm is very fast so unexpected link time increase isn't really an issue, and the exact order of sections already isn't guaranteed by ELF and differs between linkers so I don't expect there to be correctness issues.
Speaking of whether we should enable or disable it by default, I second Michael. In most cases, if your object files contain .llvm.call-graph-profile sections, that's because you are instructed clang/llvm to do so, and if lld doesn't reorder sections in that case, most users wonder why lld doesn't work "correctly". From that UX perspective, that should be enabled by default.
ELF/Driver.cpp | ||
---|---|---|
773 | nit: you probably should spend a bit more time to read existing code around the location where you add new code to make your new code consistent with existing one. All the other Config members have the exact same name with a corresponding command line flag. For example, Config->EhFrameHdr corresponds to --eh-frame-hdr and Config->EmitRelocs corresponds to --emit-relocs. Please make your new code consistent with existing code. |
ELF/Driver.cpp | ||
---|---|---|
773 | I know, but the variable CallGraphProfile is already used as a map, what should I rename it to? |
Name the bool Config->CallGraphProfile and rename existing COnfig->CallGraphProfile to CallGraphInfo
ELF/Driver.cpp | ||
---|---|---|
773 | bool CallGraphProfile; llvm::MapVector<std::pair<const InputSectionBase *, const InputSectionBase *>, uint64_t> CallGraphInfo; Let me know if you have other preference |
ELF/Driver.cpp | ||
---|---|---|
773 | Yeah you should, because you essentially should keep code beautiful/readable for those who read this code for the first time. If in doubt, forget about history of code and see new code with fresh eye. I'm sure you'll find this code inconsistent and odd. |
ELF/Driver.cpp | ||
---|---|---|
773 | This is a much less descriptive name. I would call the option {-no}-call-graph-profile-sort as you are disabling the sorting based on the profile, not the profile itself. Then the option name becomes CallGraphProfileSort. |
ELF/CallGraphSort.cpp | ||
---|---|---|
99 ↗ | (On Diff #171177) | Unrelated change? |
ELF/CallGraphSort.cpp | ||
---|---|---|
99 ↗ | (On Diff #171177) | Sorry I noticed it in my "Revert a comment change" but maybe that command failed |
Change the text file from cgprofile-obj-warn.s to cgprofile-obj.s as I will delete cgprofile-obj-warn.s in https://reviews.llvm.org/D53669
Use llvm-nm instead of llvm-readobj to check symbol addresses
nit: you probably should spend a bit more time to read existing code around the location where you add new code to make your new code consistent with existing one. All the other Config members have the exact same name with a corresponding command line flag. For example, Config->EhFrameHdr corresponds to --eh-frame-hdr and Config->EmitRelocs corresponds to --emit-relocs. Please make your new code consistent with existing code.