[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

In several places you have `SourceLocationSpec::Create` followed by a `lldbassert`. That's not sufficient, because in a non-assert build, the lldbassert is going to print a message but not halt the program and we'll crash when trying to dereference the expected. You'll need to add proper error handling to those places to bail out if the location spec is not valid.



================
Comment at: lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h:63
   bool m_skip_prologue;
-  bool m_exact_match;
+  SourceLocationSpec m_location_spec;
 
----------------
Move this before the bool to reduce padding. 


================
Comment at: lldb/source/API/SBThread.cpp:856
     SymbolContextList sc_list;
-    frame_sc.comp_unit->ResolveSymbolContext(step_file_spec, line,
-                                             check_inlines, exact,
+    frame_sc.comp_unit->ResolveSymbolContext(*location_spec,
                                              eSymbolContextLineEntry, sc_list);
----------------
This will crash in a non-assert build. You can't rely on the assert to make this unreachable. 


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:274-275
+            m_location_spec.GetLine());
+  if (m_location_spec.HasColumn())
+    s->Printf("column = %u, ", *m_location_spec.GetColumn());
+  s->Printf("exact_match = %d", m_location_spec.GetExactMatch());
----------------
I'd get the column, check if it's valid, and use it, instead of calling `HasColumn` before `GetColumn`. 


================
Comment at: lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp:116
+    lldbassert(location_spec && "Invalid SourceLocationSpec.");
+    cu->ResolveSymbolContext(*location_spec, eSymbolContextEverything, sc_list);
     // Find all the function names:
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965