[PATCH] D79668: [lld-macho] Support -order_file

[Jez Ng via Phabricator via llvm-commits]

int3 marked 4 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/Config.h:41
+  size_t anyObjectFile = 0;
+  llvm::StringMap<size_t> objectFiles;
 };
----------------
I guess I should change this to a DenseMap too


================
Comment at: lld/MachO/Driver.cpp:132
+static bool isArchString(StringRef s) {
+  static StringSet<> archNames{"ppc",    "ppc64", "i386",
+                               "x86_64", "arm",   "arm64"};
----------------
smeenai wrote:
> I'm assuming the names are the same ld64 uses? ("arm" in particular.)
> 
> Also, considering these are string constants and there's therefore no ownership concerns, a `DenseSet<StringRef>` might be better, for the same reasons as discussed above with the DenseMap? (Tbh, for this number of entries, just a linear search might be faster still, but that's definitely getting into unnecessary premature optimization territory :)).
> I'm assuming the names are the same ld64 uses? ("arm" in particular.)

yup


================
Comment at: lld/MachO/Driver.cpp:171
+
+      if (tok.endswith(".o"))
+        objectFile = tok;
----------------
smeenai wrote:
> It's valid (albeit unlikely) for a symbol name to end with `.o`. It's also valid (albeit unlikely) for a symbol to have the same name as an architecture.
> 
> Can we keep track of fields explicitly instead? For each line, build up its colon delimited fields. If you have more than three fields, it's an error. If you have three fields, the first one should be an architecture (which we should verify), the second one should be an object file (which we should verify), and the third one should be the symbol name. If you have only one field, it's the symbol name. And so on.
I think ld64 makes these assumptions as well (that symbol names in the order file will never end with an `.o`), but yeah, I'll try and make this more robust


================
Comment at: lld/test/MachO/order-file.s:61
+
+# RUN: echo "_bar" > %t/ord8
+# RUN: echo "_main" >> %t/ord8
----------------
smeenai wrote:
> Probably worth adding a comment about what's going on here ... it took me a bit to understand it.
> 
> With multiple symbols pointing to the same location, I think it's technically arbitrary which one `llvm-objdump` chooses to display, but it seems like it's just picking the first name, so that works for us.
> 
> Is the intention here that `_foo` and `_bar` are different symbols in the same section (and they'd be split into their own sections once we have subsections via symbols), or that they're aliases to the same symbol (and therefore ordering one should order the other)?
> they're aliases to the same symbol (and therefore ordering one should order the other)?

This latter one is the reason. The subsections code actually looks for and avoids creating a new subsection when two symbols alias the same location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79668