[Lldb-commits] [PATCH] D95712: [lldb/bindings] Add Python ScriptedProcess base class to lldb module

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D95712#2563091 <https://reviews.llvm.org/D95712#2563091>, @labath wrote:

> Unfortunately I don't have the bandwidth to review this, though I feel that this should have a wider discussion, given that its destined to become a pretty big chunk of our public immutable api surface.

Thanks for taking the time to peek at this :) !

> Some questions to seed that discussion:
>
>   get_num_memory_regions() -> int:
>   get_memory_region_at_index(idx: int) -> lldb.SBMemoryRegionInfo:
>
> This means that the implementation needs to know the exact count of memory regions at any given moment. Elsewhere, we've handled this by having a single api like `get_memory_region_containing_address`. This permits (but doesn't force) the implementation to be lazy in calculating the memory regions, and the caller can still enumerate the memory regions by starting with `get_memory_region_containing_address(0)` and continuing by `get_memory_region_containing_address(prev_region.base + prev_region+size)`. I think we should do the same here.
>
>   get_num_threads() -> int:
>   get_thread_at_index(idx: int) -> Dict:
>
> This suffers from the same problem, though the solution is not that simple. But given that, not too long ago, we've (me&Jim) had a long discussion about how materializing all the threads all the time is prohibitively expensive, it might be good to elaborate on how exactly is this supposed to work (must the implementation always return all threads, or can it have threads disappear temporarily, etc.)

Sounds good! Regarding the threads, after talking to @jingham and @jasonmolenda, it sounds like XNU doesn't have a way to provide all its kernel threads at a single time, only the one loaded in each CPU core.
I changed the python interface to allow lazy loading, using the address to fetch a memory region or a thread ID to fetch a certain thread.

>   get_register_for_thread(tid:int) -> Dict:
>
> I guess this should be at least get_register**s**_for_thread

Typo :p !

>   read_memory_at_address(addr:int) -> lldb.SBData:
>
> How much memory is this supposed to read?

Right ... Originally, I didn't want to limit the users on how much data they should store in the SBData, that's why I didn't provided the size and checked the SBData size in the C++ interface.
In hindsight, this was a wrong design.

>   can_debug() -> bool:
>
> What's the use case for this?

Indeed, that didn't serve much purpose. Got rid of it :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95712