[WASM] Avoid passing uninit section indices when not creating code or data sections

[guiand]

Summary

Currently, section indices may be passed uninitialized by value if writing the section fails. Removes section indices form class initialization and returns them from the write{Code,Data}Section function calls instead.

[sbc100]

How about one of these two options:

1. initialing CodeSectionIndex and DataSectionIndex to 0 in the class declaration? (and in reset() I guess too?). You can treat 0 as invalid for both of them.

2. Remove CodeSectionIndex and DataSectionIndex completely from the class and just return the index value from writeCodeSection and writeDataSection (again with 0 meaning none).

Either of those I think would be fine.

[sbc100]

Did you see this happening in practice? Did msan catch it?

[guiand]

The second option sounds good to me! I'll make the change tomorrow.

I've been working on stricter msan checks to flag uninit data crossing function boundaries (as it's UB in C++). You can find that change pending at https://reviews.llvm.org/D81699.

[guiand]

Updated with returning the indices from the writeSection functions.

[sbc100]

Thanks!

[sbc100]

I guess its not easy to write a test for this since its a use of uninitialized data which can be non-deterministic?

[guiand]

Yeah, but I think the current check-clang tests (under msan once the changes go through) will be useful for that.

[sbc100]

This change should be ready to land. Do you have commit access to llvm?

[guiand]

Ah, no I don't have commit access yet. Would you mind helping me land this? Thanks so much!

[guiand]

Thanks, Sam!

llvm/lib/MC/WasmObjectWriter.cpp

 class WasmObjectWriter : public MCObjectWriter {
   support::endian::Writer W;
 
   /// The target specific Wasm writer instance.
   std::unique_ptr<MCWasmObjectTargetWriter> TargetObjectWriter;
 
   // Relocations for fixing up references in the code section.
   std::vector<WasmRelocationEntry> CodeRelocations;
-  uint32_t CodeSectionIndex;
-
   // Relocations for fixing up references in the data section.
   std::vector<WasmRelocationEntry> DataRelocations;
-  uint32_t DataSectionIndex;
 
   // Index values to use for fixing up call_indirect type indices.
   // Maps function symbols to the index of the type of the function
   DenseMap<const MCSymbolWasm *, uint32_t> TypeIndices;
   // Maps function symbols to the table element index space. Used
   // for TABLE_INDEX relocation types (i.e. address taken functions).
   DenseMap<const MCSymbolWasm *, uint32_t> TableIndices;
   // Maps function/global symbols to the function/global/event/section index
private:
 
   void writeTypeSection(ArrayRef<WasmSignature> Signatures);
   void writeImportSection(ArrayRef<wasm::WasmImport> Imports, uint32_t DataSize,
                           uint32_t NumElements);
   void writeFunctionSection(ArrayRef<WasmFunction> Functions);
   void writeExportSection(ArrayRef<wasm::WasmExport> Exports);
   void writeElemSection(ArrayRef<uint32_t> TableElems);
   void writeDataCountSection();
-  void writeCodeSection(const MCAssembler &Asm, const MCAsmLayout &Layout,
+  uint32_t writeCodeSection(const MCAssembler &Asm, const MCAsmLayout &Layout,
                             ArrayRef<WasmFunction> Functions);
-  void writeDataSection();
+  uint32_t writeDataSection();
   void writeEventSection(ArrayRef<wasm::WasmEventType> Events);
   void writeGlobalSection(ArrayRef<wasm::WasmGlobal> Globals);
   void writeRelocSection(uint32_t SectionIndex, StringRef Name,
                          std::vector<WasmRelocationEntry> &Relocations);
   void writeLinkingMetaDataSection(
       ArrayRef<wasm::WasmSymbolInfo> SymbolInfos,
       ArrayRef<std::pair<uint16_t, uint32_t>> InitFuncs,
       const std::map<StringRef, std::vector<WasmComdatEntry>> &Comdats);
if (DataSegments.empty())
     return;
 
   SectionBookkeeping Section;
   startSection(Section, wasm::WASM_SEC_DATACOUNT);
   encodeULEB128(DataSegments.size(), W.OS);
   endSection(Section);
 }
 
-void WasmObjectWriter::writeCodeSection(const MCAssembler &Asm,
+uint32_t WasmObjectWriter::writeCodeSection(const MCAssembler &Asm,
                                             const MCAsmLayout &Layout,
                                             ArrayRef<WasmFunction> Functions) {
   if (Functions.empty())
-    return;
+    return 0;
 
   SectionBookkeeping Section;
   startSection(Section, wasm::WASM_SEC_CODE);
-  CodeSectionIndex = Section.Index;
 
   encodeULEB128(Functions.size(), W.OS);
 
   for (const WasmFunction &Func : Functions) {
     auto &FuncSection = static_cast<MCSectionWasm &>(Func.Sym->getSection());
 
     int64_t Size = 0;
     if (!Func.Sym->getSize()->evaluateAsAbsolute(Size, Layout))
       report_fatal_error(".size expression must be evaluatable");
 
     encodeULEB128(Size, W.OS);
     FuncSection.setSectionOffset(W.OS.tell() - Section.ContentsOffset);
     Asm.writeSectionData(W.OS, &FuncSection, Layout);
   }
 
   // Apply fixups.
   applyRelocations(CodeRelocations, Section.ContentsOffset);
 
   endSection(Section);
+  return Section.Index;
 }
 
-void WasmObjectWriter::writeDataSection() {
+uint32_t WasmObjectWriter::writeDataSection() {
   if (DataSegments.empty())
-    return;
+    return 0;
 
   SectionBookkeeping Section;
   startSection(Section, wasm::WASM_SEC_DATA);
-  DataSectionIndex = Section.Index;
 
   encodeULEB128(DataSegments.size(), W.OS); // count
 
   for (const WasmDataSegment &Segment : DataSegments) {
     encodeULEB128(Segment.InitFlags, W.OS); // flags
     if (Segment.InitFlags & wasm::WASM_SEGMENT_HAS_MEMINDEX)
       encodeULEB128(0, W.OS); // memory index
     if ((Segment.InitFlags & wasm::WASM_SEGMENT_IS_PASSIVE) == 0) {
       W.OS << char(wasm::WASM_OPCODE_I32_CONST);
       encodeSLEB128(Segment.Offset, W.OS); // offset
       W.OS << char(wasm::WASM_OPCODE_END);
     }
     encodeULEB128(Segment.Data.size(), W.OS); // size
     Segment.Section->setSectionOffset(W.OS.tell() - Section.ContentsOffset);
     W.OS << Segment.Data; // data
   }
 
   // Apply fixups.
   applyRelocations(DataRelocations, Section.ContentsOffset);
 
   endSection(Section);
+  return Section.Index;
 }
 
 void WasmObjectWriter::writeRelocSection(
     uint32_t SectionIndex, StringRef Name,
     std::vector<WasmRelocationEntry> &Relocs) {
   // See: https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md
   // for descriptions of the reloc sections.
 
uint64_t WasmObjectWriter::writeObject(MCAssembler &Asm,
   writeFunctionSection(Functions);
   // Skip the "table" section; we import the table instead.
   // Skip the "memory" section; we import the memory instead.
   writeEventSection(Events);
   writeGlobalSection(Globals);
   writeExportSection(Exports);
   writeElemSection(TableElems);
   writeDataCountSection();
-  writeCodeSection(Asm, Layout, Functions);
-  writeDataSection();
+  uint32_t CodeSectionIndex = writeCodeSection(Asm, Layout, Functions);
+  uint32_t DataSectionIndex = writeDataSection();
   for (auto &CustomSection : CustomSections)
     writeCustomSection(CustomSection, Asm, Layout);
   writeLinkingMetaDataSection(SymbolInfos, InitFuncs, Comdats);
   writeRelocSection(CodeSectionIndex, "CODE", CodeRelocations);
   writeRelocSection(DataSectionIndex, "DATA", DataRelocations);
   writeCustomRelocSections();
   if (ProducersSection)
     writeCustomSection(*ProducersSection, Asm, Layout);