[PATCH] D105866: [lld-macho] Use intermediate arrays to store opcodes

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

lgtm with minor changes. Do you have commit rights or should I commit it for you?



================
Comment at: lld/MachO/SyntheticSections.cpp:281-282
+struct BindIR {
+  uint8_t opcode = 0xF0;
+  uint64_t data = 0;
+};
----------------
do we need these default values?


================
Comment at: lld/MachO/SyntheticSections.cpp:298-299
   if (lastBinding.segment != seg) {
-    os << static_cast<uint8_t>(BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB |
-                               seg->index);
-    encodeULEB128(offset, os);
+    opcodes.push_back(
+        {BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB | seg->index, offset});
     lastBinding.segment = seg;
----------------
ah I guess we need the cast


================
Comment at: lld/MachO/SyntheticSections.cpp:334
+static void flushOpcodes(BindOpcodeOpt &op, raw_svector_ostream &os) {
+  uint8_t opcode = op.opcode & 0xF0;
+  switch (opcode) {
----------------
thevinster wrote:
> int3 wrote:
> > I don't think there's anything to mask here, since we are explicitly setting `op.opcode` to one of the opcode values minus any extra data. But we could do an assert to be on the safe side :)
> Doesn't this operation `BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB | seg->index` mean it's possible that extra data could be embedded in an opcode value? Otherwise, why perform `seg->index` in the first place?
derp yeah I forgot about that one


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105866