[WebAssembly] Add missing --demangle arg

[ncw]

Summary

Previously, Config->Demangle was not only not hooked up to commandline handling, but was uninitialised

[sbc100]

I think perhaps we should just drop the max-memory flag completely? At least until we know they its needed. emscripten doesn't need it and nor does the wasm waterfall.

Common/Args.cpp

30 ↗

(On Diff #137739)

I would hope we don't need to add this.

wasm/Writer.cpp

120 ↗

(On Diff #137739)

I'm OK with 0 meaning no max. Especially if i means you can avoid adding an duplicate getInteger helper for options?

TBH, I'm torn about weather to just remove this option though. Do you know of any users who what to set a max?

618 ↗

(On Diff #137739)

Should probably error out if InitialMemory is not enough, rather than just making it bigger.

Also, should we have have these value specific in page size?

If not we should error out if user specified a non-aligned value (like we do for StackSize).

[ruiu]

Please split it into two patches, one for --demangle and the other for --initial/max-memory.

[ncw]

OK, I'll split this in two, and fix the nits regarding 0 for unset and forcing aligned values.

I think it is worth keeping MaxMemory and InitialMemory, even though they have limited use-cases. It's a memory-sanity feature, to ask the browser to keep a lid on your memory usage (based on what you know your code should be using), rather than letting bugs and leaks chew up RAM until it's exhausted. I think people probably should use it as a matter of course, to avoid reports of spiralling memory usage from users of their Wasm library! Maybe no-one will bother using it though.

Common/Args.cpp

30 ↗

(On Diff #137739)

It's there to prevent people entering a negative number. Is that right, to have an unsigned variant? I mean we could parse it as a signed int, but then we'd have to add error-checking code to handle negative values, whereas punting that problem to llvm::to_integer seems simpler.

[ncw]

wasm/Writer.cpp

120 ↗

(On Diff #137739)

Oh - a further thought on zero. I remember now why I didn't do that, it's because for both Initial and Max is it possible to have zero as a legal value? Can a module not use the memory at all? I'm not sure we currently support it, but one day we might do. Zero does have a real "semantic" meaning for maximum, which is the opposite of "no maximum", whereas UINT32_MAX has a "semantic" meaning that exactly matches "no maximum".

[ncw]

Updated to remove max/initial mem args, this just covers demangle now

[sbc100]

Great! Much more precise.

[ruiu]

LGTM

test/wasm/demangle.ll

+; RUN: llc -filetype=obj %s -o %t.o
+; RUN: not wasm-ld --check-signatures --undefined _Z3fooi \
+; RUN:     -o %t.wasm %t.o 2>&1 | FileCheck %s
+
+; CHECK: error: undefined symbol: foo(int)
+
+; RUN: not wasm-ld --check-signatures --no-demangle --undefined _Z3fooi \
+; RUN:     -o %t.wasm %t.o 2>&1 |  FileCheck -check-prefix=CHECK-NODEMANGLE %s
+
+; CHECK-NODEMANGLE: error: undefined symbol: _Z3fooi
+
+target triple = "wasm32-unknown-unknown-wasm"
+
+define hidden void @_start() local_unnamed_addr {
+entry:
+    ret void
+}

wasm/Driver.cpp

for (auto *Arg : Args.filtered(OPT_mllvm))
     V.push_back(Arg->getValue());
   cl::ParseCommandLineOptions(V.size(), V.data());
 
   errorHandler().ErrorLimit = args::getInteger(Args, OPT_error_limit, 20);
 
   Config->AllowUndefined = Args.hasArg(OPT_allow_undefined);
   Config->CheckSignatures =
       Args.hasFlag(OPT_check_signatures, OPT_no_check_signatures, false);
+  Config->Demangle = Args.hasFlag(OPT_demangle, OPT_no_demangle, true);
   Config->Entry = getEntry(Args, Args.hasArg(OPT_relocatable) ? "" : "_start");
   Config->ImportMemory = Args.hasArg(OPT_import_memory);
   Config->OutputFile = Args.getLastArgValue(OPT_o);
   Config->Relocatable = Args.hasArg(OPT_relocatable);
   Config->GcSections =
       Args.hasFlag(OPT_gc_sections, OPT_no_gc_sections, !Config->Relocatable);
   Config->PrintGcSections =
       Args.hasFlag(OPT_print_gc_sections, OPT_no_print_gc_sections, false);

wasm/Options.td

 include "llvm/Option/OptParser.td"
 
 // For options whose names are multiple letters, either one dash or
 // two can precede the option name except those that start with 'o'.
 class F<string name>: Flag<["--", "-"], name>;
 class J<string name>: Joined<["--", "-"], name>;
 class S<string name>: Separate<["--", "-"], name>;
 
 multiclass Eq<string name> {
   def "": Separate<["--", "-"], name>;
   def _eq: Joined<["--", "-"], name # "=">, Alias<!cast<Separate>(NAME)>;
 }
 
+multiclass B<string name, string help1, string help2> {
+  def NAME: Flag<["--", "-"], name>, HelpText<help1>;
+  def no_ # NAME: Flag<["--", "-"], "no-" # name>, HelpText<help2>;
+}
+
 // The follow flags are shared with the ELF linker
 def color_diagnostics: F<"color-diagnostics">,
   HelpText<"Use colors in diagnostics">;
 
 def color_diagnostics_eq: J<"color-diagnostics=">,
   HelpText<"Use colors in diagnostics">;
 
+defm demangle: B<"demangle",
+    "Demangle symbol names",
+    "Do not demangle symbol names">;
+
 def entry: S<"entry">, MetaVarName<"<entry>">,
   HelpText<"Name of entry point symbol">;
 
 def error_limit: J<"error-limit=">,
   HelpText<"Maximum number of errors to emit before stopping (0 = no limit)">;
 
 def fatal_warnings: F<"fatal-warnings">,
   HelpText<"Treat warnings as errors">;