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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I didn't go into all the details of this patch, as I am guessing this will still go through a number of revisions, but I did make a couple notes of things that caught my eye while glancing over it.



================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:50
+class BreakpointInjectedSite
+    : public std::enable_shared_from_this<BreakpointInjectedSite>,
+      public BreakpointSite {
----------------
BreakpointSite is already shared_ptr-enabled. Why this? I don't think that's how you're supposed to use this class.


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointSite.h:54-56
+   static bool classof(const BreakpointSite *bp_site) {
+     return bp_site->getKind() == eKindBreakpointSite;
+   }
----------------
This is weird. So, in OO terminology, `BreakpointInjectedSite` "is-a" `BreakpointSite`, but if you ask `llvm::isa<BreakpointSite>(injected_site)`, it will return false?


================
Comment at: lldb/include/lldb/Expression/ExpressionVariable.h:210
+public:
+  typedef AdaptedIterable<collection, lldb::ExpressionVariableSP,
+                          vector_adapter>
----------------
What is the purpose of this `AdaptedIterable`, and why is it better than just returning  say `ArrayRef<lldb::ExpressionVariableSP` ?


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:226
+      m_trap_addr = addr;
+      LLDB_LOGV(log, "Injected trap address: {:llx}", addr);
+      return true;
----------------
This isn't a proper format string. You can find their general description here <http://llvm.org/docs/ProgrammersManual.html#formatting-strings-the-formatv-function>, and the specifics are generally next to the definition of the actual format provider (e.g. <https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/FormatProviders.h#L102>). I find it very helpful to look at their unit tests too: <https://github.com/llvm-mirror/llvm/blob/master/unittests/Support/FormatVariadicTest.cpp>.


================
Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.h:96-98
+  uint64_t GetJumpOpcode() override { return X64_JMP_OPCODE; }
+
+  size_t GetJumpSize() override { return X64_JMP_SIZE; }
----------------
replace with `ArrayRef<uint8_t> GetJumpOpcode();` or something similar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249