[lld] r328700 - [WebAssembly] Name Config members after commandline argument. NFC
Nicholas Wilson via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 28 14:02:27 PDT 2018
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
More information about the llvm-commits
mailing list