[PATCH] D103006: [lld][MachO] Add support for LC_DATA_IN_CODE

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:624
+              {static_cast<uint32_t>(it->offset - beginAddr +
+                                     isec->getFileOffset()),
+               it->length, it->kind});
----------------
int3 wrote:
> shouldn't it be `getOffset()`? we shouldn't be mixing addresses and file offsets in the same calculation... the two can diverge (since zerofill sections have zero file size but occupy a nonzero address range). Sometimes they line up but they should be treated as effectively different types.
> 
> also we did discuss that DIC offsets probably only make sense if you assume that there's exactly one code section, but the loop here attempts to handle the case of multiple code sections. I don't think this offset calculation makes sense if there are multiple sections, since the offset is relative to the start of the OutputSection, but there's nothing in each DIC entry to say which section it belongs to. What does ld64 do here -- does it calculate offsets from the first code section?
> I don't think this offset calculation makes sense if there are multiple sections, since the offset is relative to the start of the OutputSection, but there's nothing in each DIC entry to say which section it belongs to.

Nevermind, I get it -- the offset is from the start of the file (or as we've discussed offline, the `__TEXT` segment), so it makes sense regardless of the number of code sections present.

FWIW, local experimentation indicates that ld64 just drops DIC entries that are within code sections not called `__text`, though clang/llvm-mc is perfectly willing to emit them. I don't think it hurts to emit extra DIC entries in the final binary for now, since it probably makes the implementation simpler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103006