- 
                Notifications
    
You must be signed in to change notification settings  - Fork 15.1k
 
[lldb-dap] assembly breakpoints #139969
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-dap] assembly breakpoints #139969
Changes from 6 commits
d6325b3
              ee49203
              dbb2f9b
              3699524
              61623de
              f5fd76d
              40f68e4
              199512f
              dbad33b
              2e9edca
              0515c6d
              9dbca55
              396970d
              7a0e0ee
              71dce6c
              15e7294
              044fd90
              ef75e4d
              f9d9024
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -955,6 +955,13 @@ def request_setBreakpoints(self, file_path, line_array, data=None): | |
| """ | ||
| (dir, base) = os.path.split(file_path) | ||
| source_dict = {"name": base, "path": file_path} | ||
| return self.request_setBreakpoints_with_source(source_dict, line_array, data) | ||
| 
     | 
||
| def request_setBreakpointsAssembly(self, sourceReference, line_array, data=None): | ||
| source_dict = {"sourceReference": sourceReference} | ||
| return self.request_setBreakpoints_with_source(source_dict, line_array, data) | ||
| 
     | 
||
| def request_setBreakpoints_with_source(self, source_dict, line_array, data=None): | ||
                
       | 
||
| args_dict = { | ||
| "source": source_dict, | ||
| "sourceModified": False, | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| C_SOURCES := main.c | ||
| 
     | 
||
| include Makefile.rules | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| """ | ||
| Test lldb-dap setBreakpoints request | ||
                
       | 
||
| """ | ||
| 
     | 
||
| 
     | 
||
| import dap_server | ||
| import shutil | ||
| from lldbsuite.test.decorators import * | ||
| from lldbsuite.test.lldbtest import * | ||
| from lldbsuite.test import lldbutil | ||
| import lldbdap_testcase | ||
| import os | ||
| 
     | 
||
| 
     | 
||
| class TestDAP_setBreakpointsAssembly(lldbdap_testcase.DAPTestCaseBase): | ||
| # @skipIfWindows | ||
| def test_functionality(self): | ||
                
       | 
||
| """Tests hitting assembly source breakpoints""" | ||
| program = self.getBuildArtifact("a.out") | ||
| self.build_and_launch(program) | ||
| 
     | 
||
| self.dap_server.request_evaluate( | ||
| "`settings set stop-disassembly-display no-debuginfo", context="repl" | ||
| ) | ||
| 
     | 
||
| assmebly_func_breakpoints = self.set_function_breakpoints(["assembly_func"]) | ||
| self.continue_to_breakpoints(assmebly_func_breakpoints) | ||
| 
     | 
||
| assembly_func_frame = self.get_stackFrames()[0] | ||
| self.assertIn( | ||
| "sourceReference", | ||
| assembly_func_frame.get("source"), | ||
| "Expected assembly source frame", | ||
| ) | ||
| 
     | 
||
| line = assembly_func_frame["line"] | ||
| 
     | 
||
| # Set an assembly breakpoint in the next line and check that it's hit | ||
| assembly_breakpoint_ids = self.set_source_breakpoints_assembly( | ||
| assembly_func_frame["source"]["sourceReference"], [line + 1] | ||
| ) | ||
| self.continue_to_breakpoints(assembly_breakpoint_ids) | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| #include <stddef.h> | ||
                
      
                  eronnen marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||
| 
     | 
||
| __attribute__((nodebug)) int assembly_func(int n) { | ||
| n += 1; | ||
| n += 2; | ||
| n += 3; | ||
| 
     | 
||
| return n; | ||
| } | ||
| 
     | 
||
| int main(int argc, char const *argv[]) { | ||
| assembly_func(10); | ||
| return 0; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| 
          
            
          
           | 
    @@ -9,10 +9,12 @@ | |||||
| #include "Breakpoint.h" | ||||||
| #include "DAP.h" | ||||||
| #include "JSONUtils.h" | ||||||
| #include "LLDBUtils.h" | ||||||
| #include "lldb/API/SBAddress.h" | ||||||
| #include "lldb/API/SBBreakpointLocation.h" | ||||||
| #include "lldb/API/SBLineEntry.h" | ||||||
| #include "lldb/API/SBMutex.h" | ||||||
| #include "lldb/lldb-enumerations.h" | ||||||
| #include "llvm/ADT/StringExtras.h" | ||||||
| #include <cstddef> | ||||||
| #include <cstdint> | ||||||
| 
          
            
          
           | 
    @@ -63,14 +65,31 @@ protocol::Breakpoint Breakpoint::ToProtocolBreakpoint() { | |||||
| std::string formatted_addr = | ||||||
| "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(m_bp.GetTarget())); | ||||||
| breakpoint.instructionReference = formatted_addr; | ||||||
| 
     | 
||||||
| lldb::StopDisassemblyType stop_disassembly_display = | ||||||
| GetStopDisassemblyDisplay(m_dap.debugger); | ||||||
| auto line_entry = bp_addr.GetLineEntry(); | ||||||
| const auto line = line_entry.GetLine(); | ||||||
| if (line != UINT32_MAX) | ||||||
| breakpoint.line = line; | ||||||
| const auto column = line_entry.GetColumn(); | ||||||
| if (column != 0) | ||||||
| breakpoint.column = column; | ||||||
| breakpoint.source = CreateSource(line_entry); | ||||||
| if (!ShouldDisplayAssemblySource(line_entry, stop_disassembly_display)) { | ||||||
| const auto line = line_entry.GetLine(); | ||||||
| if (line != UINT32_MAX) | ||||||
| breakpoint.line = line; | ||||||
| const auto column = line_entry.GetColumn(); | ||||||
| if (column != 0) | ||||||
| breakpoint.column = column; | ||||||
| breakpoint.source = CreateSource(line_entry); | ||||||
| } else { | ||||||
| // Breakpoint made by assembly | ||||||
                
       | 
||||||
| // Breakpoint made by assembly | |
| // Assembly breakpoint. | 
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| 
          
            
          
           | 
    @@ -169,6 +169,8 @@ struct DAP { | |||||
| Variables variables; | ||||||
| lldb::SBBroadcaster broadcaster; | ||||||
| llvm::StringMap<SourceBreakpointMap> source_breakpoints; | ||||||
| llvm::DenseMap<int64_t, llvm::DenseMap<uint32_t, SourceBreakpoint>> | ||||||
| assembly_breakpoints; | ||||||
| FunctionBreakpointMap function_breakpoints; | ||||||
| InstructionBreakpointMap instruction_breakpoints; | ||||||
| std::optional<std::vector<ExceptionBreakpoint>> exception_breakpoints; | ||||||
| 
          
            
          
           | 
    @@ -219,6 +221,9 @@ struct DAP { | |||||
| llvm::StringSet<> modules; | ||||||
| /// @} | ||||||
| 
     | 
||||||
| /// Number of lines of assembly code to show when no debug info is available. | ||||||
| uint32_t number_of_assembly_lines_for_nodebug = 32; | ||||||
                
       | 
||||||
| uint32_t number_of_assembly_lines_for_nodebug = 32; | |
| static constexpr uint32_t number_of_assembly_lines_for_nodebug = 32; | 
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.
💯
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 
          
            
          
           | 
    @@ -7,7 +7,6 @@ | |||||||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||||||||
| 
     | 
||||||||||||||||||
| #include "DAP.h" | ||||||||||||||||||
| #include "JSONUtils.h" | ||||||||||||||||||
| #include "RequestHandler.h" | ||||||||||||||||||
| #include <vector> | ||||||||||||||||||
| 
     | 
||||||||||||||||||
| 
        
          
        
         | 
    @@ -19,19 +18,50 @@ namespace lldb_dap { | |||||||||||||||||
| llvm::Expected<protocol::BreakpointLocationsResponseBody> | ||||||||||||||||||
| BreakpointLocationsRequestHandler::Run( | ||||||||||||||||||
| const protocol::BreakpointLocationsArguments &args) const { | ||||||||||||||||||
| std::string path = args.source.path.value_or(""); | ||||||||||||||||||
| uint32_t start_line = args.line; | ||||||||||||||||||
| uint32_t start_column = args.column.value_or(LLDB_INVALID_COLUMN_NUMBER); | ||||||||||||||||||
| uint32_t end_line = args.endLine.value_or(start_line); | ||||||||||||||||||
| uint32_t end_column = | ||||||||||||||||||
| args.endColumn.value_or(std::numeric_limits<uint32_t>::max()); | ||||||||||||||||||
| 
     | 
||||||||||||||||||
| // Find all relevant lines & columns | ||||||||||||||||||
                
       | 
||||||||||||||||||
| // Find all relevant lines & columns | |
| // Find all relevant lines & columns. | 
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.
💯
        
          
              
                Outdated
          
        
      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 think you should be able to create the locations in place like this:
| for (auto &l : locations) { | |
| protocol::BreakpointLocation lc; | |
| lc.line = l.first; | |
| lc.column = l.second; | |
| breakpoint_locations.push_back(std::move(lc)); | |
| } | |
| for (auto &l : locations) | |
| breakpoint_locations.emplace_back({l.first, l.second}); | 
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.
For some reason got no matching constructor for initialization, but the following works
breakpoint_locations.push_back({l.first, l.second, std::nullopt, std::nullopt});        
          
              
                Outdated
          
        
      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.
You can avoid the template by taking a reference to a SmallVectorImpl<std::pair<uint32_t, uint32_t>>, which SmallVector inherits from.
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.
Looking at how this is called, maybe you don't even need to take this by reference, and you can have this return a llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8>. Honestly I don't even know if the SmallVector optimization is worth it here, personally I would've just returned a regular std::vector. If you change the return type, you can also change the name of the method from Add to Get. Now it looks like there's potentially multiple calls adding values to the location list.
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.
Agreem chganged to return std::vector
        
          
              
                  da-viper marked this conversation as resolved.
              
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
              
                  da-viper marked this conversation as resolved.
              
          
            Show resolved
            Hide resolved
        
              
          
              
                Outdated
          
        
      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.
| // start_line is relative to the symbol's start address | |
| // start_line is relative to the symbol's start address. | 
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 have mixed feelings about adding this API. Generally, we usually use streams when we have potentially multiline output which isn't the case. Here I'm also worried about setting a precedent that will lead to having more methods overloaded with streams. If this were private API I wouldn't mind, but since we guarantee ABI stability for the SB API, once an API has been added, we have to support it forever.
How about adding a helper to LLDBUtils that takes a
SBFileSpecand returns the path as a std::string?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.
makes sense, I was trying to avoid the potential problem of the path being longer than
MAX_PATHbut it's still possible to get the real size usingGetFilenameandGetDirectoryThere 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.
moved to LLDBUtils