This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pieandcakes on Aug 1 2016, 12:10 PM.

Details

Reviewers
zturner
ki.stfu
Summary

Diff Detail

Repository
rL LLVM

Event Timeline

pieandcakes retitled this revision from to [LLDB-MI] removing requirement of a parameter for -break-insert's -f flag.
pieandcakes updated this object.
pieandcakes added a reviewer: lldb-commits.
pieandcakes set the repository for this revision to rL LLVM.
pieandcakes added a project: Restricted Project.
Eugene.Zelenko edited reviewers, added: clayborg, zturner; removed: lldb-commits.Aug 1 2016, 4:48 PM
Eugene.Zelenko added a subscriber: lldb-commits.

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.

ki.stfu requested changes to this revision.Aug 1 2016, 10:03 PM
ki.stfu added a reviewer: ki.stfu.
This revision now requires changes to proceed.Aug 1 2016, 10:03 PM

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

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

clayborg resigned from this revision.Aug 8 2016, 10:52 AM
clayborg removed a reviewer: clayborg.
pieandcakes updated this revision to Diff 70088.Sep 1 2016, 4:05 PM
pieandcakes edited edge metadata.

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

pieandcakes updated this revision to Diff 70107.Sep 1 2016, 5:09 PM
pieandcakes edited edge metadata.

Fixed bracket.

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

ki.stfu requested changes to this revision.Sep 11 2016, 11:16 PM
ki.stfu edited edge metadata.

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

revert this

tools/lldb-mi/MICmnResources.h
239

revert this

This revision now requires changes to proceed.Sep 11 2016, 11:16 PM

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);