[Lldb-commits] [PATCH] D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D88266#2301934 <https://reviews.llvm.org/D88266#2301934>, @clayborg wrote:

> Jim: were you able to repro this with a simple a.out program? Is there something worse going on here like maybe incompatible libc++/libstdc++ implementations?

The crash is pretty easy to repro.  This is pretty much what the code in question was doing:

  #include <memory>
  
  class Foo {
  public:
    int first = 10;
    int second = 20;
    std::weak_ptr<Foo> my_wp;
  
    Foo() : first(10), second(20), my_wp() {}
    virtual int doSomething() {
      return first * second;
    }
  };
  
  int
  main()
  {
    Foo my_foo;
    std::shared_ptr<Foo> my_sp(my_foo.my_wp);
    return my_sp.get() ? 0 : 1 ;
  }

Compile and run that on at least on macOS and you get:

   > ./tryit
  libc++abi.dylib: terminating with uncaught exception of type std::__1::bad_weak_ptr: bad_weak_ptr

And looking at the implementation, the SP constructor from a WP explicitly throws if the weak pointer's __cntrl is null.  I'm not sure why that is desirable, but it's clearly intended.

However, looking at the code again, the weak_ptr::lock call really should succeed, just returning an empty shared pointer.  And indeed, I can't reproduce that crash anymore.  I must have had some other changes I had made in the course of investigation lying around that were confusing the issue.

I've resubmitted the patch with the same set of changes, but making the SP by calling lock on the WP rather than calling the SP from WP constructor.



================
Comment at: lldb/include/lldb/Core/StructuredDataImpl.h:72-74
+    lldb::StructuredDataPluginSP plugin_sp;
+    if (!m_plugin_wp.expired())
+      plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
----------------
jingham wrote:
> labath wrote:
> > This is racy. The right way to do that is to call `m_plugin_wp.lock()` and then check the validity of the returned shared_ptr.
> I tried that, but at least on the version of the STL on the latest macOS if the weak pointer has just been default initialized if you call lock on it it crashes.  expired was the only call I could make that didn't crash.
To be clear.  I started looking at this because SBStructuredData.GetDescription was crashing at this line:

      plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);

trying to lock the wp, in the case where m_plugin_wp is default initialized.  I also tried calling lock directly and unsurprisingly that also crashed.


================
Comment at: lldb/include/lldb/Core/StructuredDataImpl.h:72-74
+    lldb::StructuredDataPluginSP plugin_sp;
+    if (!m_plugin_wp.expired())
+      plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
----------------
jingham wrote:
> jingham wrote:
> > labath wrote:
> > > This is racy. The right way to do that is to call `m_plugin_wp.lock()` and then check the validity of the returned shared_ptr.
> > I tried that, but at least on the version of the STL on the latest macOS if the weak pointer has just been default initialized if you call lock on it it crashes.  expired was the only call I could make that didn't crash.
> To be clear.  I started looking at this because SBStructuredData.GetDescription was crashing at this line:
> 
>       plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
> 
> trying to lock the wp, in the case where m_plugin_wp is default initialized.  I also tried calling lock directly and unsurprisingly that also crashed.
What actually happens is that the lock call throws a "bad weak pointer" exception.  So maybe the correct programming approach it to try the lock and catch the exception?  But we build with rtti off and apparently w/o that you can't do try/catch (or at least that's what clang told me...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88266/new/

https://reviews.llvm.org/D88266