[WebAssembly][MC] Fix leak of std::string members in MCSymbolWasm

[sbc100]

Summary

Fixes: https://bugs.llvm.org/show_bug.cgi?id=45452

[sbc100]

• revert

[sunfish]

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h

56

Can this function be private in WebAssemblyAsmPrinter?

[sbc100]

feedback

[dblaikie]

llvm/include/llvm/MC/MCSymbolWasm.h

26–28

Do these need to be std::strings, or could they just be StringRefs?

It looks like they're only set/get in this class, never modified by it - StringRefs might be more suitable? (does the empty string need to be a separate state from "none"/null - I'd probably still go with Optional<StringRef> then - though opinions certainly might differ there - in that case (where opinions differ & you prefer std::string* over Optional<StringRef> here) I'd at least make them const std::string*))

[sbc100]

llvm/include/llvm/MC/MCSymbolWasm.h

26–28

Hm, you are correct. I'm not sure why I didn't go with Optional<StringRef>. I'll update.

llvm/include/llvm/MC/MCSymbolWasm.h

 namespace llvm {
 
 class MCSymbolWasm : public MCSymbol {
   wasm::WasmSymbolType Type = wasm::WASM_SYMBOL_TYPE_DATA;
   bool IsWeak = false;
   bool IsHidden = false;
   bool IsComdat = false;
   mutable bool IsUsedInGOT = false;
-  Optional<std::string> ImportModule;
-  Optional<std::string> ImportName;
-  Optional<std::string> ExportName;
-  wasm::WasmSignature *Signature = nullptr;
   Optional<wasm::WasmGlobalType> GlobalType;
   Optional<wasm::WasmEventType> EventType;
 
+  // Non-owning pointers since MCSymbol must be trivially destructible.
+  std::string *ImportModule = nullptr;
+  std::string *ImportName = nullptr;
+  std::string *ExportName = nullptr;
+  wasm::WasmSignature *Signature = nullptr;
+
   /// An expression describing how to calculate the size of a symbol. If a
   /// symbol has no size this field will be NULL.
   const MCExpr *SymbolSize = nullptr;
 
 public:
   MCSymbolWasm(const StringMapEntry<bool> *Name, bool isTemporary)
       : MCSymbol(SymbolKindWasm, Name, isTemporary) {}
   static bool classof(const MCSymbol *S) { return S->isWasm(); }
public:
   void setWeak(bool isWeak) { IsWeak = isWeak; }
 
   bool isHidden() const { return IsHidden; }
   void setHidden(bool isHidden) { IsHidden = isHidden; }
 
   bool isComdat() const { return IsComdat; }
   void setComdat(bool isComdat) { IsComdat = isComdat; }
 
-  bool hasImportModule() const { return ImportModule.hasValue(); }
+  bool hasImportModule() const { return ImportModule != nullptr; }
   const StringRef getImportModule() const {
-      if (ImportModule.hasValue()) {
-          return ImportModule.getValue();
-      }
-      // Use a default module name of "env" for now, for compatibility with
-      // existing tools.
-      // TODO(sbc): Find a way to specify a default value in the object format
-      // without picking a hardcoded value like this.
-      return "env";
+    if (ImportModule)
+      return StringRef(*ImportModule);
   }
-  void setImportModule(StringRef Name) {
-    ImportModule = std::string(std::string(Name));
-  }
+  void setImportModule(std::string *Name) { ImportModule = Name; }
 
-  bool hasImportName() const { return ImportName.hasValue(); }
+  bool hasImportName() const { return ImportName != nullptr; }
   const StringRef getImportName() const {
-      if (ImportName.hasValue()) {
-          return ImportName.getValue();
-      }
-      return getName();
+    if (ImportName)
+      return StringRef(*ImportName);
   }
-  void setImportName(StringRef Name) {
-    ImportName = std::string(std::string(Name));
-  }
+  void setImportName(std::string *Name) { ImportName = Name; }
 
-  bool hasExportName() const { return ExportName.hasValue(); }
-  const StringRef getExportName() const { return ExportName.getValue(); }
-  void setExportName(StringRef Name) {
-    ExportName = std::string(std::string(Name));
+  bool hasExportName() const { return ExportName != nullptr; }
+  const StringRef getExportName() const {
+    assert(ExportName);
+    return StringRef(*ExportName);
   }
+  void setExportName(std::string *Name) { ExportName = Name; }
 
   void setUsedInGOT() const { IsUsedInGOT = true; }
   bool isUsedInGOT() const { return IsUsedInGOT; }
 
   const wasm::WasmSignature *getSignature() const { return Signature; }
   void setSignature(wasm::WasmSignature *Sig) { Signature = Sig; }
 
   const wasm::WasmGlobalType &getGlobalType() const {

llvm/lib/MC/MCContext.cpp

 
 MCSymbol *MCContext::getOrCreateLSDASymbol(StringRef FuncName) {
   return getOrCreateSymbol(Twine(MAI->getPrivateGlobalPrefix()) + "__ehtable$" +
                            FuncName);
 }
 
 MCSymbol *MCContext::createSymbolImpl(const StringMapEntry<bool> *Name,
                                       bool IsTemporary) {
+  static_assert(std::is_trivially_destructible<MCSymbolCOFF>(),
+                "MCSymbol classes must be trivially destructible");
+  static_assert(std::is_trivially_destructible<MCSymbolELF>(),
+                "MCSymbol classes must be trivially destructible");
+  static_assert(std::is_trivially_destructible<MCSymbolMachO>(),
+                "MCSymbol classes must be trivially destructible");
+  static_assert(std::is_trivially_destructible<MCSymbolWasm>(),
+                "MCSymbol classes must be trivially destructible");
+  static_assert(std::is_trivially_destructible<MCSymbolXCOFF>(),
+                "MCSymbol classes must be trivially destructible");
   if (MOFI) {
     switch (MOFI->getObjectFileType()) {
     case MCObjectFileInfo::IsCOFF:
       return new (Name, *this) MCSymbolCOFF(Name, IsTemporary);
     case MCObjectFileInfo::IsELF:
       return new (Name, *this) MCSymbolELF(Name, IsTemporary);
     case MCObjectFileInfo::IsMachO:
       return new (Name, *this) MCSymbolMachO(Name, IsTemporary);

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

 };
 
 class WebAssemblyAsmParser final : public MCTargetAsmParser {
   MCAsmParser &Parser;
   MCAsmLexer &Lexer;
 
   // Much like WebAssemblyAsmPrinter in the backend, we have to own these.
   std::vector<std::unique_ptr<wasm::WasmSignature>> Signatures;
+  std::vector<std::unique_ptr<std::string>> Names;
 
   // Order of labels, directives and instructions in a .s file have no
   // syntactical enforcement. This class is a callback from the actual parser,
   // and yet we have to be feeding data to the streamer in a very particular
   // order to ensure a correct binary encoding that matches the regular backend
   // (the streamer does not enforce this). This "state machine" enum helps
   // guarantee that correct order.
   enum ParserState {
#include "WebAssemblyGenAsmMatcher.inc"
   bool error(const Twine &Msg) {
     return Parser.Error(Lexer.getTok().getLoc(), Msg);
   }
 
   void addSignature(std::unique_ptr<wasm::WasmSignature> &&Sig) {
     Signatures.push_back(std::move(Sig));
   }
 
+  std::string *storeName(StringRef Name) {
+    std::unique_ptr<std::string> N = std::make_unique<std::string>(Name);
+    Names.push_back(std::move(N));
+    return Names.back().get();
+  }
+
   std::pair<StringRef, StringRef> nestingString(NestingType NT) {
     switch (NT) {
     case Function:
       return {"function", "end_function"};
     case Block:
       return {"block", "end_block"};
     case Loop:
       return {"loop", "end_loop"};
bool ParseDirective(AsmToken DirectiveID) override {
     if (DirectiveID.getString() == ".export_name") {
       auto SymName = expectIdent();
       if (SymName.empty())
         return true;
       if (expect(AsmToken::Comma, ","))
         return true;
       auto ExportName = expectIdent();
       auto WasmSym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(SymName));
-      WasmSym->setExportName(ExportName);
+      WasmSym->setExportName(storeName(ExportName));
       TOut.emitExportName(WasmSym, ExportName);
     }
 
     if (DirectiveID.getString() == ".import_module") {
       auto SymName = expectIdent();
       if (SymName.empty())
         return true;
       if (expect(AsmToken::Comma, ","))
         return true;
       auto ImportModule = expectIdent();
       auto WasmSym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(SymName));
-      WasmSym->setImportModule(ImportModule);
+      WasmSym->setImportModule(storeName(ImportModule));
       TOut.emitImportModule(WasmSym, ImportModule);
     }
 
     if (DirectiveID.getString() == ".import_name") {
       auto SymName = expectIdent();
       if (SymName.empty())
         return true;
       if (expect(AsmToken::Comma, ","))
         return true;
       auto ImportName = expectIdent();
       auto WasmSym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(SymName));
-      WasmSym->setImportName(ImportName);
+      WasmSym->setImportName(storeName(ImportName));
       TOut.emitImportName(WasmSym, ImportName);
     }
 
     if (DirectiveID.getString() == ".eventtype") {
       auto SymName = expectIdent();
       if (SymName.empty())
         return true;
       auto WasmSym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(SymName));

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h

 class WebAssemblyMCInstLower;
 
 class LLVM_LIBRARY_VISIBILITY WebAssemblyAsmPrinter final : public AsmPrinter {
   const WebAssemblySubtarget *Subtarget;
   const MachineRegisterInfo *MRI;
   WebAssemblyFunctionInfo *MFI;
   // TODO: Do the uniquing of Signatures here instead of ObjectFileWriter?
   std::vector<std::unique_ptr<wasm::WasmSignature>> Signatures;
+  std::vector<std::unique_ptr<std::string>> Names;
+
+  std::string *storeName(StringRef Name) {
+    std::unique_ptr<std::string> N = std::make_unique<std::string>(Name);
+    Names.push_back(std::move(N));
+    return Names.back().get();
+  }
 
 public:
   explicit WebAssemblyAsmPrinter(TargetMachine &TM,
                                  std::unique_ptr<MCStreamer> Streamer)
       : AsmPrinter(TM, std::move(Streamer)), Subtarget(nullptr), MRI(nullptr),
         MFI(nullptr) {}
 
   StringRef getPassName() const override {
     return "WebAssembly Assembly Printer";
   }
 
   const WebAssemblySubtarget &getSubtarget() const { return *Subtarget; }
   void addSignature(std::unique_ptr<wasm::WasmSignature> &&Sig) {
     Signatures.push_back(std::move(Sig));
   }
 
   //===------------------------------------------------------------------===//
   // MachineFunctionPass Implementation.
   //===------------------------------------------------------------------===//
 
   bool runOnMachineFunction(MachineFunction &MF) override {
     Subtarget = &MF.getSubtarget<WebAssemblySubtarget>();
     MRI = &MF.getRegInfo();
     MFI = MF.getInfo<WebAssemblyFunctionInfo>();
     return AsmPrinter::runOnMachineFunction(MF);
   }
 
   //===------------------------------------------------------------------===//
   // AsmPrinter Implementation.

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

if (F.isDeclarationForLinker()) {
       // imports. so fix the names and the tests, or rethink how import
       // delcarations work in asm files.
       getTargetStreamer()->emitFunctionType(Sym);
 
       if (TM.getTargetTriple().isOSBinFormatWasm() &&
           F.hasFnAttribute("wasm-import-module")) {
         StringRef Name =
             F.getFnAttribute("wasm-import-module").getValueAsString();
-        Sym->setImportModule(Name);
+        Sym->setImportModule(storeName(Name));
         getTargetStreamer()->emitImportModule(Sym, Name);
       }
       if (TM.getTargetTriple().isOSBinFormatWasm() &&
           F.hasFnAttribute("wasm-import-name")) {
         StringRef Name =
             F.getFnAttribute("wasm-import-name").getValueAsString();
-        Sym->setImportName(Name);
+        Sym->setImportName(storeName(Name));
         getTargetStreamer()->emitImportName(Sym, Name);
       }
     }
 
     if (F.hasFnAttribute("wasm-export-name")) {
       auto *Sym = cast<MCSymbolWasm>(getSymbol(&F));
       StringRef Name = F.getFnAttribute("wasm-export-name").getValueAsString();
-      Sym->setExportName(Name);
+      Sym->setExportName(storeName(Name));
       getTargetStreamer()->emitExportName(Sym, Name);
     }
   }
 
   for (const auto &G : M.globals()) {
     if (!G.hasInitializer() && G.hasExternalLinkage()) {
       if (G.getValueType()->isSized()) {
         uint16_t Size = M.getDataLayout().getTypeAllocSize(G.getValueType());