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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Aug 15 04:22:27 PDT 2019

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
+  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.

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list