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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

I'm assuming the diff hasn't been updated with the OutputSection -> OutputSectionBase and MergedOutputSection -> OutputSection renaming yet (assuming you're good with that).



================
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);
+
----------------
Ktwu wrote:
> 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);
> ``` 
Yup, looks good.


================
Comment at: lld/MachO/SyntheticSections.h:165-169
 struct InStruct {
   GotSection *got = nullptr;
 };
 
 extern InStruct in;
----------------
smeenai wrote:
> I believe the `in` is short for "globally accessibly input sections". LLD ELF has a corresponding struct called `Out` for output sections (in OutputSections.h), and given that our synthetic sections are output sections now, it might make sense to adopt that name.
This one still needs addressing, though I'm okay with doing it in a follow-up if you'd prefer.


================
Comment at: lld/test/MachO/section-merge.s:45
+# DATA:           SectionData (
+# DATA-NEXT:        0000: 48C7C001 000000C3 48C7C000 000000C3
+# DATA-NEXT:      )
----------------
Might be easier to make sense of this as assembly (`llvm-objdump -d`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893