[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