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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Thanks for answering my questions.



================
Comment at: lld/COFF/LLDMapFile.cpp:69
+      return a->getRVA() < b->getRVA();
+    });
+  }
----------------
saudi wrote:
> 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?
I think switching to stable_sort should probably be a separate commit. That way, if I'm wrong, it can be rolled back without affecting anything else.


================
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) {
----------------
saudi wrote:
> 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;
> });
> 
> ```
> 
Casting to size_t is fine, but I don't think it's necessary given how you answered the first question here.

I always get nervous when I see a custom comparator that ignores some of the fields--I worry that somehow it's not providing a strict weak total ordering.  But I think it's fine here.

That said, you could just use `std::lexicographical_compare` rather than a lambda.  


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

https://reviews.llvm.org/D70557