This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add support for visibility flag
ClosedPublic

Authored by sbc100 on Nov 30 2017, 4:27 PM.

Event Timeline

sbc100 created this revision.Nov 30 2017, 4:27 PM
sbc100 edited the summary of this revision. (Show Details)Nov 30 2017, 4:28 PM
sbc100 added reviewers: ruiu, ncw.

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 edited edge metadata.Nov 30 2017, 4:33 PM

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 added inline comments.Nov 30 2017, 4:47 PM
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 updated this revision to Diff 125058.Nov 30 2017, 4:57 PM
  • rebase
ruiu added inline comments.Nov 30 2017, 8:17 PM
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 accepted this revision.Dec 1 2017, 1:54 AM

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.

This revision is now accepted and ready to land.Dec 1 2017, 1:54 AM
ncw added a comment.Dec 1 2017, 5:58 AM

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 added inline comments.Dec 1 2017, 10:14 AM
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.

sbc100 marked 2 inline comments as done.Dec 1 2017, 11:01 AM
ruiu added inline comments.Dec 1 2017, 11:56 AM
wasm/Writer.cpp
274–275

Good point! I didn't notice that.

ruiu added a comment.Dec 1 2017, 11:59 AM

Can you upload the final version again?

sbc100 updated this revision to Diff 125194.Dec 1 2017, 12:33 PM
  • review feedback

Sorry, I thought i had uploaded that already.

ruiu accepted this revision.Dec 1 2017, 12:42 PM

LGTM

ncw accepted this revision.Dec 1 2017, 12:51 PM
This revision was automatically updated to reflect the committed changes.