[Lldb-commits] [PATCH] D66248: [JIT][Command] Add "inject-condition" flag to conditional breakpoints

Med Ismail Bennani via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Aug 16 15:23:14 PDT 2019

mib marked 3 inline comments as done.
mib added inline comments.

Comment at: lldb/include/lldb/Breakpoint/BreakpointOptions.h:120
+  ///
   BreakpointOptions(const char *condition, bool enabled = true,
                     int32_t ignore = 0, bool one_shot = false,
shafik wrote:
> You have a lot of `bool` parameters, these are hard to distinguish when calling the function and easy to get mixed up during refactors and subsequent merge conflicts. It would probably be better to combine these `bool` options into a `struct` and then each option has an explicit name that that will be assigned to which makes it explicit which options are being chosen at the call site.
I only added the `bool inject_condition` parameter to the `BreakpointOptions` constructor. I also added documentation that was missing for the other parameters. I don't think having a struct for those options is a necessary since that's what the `BreakpointOptions` class is for. 

Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/main.c:30
+    printf("local_count = %d\n", local_count++);  // Find the line number of condition breakpoint for local_count
+    asm("nop");
+    asm("nop");
shafik wrote:
> Can you explain why we need the `nop`s injected?
I'm still working on dynamically patching the copied instructions, so I use a `nop` sled for now to test my feature.

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list