Page MenuHomePhabricator

[wasm-ld] Allow importing/exporting the output module's memory with arbitrary names
ClosedPublic

Authored by sunfish on Oct 13 2022, 11:01 AM.

Details

Summary

This adds an --export-memory option to wasm-ld which allows passing
a name to give to the exported memory, and extends --import-memory to
allow passing a <module>,<name> pair specifying where the memory should
be imported from.

This is based on https://reviews.llvm.org/D131376, with the main
difference being that it only supports exporting memory by one name.

I know the discussion in D131376 was toward looking into ways to do this by
declaring memories in the source code, and that still makes sense to look into
for more advanced use cases, but for the common case, we don't have any
other reason to declare memory explicitly; we just want lld to use a different
name from the one hard-coded into it, so this adds a way to do that.

Diff Detail

Event Timeline

sunfish created this revision.Oct 13 2022, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 11:01 AM
Herald added subscribers: pmatos, asb. · View Herald Transcript
sunfish requested review of this revision.Oct 13 2022, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 11:01 AM
Herald added a subscriber: aheejin. · View Herald Transcript

Seems reasonable. Does the same thing need to be don't for the indirect function table?

lld/wasm/Driver.cpp
538

why not use defaultModule and memoryName here?

544

If you use memoryName below maybe change this to default to exporting memory under its default name.?

546

Why not use memoryName here?

lld/wasm/SyntheticSections.cpp
237

Does llvm::Optional no coerce to boolean automatically here?

sunfish updated this revision to Diff 467646.Oct 13 2022, 5:42 PM
sunfish marked 4 inline comments as done.
  • Address review feedback
sbc100 added inline comments.Oct 14 2022, 11:14 AM
lld/wasm/Driver.cpp
546

Does this change the default linker behaviour to export the memory (when no flags are specified)? I didn't realize that was the default behavior.

sunfish added inline comments.Oct 21 2022, 1:46 PM
lld/wasm/Driver.cpp
546

Yes, currently wasm-ld exports the memory by default. This patch preserves that default.

abrown added a subscriber: abrown.Oct 21 2022, 2:00 PM
sbc100 accepted this revision.Oct 24 2022, 7:16 AM

lgtm.

I wonder if we should consider changing the default to not export or import the memory?

This revision is now accepted and ready to land.Oct 24 2022, 7:16 AM
This revision was landed with ongoing or failed builds.Oct 31 2022, 1:59 PM
This revision was automatically updated to reflect the committed changes.
fmayer added a subscriber: fmayer.Oct 31 2022, 4:22 PM

This broke our build bots: https://lab.llvm.org/buildbot/#/builders/19/builds/13332/steps/25/logs/stdio

FAILED: tools/lld/wasm/CMakeFiles/lldWasm.dir/Driver.cpp.o 
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/clang_build/bin/clang++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/lld/wasm -I/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/lld/wasm -I/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/lld/include -Itools/lld/include -Iinclude -I/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include -gmlt -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/lld/wasm/CMakeFiles/lldWasm.dir/Driver.cpp.o -MF tools/lld/wasm/CMakeFiles/lldWasm.dir/Driver.cpp.o.d -o tools/lld/wasm/CMakeFiles/lldWasm.dir/Driver.cpp.o -c /home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/lld/wasm/Driver.cpp
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/lld/wasm/Driver.cpp:533:30: error: 'hasValue' is deprecated: Use has_value instead. [-Werror,-Wdeprecated-declarations]
    if (config->memoryExport.hasValue()) {
                             ^~~~~~~~
                             has_value
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/ADT/Optional.h:325:3: note: 'hasValue' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use has_value instead.", "has_value")
  ^
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/Support/Compiler.h:143:50: note: expanded from macro 'LLVM_DEPRECATED'
#define LLVM_DEPRECATED(MSG, FIX) __attribute__((deprecated(MSG, FIX)))
                                                 ^
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/lld/wasm/Driver.cpp:536:31: error: 'hasValue' is deprecated: Use has_value instead. [-Werror,-Wdeprecated-declarations]
    if (!config->memoryImport.hasValue()) {
                              ^~~~~~~~
                              has_value
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/ADT/Optional.h:325:3: note: 'hasValue' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use has_value instead.", "has_value")
  ^
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/Support/Compiler.h:143:50: note: expanded from macro 'LLVM_DEPRECATED'
#define LLVM_DEPRECATED(MSG, FIX) __attribute__((deprecated(MSG, FIX)))
                                                 ^
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/lld/wasm/Driver.cpp:545:29: error: 'hasValue' is deprecated: Use has_value instead. [-Werror,-Wdeprecated-declarations]
  if (!config->memoryExport.hasValue() && !config->memoryImport.hasValue()) {
                            ^~~~~~~~
                            has_value
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/ADT/Optional.h:325:3: note: 'hasValue' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use has_value instead.", "has_value")
  ^
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/Support/Compiler.h:143:50: note: expanded from macro 'LLVM_DEPRECATED'
#define LLVM_DEPRECATED(MSG, FIX) __attribute__((deprecated(MSG, FIX)))
                                                 ^
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/lld/wasm/Driver.cpp:545:65: error: 'hasValue' is deprecated: Use has_value instead. [-Werror,-Wdeprecated-declarations]
  if (!config->memoryExport.hasValue() && !config->memoryImport.hasValue()) {
                                                                ^~~~~~~~
                                                                has_value
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/ADT/Optional.h:325:3: note: 'hasValue' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use has_value instead.", "has_value")
  ^
/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/llvm/include/llvm/Support/Compiler.h:143:50: note: expanded from macro 'LLVM_DEPRECATED'
#define LLVM_DEPRECATED(MSG, FIX) __attribute__((deprecated(MSG, FIX)))
                                                 ^
4 errors generated.