Page MenuHomePhabricator

Change LaunchThread interface to return an expected.

Authored by JDevlieghere on Jul 3 2019, 3:12 PM.



Change the interface to return an expected, instead of taking a Status pointer.

Diff Detail


Event Timeline

JDevlieghere created this revision.Jul 3 2019, 3:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 3:12 PM
jingham added a subscriber: jingham.Jul 3 2019, 4:16 PM
jingham added inline comments.
110–117 ↗(On Diff #207899)

If somebody passed you an error_ptr here, you have to fill it in correctly. The way you've written this, error_ptr won't say Failure() in the case where making the thread fails.

114 ↗(On Diff #207899)

Does this really end up returning LLDB_INVALID_HOST_THREAD?

Address Jim's feedback.

JDevlieghere marked 2 inline comments as done.Jul 3 2019, 5:02 PM
labath accepted this revision.Jul 4 2019, 11:20 PM
labath added a subscriber: labath.

Looks fine to me.

Instead of just throwing the error away when the thread creation fails (and there's no way to pass it up the stack), I'd consider logging the error instead.

3601 ↗(On Diff #207929)

I would assume this should always be true for a successfully created thread. Delete the check or change to an assert maybe?

This revision is now accepted and ready to land.Jul 4 2019, 11:20 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 10:43 AM
mgorny added a subscriber: mgorny.Jul 6 2019, 4:13 AM

This seems to have broken the build for us:

FAILED: tools/lldb/source/Host/CMakeFiles/lldbHost.dir/common/Host.cpp.o 
/usr/bin/g++  -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -DLIBXML2_DEFINED -DLLDB_CONFIGURATION_RELEASE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/lldb/source/Host -I/home/motus/netbsd8/netbsd8/llvm/tools/lldb/source/Host -Itools/lldb/include -I/home/motus/netbsd8/netbsd8/llvm/tools/lldb/include -Iinclude -I/home/motus/netbsd8/netbsd8/llvm/include -I/usr/pkg/include/python2.7 -I/home/motus/netbsd8/netbsd8/llvm/tools/clang/include -Itools/lldb/../clang/include -I/usr/pkg/include/libxml2 -I/home/motus/netbsd8/netbsd8/llvm/tools/lldb/source/. -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3 -DNDEBUG    -fno-exceptions -fno-rtti -MD -MT tools/lldb/source/Host/CMakeFiles/lldbHost.dir/common/Host.cpp.o -MF tools/lldb/source/Host/CMakeFiles/lldbHost.dir/common/Host.cpp.o.d -o tools/lldb/source/Host/CMakeFiles/lldbHost.dir/common/Host.cpp.o -c /home/motus/netbsd8/netbsd8/llvm/tools/lldb/source/Host/common/Host.cpp
/home/motus/netbsd8/netbsd8/llvm/tools/lldb/source/Host/common/Host.cpp: In static member function 'static lldb_private::HostThread lldb_private::Host::StartMonitoringChildProcess(const MonitorChildProcessCallback&, lldb::__pid_t, bool)':
/home/motus/netbsd8/netbsd8/llvm/tools/lldb/source/Host/common/Host.cpp:115:72: error: no matching function for call to 'lldb_private::ThreadLauncher::LaunchThread(char [256], void* (&)(void*), MonitorInfo*&, std::nullptr_t)'
       thread_name, MonitorChildProcessThreadFunction, info_ptr, nullptr);
In file included from /home/motus/netbsd8/netbsd8/llvm/tools/lldb/source/Host/common/Host.cpp:57:0:
/home/motus/netbsd8/netbsd8/llvm/tools/lldb/include/lldb/Host/ThreadLauncher.h:23:3: note: candidate: static llvm::Expected<lldb_private::HostThread> lldb_private::ThreadLauncher::LaunchThread(llvm::StringRef, lldb::thread_func_t, lldb::thread_arg_t, size_t)
   LaunchThread(llvm::StringRef name, lldb::thread_func_t thread_function,
/home/motus/netbsd8/netbsd8/llvm/tools/lldb/include/lldb/Host/ThreadLauncher.h:23:3: note:   no known conversion for argument 4 from 'std::nullptr_t' to 'size_t {aka long unsigned int}'
At global scope:
cc1plus: warning: unrecognized command line option '-Wno-vla-extension'
cc1plus: warning: unrecognized command line option '-Wno-deprecated-register'
labath added inline comments.Jul 8 2019, 2:00 PM

I'm sorry to be such a nag, but this is incorrect for two reasons:

  • this won't clear the error object if logging is disabled. There's an LLDB_LOG_ERROR macro to handle that correctly.
  • llvm::formatv does not accept {} as a format argument. You need to use {0} instead.