Page MenuHomePhabricator

[dsymutil] Implement the --threads option
ClosedPublic

Authored by JDevlieghere on Oct 26 2017, 6:08 PM.

Details

Summary

This patch adds the --threads option to dsymutil to process architectures in parallel. The feature is already present in the version distributed with Xcode, but was not yet upstreamed.

This is NFC as far as the linking behavior is concerned. As threads are used automatically, the current tests cover the change in implementation.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Oct 26 2017, 6:08 PM
vsk added inline comments.Oct 26 2017, 6:38 PM
tools/dsymutil/dsymutil.cpp
70 ↗(On Diff #120526)

Could you check that dsymutil produces the same output for some test input with/without threading enabled?

330 ↗(On Diff #120526)

If NumThreads isn't set, why not set it to hardware_concurrency()? Later, we can set NumThreads to the min of itself and the debug map size, since it seems like there's no need to spawn any more threads than that.

356 ↗(On Diff #120526)

I think there's a chance that an exit from one thread can leave partially-written files from another thread on the FS. Maybe as a first step, we should migrate to ToolOutputFile?

365 ↗(On Diff #120526)

Wdyt of wrapping the ThreadPool in a helper class that only constructs the pool if NumThreads > 1? It's not a big deal, but it's strange to me that we'd spawn/tear down a thread but never use it.

aprantl added inline comments.Oct 27 2017, 8:51 AM
tools/dsymutil/dsymutil.cpp
67 ↗(On Diff #120526)

this mentions (n) but doesn't set the value_desc() to n.
It might be nice to add a test similar to the llvm-dwarfdump cmdline.test.

JDevlieghere planned changes to this revision.Oct 27 2017, 9:23 AM
JDevlieghere marked 4 inline comments as done.
tools/dsymutil/dsymutil.cpp
365 ↗(On Diff #120526)

I'd prefer not to do this, as this should really only be a temporary workaround util the recursion stuff is fixed.

aprantl accepted this revision.Oct 27 2017, 11:35 AM
This revision is now accepted and ready to land.Oct 27 2017, 11:35 AM
vsk accepted this revision.Oct 27 2017, 4:59 PM
This revision was automatically updated to reflect the committed changes.
jgravelle-google added inline comments.
llvm/trunk/tools/dsymutil/dsymutil.cpp
66

For reasons I don't fully understand, this flag breaks the WebAssembly waterfall's build. https://wasm-stat.us/builders/linux/builds/25787

Our build uses -DLLVM_BUILD_LLVM_DYLIB=ON and -DLLVM_LINK_LLVM_DYLIB=ON, and we include lld, which my theory is then that both options are statically included in libLLVM, even though they should otherwise share no code. And that's problematic.
Setting those to OFF, and even using -DBUILD_SHARED_LIBS=ON instead causes no issues.

I'm not entirely sure what to do about that and I'm pretty sure this isn't the right place for it, but this might get more eyes involved.

vsk added inline comments.Oct 31 2017, 4:09 PM
llvm/trunk/tools/dsymutil/dsymutil.cpp
66

That's strange. llvm-cov and llvm-profdata both have a num-threads cl::opt, and we're not seeing the same issue there.

Our build uses -DLLVM_BUILD_LLVM_DYLIB=ON and -DLLVM_LINK_LLVM_DYLIB=ON, and we include lld, which my theory is then that both options are statically included in libLLVM, even though they should otherwise share no code.

dsymutil is a tool — I'm surprised that it makes it into libLLVM at all. Could this be a bug in the CMake scripts?

Thanks Jacob for reporting this. I can indeed reproduce the problem locally by passing those flags to cmake. I'll have a look tomorrow and see what I can find out.

The conflicting option is defined in lib/LTO/ThinLTOCodeGenerator.cpp. I'm thinking we should just make llvm-dsymutil consistent with llvm-cov and llvm-profdata and rename the option to -num-threads and -j?

How does the conflict manifest? The option registry shouldn't conflict because dsymutil's option is in a separate category. Or is it the alias that is conflicting? It would be good to understand this. Renaming the option would break compatibility for existing users of dsymutil, so we should avoid that.

How does the conflict manifest? The option registry shouldn't conflict because dsymutil's option is in a separate category. Or is it the alias that is conflicting? It would be good to understand this. Renaming the option would break compatibility for existing users of dsymutil, so we should avoid that.

It's definitely the threads option that conflict. Unfortunately you cannot have two options with the same name, even if they belong to a different category.

As discussed offline, I'll rename the options, add aliases for the internal version and mark them as deprecated/for compatibility.