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

[Alex Langford via Phabricator via lldb-commits]

xiaobai added inline comments.


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:341
+          "\n"
+          "   typedef unsigned int            uint32_t;\n"
+          "   typedef unsigned long long      uint64_t ;\n"
----------------
labath wrote:
> aprantl wrote:
> > 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?
> This entire function is target specific. I would struggle to find a single line in this expression that could be reused on linux for instance (and let's not even talk about windows..).
> I still believe that the simplest approach here would be to allocate memory for this structure on the stack, side-stepping the whole "how to allocate memory in a target-independent manner" discussion completely.
As others have state, this is entirely target-specific. I'd really rather avoid behavior specific to one platform in something a general as lldbBreakpoint.


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:12
+#include "Plugins/ExpressionParser/Clang/ClangExpressionVariable.h"
+#include "Plugins/ExpressionParser/Clang/ClangUserExpression.h"
+#include "lldb/Expression/ExpressionVariable.h"
----------------
Please don't introduce dependencies on Plugins in non-plugin libraries. I'm working on removing them.

Is BreakpointInjectedSite inherently tied to clang? If so, why not move it to the ClangExpressionParser plugin? If not, there should be some kind of generalization to ExpressionParser and PersistentExpressions.


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:246
+  ClangUserExpression *clang_expr =
+      llvm::dyn_cast<ClangUserExpression>(m_condition_expression_sp.get());
+
----------------
labath wrote:
> This also looks like an improper core code -> plugin dependency.
Indeed it is. Let's find a way to avoid that.


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:264
+  for (uint32_t i = 0; i < num_elements; ++i) {
+    const clang::NamedDecl *decl = nullptr;
+    llvm::Value *value = nullptr;
----------------
Could you use CompilerDecl here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249