[PATCH] D97657: [lld][WebAssembly] Initial support merging string data

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

I think I'm generally fine with keeping things that are direct copies from ELF, although it would be nice if it were even easier to tell which pieces those are (there is one comment in a header; is it just that function though?)



================
Comment at: lld/wasm/InputChunks.h:216
+public:
+  SyntheticMergedDataSegment(StringRef name, uint32_t flags_,
+                             uint32_t alignment_)
----------------
Do we care about these lint checks? (I guess lld already has different style than LLVM?)


================
Comment at: lld/wasm/OutputSegment.cpp:42
+  LLVM_DEBUG(llvm::dbgs() << "finalizeInputSegments: " << name << "\n");
+  std::vector<SyntheticMergedDataSegment *> mergeSegments;
+  std::vector<InputSegment *> newSegments;
----------------
sbc100 wrote:
> sbc100 wrote:
> > dschuff wrote:
> > > the name `mergeSegments` is slightly confusing given (IIUC) that "merge segments" is used elsewhere to refer to segments before merging and here they are already-merged segments.
> > Ah yes, perhaps `mergedSegments` is better.
> Actually this also comes from ELF which has `std::vector<MergeSyntheticSection *> mergeSections;`   but they have slightly different class naming going on.
I also had a question about the class naming above.


================
Comment at: lld/wasm/OutputSegment.cpp:54
+
+    auto i = llvm::find_if(mergeSegments, [=](SyntheticMergedDataSegment *seg) {
+      return seg->flags == ms->flags && seg->alignment == ms->alignment;
----------------
sbc100 wrote:
> dschuff wrote:
> > this capture could probably just be `ms` instead of `=`
> I copied this from `lld/ELF/OutputSections.cpp`.   Is there a runtime difference are you suggesting that for readability?
mostly just for readability. I think auto-capturing a simple local variable like that should end up the same in the code.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:798
+  DwarfStrSection = Ctx->getWasmSection(
+      ".debug_str", SectionKind::getMetadata(), wasm::WASM_SEG_FLAG_STRINGS);
   DwarfLocSection =
----------------
sbc100 wrote:
> dschuff wrote:
> > So this sets the section flag as a string section, but we just don't merge them yet?
> Yes, currently this will not get used anywhere, or written the binary format.  If you prefer I can drop this part of the change and add it in the followup.
I don't necessarily object; but you also mentioned above that the handling (or just the naming?) might be different for custom sections compared to custom sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97657