This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Plugins] Fix ScriptedThread IndexID reporting
ClosedPublic

Authored by mib on Jan 11 2022, 7:06 PM.

Details

Summary

When listing all the Scripted Threads of a ScriptedProcess, we can see that all
have the thread index set to 1. This is caused by the lldb_private::Thread
constructor, which sets the m_index_id member using the provided thread id tid.

Because the call to the super constructor is done before instantiating
the ScriptedThreadInterface, lldb can't fetch the thread id from the
script instance, so it uses LLDB_INVALID_THREAD_ID instead.

To mitigate this, this patch takes advantage of the ScriptedThread::Create
fallible constructor idiom to defer calling the ScriptedThread constructor
(and the Thread super constructor with it), until we can fetch a valid
thread id tid from the ScriptedThreadInterface.

rdar://87432065

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Jan 11 2022, 7:06 PM
mib requested review of this revision.Jan 11 2022, 7:06 PM
mib updated this revision to Diff 399172.Jan 11 2022, 7:09 PM
mib edited the summary of this revision. (Show Details)
labath added a subscriber: labath.Jan 12 2022, 3:48 AM

It seems to me like the ScriptedThread class would benefit from using the fallible constructor idiom. If you move all the potentially failing operations (initialization of the ScriptedThreadInterface instance?) into a (public) static Expected<ScriptedThreadInterfaceSP> Create(...) method, then the constructor will not need the funky by-ref error argument, and it will be able to access the thread id sufficiently early.

mib updated this revision to Diff 400194.Jan 14 2022, 4:52 PM
mib edited the summary of this revision. (Show Details)
mib added a reviewer: labath.
mib set the repository for this revision to rG LLVM Github Monorepo.

Changed the implementation to defer constructing the ScriptedThread until we have a valid tid

That is a step in the right direction, but ideally we shouldn't by introducing new functions with double return values (value in the "real" result + error through a by-ref argument, or vice versa). We have llvm::Expected for that, and (thread id issue aside) this is actually one of the biggest reasons for using the fallible constructor idiom (real constructors can't return Expecteds, but static factories can).

In that light, I am very saddened by the existence of the ErrorWithMessage function as it encourages one to use the incorrect/old/etc. paradigm. Like, I get that it makes it handy to work with the functions which still use the old interface, but if you really want to have it, then I'd suggest to also create one which interoperates nicely with Expected-returning functions, so that you're not tempted to create new functions with by-ref error results.

Personally, I'd say this wrapper is unnecessary for the new functions, as one of the nice (opinions can vary) things about the llvm error types is that they force you to do something with those errors. Usually, when we don't have anything better to do with these errors we at least log them. So, I'd probably skip logging the errors upon creating, and log them in the caller (as it will have to handle them somehow anyway).

lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
73

this is not needed now

mib updated this revision to Diff 400803.Jan 18 2022, 3:53 AM
mib edited the summary of this revision. (Show Details)

Rebase and address @labath comments.

mib marked an inline comment as done.Jan 18 2022, 3:53 AM
labath accepted this revision.Jan 18 2022, 6:34 AM
labath added inline comments.
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
340

Although there's no way to exclude this via the type system (well... we could have a nonnull_shared_ptr<T>...), this really shouldn't ever happen. All of our Expected<unique/shared_ptr>-returning APIs assume that in the non-error case, the pointer object will be valid. At best, this deserves an assertion.

lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
59

btw, there's a llvm::createStringError, which is (slightly) shorter.

This revision is now accepted and ready to land.Jan 18 2022, 6:34 AM
mib updated this revision to Diff 400838.Jan 18 2022, 7:10 AM
mib marked 2 inline comments as done.
mib updated this revision to Diff 400843.Jan 18 2022, 7:36 AM
This revision was landed with ongoing or failed builds.Jan 24 2022, 11:27 AM
This revision was automatically updated to reflect the committed changes.