This is an archive of the discontinued LLVM Phabricator instance.

[lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={1,2,...} /threads:{1,2,...}
ClosedPublic

Authored by MaskRay on Mar 26 2020, 1:08 PM.

Details

Summary

--no-threads is a name copied from gold.
gold has --no-thread, --thread-count and several other --thread-count-*.

There are needs to customize the number of threads (running several lld
processes concurrently or customizing the number of LTO threads).
Having a single --threads=N is a straightforward replacement of gold's
--no-threads + --thread-count.

--no-threads is used rarely. So just delete --no-threads instead of
keeping it for compatibility for a while.

If --threads= is specified (ELF,wasm; COFF /threads: is similar),
--thinlto-jobs= defaults to --threads=,
otherwise all available hardware threads are used.

There is currently no way to override a --threads={1,2,...}. It is still
a debate whether we should use --threads=all.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 26 2020, 1:08 PM
MaskRay updated this revision to Diff 252964.Mar 26 2020, 1:14 PM

Don't move threads in wasm/Options.td

MaskRay updated this revision to Diff 252966.Mar 26 2020, 1:23 PM

Fix COFF/Driver.cpp

I do use --no-threads fairly often when debugging (so that trace output is not interleaved).. but I can learn to type it as --threads=1.

Harbormaster failed remote builds in B50611: Diff 252966!
Harbormaster failed remote builds in B50608: Diff 252961!
grimar added inline comments.Mar 27 2020, 2:12 AM
lld/wasm/Options.td
98–99

I'd probably expect to see a behavior when with --threads==0 we either report error or just disable multithreading (i.e. threads==0 == threads==1)? Perhaps the default could be -1.

MaskRay marked an inline comment as done.Mar 27 2020, 10:00 AM
MaskRay added a subscriber: aganea.
MaskRay added inline comments.
lld/wasm/Options.td
98–99

@aganea There is a debate about 0 vs -1. What's your opinion?

aganea added inline comments.Mar 27 2020, 10:01 AM
lld/wasm/Options.td
98–99

@grimar There's a subtlety there. If the code uses the std::heavyweight_hardware_concurrency() strategy, that means we should create a single std::thread per hardware core. That function is used in ThinLTO for example.

Not having --threads=... on the cmd-line, or --threads=0, lets the code decide (either std::heavyweight_hardware_concurrency() or std::hardware_concurrency())

--threads=N or --threads=all will always fallback to plain std::hardware_concurrency() strategy, which means to use all hardware threads (SMT or CMT). Typically, this mode is used when linking a large monolithic program.

Perhaps we could change to --threads=[no|0|1] to disable multi-threading, and let the task execute on the main thread (for debugging). Please see the definition for llvm::get_threadpool_strategy().

aganea added inline comments.Mar 27 2020, 10:13 AM
lld/wasm/Options.td
98–99

@MaskRay I feel both 0 and -1 are not user-friendly. I think it is better to say what we mean, --threads=no or --threads=default. But to follow the principle of least astonishment, --threads=0 or --threads=1 could disable multi-threading. WDYT?

MaskRay updated this revision to Diff 253241.Mar 27 2020, 3:46 PM
MaskRay retitled this revision from [lld][ELF][WebAssembly] Replace --(no-)threads with --threads=N to [lld][ELF][WebAssembly] Replace --(no-)threads with --threads={all,1,2,...}.
MaskRay edited the summary of this revision. (Show Details)

Default to --threads=all
Default --thinlto-jobs= to --threads=
Disallow --threads=0 and --threads=-1

MaskRay marked 2 inline comments as done.Mar 27 2020, 3:49 PM
sbc100 added inline comments.Mar 27 2020, 4:06 PM
lld/wasm/Driver.cpp
387

Just, FYI, if there is sufficient code shared with the ELF linker there we do have Common/Args.h as a place to share common arg processing code.

MaskRay marked an inline comment as done.Mar 27 2020, 4:40 PM
MaskRay added inline comments.
lld/wasm/Driver.cpp
387

I know Common/Args.h. OPT_threads and config are of different types in ELF and in wasm, so sharing may not simplify the code.

aganea added inline comments.
lld/COFF/Driver.cpp
1147–1157

This is not consistent with the ELF part. The cmd-line flags grammar and the handling should the be similar.

lld/Common/Threads.cpp
0

Make this ThreadPoolStrategy parallel::Strategy; and move to llvm/lib/Support/Parallel.[h|cpp], and use in Executor::getDefaultExecutor() when constructing the ThreadPoolExecutor.

Otherwise --threads=N will have no effect on the functions used by lld/Common/Threads.h.

lld/ELF/Driver.cpp
1040

What about doing this instead:

config->thinLTOJobs = args.getLastArgValue(OPT_thinlto_jobs);

if (auto *arg = args.getLastArg(OPT_threads)) {
  auto S = get_threadpool_strategy(arg->getValue());
  if (!S)
    error("--threads: invalid thread count: " + arg->getValue());
  parallel::Strategy = *S;

  // maybe warn if --thinlto-jobs= is provided along with --threads=
  config->thinLTOJobs = arg->getValue();
}

This makes for consistent behavior for any of the thread-count flags: --threads, /opt:lldltojobs=, -flto-jobs=, --thinlto-jobs=, -thinlto-threads=, --plugin-opt=jobs=.

And that way, we could uniformly extend the cmd-line flags grammar, if need be (for example if we want platform-independent takset/NUMA control). See @ikitayama 's issue: http://lists.llvm.org/pipermail/llvm-dev/2020-March/140432.html

lld/ELF/SyntheticSections.cpp
2753–2754

I realized the algorithm below didn't make sense if concurrency is not a power-of-two (including 2^0 when multi-threading is disabled/off).

Could you please rebase on rG42dc667db248e27a44dc245d5c39ce1f8ad26a85 -- reverted a mistake I did.

3195–3196

if (parallel::Strategy.ThreadsRequested != 1)

lld/include/lld/Common/Threads.h
67

Remove (see first comment).

71

if (parallel::Strategy.ThreadsRequested != 1)

78

Ditto.

lld/wasm/Driver.cpp
380

Same comment as for the ELF part.

MaskRay updated this revision to Diff 253500.Mar 29 2020, 11:40 PM
MaskRay marked 12 inline comments as done.Mar 29 2020, 11:40 PM

Address review comments

MaskRay retitled this revision from [lld][ELF][WebAssembly] Replace --(no-)threads with --threads={all,1,2,...} to [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...}.Mar 29 2020, 11:42 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay added inline comments.
lld/Common/Threads.cpp
0

I only intended to change the option name, but now it is clear that doing that will cause a future rename churn (enabledThreads -> llvm::parallel::strategy). Let's just finish it with one patch.

lld/ELF/Driver.cpp
1040

get_threadpool_strategy interprets 0 differently from lld (your previous suggestion is that we should error), and "" differently (lld probably should reject an empty string).

If this takes time to get a consensus, we can make this change separately.

lld/ELF/SyntheticSections.cpp
2753–2754

This explained an internal failure I saw. I "fixed" that with 34bdddf9a13cfdbbb5506dc89cf8e781be53105f.

Though I have touched this piece of code several times. I forgot there was a bug after your previous change..

3195–3196

I'll simplify it further.

MaskRay updated this revision to Diff 253501.Mar 29 2020, 11:46 PM

Some adjustment

MaskRay updated this revision to Diff 253502.Mar 29 2020, 11:48 PM

Fix ELF/SyntheticSections.cpp

grimar added inline comments.Mar 30 2020, 2:04 AM
lld/test/ELF/threads.test
2

llvm-mc /dev/null -o %t.o -filetype=obj -triple=xxx?

(the same for the wasm test)

13

Perhaps use FileCheck -DVAL=xxx to make checks explicit?

aganea added inline comments.Mar 30 2020, 7:06 AM
lld/ELF/Driver.cpp
1040

In that case, I would modify get_threadpool_strategy. This function was only added recently, and I'm not fond of =0, we can error out in that case in get_threadpool_strategy, to match what was discussed so far. I would prefer matching behavior for all thread flags across LLVM. But yes, we can do after if you prefer.

llvm/lib/Support/Parallel.cpp
23

llvm::parallel::Strategy. LLVM uses first letter uppercase for variables, whereas LLD uses lowercase.

MaskRay updated this revision to Diff 253614.Mar 30 2020, 9:19 AM
MaskRay marked 4 inline comments as done.

Address comments

lld/ELF/Driver.cpp
1040

If we delete "" and "0" cases from get_threadpool_strategy, then there will be no way to return the default value. This probably means get_threadpool_strategy does not meed lld's needs without an API change. We can defer the change.

llvm/lib/Support/Parallel.cpp
23

llvm::parallel::{seq,par} are lower cased variables. I think this should not matter.

aganea added inline comments.Mar 30 2020, 9:52 AM
llvm/lib/Support/Parallel.cpp
138

Do you think you can move this inside the ThreadPoolExecutor class, like ThreadPoolExecutor::Deleter?

MaskRay updated this revision to Diff 253638.Mar 30 2020, 10:48 AM

Move a function local Creator to ThreadPoolExecutor::Creator

MaskRay marked an inline comment as done.Mar 30 2020, 10:48 AM

Thanks for the quick changes! Just a few more things and I think it should be good to go.

lld/COFF/Driver.cpp
1156

nit: use parallel::strategy = hardware_concurrency(threads); to ensure any future change to the default strategy does not change this behavior (same comment for other places below).

1157

Overwrite config->thinLTOJobs here, like the ELF part.

lld/COFF/Options.td
223
"Number of threads. 'all' (default) means all of concurrent threads supported. '1' disables multi-threading."

Same comment for the other drivers.

lld/ELF/Driver.cpp
1047

In absolute terms, do we really need two options for setting multi-threading? (-threads= for general-purpose code, and -thinlto-jobs= for ThinLTO threads)
We can address this later, but it might complicate the setup for users. @tejohnson

llvm/lib/Support/Parallel.cpp
23

Please add a comment above signifying that this defaults to using all hardware threads.

MaskRay updated this revision to Diff 253698.Mar 30 2020, 2:03 PM
MaskRay marked 7 inline comments as done.

Address review comments

lld/ELF/Driver.cpp
1047

I had the same question:) I think even if people start to migrate to --threads, -thinlto-jobs= may be useful for debugging ThinLTO issues. --thinlto-jobs=1 can be used together with --save-temps, without slowding down other parallel loops. Whether this is worth a separate option needs more thoughts.

MaskRay edited the summary of this revision. (Show Details)Mar 30 2020, 2:09 PM
aganea accepted this revision.Mar 30 2020, 2:42 PM

LGTM, thanks! Maybe wait a bit in case there are other comments.

Some todos for the record:

  • This line: https://github.com/llvm/llvm-project/blob/master/lld/lib/ReaderWriter/MachO/LayoutPass.cpp#L464 - should use the parallelSort() API, but could be done later.
  • parallel::strategy.ThreadsRequested == 1 is a bit verbose and error prone, it could be an API like parallel::strategy.useThreads() - this could be done in a subsequent commit.
  • Is there value for the code in lld/Common/Threads.h? It could be replaced with direct calls to Parallel.h APIs instead? (assuming they handle the "no multi-threading" case)
  • Same function for handling --threads= in all three drivers and get_threadpool_strategy().
This revision is now accepted and ready to land.Mar 30 2020, 2:42 PM
rnk accepted this revision.Mar 30 2020, 3:41 PM

lgtm

Thanks @aganea for reviewing. I didn't review thoroughly, but I approve of the direction.

MaskRay updated this revision to Diff 253732.Mar 30 2020, 3:58 PM

Thanks @aganea for reviewing and a list of TODO items in https://reviews.llvm.org/D76885#1950989 , and @rnk.

Add .Cm 1 disables multi-threading to docs/ld.lld.1
Fix a comment --thinlto-jobs= -> /opt:lldltojobs= in COFF/Driver.cpp

Will wait a bit and see if there are other comments.

ruiu added a comment.Mar 30 2020, 9:06 PM

Do we need all? It's the default behavior, so if you want to specify --threads=all, you can just delete it altogether.

Are you going to make the same change for the other ports? I think wasm-ld mimics GNU linker's options, and I don't think we want to deviate too much from it, even if it simplifies the command line options.

MaskRay added a comment.EditedMar 30 2020, 10:35 PM

Do we need all? It's the default behavior, so if you want to specify --threads=all, you can just delete it altogether.

--threads=all is a way to override a previous --threads=3. We just need a way to reset the behavior to a previous state. It is just for completeness.

Are you going to make the same change for the other ports? I think wasm-ld mimics GNU linker's options, and I don't think we want to deviate too much from it, even if it simplifies the command line options.

--threads and --no-threads are gold-only options. They have currently mostly debug options. GNU ld does not have it. gold actually has more options but they are probably overkill:

--thread-count COUNT
       Number of threads to use

--thread-count-initial COUNT
       Number of threads to use in initial pass

--thread-count-middle COUNT Number of threads to use in middle pass

--thread-count-final COUNT
       Number of threads to use in final pass

For wasm-ld, @sbc100 mentioned that he can change his habit to adapt the new --threads=

ruiu added a comment.Mar 30 2020, 11:07 PM

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads
  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

MaskRay added a comment.EditedMar 31 2020, 8:20 AM

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads
  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

Nobody uses --threads. I will delete --threads=all. Let's wait for other opinion whether we need an options restoring the default behavior.

One argument for --threads=all is that --thinlto-jobs=all is supported, but this point will be moot if we are going to revisit it.

MaskRay updated this revision to Diff 253898.Mar 31 2020, 8:29 AM
MaskRay retitled this revision from [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={all,1,2,...} /threads:{all,1,2,...} to [lld][COFF][ELF][WebAssembly] Replace --[no-]threads /threads[:no] with --threads={1,2,...} /threads:{1,2,...}.

Delete --threads=all

MaskRay updated this revision to Diff 253900.Mar 31 2020, 8:45 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

This revision was automatically updated to reflect the committed changes.

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads

I don't disagree with your arguments but from the man page it looks like make -j without any args means there is no upper limit at all:

j [jobs], --jobs[=jobs]
            Specifies  the  number of jobs (commands) to run simultaneously.  If there is more than one -j option, the last one
            is effective.  If the -j option is given without an argument, make will not limit the number of jobs that  can  run
            simultaneously.

Unless I'm misreading. I guess that is why we very rarely see it in the wild.

  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

Just a quick question to bounce on what Rui said: why not make this flag -j instead of --threads? Everyone is indeed used to -j. Is there an argument for it being --threads?

@aganea make/ninja/parallel spawn processes (jobs). For an ld, its has a single job. --threads= might be a bit implementation specific but I cannot think of a better term expression parallelism within a job...

There is another very weak argument. Because j is a prefix of just-symbols, GNU style getopt_long treats -j as an abbreviation of --just-symbols (which is implemented along with -rpath). If we add -j to lld, it can do different things from GNU ld. This makes a short option -j less favorable...

It looks like this change broke our buildbot: http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/6358

Could you please recommit this with a fix, or revert this?

broadwaylamb added inline comments.Mar 31 2020, 1:23 PM
lld/test/ELF/threads.s
1 ↗(On Diff #253905)

This bot builds a cross-toolchain that can only produce binaries for ARM, so this test fails because it only supports x86_64. Maybe you can mark it as unsupported?

MaskRay marked 2 inline comments as done.Mar 31 2020, 3:25 PM
MaskRay added inline comments.
lld/test/ELF/threads.s
1 ↗(On Diff #253905)
broadwaylamb added inline comments.Mar 31 2020, 4:09 PM
lld/test/ELF/threads.s
1 ↗(On Diff #253905)

The failure is gone, thanks!

This comment was removed by smeenai.

For people who were using -Wl,--no-threads to disable parallelism in (some parts of) their build system, what's the way forward if they want to support both older and newer LLD versions?

(I suppose feature flag detection is one way, but that's not really a thing on some build systems, including Bazel, I believe.)

MaskRay marked an inline comment as done.Apr 1 2020, 12:33 PM

For people who were using -Wl,--no-threads to disable parallelism in (some parts of) their build system, what's the way forward if they want to support both older and newer LLD versions?

(I suppose feature flag detection is one way, but that's not really a thing on some build systems, including Bazel, I believe.)

-Wl,--no-threads should be very rare. https://github.com/halide/Halide/blob/master/src/runtime/hexagon_remote/Makefile and libsodium_1.0.18-1/dist-build/wasm32-wasi.sh are the only open source cases I can find. You probably should make the change while bumping the version of lld.

phosek added a subscriber: phosek.EditedApr 1 2020, 4:08 PM

Instead of --threads=all, can you use --threads? --threads has several advantages over --threads=all:

  • it is consistent with other command such as make, which takes -j for "all" and -jN for N threads
  • it is compatible with gold
  • "all" isn't technically very accurate. A more accurate option name would be something like --threads=same-as-cpu-threads, but that's fringe. By eliminating an argument, we can avoid naming it.

If we are doing this, I'd like to see this to be added to the ELF version.

Nobody uses --threads. I will delete --threads=all. Let's wait for other opinion whether we need an options restoring the default behavior.

I disagree, we've been using --threads for some time (there are 5 different instances of this flag just in the Fuchsia build, see for example https://fuchsia.googlesource.com/fuchsia/+/refs/heads/master/build/config/fuchsia/BUILD.gn#48) and this change broke our build.

One argument for --threads=all is that --thinlto-jobs=all is supported, but this point will be moot if we are going to revisit it.

phosek added a comment.Apr 1 2020, 4:21 PM

I think the removal of --thread could have been handled more gracefully, e.g. print a warning but retain the existing functionality, and later follow up with the removal. Right now our only options are to either disable threading until we roll the new toolchain and slow down the build in the meantime, or disable the canary bots and loose coverage for upstream changes, neither of which is great.

sbc100 added a comment.Apr 1 2020, 4:24 PM

I think the removal of --thread could have been handled more gracefully, e.g. print a warning but retain the existing functionality, and later follow up with the removal. Right now our only options are to either disable threading until we roll the new toolchain and slow down the build in the meantime, or disable the canary bots and loose coverage for upstream changes, neither of which is great.

I tend to agree. I think the assumption was that this was only used by developers to debug (at least that was my only use for it). I think you have proved that assumption wrong.

I think the removal of --thread could have been handled more gracefully, e.g. print a warning but retain the existing functionality, and later follow up with the removal. Right now our only options are to either disable threading until we roll the new toolchain and slow down the build in the meantime, or disable the canary bots and loose coverage for upstream changes, neither of which is great.

--threads was the default so you can just delete it from your BUILD.gn There is no functional difference before or after this change.

dmajor added a subscriber: dmajor.Jul 2 2020, 8:42 AM

This commit breaks the build when -DLLVM_ENABLE_THREADS=OFF is used. Could you please take a look?

Undefined symbols for architecture x86_64:
  "llvm::parallel::strategy", referenced from:
      readConfigs(llvm::opt::InputArgList&) in liblldELF.a(Driver.cpp.o)
      lld::coff::LinkerDriver::link(llvm::ArrayRef<char const*>) in liblldCOFF.a(Driver.cpp.o)
      lld::wasm::(anonymous namespace)::LinkerDriver::link(llvm::ArrayRef<char const*>) in liblldWasm.a(Driver.cpp.o)
      lld::unlinkAsync(llvm::StringRef) in liblldCommon.a(Filesystem.cpp.o)
      lld::elf::MergeNoTailSection::finalizeContents() in liblldELF.a(SyntheticSections.cpp.o)
      createSymbols(llvm::ArrayRef<std::__1::vector<lld::elf::GdbIndexSection::NameAttrEntry, std::__1::allocator<lld::elf::GdbIndexSection::NameAttrEntry> > >, std::__1::vector<lld::elf::GdbIndexSection::GdbChunk, std::__1::allocator<lld::elf::GdbIndexSection::GdbChunk> > const&) in liblldELF.a(SyntheticSections.cpp.o)
      void lld::elf::doIcf<llvm::object::ELFType<(llvm::support::endianness)1, false> >() in liblldELF.a(ICF.cpp.o)
      ...
dmajor added a comment.Jul 8 2020, 9:33 AM

I filed https://bugs.llvm.org/show_bug.cgi?id=46641 to make sure it doesn't get lost.

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

Regardless of what we decide to do here.. we should remove that flag from wasi-sdk build system. I have no idea why it was added, perhaps there were some bugs related to threading early on in the development of wasm-ld and this just never got removed.

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

Sam is the code owner of the WebAssembly backend. He makes the decision whether --no-threads needs to be supported.

Personally I recognize that the removal of the option caused some friction but (at least for COFF/ELF) I don't think the use cases are critical enough to be supported.

--no-threads is used rarely. So just delete --no-threads instead of keeping it for compatibility for a while.

I encountered a frustrating issue with seemingly-unrelated symptoms that eventually traced back to this change.

The wasi-sdk project was building libcxx, and its build system had set CMAKE_EXE_LINKER_FLAGS="-Wl,--no-threads" (I don't know why, wasi-sdk is an unfamiliar project to me, I just needed to build it for something else). When cmake was doing its usual "is XYZ supported" tests, the --no-threads option caused the checks to fail, so cmake believed that for example LIBCXX_SUPPORTS_FNO_EXCEPTIONS_FLAG was false. But the cmake was overall still successful. So we got an apparently successful libcxx build but it didn't have the settings that were requested. This manifested as weird issues in a faraway place later. It took a lot of time to track down.

It's not clear to me why the --no-threads made the "is XYZ supported" checks fail but the actual build still succeeded. I'm pretty worried though about introducing this kind of silent behavior change. Do you think other projects may be susceptible to such a problem? Should the lack of --no-threads compatibility be reconsidered for the 11 release branch?

Sam is the code owner of the WebAssembly backend. He makes the decision whether --no-threads needs to be supported.

Personally I recognize that the removal of the option caused some friction but (at least for COFF/ELF) I don't think the use cases are critical enough to be supported.

I'm OK with fixing this downstream in this case I think. There should be very few users of this flag with wasm-ld but also I wouldn't want the flag names for the same feature to be different between different lld backends.

In terms of the way this problem manifested, It makes sense that the cmake feature detection would fail because it wouldn't be able to link any of the test programs it was building. I imagine the overall project still built because there are no executables in this case, only libraries, so there is no link step in the build, only llvm-ar. For most projects this wouldn't be and issue and they would see a clear link failure.

I've now opened https://github.com/WebAssembly/wasi-sdk/pull/151/ to remove the --no-threads from wasi-sdk, which was a workaround for an old bug that has long been fixed.

dmajor added a comment.Aug 6 2020, 9:52 AM

I've now opened https://github.com/WebAssembly/wasi-sdk/pull/151/ to remove the --no-threads from wasi-sdk, which was a workaround for an old bug that has long been fixed.

With that PR now merged, I've confirmed that the latest wasi-sdk tree no longer hits the issue I was seeing. Thanks everyone.