[PATCH] D70557: [lld][COFF] Add support for /map

[Sylvain Audi via Phabricator via llvm-commits]

saudi marked 2 inline comments as done.
saudi added inline comments.


================
Comment at: lld/COFF/LLDMapFile.cpp:69
+      return a->getRVA() < b->getRVA();
+    });
+  }
----------------
amccarth wrote:
> Stupid question:  Is it possible for two symbols to share an address?  If so, should this be a `std::stable_sort` for deterministic build outputs?
There definitely are symbols that share an address.
Note that LLDMapFile.cpp was previously MapFile.cpp which was renamed and mostly unchanged (just adapted for the rename).
But I don't mind fixing it on top, should I do that in this commit?


================
Comment at: lld/COFF/MapFile.cpp:81
+    return a.first < b.first || (a.first == b.first && a.second < b.second);
+  });
+  auto end = std::unique(v.begin(), v.end(), [](const auto &a, const auto &b) {
----------------
amccarth wrote:
> Is the lambda necessary in this sort?  Don't `std::pair`s naturally use lexicographic ordering?
> 
> Do all the `Defined *`s live inside a single object l(ike an array)?  If not, then comparing pointers to individual ones is technically undefined behavior though it's very likely to work as expected.
Indeed, the `Define*`s come from various `ObjFile`s so we can't expect them to come from the same array.

This sort is just to regroup entries with the same pointer value, for std::unique to work.
I just noticed:
  - comparing the entire pair is not necessary here  (we just need to regroup same pointers)
  - we just need to compare the value of the pointers, so maybe converting to size_t would be more relevant?

I guess the sort could become :

```
parallelSort(v, [](const auto &a, const auto &b) {
  return (size_t)a.first < (size_t)b.first;
});

```



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70557/new/

https://reviews.llvm.org/D70557