[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix valobj_sp null pointer checks in lldb/source/Plugins/Language/

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:225
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
----------------
kastiglione wrote:
> Michael137 wrote:
> > Michael137 wrote:
> > > Michael137 wrote:
> > > > this won't initialise the parent constructor though will it? Just creates a temporary and immediately destructs it? You might need to put it back into the initialiser list and use a ternary
> > > Oh but `SyntheticChildrenFrontEnd` seems to always take a reference. The interfaces seem to be a little mismatched
> > Perhaps the null-check in the constructor is just redundant and should actually be an assert.
> > 
> > @jingham might know more about the assumptions here
> @xgupta reiterating Michael's point, I think this change results in mis-construction. Have you run the test suite, I would expect some failures with this change.
> 
> I agree that this should be an assert. The other option would be to change the constructor's signature, using a `lldb::ValueObject &` instead of `lldb::ValueObjectSP`, which puts the onus on callers to handle any null-ness. Are we aware of any callers passing null?
> 
> 
FYI, from offline conversation with Jim:

```
This is one of those things where you really should have a general guard against trying to create a FrontEnd with a null ValueObjectSP, since this isn't going to do any good.  But then you worry "what if one leaks through by accident" so you put some fallback code when that happens.  We were really insistent that "if we make a programming error in viewing a variable we'd really like to limit the damage and not crash..." So we didn't go straight to asserts in all these places the way clang, for instance, does.  Taking down a whole debug session because one variable was bizarre (maybe the DWARF was bad in a way that confused us...) is not acceptable, there's too much state in the investigations the debugger supports so we were often over cautious about these sort of checks.

The better solution would be to go to all the places that produce synthetic children and make sure that they error out early when told to format empty SP's.

Not sure how doable that is.  For most cases, the Synthetic Child Providers come from type matches, and an empty ValueObjectSP doesn't have a type, so it would never make a match.  But we do have a few "try it against anything" formatters now, things like the C++ std::function matchers, and the python functional matchers, so there are places where we might end up screwing this up.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142341