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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu marked 11 inline comments as done.
Ktwu added inline comments.


================
Comment at: lld/MachO/ExportTrie.h:27
   size_t build();
-  void writeTo(uint8_t *buf);
+  void writeTo(uint8_t *buf) const;
 
----------------
smeenai wrote:
> @Ktwu, if these changes were incorporated in D76977, would you not need to depend on that? That one's a meatier review than this one, so it'd be easier to get this in first if possible.
As discussed in the meeting, section merging depends on being able to have more than one symbol at a time for testing. The export trie is a hard dependency if we want to test this properly :/


================
Comment at: lld/MachO/MergedOutputSection.cpp:33
+void MergedOutputSection::finalize() {
+  uint64_t iaddr = addr;
+  fileSize = 0;
----------------
smeenai wrote:
> Super nit: `isecAddr` is a bit more understandable IMO.
Sure


================
Comment at: lld/MachO/MergedOutputSection.cpp:51
+
+// TODO: this is most likely wrong; reconsider how section flags
+// are actually merged.
----------------
smeenai wrote:
> Could you elaborate on what might be wrong in this comment?
I'll add this to the comment, but in short, there's no research that's gone into validating what the merge behavior for flags like this ought to be (it's also why there are no tests for flag merging).


================
Comment at: lld/MachO/MergedOutputSection.cpp:65
+
+  // Negate pure instruction presence if any segment isn't pure.
+  uint32_t pureMask = ~(MachO::S_ATTR_PURE_INSTRUCTIONS & inputFlags & flags);
----------------
smeenai wrote:
> segment -> section
Oops thanks.


================
Comment at: lld/MachO/MergedOutputSection.cpp:66
+  // Negate pure instruction presence if any segment isn't pure.
+  uint32_t pureMask = ~(MachO::S_ATTR_PURE_INSTRUCTIONS & inputFlags & flags);
+
----------------
smeenai wrote:
> If I'm understanding this correctly, it'll end up negating the pure bit when you don't want it to. If both `inputFlags` and `flags` have the bit set, the result of the AND for that bit will be 1, so it'll be 0 in your mask, so it'll get unset.
Ahhh nice catch (and ideally one that a test will confirm). I believe what I want is:

```
 uint32_t pureMask = ~MachO::S_ATTR_PURE_INSTRUCTIONS | (inputFlags & flags);
``` 


================
Comment at: lld/MachO/MergedOutputSection.h:19
+
+class MergedOutputSection : public OutputSection {
+public:
----------------
int3 wrote:
> smeenai wrote:
> > Could you add a class comment about what this represents?
> > 
> > I'd prefer renaming `OutputSection` to `OutputSectionBase` and `MergedOutputSection` to just `OutputSection`, to be more in line with ELF, but I don't feel super strongly about that.
> +1 on the renaming
Sure.


================
Comment at: lld/MachO/OutputSection.h:26-27
+  // These accessors will only be valid after finalizing the section.
+  virtual uint64_t getFileOffset() const;
+  virtual uint64_t getSegmentOffset() const;
+
----------------
smeenai wrote:
> Why do these need to be virtual?
ah, they don't, it's refactoring cruft


================
Comment at: lld/MachO/OutputSection.h:41
+  // Some sections may allow coalescing other raw input sections.
+  virtual void mergeInput(InputSection *input);
+
----------------
smeenai wrote:
> Is this needed for anything other than MergedOutputSection?
No, it's not needed except for MergedOutputSection. Should I dynamically cast each output section before trying to merge an input section into it instead (I wanted to avoid the runtime hit doing that).


================
Comment at: lld/MachO/OutputSegment.cpp:38
 
-void OutputSegment::addSection(InputSection *isec) {
-  isec->parent = this;
-  std::vector<InputSection *> &vec = sections[isec->name];
-  if (vec.empty() && !isec->isHidden()) {
-    ++numNonHiddenSections;
+size_t OutputSegment::numNonHiddenSections() const {
+  size_t count = 0;
----------------
smeenai wrote:
> If this is gonna be called often, we should either do some sort of caching, or else just do the computation as part of `addOutputSection` and make it a public member variable.
I believe I hit issues trying to cache this on adding an OutputSection; I think the GotSection made things difficult since its isNeeded() attribute is dynamic.


================
Comment at: lld/MachO/OutputSegment.h:73
+    if (it == orderMap.end()) {
+      return &orderMap.find(defaultPosition)->second;
+    }
----------------
smeenai wrote:
> Given that this might be the common case, would it make sense to cache the `find` somehow?
Good idea!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893