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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.


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


================
Comment at: lldb/include/lldb/Expression/ExpressionVariable.h:210
+public:
+  typedef AdaptedIterable<collection, lldb::ExpressionVariableSP,
+                          vector_adapter>
----------------
labath wrote:
> What is the purpose of this `AdaptedIterable`, and why is it better than just returning  say `ArrayRef<lldb::ExpressionVariableSP` ?
Given a class with a container, `AdaptedIterable` will generate the proper iterator accessor for this class.

In this case, it's pretty convenient to do:
```
for (auto var : expr_vars)
{ }
```

instead of:
```
for (auto var : expr_vars.GetContainer())
{ }
```

It doesn't require to have a getter on the container to iterate over it.

Most of LLDB container classes are built upon `std` containers (vector, map ...).
I could change it to `ArrayRef<lldb::ExpressionVariableSP>`, but I don't see why using it would be better, + it might break things (e.g. ArrayRef doesn't implement some of the `std` containers method like `erase()` or `clear()`).


================
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)
----------------
aprantl wrote:
> how does this test ensure that the breakpoint was actually injected and that it didn't fall back to a regular breakpoint?
The breakpoint doesn't get injected if we fallback to a regular breakpoint.
And I check the state of the breakpoint on the command above: 
```
self.assertFalse(location.GetInjectCondition(), VALID_BREAKPOINT_LOCATION)
```


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:47
+               loc_sp->GetConditionText());
+      return false;
+    }
----------------
aprantl wrote:
> Should this return an error instead of logging and returning false?
Error is handled on the upper function, at the process level.


================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:443
+      int64_t operand = op.getRawOperand(0);
+      expr += "   src_addr = " + std::to_string(operand) +
+              ";\n"
----------------
aprantl wrote:
> This seems unnecessarily slow. Perhaps use an llvm::Twine?
As discussed, using `llvm::Twine` doesn't fit the implementation. Since this part of the code will probably change in the future, to support more DWARF Expression, I'll leave it as is for now, and try to come up with a more efficient approach in the coming patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249