This is an archive of the discontinued LLVM Phabricator instance.

[lldb][LocateModuleCallback] Implement API, Python interface
ClosedPublic

Authored by splhack on Jun 25 2023, 6:18 PM.

Details

Summary

RFC https://discourse.llvm.org/t/rfc-python-callback-for-target-get-module/71580

Use SWIG for the locate module callback the same as other Python callbacks.
TestLocateModuleCallback.py verifies the functionalities.

Diff Detail

Event Timeline

splhack created this revision.Jun 25 2023, 6:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2023, 6:18 PM
splhack requested review of this revision.Jun 25 2023, 6:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2023, 6:18 PM
clayborg added inline comments.Jul 10 2023, 4:09 PM
lldb/include/lldb/API/SBDefines.h
129

If we are putting this into SBPlatform, this should probably be named "SBPlatformGetModuleCallback".

The name might be a bit more clear if it was "SBPlatformLocateModuleCallback"? Open to suggestions or fine to lave this as "SBPlatformGetModuleCallback" if everyone likes that.

lldb/include/lldb/API/SBPlatform.h
181

remove "Target" from the function name? Or rename to "SetLocateModuleCallback(...)" if we end up renaming the callback type to SBPlatformLocateModuleCallback

lldb/source/API/SBPlatform.cpp
669

You are not passing the "callback" into Platform::SetTargetGetModuleCallback??

Are we missing changes to the file "lldb/include/lldb/Target/Platform.h"?

splhack added inline comments.Jul 10 2023, 6:02 PM
lldb/include/lldb/API/SBDefines.h
129

As commented on D153734, I'll rename it to SBPlatformLocateModuleCallback.

typedef SBError (*SBPlatformLocateModuleCallback)(
    SBDebugger debugger,
    SBModuleSpec &module_spec,
    SBFileSpec &module_file_spec,
    SBFileSpec &symbol_file_spec);
lldb/include/lldb/API/SBPlatform.h
181

I'll rename it to SetLocateModuleCallback.

SBError SetLocateModuleCallback(lldb::SBPlatformLocateModuleCallback callback,
                                                       void *callback_baton);
lldb/source/API/SBPlatform.cpp
669

will update it to support C callback function as well as Python.

mib added a comment.Jul 11 2023, 11:24 AM

LGTM with @clayborg comments.

splhack updated this revision to Diff 539238.Jul 11 2023, 12:22 PM
  • Rename 'get module callback' to 'locate module callback'
  • SBPlatform will do
    • capture callback(SBPlatformLocateModuleCallback) and baton(void *)
    • convert ModuleSpec/FileSpec from/to SBModuleSpec/SBFileSpec for calling the callback
splhack retitled this revision from [lldb][TargetGetModuleCallback] Implement Python interface to [lldb][LocateModuleCallback] Implement API, Python interface.Jul 11 2023, 12:23 PM
splhack edited the summary of this revision. (Show Details)

It would be nice to pass the baton down into the platform layer if possible so that internal users could use the callback + baton. See comments in the other diff and see if you agree

splhack added a comment.EditedJul 11 2023, 5:33 PM

@clayborg yes, it'd be possible to pass the baton down to Platform.h since it is just void *, so no dependencies required.

But, first, let me explain the callback and baton flow.


For Python, user will set a callable Python object as the callback.
For C++ API, user will set a function pointer with a baton as the callback.

Both Python callback and C++ API callback will go into SBPlatform SetLocateModuleCallback.
SetLocateModuleCallback will use a lambda in order to convert ModuleSpec and FileSpec to SBModuleSpec and SBFileSpec.
This is because Platform.h and Target.cpp are not able to use SBModuleSpec and SBFileSpec.

|               |                |                   |              |          |
| user callback |<-SBModuleSpec->| SBPlatform lambda |<-ModuleSpec->| Platform |
|      baton    |  SBFileSpec    |  captures callback|  FileSpec    |          |
|               |                |           baton   |              |          |

This summary is what things are retained by which layer.

  • Python
    • the user callable object
  • SBPlatform
    • C++ API: the user callback and the baton
    • Python: LLDBSwigPythonCallLocateModuleCallback and the user callable object as a baton
  • Platform
    • SBPlatform lambda in std::function

There are three things.

  • SBPlatform lambda created by SetLocateModuleCallback, the type is Platform::LocateModuleCallback.
  • The user callback or LLDBSwigPythonCallLocateModuleCallback, the type is SBPlatformLocateModuleCallback.
  • The baton (for Python, this is the callable object)

And back to 'passing the baton down to Platform.h'.
As the locate callback implementation in D153734 lldb/unittests/Target/LocateModuleCallbackTest.cpp,
internal users can use lambda to capture/retain anything for the callback. For example, this is captured by the locate callback in this case.
Therefore, no need to pass the baton down to Platform.h.

m_platform_sp->SetLocateModuleCallback([this](const ModuleSpec &module_spec,
                                              FileSpec &module_file_spec,
                                              FileSpec &symbol_file_spec) {
  CheckCallbackArgs(module_spec, module_file_spec, symbol_file_spec);
  symbol_file_spec.SetPath(GetInputFilePath(k_breakpad_symbol_file));
  return Status();
});
bulbazord added inline comments.Jul 11 2023, 5:36 PM
lldb/source/API/SBPlatform.cpp
664–689

Suggestion: Invert the conditions and use early returns. This function has quite a bit of nesting and I think the readability could be improved.

splhack updated this revision to Diff 539351.Jul 11 2023, 5:59 PM

update SetLocateModuleCallback

splhack added inline comments.Jul 11 2023, 6:01 PM
lldb/source/API/SBPlatform.cpp
664–689

Thanks, yes it looks better. Also added a comment.

clayborg accepted this revision.Jul 12 2023, 10:29 AM

Thanks for the detailed explanation. We don't want internal users to add a callback with a baton to Platform.h as this would interfere with the APIs, so it is fine to not have a baton in Platform.h.

This revision is now accepted and ready to land.Jul 12 2023, 10:29 AM

@splhack the shell tests are all asserting on Darwin systems (on my desktop and on the bots https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/ ):

[ RUN      ] LocateModuleCallbackTest.GetOrCreateModuleCallbackSuccessWithModuleAndBreakpadSymbol
Assertion failed: (default_platform_sp), function Debugger, file Debugger.cpp, line 837.
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  TargetTests              0x000000010048a5b0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 80
1  TargetTests              0x000000010048ab4c PrintStackTraceSignalHandler(void*) + 28
2  TargetTests              0x000000010048887c llvm::sys::RunSignalHandlers() + 152
3  TargetTests              0x000000010048bd08 SignalHandler(int) + 276
4  libsystem_platform.dylib 0x0000000183a65a24 _sigtramp + 56
5  libsystem_pthread.dylib  0x0000000183a36cc0 pthread_kill + 288
6  libsystem_c.dylib        0x0000000183946a80 abort + 180
7  libsystem_c.dylib        0x0000000183945d9c err + 0
8  TargetTests              0x000000010050afa0 lldb_private::Debugger::Debugger(void (*)(char const*, void*), void*) + 1700
9  TargetTests              0x0000000100509be8 lldb_private::Debugger::Debugger(void (*)(char const*, void*), void*) + 44
10 TargetTests              0x0000000100509b10 lldb_private::Debugger::CreateInstance(void (*)(char const*, void*), void*) + 68
11 TargetTests              0x00000001001e9434 (anonymous namespace)::LocateModuleCallbackTest::SetUp() + 124

In these tests,

   835 	  // Always add our default platform to the platform list
-> 836 	  PlatformSP default_platform_sp(Platform::GetHostPlatform());
   837 	  assert(default_platform_sp);
   838 	  m_platform_list.Append(default_platform_sp, true);

Platform::GetHostPlatform() is returning no platform. I don't know the contracts for this call offhand and if there might be an issue with how the debugger was created that is causing this.

@jasonmolenda thanks for the report. let me check.

At least ninja check-lldb-unit check-lldb-api-python_api-sbplatform is ok on macOS arm64.

Hm, let me try a clean build. ninja check-lldb-unit repo'ed the issue on my desktop.

I'm checking other Debugger::CreateInstance calls in unittests.
Looks like it needs to call Platform::SetHostPlatform which is missing in LocateModuleCallbackTest.cpp.

@jasonmolenda D155135 should fix the Debugger::CreateInstance assert.
(The assert does not repro locally though)

D155124 should fix the Windows test failures.