This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Re-enable threading
ClosedPublic

Authored by JDevlieghere on Dec 13 2017, 5:58 AM.

Details

Summary

Threading was disabled in r317263 because it broke a test in combination
with -DLLVM_ENABLE_THREADS=OFF. This was because a ThreadPool warning
was piped to llvm-dwarfdump which was expecting to read an object from
stdin.

This patch re-enables threading and fixes the offending test.
Unfortunately this required more than just moving the ThreadPool out of
the for loop because of the TempFile refactoring that took place in the
meantime.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Dec 13 2017, 5:58 AM
  • Improve explanation for using std::shared_ptr
aprantl accepted this revision.Dec 13 2017, 8:37 AM

seems good to me but I'm not very familiar with Threadpool.

test/tools/dsymutil/ARM/fat-threading.test
3 ↗(On Diff #126760)

Can you add a comment explaining what is being tested here? It's non-obvious because the a casual reader might not know that we spawn one thread per arch.

tools/dsymutil/dsymutil.cpp
414 ↗(On Diff #126760)

remove extra {

This revision is now accepted and ready to land.Dec 13 2017, 8:37 AM
davide requested changes to this revision.Dec 13 2017, 9:59 AM
davide added a subscriber: davide.
davide added inline comments.
test/tools/dsymutil/X86/alias.test
2 ↗(On Diff #126760)

do you really need to use a relative path here?
Also, do you need -o - even thoughh you're piping to another tool?

tools/dsymutil/dsymutil.cpp
416 ↗(On Diff #126760)

you don't need the braces around the else as well.

This revision now requires changes to proceed.Dec 13 2017, 9:59 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Dec 13 2017, 10:03 AM
This revision was automatically updated to reflect the committed changes.
JDevlieghere marked 4 inline comments as done.Dec 13 2017, 10:11 AM

I didn't see you had requested changes between seeing Adrian's accept and actually landing it. I'm happy to address this in a follow-up commit!

test/tools/dsymutil/X86/alias.test
2 ↗(On Diff #126760)

How else would you specify the path?

Omitting -o - would not pipe the output to stdout but rather create inputfile.dwarf.

I didn't see you had requested changes between seeing Adrian's accept and actually landing it. I'm happy to address this in a follow-up commit!

I see, I think you already addressed my issue, no worries. Thanks!