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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:962
+  case Triple::Wasm:
+    return Ctx->getWasmSection(Name, SectionKind::getMetadata(), utostr(Hash),
+                               ~0);
----------------
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 :)


================
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.)
----------------
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.


================
Comment at: llvm/test/DebugInfo/WebAssembly/dwarf-headers.ll:47
+;
+; SINGLE-4: .debug_types contents:
+; SINGLE-4: 0x00000000: Type Unit: {{.*}} version = 0x0004, abbr_offset
----------------
dblaikie wrote:
> aardappel wrote:
> > I guess this doesn't actually test that only one copy of these remain after linking? That's already tested in LLD?
> Certainly lld shouldn't be tested here, no (llvm tests can't depend on other subprojects like lld) - but yeah, some kind of comdat deduplication tests should exist in lld (they don't have to be DWARF specific - the DWARF type deduplication is meant to piggy-back on the existing comdat deduplication infrastructure in thel inker)
@aardappel yes, pretty much what @dblaikie said :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88603