Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lld][WebAssembly] Always search *.so for -Bdynamic #84288

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Mar 7, 2024

Search *.so libraries regardless of -pie to make it a bit more straightforward to build non-pie dynamic-linked executables.

Flip the default to -Bstatic as I think it's what most users expect for the default as of today.
The assumption here is that, because dynamic-linking is not widely used for WebAssembly, the most users do not specify -Bdynamic or -Bstatic, expecting static link.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 7, 2024

@llvm/pr-subscribers-lld-wasm

@llvm/pr-subscribers-lld

Author: YAMAMOTO Takashi (yamt)

Changes

Search *.so libraries regardless of -pie to make it a bit more straightforward to build non-pie dynamic-linked executables.

Flip the default to -Bstatic as I think it's what most users expect for the default as of today.
The assumption here is that, because dynamic-linking is not widely used for WebAssembly, the most users do not specify -Bdynamic or -Bstatic, expecting static link.


Full diff: https://github.com/llvm/llvm-project/pull/84288.diff

3 Files Affected:

  • (modified) lld/wasm/Config.h (+1-1)
  • (modified) lld/wasm/Driver.cpp (+1-3)
  • (modified) lld/wasm/Options.td (+2-2)
diff --git a/lld/wasm/Config.h b/lld/wasm/Config.h
index 266348fef40316..c351e1cef10532 100644
--- a/lld/wasm/Config.h
+++ b/lld/wasm/Config.h
@@ -72,7 +72,7 @@ struct Configuration {
   bool stripAll;
   bool stripDebug;
   bool stackFirst;
-  bool isStatic = false;
+  bool isStatic = true;
   bool trace;
   uint64_t globalBase;
   uint64_t initialHeap;
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index df7d4d1cc3d679..2599eca9389348 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -331,9 +331,7 @@ static std::optional<std::string> findFromSearchPaths(StringRef path) {
 // search paths.
 static std::optional<std::string> searchLibraryBaseName(StringRef name) {
   for (StringRef dir : config->searchPaths) {
-    // Currently we don't enable dynamic linking at all unless -shared or -pie
-    // are used, so don't even look for .so files in that case..
-    if (ctx.isPic && !config->isStatic)
+    if (!config->isStatic)
       if (std::optional<std::string> s = findFile(dir, "lib" + name + ".so"))
         return s;
     if (std::optional<std::string> s = findFile(dir, "lib" + name + ".a"))
diff --git a/lld/wasm/Options.td b/lld/wasm/Options.td
index 70b5aadc26c2a0..7e954822ef6425 100644
--- a/lld/wasm/Options.td
+++ b/lld/wasm/Options.td
@@ -38,9 +38,9 @@ multiclass B<string name, string help1, string help2> {
 // The following flags are shared with the ELF linker
 def Bsymbolic: F<"Bsymbolic">, HelpText<"Bind defined symbols locally">;
 
-def Bdynamic: F<"Bdynamic">, HelpText<"Link against shared libraries (default)">;
+def Bdynamic: F<"Bdynamic">, HelpText<"Link against shared libraries">;
 
-def Bstatic: F<"Bstatic">, HelpText<"Do not link against shared libraries">;
+def Bstatic: F<"Bstatic">, HelpText<"Do not link against shared libraries (default)">;
 
 def build_id: F<"build-id">, HelpText<"Alias for --build-id=fast">;
 

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good change.

How about we keep -Bdynamic as the default when building with -shared and/or -pie ? It seems like in those modes we want to consume shared libraries by default.

Another alternative is that we always prefer shared libraries unless -static is passed? then it would be up to the clang driver (or users) to specify -static in the presence of both .so and .a files.

Ideally we would have some flag

@yamt
Copy link
Contributor Author

yamt commented Mar 8, 2024

Seems like a good change.

How about we keep -Bdynamic as the default when building with -shared and/or -pie ? It seems like in those modes we want to consume shared libraries by default.

it sounds reasonable to me.

Another alternative is that we always prefer shared libraries unless -static is passed? then it would be up to the clang driver (or users) to specify -static in the presence of both .so and .a files.

as the latest wasi-sdk ships *.so files, i guess it surprises many users.

Ideally we would have some flag

what do you mean?

@MaskRay
Copy link
Member

MaskRay commented Mar 8, 2024

In ELF linkers, -Bdynamic is the default. Clang Driver -static passes -static to the linker.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 8, 2024

In ELF linkers, -Bdynamic is the default. Clang Driver -static passes -static to the linker.

The problem is that dynamic linking under wasm is still experimental so we probably want to keep the default as static for now.

IIUC we have several options for how to achieve "opt-in" dynamic linking:

  1. Make -Bstatic the default. The downside of this is that it will be annoying if we ever want to change it in the future.
  2. Don't include any .so files in the normal wasi-sdk, but only in an experimental one. Users of the experimental sdk would have to know that they need to pass -static to get the current behaviour.
  3. Have the clang driver enforce the "static-by-default" by having it inject -static for wasm/wasi targets. This has the same downside as (1) except direct callers of wasm-ld won't be effected by any change in defaults.

The simplest option for now seem like (1).

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a test we can write for this?

I'm actually not sure what would happen if you did -Bdynamic without -shared or -pie .. I don't think that is something that is currently supported. Have you tried it?

@@ -72,7 +72,7 @@ struct Configuration {
bool stripAll;
bool stripDebug;
bool stackFirst;
bool isStatic = false;
bool isStatic = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a comment: "Because dyamanic linking under Wasm is still experimental we default to static linking"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@yamt
Copy link
Contributor Author

yamt commented Mar 16, 2024

I'm actually not sure what would happen if you did -Bdynamic without -shared or -pie .. I don't think that is something that is currently supported. Have you tried it?

yes. it has been working for toywasm at least.
https://github.com/yamt/garbage/blob/1ddf7388fc91c31beed54101685e5d90573b43ef/c/shlib/wasi.sh#L70-L91

yamt added 3 commits May 17, 2024 16:32
Search *.so libraries regardless of -pie to make it a bit more
straightforward to build non-pie dynamic-linked executables.

Flip the default to -Bstatic as I think it's what most users
expect for the default as of today.
The assumption here is that, because dynamic-linking is not widely
used for WebAssembly, the most users do not specify -Bdynamic or
-Bstatic, expecting static link.
// -Bdynamic by default if -pie or -shared is specified.
if (config->pie || config->shared) {
config->isStatic = false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention removes braces in this single line statement case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants