[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:246-248
+  DataExtractor data(&promise_addr, sizeof(promise_addr),
+                     process_sp->GetByteOrder(),
+                     process_sp->GetAddressByteSize());
----------------
labath wrote:
> Have you checked there won't be a use-after-free problem here, given that this data extractor will refer to the stack object?
> 
> To create persistent data, you need to use the DataBufferSP constructor, but I'm wondering if we couldn't fix this by creating the (non-pointer) object using the `CreateValueObjectFromAddress` function, as above, but then actually use valobj->AddressOf as the synthetic child.
> 
> I am also somewhat surprised that we need to use the GetAddressOf trick here, as this seems to indicate that the coroutine contains (in the proper C "subobject" kind of way) the promise object. That's not necessarily wrong, but it makes me think we may be "breaking the cycle" at the wrong place.
Thanks for taking a look!

> To create persistent data, you need to use the DataBufferSP constructor

good point, I will keep this in mind as a fallback in case we don't decide to follow any of the other directions you hinted at.

> wondering if we couldn't fix this by creating the (non-pointer) object using the CreateValueObjectFromAddress function, as above, but then actually use valobj->AddressOf as the synthetic child

Good idea! I will give it a try


> [...] as this seems to indicate that the coroutine contains (in the proper C "subobject" kind of way) the promise object. That's not necessarily wrong, but it makes me think we may be "breaking the cycle" at the wrong place.

The physical layout of this is:
```
// in the standard library
template<typename promise_type>
struct exception_handle<promise_type> {
    __coro_frame<promise_type>* __hdl; // <--- this is the pointer we read with `GetCoroFramePtrFromHandle`
};

// compiler-generated coroutine frame. Generated ad-hoc per coroutine
struct __coro_frame<promise_type> {
     // The ABI guaranteees that hose two pointers are always the first two pointers in the struct.
     void (*resume)(void*); // function pointer for type erasure
     void (*destroy)(void*); // function pointer for type erasure
     // Next comes our promise type. This is under the control of the program author
     promise_type promise;
     // Next comes any compiler-generated, internal state which gets persisted across suspension points. 
     // The functions pointed to by `resume`/`destroy` know how to interpret this part of the coroutine frame.
     int __suspension_point_id;
     double __some_internal_state;
     std::string __some_other_internal_state;
     ....
};
```

The programmer (i.e., most likely the user of this pretty-printer), wrote only the "promise" explicitly in his code. Everything else is compiler-generated. As such, the lldb-user will usually look for the "promise" first, and I would like to make it easy to find it, by exposing it as a top-level children of the `exception_handle` instead of hiding it inside a sub-child.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132815