[WebAssembly] Add support for visibility flag

[sbc100]

Summary

Patch by Nicholas Wilson (https://reviews.llvm.org/D40555)

[sbc100]

I hope you don't mind I made a few mods to your patch so that the entry point was alwasy exports (regardless of visibility). This is because wasm clang will defaults to hidden for all symbols.

[ruiu]

Can you submit typo-fixing changes as a separate patch? You don't need any code review for such trivial change.

wasm/Writer.cpp

274–275

Can this condition happen? I thought that it is an error if -entry was given but the symbol was not found.

[sbc100]

wasm/Writer.cpp

274–275

We have an --allow-undefined flag, which basically skips the step when we check for undefined symbols in the symbol table. Using this flag you can end up with and undefined _start in your binary, I'm not sure we want to allow that, perhaps we should error out where if _start is not defined? That can be a separate change perhaps?

[sbc100]

• rebase

[ruiu]

wasm/Writer.cpp

262–265

I think that bool ExportEntry = !Config->Relocatable && EntrySym && EntrySym->isDefined() is more readable.

274–275

Ah, that's true. Even though the --allow-undefined flag is mostly for debugging, it is a valid use case. But if that's the case, don't you want to check if EntrySym is not nullptr?

280

Please use error to report a soft error. We use fatal only to report corrupted input files.

[ncw]

Thanks for adding those improvements, good with me.

ruiu's spotted a couple of minor improvements.

I think it is already an error if the entry-point symbol can't be found, but the actual check for that is in Driver.cpp, the check in Writer is just a belt-and-braces thing and doesn't matter too much.

[ncw]

Hmm, I can't update this Phabricator diff to apply ruiu's requested fixes... Sorry Sam, you'll have to apply them yourself.

Here below are the changes ruiu requested. Also included below is a suggested change to get rid of the if (Sym == EntrySym) special-casing, and reuse the existing WrittenToSymtab tracking and save a couple of lines.

diff --git a/wasm/Writer.cpp b/wasm/Writer.cpp
index c9455b0aa..bae8e3a51 100644
--- a/wasm/Writer.cpp
+++ b/wasm/Writer.cpp
@@ -259,10 +259,10 @@ void Writer::createTableSection() {
 void Writer::createExportSection() {
   // Memory is and main function are exported for executables.
   bool ExportMemory = !Config->Relocatable && !Config->ImportMemory;
-  bool ExportEntry = !Config->Relocatable;
   bool ExportOther = true; // ??? TODO Config->Relocatable;
   bool ExportHidden = Config->Relocatable;
-  Symbol *EntrySym = ExportEntry ? Symtab->find(Config->Entry) : nullptr;
+  Symbol *EntrySym = !Config->Relocatable ? Symtab->find(Config->Entry) : nullptr;
+  bool ExportEntry = EntrySym && EntrySym->isDefined();
 
   uint32_t NumExports = 0;
 
@@ -270,13 +270,11 @@ void Writer::createExportSection() {
     ++NumExports;
 
   if (ExportEntry) {
-    if (!EntrySym->isDefined())
-      ExportEntry = false;
-    else
-      ++NumExports;
+    EntrySym->WrittenToSymtab = true;
+    ++NumExports;
 
     if (!EntrySym->isFunction())
-      fatal("entry point is not a function: " + EntrySym->getName());
+      error("entry point is not a function: " + EntrySym->getName());
   }
 
   if (ExportOther) {
@@ -285,8 +283,6 @@ void Writer::createExportSection() {
         if (!Sym->isFunction() || Sym->isLocal() || Sym->isUndefined() ||
             (Sym->isHidden() && !ExportHidden) || Sym->WrittenToSymtab)
           continue;
-        if (Sym == EntrySym)
-          continue;
         Sym->WrittenToSymtab = true;
         ++NumExports;
       }
@@ -310,6 +306,8 @@ void Writer::createExportSection() {
   }
 
   if (ExportEntry) {
+    EntrySym->WrittenToSymtab = false;
+    log("Export entrypoint: " + EntrySym->getName());
     WasmExport EntryExport;
     EntryExport.Name = Config->Entry;
     EntryExport.Kind = WASM_EXTERNAL_FUNCTION;
@@ -323,8 +321,6 @@ void Writer::createExportSection() {
         if (!Sym->isFunction() || Sym->isLocal() || Sym->isUndefined() ||
             (Sym->isHidden() && !ExportHidden) || !Sym->WrittenToSymtab)
           continue;
-        if (Sym == EntrySym)
-          continue;
         Sym->WrittenToSymtab = false;
         log("Export: " + Sym->getName());
         WasmExport Export;

[sbc100]

wasm/Writer.cpp

274–275

Entry is added to the symbol table as undefined in Driver so it should always be part of the symbol table, but that symbols table can have undefined symbols it in iff --allow-undefined is passed. So it wont be null in that case.

[ruiu]

wasm/Writer.cpp

274–275

Good point! I didn't notice that.

[ruiu]

Can you upload the final version again?

[sbc100]

• review feedback

[sbc100]

Sorry, I thought i had uploaded that already.

[ruiu]

LGTM

test/wasm/Inputs/call-indirect.ll

 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown-wasm"
 
 @indirect_bar = hidden local_unnamed_addr global i32 ()* @bar, align 4
 
 ; Function Attrs: norecurse nounwind readnone
-define hidden i32 @bar() #0 {
+define i32 @bar() #0 {
 entry:
   ret i32 1
 }
 
 ; Function Attrs: nounwind
-define hidden void @call_bar_indirect() local_unnamed_addr #1 {
+define void @call_bar_indirect() local_unnamed_addr #1 {
 entry:
   %0 = load i32 ()*, i32 ()** @indirect_bar, align 4
   %call = tail call i32 %0() #2
   ret void
 }

test/wasm/Inputs/hidden.ll

+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown-wasm"
+
+; Function Attrs: norecurse nounwind readnone
+define hidden i32 @archiveHidden() #0 {
+entry:
+    ret i32 0
+}
+
+; Function Attrs: norecurse nounwind readnone
+define i32 @archiveDefault() #1 {
+entry:
+    ret i32 0
+}

test/wasm/Inputs/ret32.ll

 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown-wasm"
 
 ; Function Attrs: norecurse nounwind readnone
-define hidden i32 @ret32(float %arg) #0 {
+define i32 @ret32(float %arg) #0 {
 entry:
     ret i32 0
      ; ptrtoint (i32 (float)* @ret32 to i32)
 }

test/wasm/Inputs/ret64.ll

 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown-wasm"
 
-define hidden i64 @ret64(double %arg) local_unnamed_addr #0 {
+define i64 @ret64(double %arg) local_unnamed_addr #0 {
 entry:
     ret i64 1
 }

test/wasm/Inputs/weak-alias.ll

 define i32 @foo() #0 {
 entry:
   ret i32 0
 }
 
 @bar = weak alias i32 (), i32 ()* @foo
 
-define hidden i32 @call_bar() #0 {
+define i32 @call_bar() #0 {
 entry:
   %call = call i32 @bar()
   ret i32 %call
 }

test/wasm/call-indirect.ll

 ; RUN: llc -filetype=obj %p/Inputs/call-indirect.ll -o %t2.o
 ; RUN: llc -filetype=obj %s -o %t.o
 ; RUN: lld -flavor wasm -o %t.wasm %t2.o %t.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
 
 ; bitcode generated from the following C code:
 ; int foo(void) { return 1; }
 ; int (*indirect_func)(void) = &foo;
 ; void _start(void) { indirect_func(); }
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown-wasm"
 
-@indirect_func = hidden local_unnamed_addr global i32 ()* @foo, align 4
+@indirect_func = local_unnamed_addr global i32 ()* @foo, align 4
 
 ; Function Attrs: norecurse nounwind readnone
-define hidden i32 @foo() #0 {
+define i32 @foo() #0 {
 entry:
   ret i32 1
 }
 
 ; Function Attrs: nounwind
-define hidden void @_start() local_unnamed_addr #1 {
+define void @_start() local_unnamed_addr #1 {
 entry:
   %0 = load i32 ()*, i32 ()** @indirect_func, align 4
   %call = tail call i32 %0() #2
   ret void
 }
 
 ; CHECK:      !WASM
 ; CHECK-NEXT: FileHeader:      

 ; CHECK-NEXT:         InitExpr:        
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           66576
 ; CHECK-NEXT:   - Type:            EXPORT
 ; CHECK-NEXT:     Exports:         
 ; CHECK-NEXT:       - Name:            memory
 ; CHECK-NEXT:         Kind:            MEMORY
 ; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            _start
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           3
 ; CHECK-NEXT:       - Name:            bar
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           0
 ; CHECK-NEXT:       - Name:            call_bar_indirect
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           1
 ; CHECK-NEXT:       - Name:            foo
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           2
-; CHECK-NEXT:       - Name:            _start
-; CHECK-NEXT:         Kind:            FUNCTION
-; CHECK-NEXT:         Index:           3
 ; CHECK:        - Type:            ELEM
 ; CHECK-NEXT:     Segments:        
 ; CHECK-NEXT:       - Offset:          
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           1
 ; CHECK-NEXT:         Functions:       [ 0, 2 ]
 ; CHECK-NEXT:   - Type:            CODE
 ; CHECK-NEXT:     Functions:       

test/wasm/entry.ll

 ; RUN: llc -filetype=obj %s -o %t.o
 ; RUN: lld -flavor wasm -e entry -o %t.wasm %t.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
 ; RUN: lld -flavor wasm --entry=entry -o %t.wasm %t.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown-wasm"
 
-define hidden void @entry() local_unnamed_addr #0 {
+define void @entry() local_unnamed_addr #0 {
 entry:
   ret void
 }
 
 ; CHECK:   - Type:            EXPORT
 ; CHECK:     Exports:         
 ; CHECK:       - Name:            memory
 ; CHECK:         Kind:            MEMORY
 ; CHECK:         Index:           0
 ; CHECK:       - Name:            entry
 ; CHECK:         Kind:            FUNCTION
 ; CHECK:         Index:           0

test/wasm/visibility-hidden.ll

+; RUN: llc -mtriple wasm32-unknown-unknown-wasm -filetype=obj -o %t.o %s
+; RUN: llc -mtriple=wasm32-unknown-unknown-wasm -filetype=obj %S/Inputs/hidden.ll -o %t2.o
+; RUN: llvm-ar rcs %t2.a %t2.o
+; RUN: lld -flavor wasm %t.o %t2.a -o %t.wasm
+; RUN: obj2yaml %t.wasm | FileCheck %s
+
+; Test that hidden symbols are not exported, whether pulled in from an archive
+; or directly.
+
+define hidden i32 @objectHidden() {
+entry:
+    ret i32 0
+}
+
+define i32 @objectDefault() {
+entry:
+    ret i32 0
+}
+
+declare i32 @archiveHidden()
+declare i32 @archiveDefault()
+
+define i32 @_start() {
+entry:
+  %call1 = call i32 @objectHidden()
+  %call2 = call i32 @objectDefault()
+  %call3 = call i32 @archiveHidden()
+  %call4 = call i32 @archiveDefault()
+  ret i32 0
+}
+
+; CHECK:        - Type:            EXPORT
+; CHECK-NEXT:     Exports:         
+; CHECK-NEXT:       - Name:            memory
+; CHECK-NEXT:         Kind:            MEMORY
+; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            _start
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           2
+; CHECK-NEXT:       - Name:            archiveDefault
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           4
+; CHECK-NEXT:       - Name:            objectDefault
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           1
+; CHECK-NEXT:   - Type:

test/wasm/weak-alias-overide.ll

 ; CHECK-NEXT:         InitExpr:        
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           66560
 ; CHECK-NEXT:   - Type:            EXPORT
 ; CHECK-NEXT:     Exports:         
 ; CHECK-NEXT:       - Name:            memory
 ; CHECK-NEXT:         Kind:            MEMORY
 ; CHECK-NEXT:         Index:           0
-; CHECK-NEXT:       - Name:            bar
-; CHECK-NEXT:         Kind:            FUNCTION
-; CHECK-NEXT:         Index:           0
 ; CHECK-NEXT:       - Name:            _start
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           1
+; CHECK-NEXT:       - Name:            bar
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           0
 ; CHECK-NEXT:       - Name:            foo
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           2
 ; CHECK-NEXT:       - Name:            call_bar
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           3
 ; CHECK-NEXT:   - Type:            CODE
 ; CHECK-NEXT:     Functions:       

test/wasm/weak-alias.ll

 ; CHECK-NEXT:         InitExpr:        
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           66560
 ; CHECK-NEXT:   - Type:            EXPORT
 ; CHECK-NEXT:     Exports:         
 ; CHECK-NEXT:       - Name:            memory
 ; CHECK-NEXT:         Kind:            MEMORY
 ; CHECK-NEXT:         Index:           0
-; CHECK-NEXT:       - Name:            bar
-; CHECK-NEXT:         Kind:            FUNCTION
-; CHECK-NEXT:         Index:           1
 ; CHECK-NEXT:       - Name:            _start
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            bar
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           1
 ; CHECK-NEXT:       - Name:            foo
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           1
 ; CHECK-NEXT:       - Name:            call_bar
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           2
 ; CHECK-NEXT:   - Type:            CODE
 ; CHECK-NEXT:     Functions:       

test/wasm/weak-external.ll

 ; RUN: llc -mtriple wasm32-unknown-unknown-wasm -filetype=obj -o %t.o %s
 ; RUN: lld -flavor wasm -strip-debug %t.o -o %t.wasm
 ; RUN: obj2yaml %t.wasm | FileCheck %s
 
 ; Test that undefined weak externals (global_var) and (foo) don't cause
 ; link failures and resolve to zero.
 
 @global_var = extern_weak global i32, align 4
 
 declare extern_weak i32 @foo()
 
-define hidden i8* @get_address_of_foo() #0 {
+define i8* @get_address_of_foo() #0 {
 entry:
   ret i8* bitcast (i32 ()* @foo to i8*)
 }
 
-define hidden i32* @get_address_of_global_var() #0 {
+define i32* @get_address_of_global_var() #0 {
   ret i32* @global_var
 }
 
-define hidden i32 @_start() #0 {
+define i32 @_start() #0 {
 entry:
     %0 = load i32, i32* @global_var, align 4
     ret i32 %0
 }
 
 ; CHECK:      --- !WASM
 ; CHECK-NEXT: FileHeader:
 ; CHECK-NEXT:   Version:         0x00000001

 ; CHECK-NEXT:         InitExpr:
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           66560
 ; CHECK-NEXT:   - Type:            EXPORT
 ; CHECK-NEXT:     Exports:
 ; CHECK-NEXT:       - Name:            memory
 ; CHECK-NEXT:         Kind:            MEMORY
 ; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            _start
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           2
 ; CHECK-NEXT:       - Name:            get_address_of_foo
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           0
 ; CHECK-NEXT:       - Name:            get_address_of_global_var
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Index:           1
-; CHECK-NEXT:       - Name:            _start
-; CHECK-NEXT:         Kind:            FUNCTION
-; CHECK-NEXT:         Index:           2
 ; CHECK-NEXT:   - Type:            ELEM
 ; CHECK-NEXT:     Segments:        
 ; CHECK-NEXT:       - Offset:          
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           1
 ; CHECK-NEXT:         Functions:       [ 0 ]
 ; CHECK-NEXT:   - Type:            CODE
 ; CHECK-NEXT:     Functions:

wasm/Symbols.h

public:
   }
   bool isFunction() const {
     return SymbolKind == DefinedFunctionKind ||
            SymbolKind == UndefinedFunctionKind;
   }
   bool isGlobal() const { return !isFunction(); }
   bool isLocal() const { return IsLocal; }
   bool isWeak() const;
+  bool isHidden() const;
 
   // Returns the symbol name.
   StringRef getName() const { return Name; }
 
   // Returns the file from which this symbol was created.
   InputFile *getFile() const { return File; }
 
   uint32_t getGlobalIndex() const;

wasm/Symbols.cpp

void Symbol::update(Kind K, InputFile *F, const WasmSymbol *WasmSym,
   File = F;
   Sym = WasmSym;
   Segment = Seg;
   FunctionType = Sig;
 }
 
 bool Symbol::isWeak() const { return Sym && Sym->isWeak(); }
 
+bool Symbol::isHidden() const { return Sym && Sym->isHidden(); }
+
 std::string lld::toString(wasm::Symbol &Sym) {
   return wasm::displayName(Sym.getName());
 }
 
 std::string lld::toString(wasm::Symbol::Kind &Kind) {
   switch (Kind) {
   case wasm::Symbol::DefinedFunctionKind:
     return "DefinedFunction";

wasm/Writer.cpp

void Writer::createTableSection() {
   writeUleb128(OS, WASM_LIMITS_FLAG_HAS_MAX, "table flags");
   writeUleb128(OS, NumTableElems, "table initial size");
   writeUleb128(OS, NumTableElems, "table max size");
 }
 
 void Writer::createExportSection() {
   // Memory is and main function are exported for executables.
   bool ExportMemory = !Config->Relocatable && !Config->ImportMemory;
-  bool ExportMain = !Config->Relocatable;
-  bool ExportOther = true; // Config->Relocatable;
+  bool ExportOther = true; // ??? TODO Config->Relocatable;
+  bool ExportHidden = Config->Relocatable;
+  Symbol *EntrySym = Symtab->find(Config->Entry);
+  bool ExportEntry = !Config->Relocatable && EntrySym && EntrySym->isDefined();
 
   uint32_t NumExports = 0;
 
   if (ExportMemory)
     ++NumExports;
 
-  if (ExportMain && !ExportOther)
+  if (ExportEntry)
     ++NumExports;
 
   if (ExportOther) {
     for (ObjFile *File : Symtab->ObjectFiles) {
       for (Symbol *Sym : File->getSymbols()) {
         if (!Sym->isFunction() || Sym->isLocal() || Sym->isUndefined() ||
-            Sym->WrittenToSymtab)
+            (Sym->isHidden() && !ExportHidden) || Sym->WrittenToSymtab)
+          continue;
+        if (Sym == EntrySym)
           continue;
         Sym->WrittenToSymtab = true;
         ++NumExports;
       }
     }
   }
 
   if (!NumExports)
     return;
 
   SyntheticSection *Section = createSyntheticSection(WASM_SEC_EXPORT);
   raw_ostream &OS = Section->getStream();
 
   writeUleb128(OS, NumExports, "export count");
 
   if (ExportMemory) {
     WasmExport MemoryExport;
     MemoryExport.Name = "memory";
     MemoryExport.Kind = WASM_EXTERNAL_MEMORY;
     MemoryExport.Index = 0;
     writeExport(OS, MemoryExport);
   }
 
-  if (ExportMain) {
-    Symbol *Sym = Symtab->find(Config->Entry);
-    if (Sym->isDefined()) {
-      if (!Sym->isFunction())
-        fatal("entry point is not a function: " + Sym->getName());
-
-      if (!ExportOther) {
-        WasmExport MainExport;
-        MainExport.Name = Config->Entry;
-        MainExport.Kind = WASM_EXTERNAL_FUNCTION;
-        MainExport.Index = Sym->getOutputIndex();
-        writeExport(OS, MainExport);
-      }
-    }
+  if (ExportEntry) {
+    WasmExport EntryExport;
+    EntryExport.Name = Config->Entry;
+    EntryExport.Kind = WASM_EXTERNAL_FUNCTION;
+    EntryExport.Index = EntrySym->getOutputIndex();
+    writeExport(OS, EntryExport);
   }
 
   if (ExportOther) {
     for (ObjFile *File : Symtab->ObjectFiles) {
       for (Symbol *Sym : File->getSymbols()) {
-        if (!Sym->isFunction() || Sym->isLocal() | Sym->isUndefined() ||
-            !Sym->WrittenToSymtab)
+        if (!Sym->isFunction() || Sym->isLocal() || Sym->isUndefined() ||
+            (Sym->isHidden() && !ExportHidden) || !Sym->WrittenToSymtab)
+          continue;
+        if (Sym == EntrySym)
           continue;
         Sym->WrittenToSymtab = false;
         log("Export: " + Sym->getName());
         WasmExport Export;
         Export.Name = Sym->getName();
         Export.Index = Sym->getOutputIndex();
         if (Sym->isFunction())
           Export.Kind = WASM_EXTERNAL_FUNCTION;
         else
           Export.Kind = WASM_EXTERNAL_GLOBAL;
         writeExport(OS, Export);
       }
     }
-
-    // TODO(sbc): Export local symbols too, Even though they are not part
-    // of the symbol table?
   }
 }
 
 void Writer::createStartSection() {}
 
 void Writer::createElemSection() {
   if (!NumElements)
     return;