Add SBArgs to the public interface

[zturner]

Summary

I found a bug in SetCommandString() which I plan to fix in a separate patch. I sat down to write a test for the bug and realized there was no SBArgs. So this adds SBArgs so that we can make sure this kind of thing doesn't happen again in the future. Here's an output of my command session verifying that it works as expected.

d:\src\llvmbuild\ninja>bin\lldb
(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.

import lldb
args = lldb.SBArgs()
args.SetCommandString("This is a test")
args.GetArgumentCount()

4L

args.GetArgumentAtIndex(2)

'a'

args.GetArgumentAtIndex(3)

'test'

quit

(lldb) quit

[zturner]

Ugh, that didn't show up right, let's try this again.

d:\src\llvmbuild\ninja>bin\lldb
(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> import lldb
>>> args = lldb.SBArgs()
>>> args.SetCommandString("This is a test")
>>> args.GetArgumentCount()
4L
>>> args.GetArgumentAtIndex(2)
'a'
>>> args.GetArgumentAtIndex(3)
'test'
>>> quit
(lldb) quit

[zturner]

Mostly added you for the SWIG stuff, because I'm doing something kind of "new" here and I had to figure it out with some hacking.

+Greg for the overall idea.

[clayborg]

I would rather not add API if it is just used for testing and I would prefer to not have this in our public API if we don't need it.

Don't we have some internal based testing beneath the public API walls we can use?

The other way to test if the argument parsing is working is to have a program that dumps arguments and launch this process and verify the arguments by parsing the stdout of the program. I believe we have some tests that do this that were made to test this very thing.

[clayborg]

Also, no STL can be passed between shared libraries, so no std::string or std::<anything> can be in the public API.

[clayborg]

You can also test with setting the "target.run-args":

(lldb) settings set target.run-args "one 1" 'two 2' three
(lldb) settings show target.run-args
target.run-args (arguments) =
  [0]: "one 1"
  [1]: "two 2"
  [2]: "three"

[jingham]

I agree with Greg that we should not add API to the SB API's unless there is an external programmer need for it. Adding API just for testing purposes seems like a bad idea to me. It will just end up making the API fat & hard to understand.

OTOH, I wonder if it would be interesting to have an SBCommandInterpreter::HandleCommand that takes an SBArg instead of a command string. If you were trying to program HandleCommand, it might be nice not to have to cons up the string that protects all the quotes just so lldb will then take it apart again correctly. In that case, it might be nice to have a way to just build up the args by hand.

Jim

[zturner]

In D8265#139311, @clayborg wrote:

I would rather not add API if it is just used for testing and I would prefer to not have this in our public API if we don't need it.

Don't we have some internal based testing beneath the public API walls we can use?

The other way to test if the argument parsing is working is to have a program that dumps arguments and launch this process and verify the arguments by parsing the stdout of the program. I believe we have some tests that do this that were made to test this very thing.

We have gtest C++ based unit tests, but it's not hooked up on all platforms and build systems (notably it doesn't build on Windows yet, or the Xcode build). Even if it's only for tests now, it certainly seems like it could be integrated into other aspects of the SB API later by someone else.

I don't really like the program dumping thing because it's adding a bunch of layers on top of what I really want to test, which is just the argument parsing. The shell gets in the way for example, and you can't guarantee that if you call <program> <command-line> that this will end up in a call to Args::SetCommandString(<command-line>) because the shell might have done something.

We need to test that Args::SetCommandString(<command-line>) with known values of <command-line>, parses and tokenizes the way we expect. I can remove the C++ from the public API, but I kind of think that having an SBArgs is the best way to do this.

[zturner]

In D8265#139320, @clayborg wrote:

You can also test with setting the "target.run-args":

(lldb) settings set target.run-args "one 1" 'two 2' three
(lldb) settings show target.run-args
target.run-args (arguments) =
  [0]: "one 1"
  [1]: "two 2"
  [2]: "three"

I'm also not crazy about this. it's not testing the real thing. A long time ago I proposed C++ based unit tests and the reaction wasn't very warm, but on the other hand now we're saying "don't add classes to the public API just for testing". Put together, this sounds like "we don't need to test everything". I know that's not the intention, but the point is these two statements don't really reconcile very well with each other. If the SB API is the way to test things, then we should be able to add things to it for the purpose of testing. If it's not, then we need to get gtest up and running on all platforms and all build systems and we need to start writing unit tests.

[clayborg]

I don't want the API if we aren't going to use it in our API.

If we do find a use for it we will need to returns "const char *" objects and state that the "const char *" values that are returned are only valid while for the lifetime of the SBArgs object. Then all std::string values turn into "const char *" in your API. Also, if you find a use for SBArgs somewhere you can't remove or change any existing API, you can only add functions. SBArgs could be returned from SBLaunchInfo, and the arguments could be set using an SBArgs (again, don't remove or change anything).

The other reason I would rather not expose this is it opens up all sorts of question and possibilities of wether people are going to expect arguments to be expanded just like a shell? Which shell will it emulate? And all these questions we have been mulling over internally in LLDB.

[jingham]

My feelings in order of importance are:

1. For sure we should not be adding things to the SB API just for testing. That's going to make for a bad API.
2. We should test everything we can test through the SB API
3. Things that are impossible to test through the SB API need some other way to test them. a) One way would be to add some SBTest headers that augment the SB API's and add stuff just for testing. They would depend on the SB API and you would SWIG & include them in the test harness. Tests that use them would have to be marked as such since you wouldn't be able to run them against an installed lldb, but... b) Another way is to write something like C++ unit tests I kind of prefer (a) because it seems likely to me that maintaining tests based on this is less likely to depend on irrelevant implementation details of the lldb_private API's. And it means we only have one test harness to maintain.

OTOH, in the case of SBArgs, as I said earlier I'm not totally convinced you couldn't come up with a decent reason such a thing might be useful...

Jim

[zturner]

In D8265#139340, @clayborg wrote:

I don't want the API if we aren't going to use it in our API.

If we do find a use for it we will need to returns "const char *" objects and state that the "const char *" values that are returned are only valid while for the lifetime of the SBArgs object. Then all std::string values turn into "const char *" in your API. Also, if you find a use for SBArgs somewhere you can't remove or change any existing API, you can only add functions. SBArgs could be returned from SBLaunchInfo, and the arguments could be set using an SBArgs (again, don't remove or change anything).

The other reason I would rather not expose this is it opens up all sorts of question and possibilities of wether people are going to expect arguments to be expanded just like a shell? Which shell will it emulate? And all these questions we have been mulling over internally in LLDB.

It sounds like there's at least some consensus that there is *potential use* for it. You mention SBLaunchInfo, and Jim mentioned SBCommandInterpreter::HandleCommand. But is it necessary for me to add those now? It seems like someone else could do that when they're ready to use SBLaunchInfo with an SBArgs, or when they're ready to build up an args and use HandleCommand.

I don't think anyone will be confused about shell emultaion. Args -- and hence SBArgs -- have nothing to do with shell expansion. If they're confused about SBArgs, then they're also confused about Args, because SBArgs is by definition just a wrapper around it.

Either way, I'm very strongly opposed to just saying "this is untestable unless we find another use for it a well". That's not a realistic solution. I don't think that's what you're saying, just wanted to make sure my opinion is clear. So I guess the only realistic options I see are:

1. Add it to the SB API based on discussions of potential uses, and add tests.
2. C++ based unit tests.

If everyone is in agreement that we should not add things to the SB API just for tests, then we're going to have to do #2 someday anyway, because we can't add everything to the SB API. So if 2 is the only option left, should I just go ahead and do it?

Once it's done, someone will need to get it integrated into the Xcode build, because tests that nobody runs aren't very helpful.

[zturner]

It sounds to me like we should just do gtest. There are already other things that could stand to be unit tested, like the IRInterpreterMemoryMap which I found some bugs in early on when I joined the project, and it's easy to find other examples like RegularExpression.

[clayborg]

gtest would be my vote. There are bound to be things we need to test that aren't in the API and this is a good way to do this.

The other thing I can think if is if, for debug builds only, we add a static function that is available in the public API that we can call from our test suite. Then we have a python based test that just does something like:

#if defined(LLDB_CONFIGURATION_DEBUG)
static void SBDebugger::RunUnitTests();
#endif

The in python:

debugger.UnitTest()

This runs all built in unit test. These tests might need a output format that can be parsed in case any built in unit test fail. Then all classes that want to run unit tests can register a <classname::UnitTest() static function that can be run.

[zturner]

I will work on making this happen on the CMake side, so it should work for
all platforms that build with CMake. I will need some help getting this
tied into the Xcode build, but we can revisit that after it's done on the
CMake side.

include/lldb/API/SBArgs.h

+//===-- SBArgs.h ------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SBArgs_h_
+#define LLDB_SBArgs_h_
+
+#include "lldb/API/SBDefines.h"
+#include <string>
+
+namespace lldb
+{
+
+class LLDB_API SBArgs
+{
+  public:
+    SBArgs();
+
+    SBArgs(const lldb::SBArgs &rhs);
+
+    ~SBArgs();
+
+    lldb::SBArgs &operator=(const lldb::SBArgs &rhs);
+
+    void SetCommandString(const std::string &command);
+
+    std::string GetCommandString() const;
+
+    std::string GetQuotedCommandString() const;
+
+    size_t GetArgumentCount() const;
+
+    std::string GetArgumentAtIndex(size_t idx) const;
+
+    void AppendArgument(const std::string &argument);
+
+  private:
+    std::shared_ptr<lldb_private::Args> m_opaque_ptr;
+};
+
+} // namespace lldb
+
+#endif // LLDB_SBBlock_h_

include/lldb/API/SBDefines.h

 namespace lldb {
 
 class LLDB_API SBAddress;
+class LLDB_API SBArgs;
 class LLDB_API SBBlock;
 class LLDB_API SBBreakpoint;
 class LLDB_API SBBreakpointLocation;

scripts/Python/interface/SBArgs.i

+//===-- SWIG Interface for SBArgs -------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+% include "std_string.i"
+
+    namespace lldb
+{
+
+    % feature("docstring", "Represents the LLDB command string parser / tokenizer") SBArgs;
+
+    class SBArgs
+    {
+      public:
+        SBArgs();
+
+        SBArgs(const lldb::SBArgs &rhs);
+
+        ~SBArgs();
+
+        % feature("docstring", "Sets and tokenizes the command string") SetCommandString;
+        void SetCommandString(const ::std::string &command);
+
+        % feature("docstring", "Gets the untokenized command string") GetCommandString;
+        ::std::string GetCommandString() const;
+
+        % feature("docstring", "Gets the untokenized command string with all arguments quoted") GetQuotedCommandString;
+        ::std::string GetQuotedCommandString() const;
+
+        % feature("docstring", "Gets the number of arguments tokenized") GetArgumentCount;
+        size_t GetArgumentCount() const;
+
+        % feature("docstring", "Gets the argument at the specified index") GetArgumentAtIndex;
+        ::std::string GetArgumentAtIndex(size_t idx) const;
+
+        % feature("docstring", "Appends a single argument to the end of the argument list") AppendArgument;
+        void AppendArgument(const ::std::string &argument);
+    };
+
+} // namespace lldb
 No newline at end of file

scripts/lldb.swig

 %{
 #include "lldb/lldb-public.h"
 #include "lldb/API/SBAddress.h"
+#include "lldb/API/SBArgs.h"
 #include "lldb/API/SBAttachInfo.h"
 #include "lldb/API/SBBlock.h"
 #include "lldb/API/SBBreakpoint.h"

 
 /* Python interface files with docstrings. */
 %include "./Python/interface/SBAddress.i"
+%include "./Python/interface/SBArgs.i"
 %include "./Python/interface/SBAttachInfo.i"
 %include "./Python/interface/SBBlock.i"
 %include "./Python/interface/SBBreakpoint.i"

source/API/CMakeLists.txt

 # So lldbAPI must be an OBJECT library.
 add_lldb_library(lldbAPI OBJECT
   SBAddress.cpp
+  SBArgs.cpp
   SBAttachInfo.cpp
   SBBlock.cpp
   SBBreakpoint.cpp

source/API/SBArgs.cpp

+//===-- SBAddress.cpp -------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/API/SBArgs.h"
+#include "lldb/Interpreter/Args.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBArgs::SBArgs()
+    : m_opaque_ptr(new Args())
+{
+}
+
+SBArgs::SBArgs(const SBArgs &rhs)
+    : m_opaque_ptr(rhs.m_opaque_ptr)
+{
+}
+
+SBArgs::~SBArgs()
+{
+}
+
+SBArgs &SBArgs::operator=(const lldb::SBArgs &rhs)
+{
+    m_opaque_ptr = rhs.m_opaque_ptr;
+    return *this;
+}
+
+void
+SBArgs::SetCommandString(const std::string &command)
+{
+    m_opaque_ptr->SetCommandString(command);
+}
+
+std::string
+SBArgs::GetCommandString() const
+{
+    std::string result;
+    m_opaque_ptr->GetCommandString(result);
+    return result;
+}
+
+std::string
+SBArgs::GetQuotedCommandString() const
+{
+    std::string result;
+    m_opaque_ptr->GetQuotedCommandString(result);
+    return result;
+}
+
+size_t
+SBArgs::GetArgumentCount() const
+{
+    return m_opaque_ptr->GetArgumentCount();
+}
+
+std::string
+SBArgs::GetArgumentAtIndex(size_t idx) const
+{
+    return m_opaque_ptr->GetArgumentAtIndex(idx);
+}
+
+void
+SBArgs::AppendArgument(const std::string &argument)
+{
+    m_opaque_ptr->AppendArgument(argument.c_str());
+}