[PATCH] D114737: [lld][Macho] Include dead-stripped symbols in mapfile

[Roger Kim via Phabricator via llvm-commits]

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