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

Why not make this 0 and 1?

lib/MC/MCWasmStreamer.cpp

103

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

(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

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

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!

include/llvm/BinaryFormat/Wasm.h

 };
 
 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,

include/llvm/MC/MCSymbolWasm.h

 private:
   bool IsFunction = false;
   bool IsWeak = false;
+  bool IsHidden = false;
   std::string ModuleName;
   SmallVector<wasm::ValType, 1> Returns;
   SmallVector<wasm::ValType, 4> Params;

   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 {

include/llvm/Object/Wasm.h

     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

lib/MC/MCWasmStreamer.cpp

   case MCSA_WeakDefAutoPrivate:
   case MCSA_Invalid:
   case MCSA_IndirectSymbol:
+    return false;
+
   case MCSA_Hidden:
+    Symbol->setHidden(true);
+    break;
   case MCSA_Protected:
     return false;
 

lib/MC/WasmObjectWriter.cpp

                  << 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;

lib/Object/WasmObjectFile.cpp

         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;

     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:

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