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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

Looks good at a high level!



================
Comment at: lld/MachO/InputSection.h:49
 
-  ArrayRef<uint8_t> data;
-
-  // TODO these properties ought to live in an OutputSection class.
-  // Move them once available.
   uint64_t addr = 0;
   uint32_t align = 1;
----------------
LLD ELF has an `outSecOff` in its InputSections, which tracks the offset of this particular section within its output section. I think that'd be better than storing the absolute address in the InputSection.


================
Comment at: lld/MachO/OutputSection.cpp:20
+uint64_t OutputSection::getFileOffset() const {
+  return parent->fileOff + addr - parent->firstSection()->addr;
+}
----------------
What's this computation doing?


================
Comment at: lld/MachO/OutputSection.cpp:32
+
+  this->hidden |= input->isHidden();
+  this->inputs.push_back(input);
----------------
We should experiment with how ld64 handles merging hidden sections, or if that's even a thing it does. (I don't think you can specify a section is hidden yourself; ld64 just has a list of atoms it defines to be hidden.)


================
Comment at: lld/MachO/OutputSegment.cpp:36
 
-void OutputSegment::addSection(InputSection *isec) {
-  isec->parent = this;
-  std::vector<InputSection *> &vec = sections[isec->name];
-  if (vec.empty() && !isec->isHidden()) {
-    ++numNonHiddenSections;
+OutputSection *OutputSegment::addSection(InputSection *input) {
+  OutputSegment::SectionMap::iterator i = this->sections.find(input->name);
----------------
I'm wondering if it'd be better to construct an OutputSection independently and then pass that into this function instead.


================
Comment at: lld/MachO/OutputSegment.cpp:51-52
+    // While creating the output section
+  if (os->isHidden()) {
+    this->numNonHiddenSections++;
+  }
----------------
Is the check flipped?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893