[Lldb-commits] [PATCH] D140368: [lldb] Consider all breakpoints in breakpoint detection

[Pavel Kosov via Phabricator via lldb-commits]

kpdev42 added a comment.

In D140368#4007234 , @DavidSpickett 
wrote:

> The intent makes sense. We should stop and report user breakpoints triggered 
> while trying to execute some internal stepping plan, even if they overlap 
> what lldb was planning to do in the first place.
>
> Not totally sure how the change achieves that, this is quite the function. + 
> @jingham who wrote the original changes.
>
>> Currently in some cases lldb reports stop reason as "step out" or "step 
>> over" (from thread plan completion) over "breakpoint"
>
> This would be clearer if you said "(from thread plan completion) instead of 
> "breakpoint"". Took me a while to work out that it wasn't over meaning step 
> over a breakpoint.
>
> I think the test naming could be clearer. 
> `breakpoint/step_out_breakpoint/TestStepOutBreakpoint.py` implies it's just 
> about stepping out. How about 
> `breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py` ? 
> Something that is clear we're testing the interaction of automatic internal 
> stepping plans and breakpoints the user puts in.
>
> Is it worth checking that an unconditional user breakpoint is also reported?

Fixed, please take a look


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140368

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D140368: [lldb] Consider all breakpoints in breakpoint detection

[Pavel Kosov via Phabricator via lldb-commits]

kpdev42 updated this revision to Diff 484745.
kpdev42 added a comment.

Renamed the test, added more tests for unconditional (enabled/disabled) 
breakpoints and breakpoints with callbacks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140368

Files:
  lldb/source/Target/StopInfo.cpp
  lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/Makefile
  
lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
  lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/main.cpp

Index: lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/main.cpp
@@ -0,0 +1,11 @@
+int func_1() { return 1; }
+
+int func_2() {
+  func_1(); // breakpoint_1
+  return 1 + func_1();  // breakpoint_2
+}
+
+int main(int argc, char const *argv[]) {
+  func_2();  // breakpoint_0
+  return 0;  // breakpoint_3
+}
Index: lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
@@ -0,0 +1,124 @@
+"""
+Test that breakpoints (reason = breakpoint) have more priority than
+plan completion (reason = step in/out/over) when reporting stop reason after step,
+in particular 'step out' and 'step over', and in addition 'step in'.
+Check for correct StopReason when stepping to the line with breakpoint,
+which should be eStopReasonBreakpoint in general,
+and eStopReasonPlanComplete when breakpoint's condition fails or it is disabled.
+"""
+
+
+import unittest2
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class ThreadPlanUserBreakpointsTestCase(TestBase):
+
+def setUp(self):
+TestBase.setUp(self)
+
+self.build()
+exe = self.getBuildArtifact("a.out")
+src = lldb.SBFileSpec("main.cpp")
+
+# Create a target by the debugger.
+self.target = self.dbg.CreateTarget(exe)
+self.assertTrue(self.target, VALID_TARGET)
+
+# Setup four breakpoints
+self.lines = [line_number('main.cpp', "breakpoint_%i" % i) for i in range(4)]
+
+self.breakpoints = [self.target.BreakpointCreateByLocation(src, line) for line in self.lines]
+self.assertTrue(
+self.breakpoints[0] and self.breakpoints[0].GetNumLocations() == 1,
+VALID_BREAKPOINT)
+
+# Start debugging
+self.process = self.target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertIsNotNone(self.process, PROCESS_IS_VALID)
+self.thread = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, self.breakpoints[0])
+self.assertIsNotNone(self.thread, "Didn't stop at breakpoint 0.")
+
+def check_correct_stop_reason(self, breakpoint_idx, condition):
+self.assertState(self.process.GetState(), lldb.eStateStopped)
+if condition:
+# All breakpoints active, stop reason is breakpoint
+thread1 = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, self.breakpoints[breakpoint_idx])
+self.assertEquals(self.thread, thread1, "Didn't stop at breakpoint %i." % breakpoint_idx)
+else:
+# Breakpoints are inactive, stop reason is plan complete
+self.assertEquals(self.thread.GetStopReason(), lldb.eStopReasonPlanComplete,
+"Expected stop reason to be step into/over/out for inactive breakpoint %i line." % breakpoint_idx)
+
+def check_thread_plan_user_breakpoint(self, condition, set_up_breakpoints_func):
+# Make breakpoints active/inactive in different ways
+set_up_breakpoints_func(condition)
+
+self.thread.StepInto()
+# We should be stopped at the breakpoint_1 line with the correct stop reason
+self.check_correct_stop_reason(1, condition)
+
+# This step-over creates a step-out from `func_1` plan
+self.thread.StepOver()
+# We should be stopped at the breakpoint_2 line with the correct stop reason
+self.check_correct_stop_reason(2, condition)
+
+# Check explicit step-out
+self.thread.StepOut()
+# We should be stopped at the breakpoint_3 line with the correct stop reason
+self.check_correct_stop_reason(3, condition)
+
+# Run the process until termination
+self.process.Continue()
+self.assertState(self.process.GetState(), lldb.eStateExited)
+
+def change_breakpoints(self, action):
+action(self.breakpoints[1])
+action(self.breakpoints[2])
+

[Lldb-commits] [PATCH] D138344: [test][lldb-vscode] Relax assertion to allow multiple compile units returned.

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a comment.

ping :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138344

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137838: [Support] Move TargetParsers to new component

[Sam Elliott via Phabricator via lldb-commits]

lenary added a comment.

In D137838#4010987 , @dblaikie wrote:

> This has introduced a circular dependency due to the forwarding header 
> (forwarding header depends on new lib, new lib depends on support, where the 
> forwarding header is). Generally this wouldn't be acceptable (& I'd suggest 
> the patch be reverted on that basis) though I understand this is a 
> complicated migration - what's the timeline for correcting this issue?

A commit was landed to add TargetParser to the module map so it looks like part 
of Support, which solves the circular dependency issue in the short term, I 
believe.

https://reviews.llvm.org/D140420 is a proposed fix which should more clearly 
split the TargetParser from Support, but I'm not super familiar with modules 
and my linux dev environment does not work with a modules build. I'd appreciate 
any comments or guidance though I also know it needs an update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is looking much better -- and focused. I still have comments though. :)




Comment at: lldb/include/lldb/Core/Module.h:833
 
-  void LogMessageVerboseBacktrace(Log *log, const char *format, ...)
-  __attribute__((format(printf, 3, 4)));
+  void LogMessageVerboseBacktrace(Log *log, const char *format, ...);
 

Could you do this one as well? It only has one caller and it would be strange 
for it to use a different syntax than LogMessage



Comment at: lldb/include/lldb/Core/Module.h:837
+  void ReportWarning(const char *format, Args &&...args) {
+if (format && format[0])
+  ReportWarning(llvm::formatv(format, std::forward(args)...));

Let's remove this defensive nonsense while we're in here. All of the callers 
are in this patch, and none of them is passing an empty string or a null 
pointer.



Comment at: lldb/include/lldb/Utility/Status.h:65
+  template 
+  explicit Status(const char *format, Args &&...args) {
+SetErrorToGenericError();

I don't think you've converted all the callers of this one. Given it's 
pervasiveness, I think we will need to do some sort of a staged conversion, to 
ensure call sites get compile errors instead of silent breakage. For now I 
think it would be fine to have a "named constructor" using formatv (`static 
Status Status::WithFormat(fmt, args...)`). Or just leave it out...



Comment at: lldb/source/Core/Module.cpp:1157
   m_first_file_changed_log = true;
-  if (format) {
-StreamString strm;
-strm.PutCString("the object file ");
-GetDescription(strm.AsRawOstream(), lldb::eDescriptionLevelFull);
-strm.PutCString(" has been modified\n");
-
-va_list args;
-va_start(args, format);
-strm.PrintfVarArg(format, args);
-va_end(args);
-
-const int format_len = strlen(format);
-if (format_len > 0) {
-  const char last_char = format[format_len - 1];
-  if (last_char != '\n' && last_char != '\r')
-strm.EOL();
-}
-strm.PutCString("The debug session should be aborted as the original "
-"debug information has been overwritten.");
-Debugger::ReportError(std::string(strm.GetString()));
-  }
+  m_first_file_changed_log = true;
+  StreamString strm;

duplicated line



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:21-31
+  s->Printf(
+  "%s",
+  std::string(
+  llvm::formatv(
+  "{0:x+16}: Compile Unit: length = {1:x+8}, version = {2:x}, "
+  "abbr_offset = {3:x+8}, addr_size = {4:x+2} (next CU at "
+  "[{5:x+16}])\n",





Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:197
   "attach the file at the start of this error message",
-  m_offset, (unsigned)form);
+  (uint64_t)m_offset, (unsigned)form);
   *offset_ptr = m_offset;

this shouldn't be necessary



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.cpp:18-27
+  s->Printf("%s",
+std::string(
+llvm::formatv(
+"{0:x+16}: Type Unit: length = {1:x+8}, version = {2:x+4}, 
"
+"abbr_offset = {3:x+8}, addr_size = {4:x+2} (next CU at "
+"[{5:x+16}])\n",
+GetOffset(), (uint32_t)GetLength(), GetVersion(),

same here



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:585-586
+  "DW_AT_rnglists_base for CU at {0:x+16}",
+  GetOffset()))
+.c_str());
   if (std::optional off = GetRnglistTable()->getOffsetEntry(

This should be enough (also, I think doing a .str() on the format result would 
look nicer than a std::string constructor)



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3031
 log,
+
 "SymbolFileDWARF::"

delete empty line



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:11-58
+#include "AppleDWARFIndex.h"
+#include "DWARFASTParser.h"
+#include "DWARFASTParserClang.h"
+#include "DWARFCompileUnit.h"
+#include "DWARFDebugAbbrev.h"
+#include "DWARFDebugAranges.h"
+#include "DWARFDebugInfo.h"

ayermolo wrote:
> JDevlieghere wrote:
> > Unrelated change?
> This was recommended in another diff.  Although it was related to DIERef.h. I 
> can move it to the second diff, or it's own diff?
Yeah, this looks messy if the clang-format layout is significantly different 
than the original. Maybe you shouldn't do it here -- or do it as a separate 
patch (but do keep it in mind for future, particularly when 

[Lldb-commits] [PATCH] D139945: [lldb] Add scripted process launch/attach option to platform commands

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D139945#4009427 , @JDevlieghere 
wrote:

> In D139945#3999351 , @labath wrote:
>
>> For a "plugin", the scripted process is sure getting a lot of special 
>> handling in generic code. (I know this isn't being introduced here, but I 
>> wasn't involved in the previous review -- and I'm not actually sure I want 
>> to be involved here). I don't think that's necessarily a bad thing in this 
>> case, but maybe we should not be calling it a plugin in that case? We 
>> already have a couple of precedents for putting implementations of 
>> "pluggable" classes into generic code -- ProcessTrace for instance. And just 
>> like in the case of ProcessTrace (where the real plugin is the thing which 
>> handles the trace file format), here the real plugin would the the scripting 
>> language backing the scripted process?
>>
>> (Apart from that, this patch seems fine.)
>
> Maybe one way around this is to have some generic metadata that gets passed 
> to the plugin, that can be different per plugin?

That might work, but I don't think it's really necessary. My only issue is one 
of nomenclature. I don't see a problem with having a concrete Process subclass 
in the lldb core. My problem is with calling that class a "plugin", and 
pretending it's pluggable, but then introducing backdoor dependencies to it 
(usually by referring to with via its (string) plugin name). You could try to 
solve that by making it genuinely pluggable, but that could be overkill, given 
that it already contains *is* pluggable, only in a different way -- one can 
plugin a different scripting language to implement the bindings (and then 
obviously the user can plug in to implement those bindings).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139945

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139250: [lldb] Add ScriptedPlatform python implementation

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/examples/python/scripted_process/scripted_platform.py:31
+def list_processes(self):
+""" Get a list of processes that can be ran on the platform.
+

mib wrote:
> labath wrote:
> > mib wrote:
> > > labath wrote:
> > > > mib wrote:
> > > > > mib wrote:
> > > > > > mib wrote:
> > > > > > > labath wrote:
> > > > > > > > I am surprised that you want to go down the "run" path for this 
> > > > > > > > functionality. I think most of the launch functionality does 
> > > > > > > > not make sense for this use case (e.g., you can't provide 
> > > > > > > > arguments to these processes, when you "run" them, can you?), 
> > > > > > > > and it is not consistent with what the "process listing" 
> > > > > > > > functionality does for regular platforms.
> > > > > > > > 
> > > > > > > > OTOH, the "attach" flow makes perfect sense here -- you take 
> > > > > > > > the pid of an existing process, attach to it, and stop it at a 
> > > > > > > > random point in its execution. You can't customize anything 
> > > > > > > > about how that process is run (because it's already running) -- 
> > > > > > > > all you can do is choose how you want to select the target 
> > > > > > > > process.
> > > > > > > For now, there is no support for attaching to a scripted process, 
> > > > > > > because we didn't have any use for it quite yet: cripted 
> > > > > > > processes were mostly used for doing post-mortem debugging, so we 
> > > > > > > "ran" them artificially in lldb by providing some launch options 
> > > > > > > (the name of the class managing the process and an optional 
> > > > > > > user-provided dictionary) through the command line or using an 
> > > > > > > `SBLaunchInfo` object.
> > > > > > > 
> > > > > > > I guess I'll need to extend the `platform process launch/attach` 
> > > > > > > commands and `SBAttachInfo` object to also support these options 
> > > > > > > since they're required for the scripted process instantiation.
> > > > > > > 
> > > > > > > Note that we aren't really attaching to the real running process, 
> > > > > > > we're creating a scripted process that knows how to read memory 
> > > > > > > to mock the real process.
> > > > > > @labath, I'll do that work on a follow-up patch
> > > > > @labath here D139945 :) 
> > > > Thanks. However, are you still planning to use the launch path for your 
> > > > feature? Because if you're not, then I think this comment should say 
> > > > "Get a list of processes that **are running**" (or that **can be 
> > > > attached to**).
> > > > 
> > > > And if you are, then I'd like to hear your thoughts on the discrepancy 
> > > > between what "launching" means for scripted and non-scripted platforms.
> > > > 
> > > The way I see it is that the scripted platform will create a process with 
> > > the right process plugin. In the case of scripted processes, the 
> > > `ProcessLaunchInfo` argument should have the script class name set (which 
> > > automatically sets the process plugin name to "ScriptedProcess" in the 
> > > launch info). Once the process is instantiated (before the launch), the 
> > > scripted platform will need to redirect to process stop events through 
> > > its event multiplexer. So the way I see it essentially, running a regular 
> > > process with the scripted platform should be totally transparent.
> > > 
> > > Something that is also worth discussing IMO, is the discrepancy between 
> > > launching and attaching from the scripted platform:
> > > 
> > > One possibility could be that `platform process launch` would launch all 
> > > the scripted processes listed by the scripted platform and set them up 
> > > with the multiplexer, whereas `platform process attach` would just create 
> > > a scripted process individually. I know this doesn't match the current 
> > > behavior of the platform commands so if you guys think we should preserve 
> > > the expected behavior, I guess.
> > > 
> > > May be @jingham has some opinion about this ?
> > Before we do that, maybe we could take a step back. Could you explain why 
> > you chose to use the "launch" flow for this use case?
> > 
> > To me, it just seems confusing to be using "launching" for any of this, 
> > particularly given that "attaching" looks like a much better match for what 
> > is happening here:
> > - launch allows you to specify process cmdline arguments, attach does not - 
> > I don't think you will be able to specify cmdline arguments for these 
> > scripted processes
> > - launch allows you to specify env vars, attach does not -- ditto
> > - launch allows you to stop-at-entry, attach does not -- you cannot stop at 
> > entry for these processes, as they have been started already
> > - attach allows you to specify a pid, launch does not -- you (I think) want 
> > to be able to choose the process (pid) that you want to create a scripted 
> > process for
> > 
> > For me, the choice is obvious, particularly 

[Lldb-commits] [PATCH] D137838: [Support] Move TargetParsers to new component

[Francesco Petrogalli via Phabricator via lldb-commits]

fpetrogalli added a comment.

In D137838#4010987 , @dblaikie wrote:

> This has introduced a circular dependency due to the forwarding header 
> (forwarding header depends on new lib, new lib depends on support, where the 
> forwarding header is). Generally this wouldn't be acceptable (& I'd suggest 
> the patch be reverted on that basis) though I understand this is a 
> complicated migration - what's the timeline for correcting this issue?

IIUC, the fix here would be to remove the forwarding headers, right?

Francesco


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137838: [Support] Move TargetParsers to new component

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

This has introduced a circular dependency due to the forwarding header 
(forwarding header depends on new lib, new lib depends on support, where the 
forwarding header is). Generally this wouldn't be acceptable (& I'd suggest the 
patch be reverted on that basis) though I understand this is a complicated 
migration - what's the timeline for correcting this issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D140358: [lldb-vscode] Add support for disassembly view

[Enrico Loparco via Phabricator via lldb-commits]

eloparco added a comment.

@JDevlieghere let me know if there's anything I can do to make the review easier


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140358

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D140358: [lldb-vscode] Add support for disassembly view

[Enrico Loparco via Phabricator via lldb-commits]

eloparco updated this revision to Diff 484579.
eloparco added a comment.

Avoid tracking stack frame boundaries. It is unnecessary and makes the 
disassembler functionality not work in multi-threaded programs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140358

Files:
  lldb/include/lldb/API/SBTarget.h
  lldb/source/API/SBTarget.cpp
  lldb/tools/lldb-vscode/CMakeLists.txt
  lldb/tools/lldb-vscode/DisassembledInstruction.cpp
  lldb/tools/lldb-vscode/DisassembledInstruction.h
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/JSONUtils.h
  lldb/tools/lldb-vscode/LLDBUtils.cpp
  lldb/tools/lldb-vscode/LLDBUtils.h
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/VSCodeForward.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1451,8 +1451,7 @@
   // which may affect the outcome of tests.
   bool source_init_file = GetBoolean(arguments, "sourceInitFile", true);
 
-  g_vsc.debugger =
-  lldb::SBDebugger::Create(source_init_file, log_cb, nullptr);
+  g_vsc.debugger = lldb::SBDebugger::Create(source_init_file, log_cb, nullptr);
   g_vsc.progress_event_thread = std::thread(ProgressEventThreadFunction);
 
   // Start our event thread so we can receive events from the debugger, target,
@@ -1542,6 +1541,8 @@
   // The debug adapter supports 'logMessage' in breakpoint.
   body.try_emplace("supportsLogPoints", true);
 
+  body.try_emplace("supportsDisassembleRequest", true);
+
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
@@ -2117,6 +2118,162 @@
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
+std::vector
+_get_instructions_from_memory(lldb::addr_t start, uint64_t count) {
+  lldb::SBProcess process = g_vsc.target.GetProcess();
+
+  lldb::SBError error;
+  std::vector buffer(count, 0);
+  const size_t bytes_read __attribute__((unused)) = process.ReadMemory(
+  start, static_cast(buffer.data()), count, error);
+  assert(bytes_read == count && error.Success() &&
+ "unable to read byte range from memory");
+
+  // If base_addr starts in the middle of an instruction,
+  // that first instruction will not be parsed correctly (negligible)
+  std::vector sb_instructions;
+  const auto base_addr = lldb::SBAddress(start, g_vsc.target);
+  lldb::SBInstructionList instructions =
+  g_vsc.target.GetInstructions(base_addr, buffer.data(), count);
+
+  for (size_t i = 0; i < instructions.GetSize(); i++) {
+auto instr = instructions.GetInstructionAtIndex(i);
+sb_instructions.emplace_back(instr);
+  }
+  return sb_instructions;
+}
+
+auto _handle_disassemble_positive_offset(lldb::addr_t base_addr,
+ int64_t instruction_offset,
+ uint64_t instruction_count) {
+  llvm::json::Array response_instructions;
+
+  auto start_addr =
+  lldb::SBAddress(base_addr + instruction_offset, g_vsc.target);
+  lldb::SBInstructionList instructions = g_vsc.target.ReadInstructions(
+  start_addr, instruction_offset + instruction_count);
+
+  std::vector dis_instructions;
+  const auto num_instrs_to_skip = static_cast(instruction_offset);
+  for (size_t i = num_instrs_to_skip; i < instructions.GetSize(); ++i) {
+lldb::SBInstruction instr = instructions.GetInstructionAtIndex(i);
+
+auto disass_instr =
+CreateDisassembledInstruction(DisassembledInstruction(instr));
+response_instructions.emplace_back(std::move(disass_instr));
+  }
+
+  return response_instructions;
+}
+
+auto _handle_disassemble_negative_offset(
+lldb::addr_t base_addr, int64_t instruction_offset,
+uint64_t instruction_count,
+std::optional memory_reference) {
+  llvm::json::Array response_instructions;
+
+  const auto bytes_per_instruction = g_vsc.target.GetMaximumOpcodeByteSize();
+  const auto bytes_offset = -instruction_offset * bytes_per_instruction;
+  auto start_addr = base_addr - bytes_offset;
+  const auto disassemble_bytes = instruction_count * bytes_per_instruction;
+
+  auto sb_instructions =
+  _get_instructions_from_memory(start_addr, disassemble_bytes);
+
+  // Find position of requested instruction
+  // in retrieved disassembled instructions
+  auto index = sb_instructions.size() + 1;
+  for (size_t i = 0; i < sb_instructions.size(); i++) {
+if (sb_instructions[i].GetAddress().GetLoadAddress(g_vsc.target) ==
+base_addr) {
+  index = i;
+  break;
+}
+  }
+  if (index == sb_instructions.size() + 1) {
+fprintf(stderr, "current line not found in disassembled instructions\n");
+return response_instructions;
+  }
+
+  // Copy instructions into queue to easily manipulate them
+  std::deque disass_instructions;
+  for 

[Lldb-commits] [PATCH] D139248: [lldb/Interpreter] Improve ScriptedPythonInterface::GetStatusFromMethod

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 484527.
mib marked an inline comment as done.
mib added a comment.

Use `std::forward`


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

https://reviews.llvm.org/D139248

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h


Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
@@ -99,7 +99,13 @@
 return ExtractValueFromPythonObject(py_return, error);
   }
 
-  Status GetStatusFromMethod(llvm::StringRef method_name);
+  template 
+  Status GetStatusFromMethod(llvm::StringRef method_name, Args &&...args) {
+Status error;
+Dispatch(method_name, error, std::forward(args)...);
+
+return error;
+  }
 
   template  T Transform(T object) {
 // No Transformation for generic usage
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
@@ -26,14 +26,6 @@
 ScriptInterpreterPythonImpl )
 : ScriptedInterface(), m_interpreter(interpreter) {}
 
-Status
-ScriptedPythonInterface::GetStatusFromMethod(llvm::StringRef method_name) {
-  Status error;
-  Dispatch(method_name, error);
-
-  return error;
-}
-
 template <>
 StructuredData::ArraySP
 ScriptedPythonInterface::ExtractValueFromPythonObject(


Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
@@ -99,7 +99,13 @@
 return ExtractValueFromPythonObject(py_return, error);
   }
 
-  Status GetStatusFromMethod(llvm::StringRef method_name);
+  template 
+  Status GetStatusFromMethod(llvm::StringRef method_name, Args &&...args) {
+Status error;
+Dispatch(method_name, error, std::forward(args)...);
+
+return error;
+  }
 
   template  T Transform(T object) {
 // No Transformation for generic usage
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
@@ -26,14 +26,6 @@
 ScriptInterpreterPythonImpl )
 : ScriptedInterface(), m_interpreter(interpreter) {}
 
-Status
-ScriptedPythonInterface::GetStatusFromMethod(llvm::StringRef method_name) {
-  Status error;
-  Dispatch(method_name, error);
-
-  return error;
-}
-
 template <>
 StructuredData::ArraySP
 ScriptedPythonInterface::ExtractValueFromPythonObject(
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits