[Lldb-commits] [PATCH] D134333: When there are variable errors, display an error in VS Code's local variables view.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134333#3807511 <https://reviews.llvm.org/D134333#3807511>, @clayborg wrote:

> In D134333#3805945 <https://reviews.llvm.org/D134333#3805945>, @labath wrote:
>
>> Do we actually promise that strings returned by the SB API will live forever? That's not something *I* would want to promote. I always though that we're ConstStringifying the return values only when we don't know any better...
>
> Anything that does return a "const char *" should currently live forever, at least that is what we have been doing so far.

I know there have been patches recently which changed some functions to return constified strings, but my impression is that things did not start out that way. From a cursory search I can find a number of (fairly old) APIs that return strings whose lifetime is less than "forever": SBBreakpoint::GetCondition, SBBreakpoint::GetThreadName, SBData::GetString, SBFrame::Disassemble, ... The constifying changes I remember were usually tied to us changing some `const char *`s into StringRefs internally, which mean we could no longer forward the string at the SB level (as it might not be null terminated). Constructing a temporary string was not possible because it would destruct before the function returns.

However, that's not the case here. The strings lifetime is the same as the enclosing SBError object, and I don't think it's unusual for c++ objects to be handing out pointers to parts of themselves. In this case, one could write the code in question as something like:

  if (SBError err = top_scope->GetError()) {
    do_stuff(err.GetCString());
  }



> We don't want anyone thinking they should free the pointer.

I agree with that, and I am not aware of any situation where we do that.

The issue here is we were previously handing out a "Status::m_string.c_str()" pointer whose lifespan used to be tied to the SBError's lifespan, but we have a lot of APIs that return a SBError instance. So if you did code like:

> So that the temp objects can be use to safely grab the string. But seeing as we already have the GEtCString() API, I am guessing that are probably latent bugs right now due to the unexpectedly short lifespan of the string.

One can use the a temporary object to grab the string, though it is somewhat complicated by the fact that the result can be null. You need some wrapper like:

  optional<string> to_optional_string(const char *s) { if (s) return string(s); else return nullopt; }
  
  ...
  auto s = to_optional_string(get_temporary_error().GetCString());

Without the null values, a plain string assignment would work fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134333