[PATCH] D88603: [WebAssembly] Add support for DWARF type units

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:962
+  case Triple::Wasm:
+    return Ctx->getWasmSection(Name, SectionKind::getMetadata(), utostr(Hash),
+                               ~0);
----------------
dschuff wrote:
> dblaikie wrote:
> > dschuff wrote:
> > > dschuff wrote:
> > > > I may add a couple more tests to this, but I did want to ask @sbc100 about this, since I'm not 100% sure at the uniqueID field is for.
> > > also let me be more clear about the question here: what is `UniqueID` for, and is it bad that I'm just passing it a number that is totally not unique?
> > For ELF, at least, I believe the unique ID is used to know which elements are to be treated as part of the same deduplication set.
> > 
> > If Wasm support in lld does the same thing, then using the same number for every type unit would mean the linked binary would end up with only one type definition - even when the input has many varied/independent type definitions. Likely not what's intended.
> For wasm I had thought that was what the 3rd argument (Group) was for. So if that's what `UniqueID` is for, then I have the same question about Group :)
Oh, fair enough - I hadn't read closely. Yeah, guess it's up to you folks/how the wasm object format works... - so I'm with you on the "what is the uniqueID field for" (& what's the other field that's taking the hash?) & I'll leave it to you folks to... hash out.


================
Comment at: llvm/test/DebugInfo/WebAssembly/dwarf-headers.ll:24-30
+; Looking for DWARF headers to be generated correctly.
+; There are 8 variants with 5 formats: v4 CU, v4 TU, v5 normal/partial CU,
+; v5 skeleton/split CU, v5 normal/split TU.  Some v5 variants differ only
+; in the unit_type code, and the skeleton/split CU differs from normal/partial
+; by having one extra field (dwo_id).
+; (v2 thru v4 CUs are all the same, and TUs were invented in v4,
+; so we don't bother checking older versions.)
----------------
dschuff wrote:
> dblaikie wrote:
> > Given that S.plit DWARF type units don't require comdat support (the dwp tool will be aware of the type units and parse their boundaries, read their type hash from the TU Header, etc). /may/ be worth separating that functionality/testing from the comdat support/testing - but maybe not all that interesting to separate it into two separate patches. (& if you find the test coverage works better in one test, that's OK - just a thought)
> I copied this test more-or-less directly from X86 :)
> Although I will say it's kind of nice to test all the header types in one go like this just because it's slightly annoying to construct the LLVM metadata for debuginfo tests, so it's nice to be able to share that as much as possible. Previously we just didn't have enough coverage for split-dwarf anyway.
Ah, fair enough - symmetry sounds good :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88603