aprantl added inline comments.
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:35
+/// "lldb/Breakpoint/BreakpointInjectedSite.h" Class that manages the injected
+/// conditional breakpoints.
+//----------------------------------------------------------------------
----------------
This comment assumes that the reader knows what injected breakpoints are; can
you either link to the file where this is explained or add a one-paragraph
description of what this means here?
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:37
+//----------------------------------------------------------------------
+
+struct ArgumentMetadata {
----------------
Is this still recognized by doxygen if there is an empty line before the
declaration?
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:38
+
+struct ArgumentMetadata {
+
----------------
can this be an inner class of BreakpointInjectedSite?
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:58
+
+ BreakpointInjectedSite(BreakpointSiteList *list,
+ const lldb::BreakpointLocationSP &owner,
----------------
Doxygen comment explaining the \param eters?
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:64
+
+ bool BuildConditionExpression(void);
+
----------------
Please add comments to all non-obvious functions.
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:90
+ lldb::TargetSP m_target_sp;
+ Address m_real_addr;
+ Address m_trap_addr;
----------------
... and members :-)
================
Comment at: lldb/include/lldb/Target/ABI.h:143
+
+ virtual uint64_t GetJumpOpcode(void) { return 0; }
+
----------------
Comment explaining what this is supposed to do?
Is there always *exactly* one opcode that fits this requirement, and is an
Opcode always < 64 bits on all ABIs?
================
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)
----------------
how does this test ensure that the breakpoint was actually injected and that it
didn't fall back to a regular breakpoint?
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:30
+
+bool BreakpointInjectedSite::BuildConditionExpression(void) {
+ Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_JIT_LOADER);
----------------
we usually write this just as `BuildConditionExpression()`
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:47
+ loc_sp->GetConditionText());
+ return false;
+ }
----------------
Should this return an error instead of logging and returning false?
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:341
+ "\n"
+ " typedef unsigned int uint32_t;\n"
+ " typedef unsigned long long uint64_t ;\n"
----------------
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?
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:382
+ " intptr_t rcx;\n"
+ " intptr_t rax;\n"
+ "} register_context;\n\n";
----------------
This should be in an x86_64-specific subclass, I think?
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:443
+ int64_t operand = op.getRawOperand(0);
+ expr += " src_addr = " + std::to_string(operand) +
+ ";\n"
----------------
This seems unnecessarily slow. Perhaps use an llvm::Twine?
================
Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:225
+
+ // Copy saved instruction to inferior memory buffer
+ Status error;
----------------
`.` at the end :-)
================
Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:273
+ mov_buffer[1] = X64_MOV_OPCODE;
+ mov_buffer[2] = 0xE7; // %rsp SIB Bytes
+ std::memcpy(&trampoline_buffer[trampoline_size], &mov_buffer, X64_MOV_SIZE);
----------------
We don't typically use end-of-line comments in LLVM and prefer full sentences.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66249/new/
https://reviews.llvm.org/D66249
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
