[PATCH] D114737: [lld][Macho] Include dead-stripped symbols in mapfile
Roger Kim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 10 11:57:23 PST 2022
Roger marked 3 inline comments as done.
Roger added inline comments.
================
Comment at: lld/MachO/MapFile.cpp:45
+// symbols.
+static std::vector<Defined *> getSymbols(bool Live) {
std::vector<Defined *> v;
----------------
int3 wrote:
> thevinster wrote:
> > nit: Variable name should be camel case. Also, I think `dumpLive` or something around that seems to be better fit than just `Live`.
> +1 for camel case. Also, rather than calling `getSymbols` twice, each time iterating over all symbols, it would be more efficient to have the function output two vectors, one for live and one for dead symbols.
@thevinster I decided to encapsulate the "live-ness" of the symbols in their container so the word "Live" now describes the container rather than configure the behavior of the function that dumps symbols. I think the word "Live" is now more appropriate.
@int3 Done.
================
Comment at: lld/MachO/MapFile.cpp:57
+ parallelSort(v.begin(), v.end(), [Live](Defined *a, Defined *b) {
+ return Live && a->getVA() != b->getVA() ? a->getVA() < b->getVA()
+ : a->getName() < b->getName();
----------------
thevinster wrote:
> Roger wrote:
> > thevinster wrote:
> > > Why do we need to propagate `Live` to the lambda? Does ld64 order dead stripped symbols by the name only?
> > The ordering for live symbols is first by virtual address and then by name. As I understand, dead-stripped symbols don't have virtual addresses so we can just order by name.
> Do we need to have this ordered? I guess you're trying to not fork the code so passing it into the lambda makes sense, but in general, we could skip the sort for dead stripped symbols.
I guess we don't need to have it ordered. It doesn't look like ld64 orders it. I can take out the ordering if that is better but I don't really know. I've refactored the code a bit but I don't know if that changes anything.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114737/new/
https://reviews.llvm.org/D114737
More information about the llvm-commits
mailing list