Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb/Interpreter] Make ScriptedInterface Object creation more generic #68052

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

medismailben
Copy link
Member

@medismailben medismailben commented Oct 2, 2023

This patch changes the way plugin objects used with Scripted Interfaces
are created.

Instead of implementing a different SWIG method to create the object for
every scripted interface, this patch makes the creation more generic by
re-using some of the ScriptedPythonInterface templated Dispatch code.

This patch also improves error handling of the object creation by
returning an llvm::Expected.

Signed-off-by: Med Ismail Bennani ismail@bennani.ma

@medismailben medismailben changed the title [lldb] Unifying Scripted Affordance Interfaces [lldb] Unifying Scripted Affordance Interfaces (wip) Oct 2, 2023
@llvmbot llvmbot added the lldb label Oct 2, 2023
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@medismailben medismailben changed the title [lldb] Unifying Scripted Affordance Interfaces (wip) [lldb/Interpreter] Make ScriptedInterface Object creation more generic Oct 24, 2023
Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me generally. I'm curious to know if some of these can be pure virtual function as Jonas mentioned.

This patch changes the way plugin objects used with Scripted Interfaces
are created.

Instead of implementing a different SWIG method to create the object for
every scripted interface, this patch makes the creation more generic by
re-using some of the ScriptedPythonInterface templated Dispatch code.

This patch also improves error handling of the object creation by
returning an `llvm::Expected`.

Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@medismailben medismailben merged commit f22d82c into llvm:main Oct 25, 2023
3 checks passed
@DavidSpickett
Copy link
Collaborator

We still have a failure on Windows after the follow ups:

# .---command stderr------------
# | Cannot create an instance of the ScriptedProcessInterface!
# | UNREACHABLE executed at C:\Users\tcwg\david.spickett\llvm-project\lldb\include\lldb/Interpreter/Interfaces/ScriptedProcessInterface.h:29!
# | PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
# | Stack dump:
# | 0.  Program arguments: c:\\users\\tcwg\\david.spickett\\build-llvm\\bin\\lldb-test.exe ir-memory-map C:\\Users\\tcwg\\david.spickett\\build-llvm\\tools\\lldb\\test\\Shell\\Expr\\Output\\TestIRMemoryMapWindows.test.tmp C:\\Users\\tcwg\\david.spickett\\llvm-project\\lldb\\test\\Shell\\Expr/Inputs/ir-memory-map-basic
# | 1.  HandleCommand(command = "run")
# | Exception Code: 0xC000001D
# |  #0 0x00007ff71e493e0c HandleAbort C:\Users\tcwg\david.spickett\llvm-project\llvm\lib\Support\Windows\Signals.inc:424:0
# |  #1 0x00007ffe20f30ef8 (C:\WINDOWS\SYSTEM32\ucrtbased.dll+0xa0ef8)
# |  #2 0x00007ffe20f32a50 (C:\WINDOWS\SYSTEM32\ucrtbased.dll+0xa2a50)
# |  #3 0x00007ff71e460358 llvm::llvm_unreachable_internal(char const *, char const *, unsigned int) C:\Users\tcwg\david.spickett\llvm-project\llvm\lib\Support\ErrorHandling.cpp:212:0
# |  #4 0x00007ff71e999fc4 lldb_private::ScriptedProcessInterface::CreatePluginObject(class llvm::StringRef, class lldb_private::ExecutionContext &, class std::shared_ptr<class lldb_private::StructuredData::Dictionary>, class lldb_private::StructuredData::Generic *) C:\Users\tcwg\david.spickett\llvm-project\lldb\include\lldb\Interpreter\Interfaces\ScriptedProcessInterface.h:28:0

I'm looking into it.

@DavidSpickett
Copy link
Collaborator

Somehow it is getting to:

  virtual llvm::Expected<StructuredData::GenericSP>
  CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
                     StructuredData::DictionarySP args_sp,
                     StructuredData::Generic *script_obj = nullptr) {
    llvm_unreachable(
        "Cannot create an instance of the ScriptedProcessInterface!");
  }

Starting to wonder if we're relying on some undefined order of evaluation somewhere.

@DavidSpickett
Copy link
Collaborator

Never mind, this could have been the result of using an incompatible Python version. I'll get a stacktrace with one I know is used on the bot.

DavidSpickett added a commit that referenced this pull request Oct 26, 2023
Since #68052 this has been failing.

https://lab.llvm.org/buildbot/#/builders/219/builds/6545

Follow up changes have not fixed it, XFAIL while I debug it.
DavidSpickett added a commit that referenced this pull request Oct 27, 2023
…tePluginObject

After #68052 this function changed from returning
a nullptr with `return {};` to returning Expected and hitting `llvm_unreachable` before it could
do so.

I gather that we're never supposed to call this function, but on Windows we actually do call
this function because `interpreter->CreateScriptedProcessInterface()` returns
`ScriptedProcessInterface` not `ScriptedProcessPythonInterface`. Likely because
`target_sp->GetDebugger().GetScriptInterpreter()` also does not return a Python related class.

The previously XFAILed test crashed with:
```
 # .---command stderr------------
 # | PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
 # | Stack dump:
 # | 0.  Program arguments: c:\\users\\tcwg\\david.spickett\\build-llvm\\bin\\lldb-test.exe ir-memory-map C:\\Users\\tcwg\\david.spickett\\build-llvm\\tools\\lldb\\test\\Shell\\Expr\\Output\\TestIRMemoryMapWindows.test.tmp C:\\Users\\tcwg\\david.spickett\\llvm-project\\lldb\\test\\Shell\\Expr/Inputs/ir-memory-map-basic
 # | 1.  HandleCommand(command = "run")
 # | Exception Code: 0xC000001D
 # | #0 0x00007ff696b5f588 lldb_private::ScriptedProcessInterface::CreatePluginObject(class llvm::StringRef, class lldb_private::ExecutionContext &, class std::shared_ptr<class lldb_private::StructuredData::Dictionary>, class lldb_private::StructuredData::Generic *) C:\Users\tcwg\david.spickett\llvm-project\lldb\include\lldb\Interpreter\Interfaces\ScriptedProcessInterface.h:28:0
 # | #1 0x00007ff696b1d808 llvm::Expected<std::shared_ptr<lldb_private::StructuredData::Generic> >::operator bool C:\Users\tcwg\david.spickett\llvm-project\llvm\include\llvm\Support\Error.h:567:0
 # | #2 0x00007ff696b1d808 lldb_private::ScriptedProcess::ScriptedProcess(class std::shared_ptr<class lldb_private::Target>, class std::shared_ptr<class lldb_private::Listener>, class lldb_private::ScriptedMetadata const &, class lldb_private::Status &) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Plugins\Process\scripted\ScriptedProcess.cpp:115:0
 # | #3 0x00007ff696b1d124 std::shared_ptr<lldb_private::ScriptedProcess>::shared_ptr C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1478:0
 # | #4 0x00007ff696b1d124 lldb_private::ScriptedProcess::CreateInstance(class std::shared_ptr<class lldb_private::Target>, class std::shared_ptr<class lldb_private::Listener>, class lldb_private::FileSpec const *, bool) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Plugins\Process\scripted\ScriptedProcess.cpp:61:0
 # | #5 0x00007ff69699c8f4 std::_Ptr_base<lldb_private::Process>::_Move_construct_from C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1237:0
 # | #6 0x00007ff69699c8f4 std::shared_ptr<lldb_private::Process>::shared_ptr C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1534:0
 # | #7 0x00007ff69699c8f4 std::shared_ptr<lldb_private::Process>::operator= C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1594:0
 # | #8 0x00007ff69699c8f4 lldb_private::Process::FindPlugin(class std::shared_ptr<class lldb_private::Target>, class llvm::StringRef, class std::shared_ptr<class lldb_private::Listener>, class lldb_private::FileSpec const *, bool) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Target\Process.cpp:396:0
 # | #9 0x00007ff6969bd708 std::_Ptr_base<lldb_private::Process>::_Move_construct_from C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1237:0
 # | #10 0x00007ff6969bd708 std::shared_ptr<lldb_private::Process>::shared_ptr C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1534:0
 # | #11 0x00007ff6969bd708 std::shared_ptr<lldb_private::Process>::operator= C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1594:0
 # | #12 0x00007ff6969bd708 lldb_private::Target::CreateProcess(class std::shared_ptr<class lldb_private::Listener>, class llvm::StringRef, class lldb_private::FileSpec const *, bool) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Target\Target.cpp:215:0
 # | #13 0x00007ff696b13af0 std::_Ptr_base<lldb_private::Process>::_Ptr_base C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1230:0
 # | #14 0x00007ff696b13af0 std::shared_ptr<lldb_private::Process>::shared_ptr C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1524:0
 # | #15 0x00007ff696b13af0 lldb_private::PlatformWindows::DebugProcess(class lldb_private::ProcessLaunchInfo &, class lldb_private::Debugger &, class lldb_private::Target &, class lldb_private::Status &) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Plugins\Platform\Windows\PlatformWindows.cpp:495:0
 # | #16 0x00007ff6969cf590 std::_Ptr_base<lldb_private::Process>::_Move_construct_from C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1237:0
 # | #17 0x00007ff6969cf590 std::shared_ptr<lldb_private::Process>::shared_ptr C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1534:0
 # | #18 0x00007ff6969cf590 std::shared_ptr<lldb_private::Process>::operator= C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32124\include\memory:1594:0
 # | #19 0x00007ff6969cf590 lldb_private::Target::Launch(class lldb_private::ProcessLaunchInfo &, class lldb_private::Stream *) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Target\Target.cpp:3274:0
 # | #20 0x00007ff696fff82c CommandObjectProcessLaunch::DoExecute(class lldb_private::Args &, class lldb_private::CommandReturnObject &) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Commands\CommandObjectProcess.cpp:258:0
 # | #21 0x00007ff696fab6c0 lldb_private::CommandObjectParsed::Execute(char const *, class lldb_private::CommandReturnObject &) C:\Users\tcwg\david.spickett\llvm-project\lldb\source\Interpreter\CommandObject.cpp:751:0
 # `-----------------------------
 # error: command failed with exit status: 0xc000001d
```

That might be a bug on the Windows side, or an artifact of how our build is setup,
but whatever it is, having `CreatePluginObject` return an error and
the caller check it, fixes the failing test.

The built lldb can run the script command to use Python, but I'm not sure if that means
anything.
@DavidSpickett
Copy link
Collaborator

02ef12d fixes the Windows test. If the behaviour seems suspicious to you, I can check more details of the build but I don't know anything about Scripted... so I'll need you to tell me what to look for.

aemerson added a commit to apple/llvm-project that referenced this pull request Nov 6, 2023
…tedObject

This was causing a build failure and I *think* holding up the automerger.

It looks like this was deleted by an upstream commit llvm#68052
and perhaps mis-merged and therefore retained.
aemerson added a commit to apple/llvm-project that referenced this pull request Nov 6, 2023
…tedObject

This was causing a build failure and I *think* holding up the automerger.

It looks like this was deleted by an upstream commit llvm#68052
and perhaps mis-merged and therefore retained.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants