[PATCH] D77893: WIP [lld] Merge Mach-O input sections

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

Looking good! Needs tests once everything's working :)



================
Comment at: lld/MachO/InputSection.h:38
   uint64_t getFileOffset() const;
+  uint64_t getFinalizedAddr() const;
+
----------------
I think `getVA` would be more in line with the LLD naming for this concept.


================
Comment at: lld/MachO/InputSection.h:51
+  OutputSection *parent = nullptr;
+  uint64_t parentAddrOffset = 0;
 
----------------
Similarly, since this is the same notion as LLD ELF's `outSecOff`, I'd probably stick with that name, just so it's easy to map concepts between the two.


================
Comment at: lld/MachO/OutputSection.cpp:20
+uint64_t OutputSection::getFileOffset() const {
+  return parent->fileOff + addr - parent->firstSection()->addr;
+}
----------------
Ktwu wrote:
> smeenai wrote:
> > What's this computation doing?
> It's copying what InputSection did to calculate its file offset. I didn't entirely comprehend this math tbh.
Okay, this makes sense.


================
Comment at: lld/MachO/OutputSection.cpp:30
+    if (input->flags != this->flags)
+      error("Cannot add merge section with inconsistent flags");
+    this->align = std::max(this->align, input->align);
----------------
Can you add more details to this error message? It'd be ideal to have things like the section in question, the object file it's coming from, etc.

Also, is this what ld64 does?


================
Comment at: lld/MachO/OutputSection.cpp:34
+
+  this->hidden |= input->isHidden();
+  this->inputs.push_back(input);
----------------
Can you add a TODO for figuring out how we should handle input sections with conflicting hidden-ness?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893