[lld] r328700 - [WebAssembly] Name Config members after commandline argument. NFC

Wed Mar 28 14:09:07 PDT 2018

Understood. Thank you for the explanation. But then maybe we should print
out an error message if both --export-table and --import-table are
specified. The way you are checking the presence of these flags are
somewhat unusual which raised an alert when I was reading the code. I'll
create a patch and send it to you.

On Wed, Mar 28, 2018 at 2:02 PM Nicholas Wilson <
nicholas at nicholaswilson.me.uk> wrote:

> Wasm's --import-table and --export-table are exclusive. That's because
> an imported table isn't defined in the Wasm file, so it doesn't make
> sense to export it.
>
> It's a tristate option: you don't have to either import or export
> (that is, doing neither is valid). So --no-import/export-table
> wouldn't really make sense as options (to me) since exporting it isn't
> the same as not importing it.
>
> I think it does make sense to have a positive option for
> --import-table and for --export-table, with the default being to
> neither import nor export it.
>
> There is choice of course of what to do if both --import-table and
> --export-table are specified; I opted to go with the last-defined, in
> the same that --foo and --no-foo override each other. I could make
> them produce an error if both are provided, if you think that's
> clearer.
>
> Thanks for the review.
>
> Nick
>
> On 28 March 2018 at 20:51, Rui Ueyama <ruiu at google.com> wrote:
> > This is still odd. If Config->ImportTable and Config->ExportTable are
> > exclusive (and looks like they are), you should have only one boolean
> value.
> >
> > Perhaps the flags are odd in the first place. Are --export-table and
> > --import-table exclusive? If so, why?
> >
> > If something is exclusive, the standard way of representing it as a
> command
> > line flag is --foo and --no-foo, e.g. --rosegment and --no-rosegment.
> >
> >
> > On Wed, Mar 28, 2018 at 5:56 AM Nicholas Wilson via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Author: ncw
> >> Date: Wed Mar 28 05:53:29 2018
> >> New Revision: 328700
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=328700&view=rev
> >> Log:
> >> [WebAssembly] Name Config members after commandline argument. NFC
> >>
> >> This addresses a late review comment from D44427/rLLD328643
> >>
> >> Modified:
> >>     lld/trunk/wasm/Config.h
> >>     lld/trunk/wasm/Driver.cpp
> >>     lld/trunk/wasm/Writer.cpp
> >>
> >> Modified: lld/trunk/wasm/Config.h
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/lld/trunk/wasm/Config.h?rev=328700&r1=328699&r2=328700&view=diff
> >>
> >>
> ==============================================================================
> >> --- lld/trunk/wasm/Config.h (original)
> >> +++ lld/trunk/wasm/Config.h Wed Mar 28 05:53:29 2018
> >> @@ -17,19 +17,18 @@
> >>  namespace lld {
> >>  namespace wasm {
> >>
> >> -enum class ExposeAs { IMPORT, EXPORT, NONE };
> >> -
> >>  struct Configuration {
> >>    bool AllowUndefined;
> >>    bool CheckSignatures;
> >>    bool Demangle;
> >> +  bool ExportTable;
> >>    bool GcSections;
> >>    bool ImportMemory;
> >> +  bool ImportTable;
> >>    bool PrintGcSections;
> >>    bool Relocatable;
> >>    bool StripAll;
> >>    bool StripDebug;
> >> -  ExposeAs Table;
> >>    uint32_t GlobalBase;
> >>    uint32_t InitialMemory;
> >>    uint32_t MaxMemory;
> >>
> >> Modified: lld/trunk/wasm/Driver.cpp
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/lld/trunk/wasm/Driver.cpp?rev=328700&r1=328699&r2=328700&view=diff
> >>
> >>
> ==============================================================================
> >> --- lld/trunk/wasm/Driver.cpp (original)
> >> +++ lld/trunk/wasm/Driver.cpp Wed Mar 28 05:53:29 2018
> >> @@ -216,15 +216,6 @@ static StringRef getEntry(opt::InputArgL
> >>    return Arg->getValue();
> >>  }
> >>
> >> -static ExposeAs getExpose(opt::InputArgList &Args, unsigned Import,
> >> -                          unsigned Export) {
> >> -  auto *Arg = Args.getLastArg(Import, Export);
> >> -  if (!Arg)
> >> -    return ExposeAs::NONE;
> >> -  return Arg->getOption().getID() == Import ? ExposeAs::IMPORT
> >> -                                            : ExposeAs::EXPORT;
> >> -}
> >> -
> >>  static const uint8_t UnreachableFn[] = {
> >>      0x03 /* ULEB length */, 0x00 /* ULEB num locals */,
> >>      0x00 /* opcode unreachable */, 0x0b /* opcode end */
> >> @@ -305,7 +296,11 @@ void LinkerDriver::link(ArrayRef<const c
> >>    Config->SearchPaths = args::getStrings(Args, OPT_L);
> >>    Config->StripAll = Args.hasArg(OPT_strip_all);
> >>    Config->StripDebug = Args.hasArg(OPT_strip_debug);
> >> -  Config->Table = getExpose(Args, OPT_import_table, OPT_export_table);
> >> +  auto *TableArg = Args.getLastArg(OPT_import_table, OPT_export_table);
> >> +  Config->ImportTable =
> >> +      TableArg && TableArg->getOption().getID() == OPT_import_table;
> >> +  Config->ExportTable =
> >> +      TableArg && TableArg->getOption().getID() == OPT_export_table;
> >>    errorHandler().Verbose = Args.hasArg(OPT_verbose);
> >>    ThreadsEnabled = Args.hasFlag(OPT_threads, OPT_no_threads, true);
> >>
> >>
> >> Modified: lld/trunk/wasm/Writer.cpp
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/lld/trunk/wasm/Writer.cpp?rev=328700&r1=328699&r2=328700&view=diff
> >>
> >>
> ==============================================================================
> >> --- lld/trunk/wasm/Writer.cpp (original)
> >> +++ lld/trunk/wasm/Writer.cpp Wed Mar 28 05:53:29 2018
> >> @@ -127,7 +127,7 @@ void Writer::createImportSection() {
> >>    uint32_t NumImports = ImportedSymbols.size();
> >>    if (Config->ImportMemory)
> >>      ++NumImports;
> >> -  if (Config->Table == ExposeAs::IMPORT)
> >> +  if (Config->ImportTable)
> >>      ++NumImports;
> >>
> >>    if (NumImports == 0)
> >> @@ -152,7 +152,7 @@ void Writer::createImportSection() {
> >>      writeImport(OS, Import);
> >>    }
> >>
> >> -  if (Config->Table == ExposeAs::IMPORT) {
> >> +  if (Config->ImportTable) {
> >>      uint32_t TableSize = kInitialTableOffset +
> IndirectFunctions.size();
> >>      WasmImport Import;
> >>      Import.Module = "env";
> >> @@ -236,7 +236,7 @@ void Writer::createGlobalSection() {
> >>  }
> >>
> >>  void Writer::createTableSection() {
> >> -  if (Config->Table == ExposeAs::IMPORT)
> >> +  if (Config->ImportTable)
> >>      return;
> >>
> >>    // Always output a table section (or table import), even if there are
> >> no
> >> @@ -259,7 +259,7 @@ void Writer::createTableSection() {
> >>
> >>  void Writer::createExportSection() {
> >>    bool ExportMemory = !Config->Relocatable && !Config->ImportMemory;
> >> -  bool ExportTable = !Config->Relocatable && Config->Table ==
> >> ExposeAs::EXPORT;
> >> +  bool ExportTable = !Config->Relocatable && Config->ExportTable;
> >>
> >>    uint32_t NumExports =
> >>        (ExportMemory ? 1 : 0) + (ExportTable ? 1 : 0) +
> >> ExportedSymbols.size();
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180328/2a95bf2e/attachment.html>