[Lldb-commits] [PATCH] D66249: [JIT][Breakpoint] Add "BreakpointInjectedSite" and FCB Trampoline

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:35
+/// "lldb/Breakpoint/BreakpointInjectedSite.h" Class that manages the injected
+/// conditional breakpoints.
+//----------------------------------------------------------------------
----------------
This comment assumes that the reader knows what injected breakpoints are; can you either link to the file where this is explained or add a one-paragraph description of what this means here?


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:37
+//----------------------------------------------------------------------
+
+struct ArgumentMetadata {
----------------
Is this still recognized by doxygen if there is an empty line before the declaration?


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:38
+
+struct ArgumentMetadata {
+
----------------
can this be an inner class of BreakpointInjectedSite?


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:58
+
+  BreakpointInjectedSite(BreakpointSiteList *list,
+                       const lldb::BreakpointLocationSP &owner,
----------------
Doxygen comment explaining the \param eters?


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:64
+
+  bool BuildConditionExpression(void);
+
----------------
Please add comments to all non-obvious functions.


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:90
+  lldb::TargetSP m_target_sp;
+  Address m_real_addr;
+  Address m_trap_addr;
----------------
... and members :-)


================
Comment at: lldb/include/lldb/Target/ABI.h:143
+
+  virtual uint64_t GetJumpOpcode(void) { return 0; }
+
----------------
Comment explaining what this is supposed to do?

Is there always *exactly* one opcode that fits this requirement, and is an Opcode always < 64 bits on all ABIs?


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:181
+        # FCB falls back to regular conditional breakpoint that get hit once.
+        self.assertTrue(breakpoint.GetHitCount() == 1)
----------------
how does this test ensure that the breakpoint was actually injected and that it didn't fall back to a regular breakpoint?


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:30
+
+bool BreakpointInjectedSite::BuildConditionExpression(void) {
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_JIT_LOADER);
----------------
we usually write this just as `BuildConditionExpression()`


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:47
+               loc_sp->GetConditionText());
+      return false;
+    }
----------------
Should this return an error instead of logging and returning false?


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:341
+          "\n"
+          "   typedef unsigned int            uint32_t;\n"
+          "   typedef unsigned long long      uint64_t ;\n"
----------------
This seems awfully specific. Shouldn't this be target-dependent, and is there no pre-existing table in LLDB that we could derive this from?


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:382
+          "   intptr_t rcx;\n"
+          "   intptr_t rax;\n"
+          "} register_context;\n\n";
----------------
This should be in an x86_64-specific subclass, I think?


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:443
+      int64_t operand = op.getRawOperand(0);
+      expr += "   src_addr = " + std::to_string(operand) +
+              ";\n"
----------------
This seems unnecessarily slow. Perhaps use an llvm::Twine?


================
Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:225
+
+  // Copy saved instruction to inferior memory buffer
+  Status error;
----------------
`.` at the end :-)


================
Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:273
+  mov_buffer[1] = X64_MOV_OPCODE;
+  mov_buffer[2] = 0xE7; // %rsp SIB Bytes
+  std::memcpy(&trampoline_buffer[trampoline_size], &mov_buffer, X64_MOV_SIZE);
----------------
We don't typically use end-of-line comments in LLVM and prefer full sentences.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249