[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:
> 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.


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


================
Comment at: llvm/test/DebugInfo/WebAssembly/dwarf-headers.ll:47
+;
+; SINGLE-4: .debug_types contents:
+; SINGLE-4: 0x00000000: Type Unit: {{.*}} version = 0x0004, abbr_offset
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88603