Add visibility flag to Wasm symbol flags

[ncw]

Summary

The LLVM "hidden" flag needs to be passed through the Wasm intermediate objects in order for the linker to apply it to the final Wasm object.

The corresponding change in LLD is here: https://github.com/WebAssembly/lld/pull/14

[sbc100]

lgtm with comments.

And can you add a test in test/MC/WebAssembly?

include/llvm/BinaryFormat/Wasm.h

201 ↗

(On Diff #124208)

Why not make this 0 and 1?

lib/MC/MCWasmStreamer.cpp

103 ↗

(On Diff #124208)

Wont all the above cases fall through to this too?

[ncw]

• Added missing "return" to switch
• Added test to tests/MC/WebAssembly

[ncw]

include/llvm/BinaryFormat/Wasm.h

201 ↗

(On Diff #124208)

(Thanks for the review.)

It's 0x4 because the flags are ORed together when writing out into the "linking" custom section. That's similar to ELF, where all the different per-symbol flags are ORed together. (I would have split these into a separate enum if they were independent of the other values.)

lib/MC/MCWasmStreamer.cpp

103 ↗

(On Diff #124208)

D'oh, truly terrible typo, sorry - I added the blank line but not the break.

[sbc100]

lgtm, with a minor change

lib/MC/MCWasmStreamer.cpp

103 ↗

(On Diff #124208)

Can you move this down so that all the "return false" cases are still in a single block?

[sunfish]

Please also document the wasm binary encoding for visibilities in Linking.md.

[ncw]

Updated with Sam's requested re-ordering of the switch labels.

[sbc100]

still lgtm.

[sbc100]

Do you have llvm commit access to land this yourself?

[ncw]

No, I don't - I've never been that involved!

llvm/trunk/include/llvm/BinaryFormat/Wasm.h

enum : unsigned {
   WASM_STACK_POINTER  = 0x1,
   WASM_SYMBOL_INFO    = 0x2,
   WASM_DATA_SIZE      = 0x3,
   WASM_DATA_ALIGNMENT = 0x4,
   WASM_SEGMENT_INFO   = 0x5,
 };
 
 const unsigned WASM_SYMBOL_BINDING_MASK = 0x3;
+const unsigned WASM_SYMBOL_VISIBILITY_MASK = 0x4;
 
 enum : unsigned {
   WASM_SYMBOL_BINDING_GLOBAL = 0x0,
   WASM_SYMBOL_BINDING_WEAK   = 0x1,
   WASM_SYMBOL_BINDING_LOCAL  = 0x2,
+
+  WASM_SYMBOL_VISIBILITY_DEFAULT = 0x0,
+  WASM_SYMBOL_VISIBILITY_HIDDEN = 0x4,
 };
 
 #define WASM_RELOC(name, value) name = value,
 
 enum : unsigned {
 #include "WasmRelocs/WebAssembly.def"
 };
 

llvm/trunk/include/llvm/MC/MCSymbolWasm.h

 #include "llvm/MC/MCSymbol.h"
 
 namespace llvm {
 
 class MCSymbolWasm : public MCSymbol {
 private:
   bool IsFunction = false;
   bool IsWeak = false;
+  bool IsHidden = false;
   std::string ModuleName;
   SmallVector<wasm::ValType, 1> Returns;
   SmallVector<wasm::ValType, 4> Params;
   bool ParamsSet = false;
   bool ReturnsSet = false;
 
   /// An expression describing how to calculate the size of a symbol. If a
   /// symbol has no size this field will be NULL.
public:
   void setSize(const MCExpr *SS) { SymbolSize = SS; }
 
   bool isFunction() const { return IsFunction; }
   void setIsFunction(bool isFunc) { IsFunction = isFunc; }
 
   bool isWeak() const { return IsWeak; }
   void setWeak(bool isWeak) { IsWeak = isWeak; }
 
+  bool isHidden() const { return IsHidden; }
+  void setHidden(bool isHidden) { IsHidden = isHidden; }
+
   const StringRef getModuleName() const { return ModuleName; }
 
   const SmallVector<wasm::ValType, 1> &getReturns() const {
     assert(ReturnsSet);
     return Returns;
   }
 
   void setReturns(SmallVectorImpl<wasm::ValType> &&Rets) {

llvm/trunk/include/llvm/Object/Wasm.h

public:
   bool isLocal() const {
     return getBinding() == wasm::WASM_SYMBOL_BINDING_LOCAL;
   }
 
   unsigned getBinding() const {
     return Flags & wasm::WASM_SYMBOL_BINDING_MASK;
   }
 
+  bool isHidden() const {
+    return getVisibility() == wasm::WASM_SYMBOL_VISIBILITY_HIDDEN;
+  }
+
+  unsigned getVisibility() const {
+    return Flags & wasm::WASM_SYMBOL_VISIBILITY_MASK;
+  }
+
   void print(raw_ostream &Out) const {
     Out << "Name=" << Name << ", Type=" << static_cast<int>(Type)
         << ", Flags=" << Flags << " ElemIndex=" << ElementIndex
         << ", ImportIndex=" << ImportIndex;
   }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   LLVM_DUMP_METHOD void dump() const { print(dbgs()); }

llvm/trunk/lib/MC/MCWasmStreamer.cpp

bool MCWasmStreamer::EmitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) {
   case MCSA_LazyReference:
   case MCSA_Reference:
   case MCSA_SymbolResolver:
   case MCSA_PrivateExtern:
   case MCSA_WeakDefinition:
   case MCSA_WeakDefAutoPrivate:
   case MCSA_Invalid:
   case MCSA_IndirectSymbol:
-  case MCSA_Hidden:
   case MCSA_Protected:
     return false;
 
+  case MCSA_Hidden:
+    Symbol->setHidden(true);
+    break;
+
   case MCSA_Weak:
   case MCSA_WeakReference:
     Symbol->setWeak(true);
     Symbol->setExternal(true);
     break;
 
   case MCSA_Global:
     Symbol->setExternal(true);

llvm/trunk/lib/MC/WasmObjectWriter.cpp

if (S.isTemporary() && S.getName().empty())
       continue;
 
     const auto &WS = static_cast<const MCSymbolWasm &>(S);
     DEBUG(dbgs() << "MCSymbol: '" << S << "'"
                  << " isDefined=" << S.isDefined() << " isExternal="
                  << S.isExternal() << " isTemporary=" << S.isTemporary()
                  << " isFunction=" << WS.isFunction()
                  << " isWeak=" << WS.isWeak()
+                 << " isHidden=" << WS.isHidden()
                  << " isVariable=" << WS.isVariable() << "\n");
 
-    if (WS.isWeak())
-      SymbolFlags.emplace_back(WS.getName(), wasm::WASM_SYMBOL_BINDING_WEAK);
+    if (WS.isWeak() || WS.isHidden()) {
+      uint32_t Flags = (WS.isWeak() ? wasm::WASM_SYMBOL_BINDING_WEAK : 0) |
+          (WS.isHidden() ? wasm::WASM_SYMBOL_VISIBILITY_HIDDEN : 0);
+      SymbolFlags.emplace_back(WS.getName(), Flags);
+    }
 
     if (WS.isVariable())
       continue;
 
     unsigned Index;
 
     if (WS.isFunction()) {
       if (WS.isDefined(/*SetUsed=*/false)) {

llvm/trunk/lib/Object/WasmObjectFile.cpp

case wasm::WASM_SYMBOL_INFO: {
               "Invalid symbol name in linking section: " + Symbol,
               object_error::parse_failed);
         }
         uint32_t SymIndex = iter->second;
         assert(SymIndex < Symbols.size());
         Symbols[SymIndex].Flags = Flags;
         DEBUG(dbgs() << "Set symbol flags index:"
                      << SymIndex << " name:"
-                     << Symbols[SymIndex].Name << " exptected:"
+                     << Symbols[SymIndex].Name << " expected:"
                      << Symbol << " flags: " << Flags << "\n");
       }
       break;
     }
     case wasm::WASM_DATA_SIZE:
       LinkingData.DataSize = readVaruint32(Ptr);
       break;
     case wasm::WASM_SEGMENT_INFO: {
uint32_t WasmObjectFile::getSymbolFlags(DataRefImpl Symb) const {
   uint32_t Result = SymbolRef::SF_None;
   const WasmSymbol &Sym = getWasmSymbol(Symb);
 
   DEBUG(dbgs() << "getSymbolFlags: ptr=" << &Sym << " " << Sym << "\n");
   if (Sym.isWeak())
     Result |= SymbolRef::SF_Weak;
   if (!Sym.isLocal())
     Result |= SymbolRef::SF_Global;
+  if (Sym.isHidden())
+    Result |= SymbolRef::SF_Hidden;
 
   switch (Sym.Type) {
   case WasmSymbol::SymbolType::FUNCTION_IMPORT:
     Result |= SymbolRef::SF_Undefined | SymbolRef::SF_Executable;
     break;
   case WasmSymbol::SymbolType::FUNCTION_EXPORT:
     Result |= SymbolRef::SF_Executable;
     break;

llvm/trunk/test/MC/WebAssembly/visibility.ll

+; RUN: llc -mtriple wasm32-unknown-unknown-wasm -filetype=obj %s -o - | obj2yaml | FileCheck %s
+
+; Function with __attribute__((visibility("default")))
+define void @defaultVis() #0 {
+entry:
+  ret void
+}
+
+; Function with __attribute__((visibility("hidden")))
+define hidden void @hiddenVis() #0 {
+entry:
+  ret void
+}
+
+; CHECK:        - Type:            CUSTOM
+
+; CHECK:        - Type:            CUSTOM
+; CHECK-NEXT:     Name:            linking
+; CHECK-NEXT:     DataSize:        0
+; CHECK-NEXT:     SymbolInfo:
+; CHECK-NEXT:       - Name:            hiddenVis
+; CHECK-NEXT:         Flags:           4
+; CHECK-NEXT: ...