Correct the argument list of command `breakpoint read`

[MrHate]

Summary

Command breakpoint read should not accept breakpoint ids as arguments, and in fact, it is not implemented to deal with breakpoint id arguments either. So this patch is to correct the argument list of this command so that the help text won't misguide users.

[teemperor]

LGTM, good catch!

[jingham]

It is fine to take this out for now. I didn't get to finish this when I was last working on it. In the finished design you would say:

(lldb) breakpoint list --file savedBreakpoints.txt
(1) file: foo.c line:12
(2) someInterestingSymbol
...
(lldb) breakpoint read --file savedBreakpoints.txt 1

To only read in specific breakpoints from the saved file. That 's not much use till you can list what's in a file without importing it, so the breakpoint id's are not currently used.

[MrHate]

In D79722#2031990, @jingham wrote:

It is fine to take this out for now. I didn't get to finish this when I was last working on it. In the finished design you would say:

(lldb) breakpoint list --file savedBreakpoints.txt
(1) file: foo.c line:12
(2) someInterestingSymbol
...
(lldb) breakpoint read --file savedBreakpoints.txt 1

To only read in specific breakpoints from the saved file. That 's not much use till you can list what's in a file without importing it, so the breakpoint id's are not currently used.

Thanks for your explanation. I never thought of implementing the action this way, coz I just thought breakpoint ids should only be available within the context which created them. If you design such an implementation, I think it might be better if the arguments are not considered as breakpoint ids, but something like indexes within breakpoint configuration files instead. The existing argument list here seems still not suitable for your design, so I think removing it might not a wrong thing.

[jingham]

I'm not sure I agree with you. If the workflow is that you use "break list --filename" to see what is in the saved breakpoint file - which would presumably produce exactly the same listing as "break list" with any other specifier would - then it would be very natural to expect that you pick out of the list results in "break read" exactly as you would other breakpoint commands.

For instance, it would be weird if 1-4 works or specifying them by name worked for other breakpoint commands but not for "break read". So I think this is a distinction without a difference.

That said, the code to use them is not present, and they will be trivial to re-add if/when somebody gets around to finishing this. So I have no objection to removing them now.

[MrHate]

In D79722#2034512, @jingham wrote:

I'm not sure I agree with you. If the workflow is that you use "break list --filename" to see what is in the saved breakpoint file - which would presumably produce exactly the same listing as "break list" with any other specifier would - then it would be very natural to expect that you pick out of the list results in "break read" exactly as you would other breakpoint commands.

For instance, it would be weird if 1-4 works or specifying them by name worked for other breakpoint commands but not for "break read". So I think this is a distinction without a difference.

That said, the code to use them is not present, and they will be trivial to re-add if/when somebody gets around to finishing this. So I have no objection to removing them now.

Thanks for explanation again, and I think I agree with you. The importance of keeping naturalness makes sense. What I think is the break-id sources might be still different between them: the breakpoint-ids of breakpoint read should come from the process of breakpoint list --filename while the others' come from breakpoint list. Thus we still need a unique implementation for the completion for breakpoint read after break list --filename and break read <breakpoint-id> finished.

[jingham]

Yes, that's right. What you describe is doable. The lldb command completer actually sees the whole command line, not just the argument being completed. We use that in a couple of places in lldb, for instance if you do:

(lldb) break set -s libfoo.dylib -n a<TAB>

lldb only completes the symbols starting with "a" in libfoo.dylib, not from the whole executable.

Using the same mechanism, if somebody does:

(lldb) break read --filename breakpoints.txt SomeNameIDontRem<TAB>

the completer can figure out which file to read the breakpoint listing from, and pass off the right breakpoint list to the completer.

[MrHate]

Sounds that the breakpoint-id completion needs to be improved in the future. Thanks for pointing this out!

[jingham]

Yes, the most serious omission is that it doesn't complete against breakpoint names. But that is a little orthogonal to this discussion. The Breakpoint ID completer should receive a list of breakpoints and a list of breakpoint names, and complete against those. Then for something like break delete the completer should get fed the current target's list of breakpoints and breakpoint names. But for break list --filename and for break read --filename, it should get passed the list of breakpoints and names read from the file. So in the fullness of time there would be two independent bits of work.

For now just getting them to complete against names is the bit that needs doing.

lldb/source/Commands/CommandObjectBreakpoint.cpp

 
 class CommandObjectBreakpointRead : public CommandObjectParsed {
 public:
   CommandObjectBreakpointRead(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "breakpoint read",
                             "Read and set the breakpoints previously saved to "
                             "a file with \"breakpoint write\".  ",
                             nullptr),
-        m_options() {
-    CommandArgumentEntry arg;
-    CommandObject::AddIDsArgumentData(arg, eArgTypeBreakpointID,
-                                      eArgTypeBreakpointIDRange);
-    // Add the entry for the first argument for this command to the object's
-    // arguments vector.
-    m_arguments.push_back(arg);
-  }
+        m_options() {}
 
   ~CommandObjectBreakpointRead() override = default;
 
   Options *GetOptions() override { return &m_options; }
 
   class CommandOptions : public Options {
   public:
     CommandOptions() : Options() {}