- 
                Notifications
    
You must be signed in to change notification settings  - Fork 36
 
Debugger for xeus-cpp with testing framework #401
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 124. Check the log or trigger a new build to see more.
| #include <string> | ||
| 
               | 
          ||
| #include "nlohmann/json.hpp" | ||
| #include "xeus-cpp/xinterpreter.hpp" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header json.hpp is not used directly [misc-include-cleaner]
| #include "xeus-cpp/xinterpreter.hpp" | |
| #include "xeus-cpp/xinterpreter.hpp" | 
| { | ||
| class xdebuglldb_client; | ||
| 
               | 
          ||
| class XEUS_CPP_API debugger : public xeus::xdebugger_base | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: class 'debugger' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
    class XEUS_CPP_API debugger : public xeus::xdebugger_base
                       ^| using base_type = xeus::xdebugger_base; | ||
| 
               | 
          ||
| debugger( | ||
| xeus::xcontext& context, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "xeus::xcontext" is directly included [misc-include-cleaner]
include/xeus-cpp/xdebugger.hpp:11:
- #ifdef __GNUC__
+ #include <xeus/xeus_context.hpp>
+ #ifdef __GNUC__| 
               | 
          ||
| debugger( | ||
| xeus::xcontext& context, | ||
| const xeus::xconfiguration& config, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "xeus::xconfiguration" is directly included [misc-include-cleaner]
include/xeus-cpp/xdebugger.hpp:11:
- #ifdef __GNUC__
+ #include <xeus/xkernel_configuration.hpp>
+ #ifdef __GNUC__| const xeus::xconfiguration& config, | ||
| const std::string& user_name, | ||
| const std::string& session_id, | ||
| const nl::json& debugger_config | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "nlohmann::json" is directly included [misc-include-cleaner]
include/xeus-cpp/xdebugger.hpp:11:
- #ifdef __GNUC__
+ #include <nlohmann/json_fwd.hpp>
+ #ifdef __GNUC__| const nl::json& debugger_config | ||
| ); | ||
| 
               | 
          ||
| virtual ~debugger(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions]
| virtual ~debugger(); | |
| ~debugger() override; | 
| bool start() override; | ||
| void stop() override; | ||
| xeus::xdebugger_info get_debugger_info() const override; | ||
| virtual std::string get_cell_temporary_file(const std::string& code) const override; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'virtual' is redundant since the function is already declared 'override' [cppcoreguidelines-explicit-virtual-functions]
| virtual std::string get_cell_temporary_file(const std::string& code) const override; | |
| std::string get_cell_temporary_file(const std::string& code) const override; | 
| std::string send_dap_message(const nl::json& message); | ||
| std::string receive_dap_response(); | ||
| 
               | 
          ||
| xdebuglldb_client* p_debuglldb_client; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: invalid case style for private member 'p_debuglldb_client' [readability-identifier-naming]
| xdebuglldb_client* p_debuglldb_client; | |
| xdebuglldb_client* m_p_debuglldb_client; | 
| bool m_is_running; | ||
| int m_tcp_socket; | ||
| bool m_tcp_connected; | ||
| pid_t jit_process_pid; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: invalid case style for private member 'jit_process_pid' [readability-identifier-naming]
| pid_t jit_process_pid; | |
| pid_t m_jit_process_pid; | 
| bool m_is_running; | ||
| int m_tcp_socket; | ||
| bool m_tcp_connected; | ||
| pid_t jit_process_pid; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "pid_t" is directly included [misc-include-cleaner]
include/xeus-cpp/xdebugger.hpp:11:
- #ifdef __GNUC__
+ #include <sys/types.h>
+ #ifdef __GNUC__| "libclangCppInterOp.so": "lib/libclangCppInterOp.so" | ||
| } | ||
| } | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get rid of the wasm kernel, unless Jupyterlite supports it !
| if(NOT EMSCRIPTEN AND NOT WIN32) | ||
| configure_kernel("/share/jupyter/kernels/xcpp17-debugger/") | ||
| endif() | ||
| 
               | 
          
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These kernels require the --use-oop-jit flag, so shouldn't be added for Linux arm for osx x86.
Also we should have debugger kernels for c++20, c++23 and the the c kernels too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we should switch to out of process in general all kernels. I suspect there are still several missing features but we should start enumerating them at least…
| @@ -0,0 +1,512 @@ | |||
| """Test xeus-cpp debugger functionality using LLDB""" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shoud also be running the normal jupyter kenel tests for the c++ debugger kernels. We have a mechanism already in place to run the tests for multiple kernels. You just need to add them to the list of kernels in the python file.
| "\n" | ||
| " xeus-cpp: a C++ Jupyter kernel - based on Clang-repl\n"; | ||
| result["banner"] = banner; | ||
| result["debugger"] = true; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this only true for the debugger kernels. Don't we need dependent on which kernel we are using
| //NOLINTNEXTLINE (cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
| createInterpreter(Args(argv ? argv + 1 : argv, argv + argc)); | ||
| m_version = get_stdopt(); | ||
| // m_version = get_stdopt(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this has been commented out.
| 
           Hi @mcbarton , I would really appreciate if you refrain from reviewing the work here unless we request a review from your end. By "We", I mean one of the 4 Google Summer of Code mentors (me, Vipul, Vassil, Johan). We have been having a weekly call over video with @kr-2003 since the past 6 months and there is an incremental systematic approach planned to move things in. I would not like this to be a copy of the cppinterop PR compiler-research/CppInterOp#717 where we tried to address everything in PR and that ended up being a 73 commit long 2000 line diff nightmare to review. We simply cannot address everything through a single PR. For eg. this is experimental enough and we need to start with a 1 single kernel and propagate the idea to others then. If you would like to contribute to this project, please join the weekly call on Friday and raise your points there. Untill then I would appreciate if you take down your requested changes and we can ask you to join in when we need your expertise.  | 
    
| @@ -0,0 +1,512 @@ | |||
| """Test xeus-cpp debugger functionality using LLDB""" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is lldb required now? Or we put it as part of CppInterOp?
| #define XEUS_CPP_XDEBUGGER_HPP | ||
| 
               | 
          ||
| #ifdef __GNUC__ | ||
| #pragma GCC diagnostic push | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these are needed? Ideally we should write code that does not produce warnings.
| @@ -0,0 +1,118 @@ | |||
| /************************************************************************************ | |||
| * Copyright (c) 2023, xeus-cpp contributors * | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Copyright (c) 2023, xeus-cpp contributors * | |
| * Copyright (c) 2025, xeus-cpp contributors * | 
| if(NOT EMSCRIPTEN AND NOT WIN32) | ||
| configure_kernel("/share/jupyter/kernels/xcpp17-debugger/") | ||
| endif() | ||
| 
               | 
          
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we should switch to out of process in general all kernels. I suspect there are still several missing features but we should start enumerating them at least…
          
 Exactly what we (Johan, Vipul and I) started discussing last week. The benchmarks here for matrix multiplication/sorting algorithms (https://compiler-research.org/blogs/gsoc24_sahil_wrapup_blog/) are awesome. I also made @kr-2003 run the whole C++ test suite through an out of process kernel and we look like we're in decent-to-good shape. We plan to discuss more on this now that the cppinterop pR has gone in.  | 
    
Description
Debugger support for xeus-cpp.
Pytest for testing the kernel using jupyter_client.
Type of change
Please tick all options which are relevant.