[WebAssembly] COMDAT: core LLVM support

[ncw]

Summary

This adds COMDAT support to the LLVM frontend and Wasm object-file support.

For spec, see https://github.com/WebAssembly/tool-conventions/pull/31

For corresponding LLD change, see https://bugs.llvm.org/show_bug.cgi?id=35533, and D40845

[sbc100]

Thanks for doing this. Looks like a good direction!

include/llvm/BinaryFormat/Wasm.h

80

Why a new struct here but not for Function and Segment?

If we decide a new struct is needed, we have tended to prefer composition over inheritance for this kind of thing.

217

Give that we already have symbol and segment info, and that functions and globals are both symbols, would it make sense to add the COMDAT to the existing segment / symbol info? I'm don't feel strongly about this, just wondering what you think?

lib/MC/WasmObjectWriter.cpp

149

Should this maybe go in BinaryFormat/Wasm.h since its part of the binary format. Then the reader could use it too?

And maybe another struct type which is a Name + a vector of WasmComdatEntry would be useful to represent a whole COMDAT declaration?

lib/Object/WasmObjectFile.cpp

413

Read into a WasmComdatEntry maybe?

[ncw]

include/llvm/BinaryFormat/Wasm.h

80

I knew this would come up! It's a hack/workaround. I would have preferred to add the member to WasmGlobal, but WasmGlobal is used inside a union (in WasmImport) and StringRef is non-POD so can't go in a union.

I'll fix it up one way or another, WasmGlobalDecl is a (small) bodge just to get it working. Possible solutions: use a Variant instead of a union in WasmImport; or, review the hierarchy of types and come up with a cleaner representation of the Wasm data. (I just need an hour to draw some diagrams and read the existing code to understand where the Comdat field belongs.)

217

I mainly did it this way for size. C++ function names can be hundreds of characters long, so you don't want to use the name very often in the object file.

ELF in fact does something very sneaky, and in the COMDAT group directly references the symbol name in the symbol table, for ultimate compactness.

If we put the long (string) COMDAT identifier next to each segment/symbol that would be quite a bit of bloat. We could have a list of COMDAT names separately, and then in the symbol data have an integer index into the COMDAT list, but I'm not sure that's any tidier than this approach.

lib/MC/WasmObjectWriter.cpp

149

Good idea.

[ncw]

OK, I've cleaned this up, and I think it's ready to go.

• Firstly, note that this can be merged before LLD gains COMDAT support (ie - this doesn't need corresponding changes in LLD to keep working)
• I've made a matching PR in the tool-conventions repo, https://github.com/WebAssembly/tool-conventions/pull/31
• Added test coverage for each bit

Now, the contentious bit - is it actually the right design to be using ELF-style COMDAT?

Well, I think it _maybe_ is, and it's better than nothing. I think we basically need to just keep on improving LLD, and as we go we'll add more flags and bits and pieces as needed. LLD linking is a bit different to x86 linking, certainly when we start doing more clever dead code removal, and I don't want to anticipate that too much.

I think there's value in merging a simple implementation soon, and improving it.

Anything more fancy than the COMDAT support I've done here, would require a correspondingly more fancy/complicated initial review on the LLD side.

[ncw]

Update - added "flags" field and removed explicit Comdat for globals, following Dan's comments on the GitHub PR.

[ncw]

lib/MC/WasmObjectWriter.cpp

149

I've decided to keep it this way, after having a fiddle to see what it would look like.

Firstly, I'm just storing the COMDAT information in the same way as existing linking data. We don't have a struct for the "names" custom section, and when we read them in, we attach the names directly to the objects.

I actually like the way it's represented. Conceptually, the COMDAT name *is* attached to the function/data-segment. The linking section is just a way to attach extra information to the standard Wasm sections, and I think it makes sense to model it that way.

[sbc100]

Based on the discussion in the bug (https://bugs.llvm.org/show_bug.cgi?id=35533) it sounds like we need to take a look at SHF_LINK_ORDER and how that works. It sounds like its different to the "group" concept. But if we do decide groups are the way to go this change lgtm.

[ncw]

Changes:

• Rebased
• Added yaml2obj support to match obj2yaml, tweaked the YAML display for neatness

[ncw]

In D40844#951754, @sbc100 wrote:

Based on the discussion in the bug (https://bugs.llvm.org/show_bug.cgi?id=35533) it sounds like we need to take a look at SHF_LINK_ORDER and how that works. It sounds like its different to the "group" concept. But if we do decide groups are the way to go this change lgtm.

I've added a looong comment to that bug. SHF_LINK_ORDER really is very very similar to "Comdat groups" - the only differences are 1) ELF historical encoding stuff, and 2) SHF_LINK_ORDER creates "anonymous" linker groupings rather than named Comdat groups.

I basically can't see what all the fuss is about! Comdat "groupings" are what we want, and all the alternatives we're discussing provide the same semantic. It's just arguing about the name and the details? Maybe if you've been working on this for a while though the details seem bigger.

I can't see a major functional difference between what I've implemented here, and what SHF_LINK_ORDER would provide. I might be missing something though....

[sbc100]

include/llvm/Object/Wasm.h

142

I'd rather expose an ArrayRef here (I think you can use a local function-local set() to detect duplicate entries while parsing). For consistency and so that comdats appear in order.

lib/Object/WasmObjectFile.cpp

424

while (ComdatCount--)?

435

while (EntryCount--)

454

We now have isValidFunctionIndex(Index) for this.

lib/ObjectYAML/WasmYAML.cpp

64

std::map funny to me here. This this its because I worry about iteration order being consistent. I'd want the comdats to be displayed in the order they appear in the input file, not alphabetically.

How about std::vector<Comdat> where Comdat is a struct with a name + entries?

[ncw]

Updated:

• Responded to Sam's comments
• Fixed a minor issue with the function indices
• Fixed a bug in my addition to yaml2wasm

include/llvm/Object/Wasm.h

142

Good point, done.

lib/ObjectYAML/WasmYAML.cpp

64

OK, for YAML it's using a vector now. That makes sense, thanks.

[sunfish]

I'm behind on some of the discussions on this topic at the moment, but overall, this looks like it's headed in a good direction, so I'm in favor of this patch.

[sbc100]

Would you mind re-basing this, and then I'll land it.

[ncw]

Thanks!

Rebased.

NB. I haven't yet re-run the tests, this is an optimistic update - it takes a looong time to compile everything, and after a rebase pretty much everything needs to recompile. Just building the tests now.

[ncw]

MC tests pass, but I've just noticed I missed one in CodeGen/WebAssembly/comdat.ll. I won't be able to fix it today, sorry..

[sbc100]

tools/obj2yaml/wasm2yaml.cpp

52

I ended up exposing the Index directly in the WasmFunction struct to avoid the need for this.

[sbc100]

When testing with lld I noticed that the comdat group names are appearing as undefined symbols in the final output.

See: https://reviews.llvm.org/D41959

I this case it results in unresolved symbol: sharedComdat

Can you think of a way to avoid including these?

docs/LangRef.rst

 ``largest``
     The linker will choose the section containing the largest COMDAT key.
 ``noduplicates``
     The linker requires that only section with this COMDAT key exist.
 ``samesize``
     The linker may choose any COMDAT key but the sections must contain the
     same amount of data.
 
-Note that the Mach-O platform doesn't support COMDATs and ELF only supports
-``any`` as a selection kind.
+Note that the Mach-O platform doesn't support COMDATs, and ELF and WebAssembly
+only support ``any`` as a selection kind.
 
 Here is an example of a COMDAT group where a function will only be selected if
 the COMDAT key's section is the largest:
 
 .. code-block:: text
 
    $foo = comdat largest
    @foo = global i32 2, comdat($foo)

include/llvm/ADT/Triple.h

bool isARM() const {
     return getArch() == Triple::arm || getArch() == Triple::armeb;
   }
 
   /// Tests whether the target is AArch64 (little and big endian).
   bool isAArch64() const {
     return getArch() == Triple::aarch64 || getArch() == Triple::aarch64_be;
   }
 
-  /// Tests wether the target supports comdat
+  /// Tests whether the target supports comdat
   bool supportsCOMDAT() const {
-    return !isOSBinFormatMachO() && !isOSBinFormatWasm();
+    return !isOSBinFormatMachO();
   }
 
   /// @}
   /// @name Mutators
   /// @{
 
   /// setArch - Set the architecture (first) component of the triple
   /// to a known type.

include/llvm/BinaryFormat/Wasm.h

union {
     uint32_t Global;
   } Value;
 };
 
 struct WasmGlobal {
   int32_t Type;
   bool Mutable;
   WasmInitExpr InitExpr;
+  StringRef Comdat;
 };
 
 struct WasmImport {
+  WasmImport() {} // A StringRef in the union deletes the default ctor
   StringRef Module;
   StringRef Field;
   uint32_t Kind;
   union {
     uint32_t SigIndex;
     WasmGlobal Global;
     WasmTable Table;
     WasmLimits Memory;
   };
 };
 
 struct WasmLocalDecl {
   int32_t Type;
   uint32_t Count;
 };
 
 struct WasmFunction {
   std::vector<WasmLocalDecl> Locals;
   ArrayRef<uint8_t> Body;
+  StringRef Comdat;
 };
 
 struct WasmDataSegment {
   uint32_t MemoryIndex;
   WasmInitExpr Offset;
   ArrayRef<uint8_t> Content;
   StringRef Name;
   uint32_t Alignment;
   uint32_t Flags;
+  StringRef Comdat;
 };
 
 struct WasmElemSegment {
   uint32_t TableIndex;
   WasmInitExpr Offset;
   std::vector<uint32_t> Functions;
 };
 
enum : unsigned {
   WASM_OPCODE_GET_GLOBAL = 0x23,
   WASM_OPCODE_I32_CONST = 0x41,
   WASM_OPCODE_I64_CONST = 0x42,
   WASM_OPCODE_F32_CONST = 0x43,
   WASM_OPCODE_F64_CONST = 0x44,
 };
 
 enum : unsigned {
-  WASM_NAMES_FUNCTION = 0x1,
-  WASM_NAMES_LOCAL = 0x2,
-};
-
-enum : unsigned {
   WASM_LIMITS_FLAG_HAS_MAX = 0x1,
 };
 
 // Subset of types that a value can have
 enum class ValType {
   I32 = WASM_TYPE_I32,
   I64 = WASM_TYPE_I64,
   F32 = WASM_TYPE_F32,
   F64 = WASM_TYPE_F64,
 };
 
-// Linking metadata kinds.
+// Kind codes used in the custom "name" section
+enum : unsigned {
+  WASM_NAMES_FUNCTION = 0x1,
+  WASM_NAMES_LOCAL    = 0x2,
+};
+
+// Kind codes used in the custom "linking" section
 enum : unsigned {
   WASM_SYMBOL_INFO    = 0x2,
   WASM_DATA_SIZE      = 0x3,
   WASM_DATA_ALIGNMENT = 0x4,
   WASM_SEGMENT_INFO   = 0x5,
+  WASM_COMDAT_INFO    = 0x6,
+};
+
+// Kind codes used in the custom "linking" section in the WASM_COMDAT_INFO
+enum : unsigned {
+  WASM_COMDAT_DATA        = 0x1,
+  WASM_COMDAT_FUNCTION    = 0x2,
+  WASM_COMDAT_GLOBAL      = 0x3,
 };
 
 const unsigned WASM_SYMBOL_BINDING_MASK       = 0x3;
 const unsigned WASM_SYMBOL_VISIBILITY_MASK    = 0x4;
 
 const unsigned WASM_SYMBOL_BINDING_GLOBAL     = 0x0;
 const unsigned WASM_SYMBOL_BINDING_WEAK       = 0x1;
 const unsigned WASM_SYMBOL_BINDING_LOCAL      = 0x2;
 const unsigned WASM_SYMBOL_VISIBILITY_DEFAULT = 0x0;
 const unsigned WASM_SYMBOL_VISIBILITY_HIDDEN  = 0x4;
 
 #define WASM_RELOC(name, value) name = value,
 
 enum : unsigned {
 #include "WasmRelocs/WebAssembly.def"
 };
 
 #undef WASM_RELOC
 
 struct Global {
   ValType Type;
   bool Mutable;
 
   // The initial value for this global is either the value of an imported

include/llvm/Object/Wasm.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_OBJECT_WASM_H
 #define LLVM_OBJECT_WASM_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <cstddef>
 #include <cstdint>
 #include <vector>
public:
   const std::vector<wasm::WasmSignature>& types() const { return Signatures; }
   const std::vector<uint32_t>& functionTypes() const { return FunctionTypes; }
   const std::vector<wasm::WasmImport>& imports() const { return Imports; }
   const std::vector<wasm::WasmTable>& tables() const { return Tables; }
   const std::vector<wasm::WasmLimits>& memories() const { return Memories; }
   const std::vector<wasm::WasmGlobal>& globals() const { return Globals; }
   const std::vector<wasm::WasmExport>& exports() const { return Exports; }
   const wasm::WasmLinkingData& linkingData() const { return LinkingData; }
+  const llvm::StringSet<>& comdats() const { return Comdats; }
 
   uint32_t getNumberOfSymbols() const {
     return Symbols.size();
   }
 
   const std::vector<wasm::WasmElemSegment>& elements() const {
     return ElemSegments;
   }
 
   const std::vector<WasmSegment>& dataSegments() const {
private:
   Error parseStartSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseElemSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseCodeSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseDataSection(const uint8_t *Ptr, const uint8_t *End);
 
   // Custom section types
   Error parseNameSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseLinkingSection(const uint8_t *Ptr, const uint8_t *End);
+  Error parseLinkingSectionComdat(const uint8_t *&Ptr, const uint8_t *End);
   Error parseRelocSection(StringRef Name, const uint8_t *Ptr,
                           const uint8_t *End);
 
   void populateSymbolTable();
 
   wasm::WasmObjectHeader Header;
   std::vector<WasmSection> Sections;
   std::vector<wasm::WasmSignature> Signatures;
   std::vector<uint32_t> FunctionTypes;
   std::vector<wasm::WasmTable> Tables;
   std::vector<wasm::WasmLimits> Memories;
   std::vector<wasm::WasmGlobal> Globals;
   std::vector<wasm::WasmImport> Imports;
   std::vector<wasm::WasmExport> Exports;
   std::vector<wasm::WasmElemSegment> ElemSegments;
   std::vector<WasmSegment> DataSegments;
   std::vector<wasm::WasmFunction> Functions;
   std::vector<WasmSymbol> Symbols;
+  StringSet<> Comdats;
   ArrayRef<uint8_t> CodeSection;
   uint32_t StartFunction = -1;
   bool HasLinkingSection = false;
   wasm::WasmLinkingData LinkingData;
   uint32_t NumImportedGlobals = 0;
   uint32_t NumImportedFunctions = 0;
   uint32_t ImportSection = 0;
   uint32_t ExportSection = 0;

include/llvm/ObjectYAML/WasmYAML.h

struct ElemSegment {
   wasm::WasmInitExpr Offset;
   std::vector<uint32_t> Functions;
 };
 
 struct Global {
   ValueType Type;
   bool Mutable;
   wasm::WasmInitExpr InitExpr;
+  StringRef Comdat;
 };
 
 struct Import {
+  Import() {} // A StringRef in the union deletes the default ctor
   StringRef Module;
   StringRef Field;
   ExportKind Kind;
   union {
     uint32_t SigIndex;
     Global GlobalImport;
     Table TableImport;
     Limits Memory;
   };
 };
 
 struct LocalDecl {
   ValueType Type;
   uint32_t Count;
 };
 
 struct Function {
   std::vector<LocalDecl> Locals;
   yaml::BinaryRef Body;
+  StringRef Comdat;
 };
 
 struct Relocation {
   RelocType Type;
   uint32_t Index;
   yaml::Hex32 Offset;
   int32_t Addend;
 };
 
 struct DataSegment {
   uint32_t MemoryIndex;
   uint32_t SectionOffset;
   wasm::WasmInitExpr Offset;
   yaml::BinaryRef Content;
+  StringRef Comdat;
 };
 
 struct NameEntry {
   uint32_t Index;
   StringRef Name;
 };
 
 struct SegmentInfo {

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

     raw_ostream &OS, const GlobalValue *GV) const {
   emitLinkerFlagsForGlobalCOFF(OS, GV, getTargetTriple(), getMangler());
 }
 
 //===----------------------------------------------------------------------===//
 //                                  Wasm
 //===----------------------------------------------------------------------===//
 
-static void checkWasmComdat(const GlobalValue *GV) {
+static const Comdat *getWasmComdat(const GlobalValue *GV) {
   const Comdat *C = GV->getComdat();
   if (!C)
-    return;
+    return nullptr;
 
-  // TODO(sbc): At some point we may need COMDAT support but currently
-  // they are not supported.
-  report_fatal_error("WebAssembly doesn't support COMDATs, '" + C->getName() +
-                     "' cannot be lowered.");
+  if (C->getSelectionKind() != Comdat::Any)
+    report_fatal_error("WebAssembly COMDATs only support "
+                       "SelectionKind::Any, '" + C->getName() + "' cannot be "
+                       "lowered.");
+
+  return C;
 }
 
 static SectionKind getWasmKindForNamedSection(StringRef Name, SectionKind K) {
   // If we're told we have function data, then use that.
   if (K.isText())
     return SectionKind::getText();
 
   // Otherwise, ignore whatever section type the generic impl detected and use
   // a plain data section.
   return SectionKind::getData();
 }
 
 MCSection *TargetLoweringObjectFileWasm::getExplicitSectionGlobal(
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
   StringRef Name = GO->getSection();
-  checkWasmComdat(GO);
+
   Kind = getWasmKindForNamedSection(Name, Kind);
-  return getContext().getWasmSection(Name, Kind);
+
+  StringRef Group = "";
+  if (const Comdat *C = getWasmComdat(GO)) {
+    Group = C->getName();
+  }
+
+  return getContext().getWasmSection(Name, Kind, Group,
+                                     MCContext::GenericSectionID);
 }
 
 static MCSectionWasm *selectWasmSectionForGlobal(
     MCContext &Ctx, const GlobalObject *GO, SectionKind Kind, Mangler &Mang,
     const TargetMachine &TM, bool EmitUniqueSection, unsigned *NextUniqueID) {
   StringRef Group = "";
-  checkWasmComdat(GO);
+  if (const Comdat *C = getWasmComdat(GO)) {
+    Group = C->getName();
+  }
 
   bool UniqueSectionNames = TM.getUniqueSectionNames();
   SmallString<128> Name = getSectionPrefixForGlobal(Kind);
 
   if (const auto *F = dyn_cast<Function>(GO)) {
     const auto &OptionalPrefix = F->getSectionPrefix();
     if (OptionalPrefix)
       Name += *OptionalPrefix;

lib/MC/WasmObjectWriter.cpp

 struct WasmGlobal {
   wasm::ValType Type;
   bool IsMutable;
   bool HasImport;
   uint64_t InitialValue;
   uint32_t ImportIndex;
 };
 
+// Information about a single item which is part of a COMDAT.  For each data
+// segment or function which is in the COMDAT, there is a corresponding
+// WasmComdatEntry.
+struct WasmComdatEntry {
+  unsigned Kind;
+  uint32_t Index;
+};
+
 // Information about a single relocation.
 struct WasmRelocationEntry {
   uint64_t Offset;                  // Where is the relocation.
   const MCSymbolWasm *Symbol;       // The symbol to relocate with.
   int64_t Addend;                   // A value to add to the symbol.
   unsigned Type;                    // The type of the relocation.
   const MCSectionWasm *FixupSection;// The section the relocation is targeting.
 
private:
   void writeDataSection(ArrayRef<WasmDataSegment> Segments);
   void writeNameSection(ArrayRef<WasmFunction> Functions,
                         ArrayRef<WasmImport> Imports,
                         uint32_t NumFuncImports);
   void writeCodeRelocSection();
   void writeDataRelocSection();
   void writeLinkingMetaDataSection(
       ArrayRef<WasmDataSegment> Segments, uint32_t DataSize,
-      SmallVector<std::pair<StringRef, uint32_t>, 4> SymbolFlags);
+      ArrayRef<std::pair<StringRef, uint32_t>> SymbolFlags,
+      const std::map<StringRef, std::vector<WasmComdatEntry>>& Comdats);
 
   uint32_t getProvisionalValue(const WasmRelocationEntry &RelEntry);
   void applyRelocations(ArrayRef<WasmRelocationEntry> Relocations,
                         uint64_t ContentsOffset);
 
   void writeRelocations(ArrayRef<WasmRelocationEntry> Relocations);
   uint32_t getRelocationIndexValue(const WasmRelocationEntry &RelEntry);
   uint32_t getFunctionType(const MCSymbolWasm& Symbol);
void WasmObjectWriter::writeDataRelocSection() {
 
   writeRelocations(DataRelocations);
 
   endSection(Section);
 }
 
 void WasmObjectWriter::writeLinkingMetaDataSection(
     ArrayRef<WasmDataSegment> Segments, uint32_t DataSize,
-    SmallVector<std::pair<StringRef, uint32_t>, 4> SymbolFlags) {
+    ArrayRef<std::pair<StringRef, uint32_t>> SymbolFlags,
+    const std::map<StringRef, std::vector<WasmComdatEntry>>& Comdats) {
   SectionBookkeeping Section;
   startSection(Section, wasm::WASM_SEC_CUSTOM, "linking");
   SectionBookkeeping SubSection;
 
   if (SymbolFlags.size() != 0) {
     startSection(SubSection, wasm::WASM_SYMBOL_INFO);
     encodeULEB128(SymbolFlags.size(), getStream());
     for (auto Pair: SymbolFlags) {
if (Segments.size()) {
     for (const WasmDataSegment &Segment : Segments) {
       writeString(Segment.Name);
       encodeULEB128(Segment.Alignment, getStream());
       encodeULEB128(Segment.Flags, getStream());
     }
     endSection(SubSection);
   }
 
+  if (Comdats.size()) {
+    startSection(SubSection, wasm::WASM_COMDAT_INFO);
+    encodeULEB128(Comdats.size(), getStream());
+    for (const auto &C : Comdats) {
+      writeString(C.first);
+      encodeULEB128(C.second.size(), getStream());
+      for (const WasmComdatEntry &Entry : C.second) {
+        encodeULEB128(Entry.Kind, getStream());
+        encodeULEB128(Entry.Index, getStream());
+      }
+    }
+    endSection(SubSection);
+  }
+
   endSection(Section);
 }
 
 uint32_t WasmObjectWriter::getFunctionType(const MCSymbolWasm& Symbol) {
   assert(Symbol.isFunction());
   assert(TypeIndices.count(&Symbol));
   return TypeIndices[&Symbol];
 }
void WasmObjectWriter::writeObject(MCAssembler &Asm,
   wasm::ValType PtrType = is64Bit() ? wasm::ValType::I64 : wasm::ValType::I32;
 
   // Collect information from the available symbols.
   SmallVector<WasmFunction, 4> Functions;
   SmallVector<uint32_t, 4> TableElems;
   SmallVector<WasmImport, 4> Imports;
   SmallVector<WasmExport, 4> Exports;
   SmallVector<std::pair<StringRef, uint32_t>, 4> SymbolFlags;
+  std::map<StringRef, std::vector<WasmComdatEntry>> Comdats;
   SmallPtrSet<const MCSymbolWasm *, 4> IsAddressTaken;
   unsigned NumFuncImports = 0;
   SmallVector<WasmDataSegment, 4> DataSegments;
   uint32_t DataSize = 0;
 
   // Populate the IsAddressTaken set.
   for (const WasmRelocationEntry &RelEntry : CodeRelocations) {
     switch (RelEntry.Type) {
for (MCSection &Sec : Asm) {
     Segment.Name = Section.getSectionName();
     Segment.Offset = DataSize;
     Segment.Section = &Section;
     addData(Segment.Data, Section);
     Segment.Alignment = Section.getAlignment();
     Segment.Flags = 0;
     DataSize += Segment.Data.size();
     Section.setMemoryOffset(Segment.Offset);
+
+    if (const MCSymbolWasm *C = Section.getGroup()) {
+      Comdats[C->getName()].emplace_back(
+          WasmComdatEntry{wasm::WASM_COMDAT_DATA,
+                          static_cast<uint32_t>(DataSegments.size()) - 1});
+    }
   }
 
   // Handle regular defined and undefined symbols.
   for (const MCSymbol &S : Asm.symbols()) {
     // Ignore unnamed temporary symbols, which aren't ever exported, imported,
     // or used in relocations.
     if (S.isTemporary() && S.getName().empty())
       continue;
if (WS.isWeak() || WS.isHidden()) {
           (WS.isHidden() ? wasm::WASM_SYMBOL_VISIBILITY_HIDDEN : 0);
       SymbolFlags.emplace_back(WS.getName(), Flags);
     }
 
     if (WS.isVariable())
       continue;
 
     unsigned Index;
+    unsigned ComdatKind;
 
     if (WS.isFunction()) {
       if (WS.isDefined(/*SetUsed=*/false)) {
         if (WS.getOffset() != 0)
           report_fatal_error(
               "function sections must contain one function each");
 
         if (WS.getSize() == 0)
           report_fatal_error(
               "function symbols must have a size set with .size");
 
         // A definition. Take the next available index.
         Index = NumFuncImports + Functions.size();
 
         // Prepare the function.
         WasmFunction Func;
         Func.Type = getFunctionType(WS);
         Func.Sym = &WS;
         SymbolIndices[&WS] = Index;
         Functions.push_back(Func);
+        ComdatKind = wasm::WASM_COMDAT_FUNCTION;
       } else {
         // An import; the index was assigned above.
         Index = SymbolIndices.find(&WS)->second;
       }
 
       DEBUG(dbgs() << "  -> function index: " << Index << "\n");
 
       // If needed, prepare the function to be called indirectly.
if (WS.isFunction()) {
       Global.Type = PtrType;
       Global.IsMutable = false;
       Global.HasImport = false;
       Global.InitialValue = DataSection.getMemoryOffset() + Layout.getSymbolOffset(WS);
       Global.ImportIndex = 0;
       SymbolIndices[&WS] = Index;
       DEBUG(dbgs() << "  -> global index: " << Index << "\n");
       Globals.push_back(Global);
+      ComdatKind = wasm::WASM_COMDAT_GLOBAL;
     }
 
     // If the symbol is visible outside this translation unit, export it.
     if (WS.isDefined(/*SetUsed=*/false)) {
       WasmExport Export;
       Export.FieldName = WS.getName();
       Export.Index = Index;
       if (WS.isFunction())
         Export.Kind = wasm::WASM_EXTERNAL_FUNCTION;
       else
         Export.Kind = wasm::WASM_EXTERNAL_GLOBAL;
       DEBUG(dbgs() << "  -> export " << Exports.size() << "\n");
       Exports.push_back(Export);
+
       if (!WS.isExternal())
         SymbolFlags.emplace_back(WS.getName(), wasm::WASM_SYMBOL_BINDING_LOCAL);
+
+      auto &Section = static_cast<MCSectionWasm &>(WS.getSection(false));
+      if (const MCSymbolWasm *C = Section.getGroup())
+        Comdats[C->getName()].emplace_back(WasmComdatEntry{ComdatKind, Index});
     }
   }
 
   // Handle weak aliases. We need to process these in a separate pass because
   // we need to have processed the target of the alias before the alias itself
   // and the symbols are not necessarily ordered in this way.
   for (const MCSymbol &S : Asm.symbols()) {
     if (!S.isVariable())
void WasmObjectWriter::writeObject(MCAssembler &Asm,
   writeExportSection(Exports);
   // TODO: Start Section
   writeElemSection(TableElems);
   writeCodeSection(Asm, Layout, Functions);
   writeDataSection(DataSegments);
   writeNameSection(Functions, Imports, NumFuncImports);
   writeCodeRelocSection();
   writeDataRelocSection();
-  writeLinkingMetaDataSection(DataSegments, DataSize, SymbolFlags);
+  writeLinkingMetaDataSection(DataSegments, DataSize, SymbolFlags, Comdats);
 
   // TODO: Translate the .comment section to the output.
   // TODO: Translate debug sections to the output.
 }
 
 std::unique_ptr<MCObjectWriter>
 llvm::createWasmObjectWriter(std::unique_ptr<MCWasmObjectTargetWriter> MOTW,
                              raw_pwrite_stream &OS) {
   // FIXME: Can't use make_unique<WasmObjectWriter>(...) as WasmObjectWriter's
   //        destructor is private. Is that necessary?
   return std::unique_ptr<MCObjectWriter>(
       new WasmObjectWriter(std::move(MOTW), OS));
 }

lib/Object/WasmObjectFile.cpp

case wasm::WASM_SEGMENT_INFO: {
                                               object_error::parse_failed);
       for (uint32_t i = 0; i < Count; i++) {
         DataSegments[i].Data.Name = readString(Ptr);
         DataSegments[i].Data.Alignment = readVaruint32(Ptr);
         DataSegments[i].Data.Flags = readVaruint32(Ptr);
       }
       break;
     }
+    case wasm::WASM_COMDAT_INFO: {
+      if (Error Err = parseLinkingSectionComdat(Ptr, SubSectionEnd))
+        return Err;
+      break;
+    }
     default:
       Ptr += Size;
       break;
     }
     if (Ptr != SubSectionEnd)
       return make_error<GenericBinaryError>(
           "Linking sub-section ended prematurely", object_error::parse_failed);
   }
   if (Ptr != End)
     return make_error<GenericBinaryError>("Linking section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
+Error WasmObjectFile::parseLinkingSectionComdat(const uint8_t *&Ptr,
+                                                const uint8_t *End)
+{
+  uint32_t ComdatCount = readVaruint32(Ptr);
+  for (uint32_t i = 0; i < ComdatCount; i++) {
+    StringRef Name = readString(Ptr);
+    if (Name.empty() || !Comdats.insert(Name).second)
+      return make_error<GenericBinaryError>("Bad/duplicate COMDAT name",
+                                            object_error::parse_failed);
+
+    uint32_t EntryCount = readVaruint32(Ptr);
+    for (uint32_t j = 0; j < EntryCount; ++j) {
+      unsigned Kind = readVaruint32(Ptr);
+      unsigned Index = readVaruint32(Ptr);
+      switch (Kind) {
+      default:
+        return make_error<GenericBinaryError>("Invalid COMDAT entry type",
+                                              object_error::parse_failed);
+      case wasm::WASM_COMDAT_DATA:
+        if (Index >= DataSegments.size())
+          return make_error<GenericBinaryError>(
+              "COMDAT data index out of range",
+              object_error::parse_failed);
+        if (!DataSegments[Index].Data.Comdat.empty())
+          return make_error<GenericBinaryError>("Data segment in two COMDATs",
+                                                object_error::parse_failed);
+        DataSegments[Index].Data.Comdat = Name;
+        break;
+      case wasm::WASM_COMDAT_FUNCTION:
+        if (Index < NumImportedFunctions ||
+            Index >= Functions.size() + NumImportedFunctions)
+          return make_error<GenericBinaryError>("COMDAT function index out of range",
+                                                object_error::parse_failed);
+        Index -= NumImportedFunctions;
+        if (!Functions[Index].Comdat.empty())
+          return make_error<GenericBinaryError>("Function in two COMDATs",
+                                                object_error::parse_failed);
+        Functions[Index].Comdat = Name;
+        break;
+      case wasm::WASM_COMDAT_GLOBAL:
+        if (Index < NumImportedGlobals ||
+            Index >= Globals.size() + NumImportedGlobals)
+          return make_error<GenericBinaryError>("COMDAT global index out of range",
+                                                object_error::parse_failed);
+        Index -= NumImportedGlobals;
+        if (!Globals[Index].Comdat.empty())
+          return make_error<GenericBinaryError>("Global in two COMDATs",
+                                                object_error::parse_failed);
+        Globals[Index].Comdat = Name;
+        break;
+      }
+    }
+  }
+  return Error::success();
+}
+
 WasmSection* WasmObjectFile::findCustomSectionByName(StringRef Name) {
   for (WasmSection& Section : Sections) {
     if (Section.Type == wasm::WASM_SEC_CUSTOM && Section.Name == Name)
       return &Section;
   }
   return nullptr;
 }
 

lib/ObjectYAML/WasmYAML.cpp

 
 static void sectionMapping(IO &IO, WasmYAML::LinkingSection &Section) {
   commonSectionMapping(IO, Section);
   IO.mapRequired("Name", Section.Name);
   IO.mapRequired("DataSize", Section.DataSize);
   IO.mapOptional("SymbolInfo", Section.SymbolInfos);
   IO.mapOptional("SegmentInfo", Section.SegmentInfos);
 }
 
 static void sectionMapping(IO &IO, WasmYAML::CustomSection &Section) {
   commonSectionMapping(IO, Section);
   IO.mapRequired("Name", Section.Name);
   IO.mapRequired("Payload", Section.Payload);
 }
 
 static void sectionMapping(IO &IO, WasmYAML::TypeSection &Section) {
   commonSectionMapping(IO, Section);
void MappingTraits<WasmYAML::Table>::mapping(IO &IO, WasmYAML::Table &Table) {
   IO.mapRequired("ElemType", Table.ElemType);
   IO.mapRequired("Limits", Table.TableLimits);
 }
 
 void MappingTraits<WasmYAML::Function>::mapping(IO &IO,
                                                 WasmYAML::Function &Function) {
   IO.mapRequired("Locals", Function.Locals);
   IO.mapRequired("Body", Function.Body);
+  IO.mapOptional("Comdat", Function.Comdat, StringRef());
 }
 
 void MappingTraits<WasmYAML::Relocation>::mapping(
     IO &IO, WasmYAML::Relocation &Relocation) {
   IO.mapRequired("Type", Relocation.Type);
   IO.mapRequired("Index", Relocation.Index);
   IO.mapRequired("Offset", Relocation.Offset);
   IO.mapOptional("Addend", Relocation.Addend, 0);
void MappingTraits<WasmYAML::Export>::mapping(IO &IO,
   IO.mapRequired("Index", Export.Index);
 }
 
 void MappingTraits<WasmYAML::Global>::mapping(IO &IO,
                                               WasmYAML::Global &Global) {
   IO.mapRequired("Type", Global.Type);
   IO.mapRequired("Mutable", Global.Mutable);
   IO.mapRequired("InitExpr", Global.InitExpr);
+  IO.mapOptional("Comdat", Global.Comdat, StringRef());
 }
 
 void MappingTraits<wasm::WasmInitExpr>::mapping(IO &IO,
                                                 wasm::WasmInitExpr &Expr) {
   WasmYAML::Opcode Op = Expr.Opcode;
   IO.mapRequired("Opcode", Op);
   Expr.Opcode = Op;
   switch (Expr.Opcode) {

 }
 
 void MappingTraits<WasmYAML::DataSegment>::mapping(
     IO &IO, WasmYAML::DataSegment &Segment) {
   IO.mapOptional("SectionOffset", Segment.SectionOffset);
   IO.mapRequired("MemoryIndex", Segment.MemoryIndex);
   IO.mapRequired("Offset", Segment.Offset);
   IO.mapRequired("Content", Segment.Content);
+  IO.mapOptional("Comdat", Segment.Comdat, StringRef());
 }
 
 void MappingTraits<WasmYAML::SymbolInfo>::mapping(IO &IO,
                                                   WasmYAML::SymbolInfo &Info) {
   IO.mapRequired("Name", Info.Name);
   IO.mapRequired("Flags", Info.Flags);
 }
 

test/MC/WebAssembly/comdat.ll

+; RUN: llc -mtriple wasm32-unknown-unknown-wasm -filetype=obj %s -o - | obj2yaml | FileCheck %s
+
+; Function in its own COMDAT
+$basicInlineFn = comdat any
+define linkonce_odr i32 @basicInlineFn() #1 comdat {
+  ret i32 0
+}
+
+; Global, data, and function in same COMDAT
+$sharedComdat = comdat any
+@constantData = weak_odr constant [3 x i8] c"abc", comdat($sharedComdat)
+define linkonce_odr i32 @sharedFn() #1 comdat($sharedComdat) {
+  ret i32 0
+}
+
+; CHECK:       - Type:            GLOBAL
+; CHECK-NEXT:    Globals:
+; CHECK-NEXT:      - Type:            I32
+; CHECK-NEXT:        Mutable:         false
+; CHECK-NEXT:        InitExpr:
+; CHECK-NEXT:          Opcode:          I32_CONST
+; CHECK-NEXT:          Value:           0
+; CHECK-NEXT:        Comdat:          sharedComdat
+; CHECK-NEXT:  - Type:            EXPORT
+; CHECK-NEXT:    Exports:
+; CHECK-NEXT:      - Name:            basicInlineFn
+; CHECK-NEXT:        Kind:            FUNCTION
+; CHECK-NEXT:        Index:           0
+; CHECK-NEXT:      - Name:            sharedFn
+; CHECK-NEXT:        Kind:            FUNCTION
+; CHECK-NEXT:        Index:           1
+; CHECK-NEXT:      - Name:            constantData
+; CHECK-NEXT:        Kind:            GLOBAL
+; CHECK-NEXT:        Index:           1
+; CHECK-NEXT:  - Type:            CODE
+; CHECK-NEXT:    Functions:
+; CHECK-NEXT:      - Locals:
+; CHECK-NEXT:        Body:            41000B
+; CHECK-NEXT:        Comdat:          basicInlineFn
+; CHECK-NEXT:      - Locals:
+; CHECK-NEXT:        Body:            41000B
+; CHECK-NEXT:        Comdat:          sharedComdat
+; CHECK-NEXT:  - Type:            DATA
+; CHECK-NEXT:    Segments:
+; CHECK-NEXT:      - SectionOffset:   6
+; CHECK-NEXT:        MemoryIndex:     0
+; CHECK-NEXT:        Offset:
+; CHECK-NEXT:          Opcode:          I32_CONST
+; CHECK-NEXT:          Value:           0
+; CHECK-NEXT:        Content:         '616263'
+; CHECK-NEXT:        Comdat:          sharedComdat

tools/obj2yaml/wasm2yaml.cpp

 static WasmYAML::Limits make_limits(const wasm::WasmLimits &Limits) {
   WasmYAML::Limits L;
   L.Flags = Limits.Flags;
   L.Initial = Limits.Initial;
   L.Maximum = Limits.Maximum;
   return L;
 }
 
 std::unique_ptr<WasmYAML::CustomSection> WasmDumper::dumpCustomSection(const WasmSection &WasmSec) {
   std::unique_ptr<WasmYAML::CustomSection> CustomSec;
   if (WasmSec.Name == "name") {
     std::unique_ptr<WasmYAML::NameSection> NameSec = make_unique<WasmYAML::NameSection>();
     for (const object::SymbolRef& Sym: Obj.symbols()) {
       const object::WasmSymbol Symbol = Obj.getWasmSymbol(Sym);
       if (Symbol.Type != object::WasmSymbol::SymbolType::DEBUG_FUNCTION_NAME)
         continue;
       WasmYAML::NameEntry NameEntry;
if (WasmSec.Name == "name") {
     }
     for (const object::SymbolRef& Sym: Obj.symbols()) {
       const object::WasmSymbol Symbol = Obj.getWasmSymbol(Sym);
       if (Symbol.Flags != 0) {
         WasmYAML::SymbolInfo Info = { Symbol.Name, Symbol.Flags };
         LinkingSec->SymbolInfos.push_back(Info);
       }
     }
+    // (COMDATs are attached to the symbols they apply to rather than being
+    // represented as part of the custom section in the YAML output.)
     LinkingSec->DataSize = Obj.linkingData().DataSize;
     CustomSec = std::move(LinkingSec);
   } else {
     CustomSec = make_unique<WasmYAML::CustomSection>(WasmSec.Name);
   }
   CustomSec->Payload = yaml::BinaryRef(WasmSec.Content);
   return CustomSec;
 }
for (const auto &Sec : Obj.sections()) {
     }
     case wasm::WASM_SEC_GLOBAL: {
       auto GlobalSec = make_unique<WasmYAML::GlobalSection>();
       for (auto &Global : Obj.globals()) {
         WasmYAML::Global G;
         G.Type = Global.Type;
         G.Mutable = Global.Mutable;
         G.InitExpr = Global.InitExpr;
+        G.Comdat = Global.Comdat;
         GlobalSec->Globals.push_back(G);
       }
       S = std::move(GlobalSec);
       break;
     }
     case wasm::WASM_SEC_START: {
       auto StartSec = make_unique<WasmYAML::StartSection>();
       StartSec->StartFunction = Obj.startFunction();
case wasm::WASM_SEC_CODE: {
         WasmYAML::Function Function;
         for (auto &Local : Func.Locals) {
           WasmYAML::LocalDecl LocalDecl;
           LocalDecl.Type = Local.Type;
           LocalDecl.Count = Local.Count;
           Function.Locals.push_back(LocalDecl);
         }
         Function.Body = yaml::BinaryRef(Func.Body);
+        Function.Comdat = Func.Comdat;
         CodeSec->Functions.push_back(Function);
       }
       S = std::move(CodeSec);
       break;
     }
     case wasm::WASM_SEC_DATA: {
       auto DataSec = make_unique<WasmYAML::DataSection>();
       for (const object::WasmSegment &Segment : Obj.dataSegments()) {
         WasmYAML::DataSegment Seg;
         Seg.SectionOffset = Segment.SectionOffset;
         Seg.MemoryIndex = Segment.Data.MemoryIndex;
         Seg.Offset = Segment.Data.Offset;
         Seg.Content = yaml::BinaryRef(Segment.Data.Content);
+        Seg.Comdat = Segment.Data.Comdat;
         DataSec->Segments.push_back(Seg);
       }
       S = std::move(DataSec);
       break;
     }
     default:
       llvm_unreachable("Unknown section type");
       break;