[Lldb-commits] [PATCH] D153734: [lldb][TargetGetModuleCallback] Call get module callback

[Kazuki Sakamoto via Phabricator via lldb-commits]

splhack added a comment.

@clayborg 
Sure, I'll rename the type and function to `Locate` instead of `Get`.

  typedef SBError (*SBPlatformLocateModuleCallback)(
      SBDebugger debugger,
      SBModuleSpec &module_spec,
      SBFileSpec &module_file_spec,
      SBFileSpec &symbol_file_spec);

This `SBPlatformLocateModuleCallback` typedef is required for the SWIG type conversion.

However it will introduce the SB* dependencies to `Platform.h` if we use the type. Is it ok?

  void SetLocateModuleCallback(SBPlatformLocateModuleCallback callback, void *baton);

I think `Platform.h` should use `void *` instead of `SBPlatformLocateModuleCallback`. Is it correct?

  void SetLocateModuleCallback(void *callback, void *baton);



================
Comment at: lldb/source/Core/Debugger.cpp:2170-2181
+
+Status Debugger::CallTargetGetModuleCallback(
+    void *target_get_module_callback_baton, const ModuleSpec &module_spec,
+    FileSpec &module_file_spec, FileSpec &symbol_file_spec) {
+  ScriptInterpreter *script_interpreter = GetScriptInterpreter();
+  if (!script_interpreter)
+    return Status("Cannot find ScriptInterpreter");
----------------
clayborg wrote:
> I don't think we need any of this in debugger. when someone, through python calls the "SetTargetGetModuleCallback" that code should register a static function as the callback to use that will end up calling into the script interpreter.
> 
> We don't always want to call the script interpreter for all cases. Users should be able to register a native callback for this, and it shouldn't force us to always use the script interpreter.
> 
There was another reason why `Debugger` has this method. The reason was that `Debugger` is easily `gMock`-able but `Target` is not.
Please take a look at `MockDebugger` in `lldb/unittests/Target/GetModuleCallbackTest.cpp` in this diff.

I will update this diff to allow C function callback aside from Python function, but probably we need to keep this kind of method outside `Target` for `gMock` to write unit tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153734