[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Ed Maste via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292937: FreeBSD ARM support for software single step 
(authored by emaste).

Changed prior to commit:
  https://reviews.llvm.org/D25756?vs=79987=85581#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25756

Files:
  lldb/trunk/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
  lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
  lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
  lldb/trunk/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

Index: lldb/trunk/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
===
--- lldb/trunk/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
+++ lldb/trunk/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
@@ -1141,11 +1141,19 @@
 
   case SI_KERNEL:
   case TRAP_BRKPT:
-if (log)
-  log->Printf(
-  "ProcessMonitor::%s() received breakpoint event, tid = %" PRIu64,
-  __FUNCTION__, tid);
-message = ProcessMessage::Break(tid);
+if (monitor->m_process->IsSoftwareStepBreakpoint(tid)) {
+  if (log)
+log->Printf("ProcessMonitor::%s() received sw single step breakpoint "
+"event, tid = %" PRIu64,
+__FUNCTION__, tid);
+  message = ProcessMessage::Trace(tid);
+} else {
+  if (log)
+log->Printf(
+"ProcessMonitor::%s() received breakpoint event, tid = %" PRIu64,
+__FUNCTION__, tid);
+  message = ProcessMessage::Break(tid);
+}
 break;
   }
 
Index: lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
===
--- lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
+++ lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
@@ -171,7 +171,25 @@
   virtual FreeBSDThread *CreateNewFreeBSDThread(lldb_private::Process ,
 lldb::tid_t tid);
 
+  static bool SingleStepBreakpointHit(
+  void *baton, lldb_private::StoppointCallbackContext *context,
+  lldb::user_id_t break_id, lldb::user_id_t break_loc_id);
+
+  lldb_private::Error SetupSoftwareSingleStepping(lldb::tid_t tid);
+
+  lldb_private::Error SetSoftwareSingleStepBreakpoint(lldb::tid_t tid,
+  lldb::addr_t addr);
+
+  bool IsSoftwareStepBreakpoint(lldb::tid_t tid);
+
+  bool SupportHardwareSingleStepping() const;
+
+  typedef std::vector tid_collection;
+  tid_collection () { return m_step_tids; }
+
 protected:
+  static const size_t MAX_TRAP_OPCODE_SIZE = 8;
+
   /// Target byte order.
   lldb::ByteOrder m_byte_order;
 
@@ -207,10 +225,10 @@
 
   friend class FreeBSDThread;
 
-  typedef std::vector tid_collection;
   tid_collection m_suspend_tids;
   tid_collection m_run_tids;
   tid_collection m_step_tids;
+  std::map m_threads_stepping_with_breakpoint;
 
   int m_resume_signo;
 };
Index: lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
===
--- lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
+++ lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
@@ -13,6 +13,7 @@
 
 // C++ Includes
 #include 
+#include 
 
 // Other libraries and framework includes
 #include "lldb/Core/PluginManager.h"
@@ -122,6 +123,7 @@
 
   std::lock_guard guard(m_thread_list.GetMutex());
   bool do_step = false;
+  bool software_single_step = !SupportHardwareSingleStepping();
 
   for (tid_collection::const_iterator t_pos = m_run_tids.begin(),
   t_end = m_run_tids.end();
@@ -133,6 +135,11 @@
t_pos != t_end; ++t_pos) {
 m_monitor->ThreadSuspend(*t_pos, false);
 do_step = true;
+if (software_single_step) {
+  Error error = SetupSoftwareSingleStepping(*t_pos);
+  if (error.Fail())
+return error;
+}
   }
   for (tid_collection::const_iterator t_pos = m_suspend_tids.begin(),
   t_end = m_suspend_tids.end();
@@ -145,7 +152,7 @@
   if (log)
 log->Printf("process %" PRIu64 " resuming (%s)", GetID(),
 do_step ? "step" : "continue");
-  if (do_step)
+  if (do_step && !software_single_step)
 m_monitor->SingleStep(GetID(), m_resume_signo);
   else
 m_monitor->Resume(GetID(), m_resume_signo);
@@ -913,3 +920,194 @@
   "no platform or not the host - how did we get here with ProcessFreeBSD?");
   return DataBufferSP();
 }
+
+struct EmulatorBaton {
+  ProcessFreeBSD *m_process;
+  RegisterContext *m_reg_context;
+
+  // eRegisterKindDWARF -> RegisterValue
+  std::unordered_map m_register_values;
+
+  EmulatorBaton(ProcessFreeBSD *process, RegisterContext *reg_context)
+  : m_process(process), m_reg_context(reg_context) {}
+};
+
+static size_t ReadMemoryCallback(EmulateInstruction 

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Dmitry Mikulin via Phabricator via lldb-commits]

dmikulin added a comment.

@emaste , any chance to get this committed soon?


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Dmitry Mikulin via Phabricator via lldb-commits]

dmikulin updated this revision to Diff 79987.
dmikulin added a comment.

Addressed review comments.
Haven't had a chance to re-test it on an arm board.


Repository:
  rL LLVM

https://reviews.llvm.org/D25756

Files:
  source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
  source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
  source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
  source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

Index: source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
===
--- source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
+++ source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
@@ -1141,11 +1141,17 @@
 
   case SI_KERNEL:
   case TRAP_BRKPT:
-if (log)
-  log->Printf(
-  "ProcessMonitor::%s() received breakpoint event, tid = %" PRIu64,
-  __FUNCTION__, tid);
-message = ProcessMessage::Break(tid);
+if (monitor->m_process->IsSoftwareStepBreakpoint(tid)) {
+  if (log)
+log->Printf ("ProcessMonitor::%s() received sw single step breakpoint "
+ "event, tid = %" PRIu64, __FUNCTION__, tid);
+  message = ProcessMessage::Trace(tid);
+} else {
+  if (log)
+log->Printf ("ProcessMonitor::%s() received breakpoint event, tid = %"
+ PRIu64, __FUNCTION__, tid);
+  message = ProcessMessage::Break(tid);
+}
 break;
   }
 
Index: source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
===
--- source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
+++ source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
@@ -171,7 +171,27 @@
   virtual FreeBSDThread *CreateNewFreeBSDThread(lldb_private::Process ,
 lldb::tid_t tid);
 
-protected:
+  static bool
+  SingleStepBreakpointHit(void *baton,
+			  lldb_private::StoppointCallbackContext *context,
+			  lldb::user_id_t break_id,
+			  lldb::user_id_t break_loc_id);
+
+  lldb_private::Error SetupSoftwareSingleStepping(lldb::tid_t tid);
+
+  lldb_private::Error
+  SetSoftwareSingleStepBreakpoint (lldb::tid_t tid, lldb::addr_t addr);
+
+  bool IsSoftwareStepBreakpoint(lldb::tid_t tid);
+
+  bool SupportHardwareSingleStepping() const;
+
+  typedef std::vector tid_collection;
+  tid_collection () { return m_step_tids; }
+
+ protected:
+  static const size_t MAX_TRAP_OPCODE_SIZE = 8;
+
   /// Target byte order.
   lldb::ByteOrder m_byte_order;
 
@@ -207,10 +227,10 @@
 
   friend class FreeBSDThread;
 
-  typedef std::vector tid_collection;
   tid_collection m_suspend_tids;
   tid_collection m_run_tids;
   tid_collection m_step_tids;
+  std::map m_threads_stepping_with_breakpoint;
 
   int m_resume_signo;
 };
Index: source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
===
--- source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
+++ source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
@@ -13,6 +13,7 @@
 
 // C++ Includes
 #include 
+#include 
 
 // Other libraries and framework includes
 #include "lldb/Core/PluginManager.h"
@@ -122,6 +123,7 @@
 
   std::lock_guard guard(m_thread_list.GetMutex());
   bool do_step = false;
+  bool software_single_step = !SupportHardwareSingleStepping();
 
   for (tid_collection::const_iterator t_pos = m_run_tids.begin(),
   t_end = m_run_tids.end();
@@ -133,6 +135,11 @@
t_pos != t_end; ++t_pos) {
 m_monitor->ThreadSuspend(*t_pos, false);
 do_step = true;
+if (software_single_step) {
+  Error error = SetupSoftwareSingleStepping(*t_pos);
+  if (error.Fail())
+return error;
+}
   }
   for (tid_collection::const_iterator t_pos = m_suspend_tids.begin(),
   t_end = m_suspend_tids.end();
@@ -145,7 +152,7 @@
   if (log)
 log->Printf("process %" PRIu64 " resuming (%s)", GetID(),
 do_step ? "step" : "continue");
-  if (do_step)
+  if (do_step && !software_single_step)
 m_monitor->SingleStep(GetID(), m_resume_signo);
   else
 m_monitor->Resume(GetID(), m_resume_signo);
@@ -913,3 +920,194 @@
   "no platform or not the host - how did we get here with ProcessFreeBSD?");
   return DataBufferSP();
 }
+
+struct EmulatorBaton {
+  ProcessFreeBSD *m_process;
+  RegisterContext *m_reg_context;
+
+  // eRegisterKindDWARF -> RegisterValue
+  std::unordered_map m_register_values;
+
+  EmulatorBaton(ProcessFreeBSD *process, RegisterContext *reg_context)
+  : m_process(process), m_reg_context(reg_context) {}
+};
+
+static size_t ReadMemoryCallback(EmulateInstruction *instruction, void *baton,
+ const EmulateInstruction::Context ,
+ lldb::addr_t addr, void *dst, size_t length) {
+  EmulatorBaton *emulator_baton = static_cast(baton);
+
+  Error error;
+  size_t bytes_read =
+  

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Ed Maste via Phabricator via lldb-commits]

emaste added inline comments.



Comment at: source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:551-557
+} else {
+  static const uint8_t g_arm_breakpoint_opcode[] = {0xFE,0xDE,0xFF,0xE7};
+  size_t trap_opcode_size = sizeof(g_arm_breakpoint_opcode);
+  assert(bp_site);
+  if (bp_site->SetTrapOpcode(g_arm_breakpoint_opcode, trap_opcode_size))
+return trap_opcode_size;
 }

according to LLVM style this should be promoted out of an `else` block because 
of the `return` above (use early exits / don't use else after a return).

As an aside it appears 0xe7fddefe is ARM-recommended breakpoint opcode and 
Linux is the outlier. So the generic Platform::GetSoftwareBreakpointTrapOpcode 
ought to use 0xFE,0xDE,0xFF,0xE7, with 
PlatformLinux::GetSoftwareBreakpointTrapOpcode having the override?




Comment at: source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:957
+
+  // The emulator only fill in the dwarf regsiter numbers (and in some case
+  // the generic register numbers). Get the full register info from the

Few typo / nits:

only fill**s** in
s/regsiter/register/
in some case**s**



Comment at: source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:1063
+  if (emulator_ap == nullptr) {
+printf("Error: Instruction emulator not found!\n");
+return;

We should really be returning an `Error` from this fn instead, no?



Comment at: source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:1101-1104
+// Emulate instruction failed and it haven't changed PC. Advance PC
+// with the size of the current opcode because the emulation of all
+// PC modifying instruction should be successful. The failure most
+// likely caused by a not supported instruction which don't modify PC.

Hope you don't mind a minor rewording of the comment:

The emulated instruction failed and it did not change the PC. Advance the PC by 
the size of the current opcode, as all PC-modifying instructions should be 
successfully emulated. The failure was most likely caused by an unsupported 
instruction.


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Kamil Rytarowski via lldb-commits]

krytarowski added a comment.

I just scrolled the discussion so it's not really started. Thanks.

My plan is to work on ptrace(2) for about a month.

Then I will take the FreeBSD code as it is and port to NetBSD trying to make it 
functional. This is difficult part as there is everywhere OS specific 
interfaces and behavior. I hope to get it to some usable point after a month.

Then ,I will start working on remote debugging, as my previous patch for 
process tracing (it was buggy) was rejected without this property. I planned to 
merge my previous work with master as I was forced to keep catching up after 
the project changes and it was time expensive, and move my focus on proper 
platform support.

So far all my efforts were voluntary and NetBSD build bot is paid from my 
private resources. Now I will be able to spend on in full-time about 4 months.

So to make it short, hopefully I can join you in remote capabilities after New 
Year (according to my estimations).


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Kamil Rytarowski via lldb-commits]

krytarowski added a comment.

NetBSD is still focused on amd64-only port.

I start a funded project November 1st on getting x86 process plugin to work. 
More details here:

http://blog.netbsd.org/tnf/entry/funded_contract_2016_2017

Estimated time is 4 months full-time.

However there every help is appreciated and donations to TNF (The NetBSD 
Foundation) make it possible.


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Ed Maste via lldb-commits]

emaste added a subscriber: krytarowski.
emaste added a comment.

On a quick look this seems OK. I'll try to test/review in detail.

In https://reviews.llvm.org/D25756#576642, @dmikulin wrote:

> Thanks Pavel! I'll start working on it. Do you know when lldb-server Linux 
> changes were committed? I want to use those patches as a template, but it's 
> hard to locate them digging through thousands of commit log entries...


I recalled discussing this when the Linux changes were going in and wanted to 
point you at a mailing list archive, but it seems it was only in private mail. 
I'll try to send you the relevant threads.

Have a look at https://reviews.llvm.org/rL212069 for starters.

> Ed, you mentioned NetBSD work. Do you know where they are in their 
> implementation?  Anything I can do to help? Or should I start from scratch?

I hope @krytarowski can comment on that.

It's probably best to move this discussion to a new thread on lldb-dev.


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Dmitry Mikulin via lldb-commits]

dmikulin marked an inline comment as done.
dmikulin added a comment.

Thanks Pavel! I'll start working on it. Do you know when lldb-server Linux 
changes were committed? I want to use those patches as a template, but it's 
hard to locate them digging through thousands of commit log entries...

Ed, you mentioned NetBSD work. Do you know where they are in their 
implementation?  Anything I can do to help? Or should I start from scratch?


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Pavel Labath via lldb-commits]

labath added a comment.

I don't know of any good source of documentation for this, but I am willing to 
answer any questions you might have about this. Basically, the idea of 
lldb-server is that you use the same code path for remote debugging as you to 
for local. So all the debugging is done in a separate process called 
lldb-server, with which the client then communicates when it needs something 
done.

If we want to get lldb-server working on freebsd, I would propose a plan like 
this:

- rename NativeProcessLinux into something more generic (NativeProcessPosix ?), 
make a new NativeProcessLinux, which inherits from that, but contains no code.
- get NativeProcessPosix building on FreeBSD. Now, you will likely get compile 
errors as we may be using some OS interfaces that don't exist on freebsd. We 
will need to figure out how we can either: a) use interfaces which are 
available on both platforms; b) abstract the functionality so it can 
implemented in the derived classes. The biggest offender here will be ptrace 
(at one point we used signalfd(), but that is gone now), as it operates a bit 
differently on the two OSs. For the implementation of the FreeBSD part of it, 
you can use the existing ProcessFreeBSD classes for inspiration. All the other
- get lldb-server test suite passing on FreeBSD (these are all the tests in 
tools/lldb-server), which test pure server functionality)
- switch lldb client to use lldb-server instead of ProcessFreeBSD
- get the rest of lldb tests passing
- remove ProcessFreeBSD

When we were getting lldb-server working for linux, it really helped having osx 
as a reference, because it already used the same flow. I imagine that it could 
be quite helpful for you as well, if you could get a linux machine/VM to be 
able to compare how the code behaves on two platforms, but I think it should be 
possible to do it without it.

So, if you want to get started, I propose you do a preliminary invetigation -- 
set the build system so that in builds NativeProcessLinux on FreeBSD, and see 
how much code do you have to delete, so that it compiles. This should give us 
an idea of the scope of the work.

good luck :)


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Ed Maste via lldb-commits]

emaste added a comment.

On a quick look this seems OK. I'll try to test/review in detail.




Comment at: source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:925
+
+  // eRegisterKindDWARF -> RegsiterValue
+  std::unordered_map m_register_values;

typo here


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Ed Maste via lldb-commits]

emaste added a comment.

In https://reviews.llvm.org/D25756#574258, @labath wrote:

> No, I'm saying someone *should*. :P
>
> Ed looked into that at some point but, I don't think get got too far with it. 
> Adding @emaste, who should probably review this.


Yes, it needs to be done. Maybe now that NetBSD support is progressing a few of 
us can work together to make it happen.


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Pavel Labath via lldb-commits]

labath added a reviewer: emaste.
labath added a comment.

No, I'm saying someone *should*. :P

Ed looked into that at some point but, I don't think get got too far with it. 
Adding @emaste, who should probably review this.


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Dmitry Mikulin via lldb-commits]

dmikulin added a comment.

Are you saying someone is working on switching FreeBSD to lldb-server 
debugging? Can I get my hands on this patch?


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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

[Lldb-commits] [PATCH] D25756: FreeBSD ARM support for software single step.

[Pavel Labath via lldb-commits]

labath added a comment.

I'd love to see freebsd switch to lldb-server based debugging. Then we could 
make sure this code is actually reused instead of copied around (e.g. we are 
right now preparing a patch to improve this, which you could have gotten for 
free if the code was shared). 


Repository:
  rL LLVM

https://reviews.llvm.org/D25756



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