[LLDB-MI] removing requirement of a parameter for -break-insert's -f flag

[pieandcakes]

Summary

bug 28698

[ki.stfu]

Hi!

So, what you want is to create pending breakpoints without locations?

I'm not sure it's the correct use of -break-insert so that lldb-mi has to exit with an error in this case. The other question is why it doesn't do that and sets BPs without locations even if -f has not been specified but that's another story.

[pieandcakes]

Hi Ilia,

Thanks for the review. There is another change I do need to make in the Execute() method and probably add some tests.

I reviewed this spec for GDB/MI and it looks like -f should be a flag that says if location isn't found, to go ahead and set it as pending. I would assume that if pending flag is set and no location is set that it should error. Let me know what you think.

-Pierson

[ki.stfu]

Yes, that's what I said. We have to throw an error if location is not passed (even if -f is specified).

[pieandcakes]

Added error message if location is not specified per reviewers comments.

[pieandcakes]

Fixed bracket.

[pieandcakes]

Would you be willing to please take another look? I think this should be the fix.

[ki.stfu]

Hi @pieandcakes!

I'm sorry for the delay, have a lot of work. Please folllow my inline comments, rebase against ToT and update CL's summary.

tools/lldb-mi/MICmdCmdBreak.cpp

156–163

I think we can remove these lines and make pArgLocation mandatory.
For this, please specify it during registration of m_constStrArgNamedLocation in CMICmdCmdBreakInsert::ParseArgs, and then replaces the highlighted lines with:

m_brkName = pArgLocation->GetValue();

tools/lldb-mi/MICmnResources.cpp

222 ↗

(On Diff #70107)

revert this

tools/lldb-mi/MICmnResources.h

239 ↗

(On Diff #70107)

revert this

[ki.stfu]

PS: I think it will look like:

Index: tools/lldb-mi/MICmdCmdBreak.cpp
===================================================================
--- tools/lldb-mi/MICmdCmdBreak.cpp	(revision 281191)
+++ tools/lldb-mi/MICmdCmdBreak.cpp	(working copy)
@@ -84,8 +84,7 @@
   // Not implemented m_setCmdArgs.Add(new CMICmdArgValOptionShort(
   // m_constStrArgNamedHWBrkPt, false, false));
   m_setCmdArgs.Add(new CMICmdArgValOptionShort(
-      m_constStrArgNamedPendinfBrkPt, false, true,
-      CMICmdArgValListBase::eArgValType_StringQuotedNumberPath, 1));
+      m_constStrArgNamedPendinfBrkPt, false, true));
   m_setCmdArgs.Add(new CMICmdArgValOptionShort(m_constStrArgNamedDisableBrkPt,
                                                false, false));
   // Not implemented m_setCmdArgs.Add(new CMICmdArgValOptionShort(
@@ -99,7 +98,7 @@
   m_setCmdArgs.Add(new CMICmdArgValOptionShort(
       m_constStrArgNamedRestrictBrkPtToThreadId, false, true,
       CMICmdArgValListBase::eArgValType_Number, 1));
-  m_setCmdArgs.Add(new CMICmdArgValString(m_constStrArgNamedLocation, false,
+  m_setCmdArgs.Add(new CMICmdArgValString(m_constStrArgNamedLocation, true,
                                           true, false, false, true));
   return ParseValidateCmdOptions();
 }
@@ -158,12 +157,7 @@
     m_strArgOptionThreadGrp = CMIUtilString::Format("i%d", nThreadGrp);
   }
   m_bBrkPtIsPending = pArgPendingBrkPt->GetFound();
-  if (pArgLocation->GetFound())
-    m_brkName = pArgLocation->GetValue();
-  else if (m_bBrkPtIsPending) {
-    pArgPendingBrkPt->GetExpectedOption<CMICmdArgValString, CMIUtilString>(
-        m_brkName);
-  }
+  m_brkName = pArgLocation->GetValue();
   if (pArgIgnoreCnt->GetFound()) {
     pArgIgnoreCnt->GetExpectedOption<CMICmdArgValNumber, MIuint>(
         m_nBrkPtIgnoreCount);

tools/lldb-mi/MICmdCmdBreak.cpp

 {
     m_setCmdArgs.Add(new CMICmdArgValOptionShort(m_constStrArgNamedTempBrkPt, false, true));
     // Not implemented m_setCmdArgs.Add(new CMICmdArgValOptionShort( m_constStrArgNamedHWBrkPt, false, false));
-    m_setCmdArgs.Add(new CMICmdArgValOptionShort(m_constStrArgNamedPendinfBrkPt, false, true,
-                                                   CMICmdArgValListBase::eArgValType_StringQuotedNumberPath, 1));
+    m_setCmdArgs.Add(new CMICmdArgValOptionShort(m_constStrArgNamedPendinfBrkPt, false, true));
     m_setCmdArgs.Add(new CMICmdArgValOptionShort(m_constStrArgNamedDisableBrkPt, false, false));
     // Not implemented m_setCmdArgs.Add(new CMICmdArgValOptionShort( m_constStrArgNamedTracePt, false, false));
     m_setCmdArgs.Add(new CMICmdArgValOptionShort(m_constStrArgNamedConditionalBrkPt, false, true,