Add new API to SBTarget class

[apolyakov]

Summary

The new API appends an image search path to the
target's path mapping list.

[clayborg]

include/lldb/API/SBTarget.h

275–276 ↗

(On Diff #157053)

Remove this first one? Other functions were first created and gave no error feedback and then another version was added that had an error. We should start with one that has an error and not ever add this version.

include/lldb/Target/Target.h

1003–1005 ↗

(On Diff #157053)

Remove and just use "PathMappingList &Target::GetImageSearchPathList();"

lit/tools/lldb-mi/target/inputs/target-select-so-path.sh

3–11 ↗

(On Diff #157053)

This seems like it could be racy and cause problems on buildbots.

source/API/SBTarget.cpp

1460–1464 ↗

(On Diff #157053)

Remove this version that takes no error

source/Target/Target.cpp

2055–2059 ↗

(On Diff #157053)

You don't need this, just call "target. GetImageSearchPathList().Append(...)

[labath]

I am sure that particular test is worth trying to implement in lit. That script of yours is full of operating system specifics, and it's going to be super flaky.

I'd suggest keeping this as a python test, as there you can abstract the platform specifics much easier, and we also have a lot of code we can (re)use to setup socket connections safely. (e.g. lldb-server has a mode where *it* selects a port to listen on (race-free), and then you can just fetch that port and connect to it.)

Doing that from a shell script is going to be painful.

I suppose if you really want to make this a lit test, you should at least ditch the bash script and replace that bit with python.

[apolyakov]

What do you think about running tests with a hardcoded port number(as it's done in a web-services). Doing this, we get rid of additional scripts and os-specific things. AFAIK, debugserver even has its own default port.

P.S. As I saw in the lldb-mi python tests, port number is just a random value in a specific range, so, in general, it's the same as a hardcoded number.

[labath]

In D49739#1174744, @apolyakov wrote:

What do you think about running tests with a hardcoded port number(as it's done in a web-services). Doing this, we get rid of additional scripts and os-specific things. AFAIK, debugserver even has its own default port.

Even if you somehow managed (which I doubt) to find a port that has a reasonable chance of being free on most machines, then you'd still run into issues, because two of such tests could not run in parallel. Or you'd have to choose a different hard-coded port for each test, which again lowers your chance of not conflicting with anything.

Generally, I think ports are too scarce of a resource these days for anyone to be able to simply squat them and expect things will work. On unix you could do this with domain sockets, as you can put almost anything in their "address" (build folder, test name, timestamp, ...). However, that won't work on windows (though I believe it may be possible to get named pipes to do this for you there).

P.S. As I saw in the lldb-mi python tests, port number is just a random value in a specific range, so, in general, it's the same as a hardcoded number.

Can you point me to the code that does this? If the tests are just choosing a random port and hoping it will be free, then I'm not surprised they are flaky.

[apolyakov]

packages/Python/lldbsuite/test/tools/lldb-mi/signal/TestMiSignal.py test contains port = 12000 + random.randint(0, 3999).

[labath]

In D49739#1174769, @apolyakov wrote:

packages/Python/lldbsuite/test/tools/lldb-mi/signal/TestMiSignal.py test contains port = 12000 + random.randint(0, 3999).

Ok, that's bad. I don't know why the other lldb-mi tests are flaky, but these ones are definitely flaky because of that. Let's not emulate that.

The best way to get a port is to have lldb-server/debugserver choose it for you. You can do this by passing "0" as the command line port, and using the --pipe argument to have the stub write out the actual port number it has chosen back to you. That way, you're guaranteed to get a free port without any races.

[apolyakov]

Thanks, I used this lldb-server gdbserver --pipe 0 localhost:0 and got a port chosen by debugserver. But to let lldb-mi know about this port we need an additional script, is python a good choice for that?

[labath]

In D49739#1174810, @apolyakov wrote:

Thanks, I used this lldb-server gdbserver --pipe 0 localhost:0 and got a port chosen by debugserver. But to let lldb-mi know about this port we need an additional script, is python a good choice for that?

It's definitely better than bash. :)

[apolyakov]

Moved test from bash to python, removed unnecessary Target::AppendImageSearchPath.

[apolyakov]

lit/lit.cfg

59 ↗

(On Diff #157310)

Do we have debugserver only on macOS? On other OS it's called lldb-server?

source/API/SBTarget.cpp

1468 ↗

(On Diff #157310)

I didn't find nullptr check in other API functions, is it OK or it's better to add it?

[labath]

lit/lit.cfg

59 ↗

(On Diff #157310)

That is correct (well.. some platforms like FreeBSD and Windows have neither).

lit/tools/lldb-mi/target/inputs/target-select-so-path.py

8–11 ↗

(On Diff #157310)

This is still racy, because the port can be snatched from under you between the time you get the free port and the time when lldb-server binds to it. If this was the only test doing it then it might be fine, but since this is going to be running concurrently with other tests, all of which are fetching free ports, the chances of that happening add up.

(Also, binding to the wildcard address will trigger a firewall popup on some machines.)

25–26 ↗

(On Diff #157310)

Another race here: lldb-mi can attempt to connect before lldb-server has had a chance to start listening. (That's another reason to implement the port handshake, as it will serve as an synchronization point -- you get the port number only after lldb-server has successfully started and set up it's socket).

[apolyakov]

lit/tools/lldb-mi/target/inputs/target-select-so-path.py

8–11 ↗

(On Diff #157310)

There is a problem with getting port from lldb-server. If we run lldb-server gdbserver --pipe 0 ocalhost:0, it'll print port number to its stdout, but we can't get it using pipes since to do this we need to wait until lldb-server finishes that isn't what we want.

[labath]

lit/tools/lldb-mi/target/inputs/target-select-so-path.py

8–11 ↗

(On Diff #157310)

Aha, I see. lldb-server does not really expect you to pass std file descriptor as the --pipe argument. Normally you would create a separate fd and pass that instead. Doing that from python is a bit icky, but doable:

(r, w) = os.pipe()
kwargs = {}
if sys.version_info >= (3,0):
  kwargs["pass_fds"] = [w]
llgs = subprocess.Popen(["lldb-server", "gdbserver", "--pipe", str(w), ...], **kwargs)
port_bytes = os.read(r, 10)
port = int(port_bytes.decode("utf-8").strip('\x00'))

Alternatively, we could modify lldb-server to print the port number to stdout in addition to any --pipe arguments (sounds like a good addition anyway, as it enables easy free port selection for a shell user), and then you can sniff that text from here.

[apolyakov]

lit/tools/lldb-mi/target/inputs/target-select-so-path.py

8–11 ↗

(On Diff #157310)

--pipe 0 prints port number exactly to stdout, so there will not be a difference for us. It's not so simple to get port from lldb-server's stdout in python, so I don't think it will save us.

[labath]

lit/tools/lldb-mi/target/inputs/target-select-so-path.py

8–11 ↗

(On Diff #157310)

I think you're looking for this:

foo = subprocess.Popen(...)
print "The first line of output is: " + foo.stdout.readline()

Btw, using --pipe 0 works only by accident (0 is the stdin descriptor), and probably only in a terminal. Once popen() starts redirecting things, 0 will probably not refer to the thing that stdout will read from. --pipe 1 would fix that, but then we have the issue that lldb-server will close the --pipe descriptor once it's finished writing the port. That can have surprising effects as subsequent attempts to write to stdout will fail. (That's why I suggested a different implementation. Among other things, outputting something like "lldb-server listening on 127.0.0.1:4747" will make it easier to separate out the port from other things that lldb-server happens to write to stdout.)

[apolyakov]

lit/tools/lldb-mi/target/inputs/target-select-so-path.py

8–11 ↗

(On Diff #157310)

I think you're looking for this:

foo = subprocess.Popen(...)
print "The first line of output is: " + foo.stdout.readline()

I tried this, it hangs for me.

[apolyakov]

Now tcp port is choosing by debugserver.

[apolyakov]

So, do you have any thoughts about this approach letting the debugserver choose a tcp port and patch overall?

[labath]

Sorry for not responding, I've been a bit busy these days. I've wanted to make a proof-of-concept to show you that reading the port number from stdout is possible, but I haven't gotten around to it yet.

However, I am happy with the current mechanism of choosing the port too (thank you). This should make the test much more stable.

I don't have an opinion on the rest of the patch.

[apolyakov]

OK, thank you for respond. Then, I think, we should wait for review from @clayborg or @aprantl, and if they accept the patch I'll divide it into two parts: SB API part and target-select one.

[aprantl]

Seems good otherwise.

source/API/SBTarget.cpp

1468 ↗

(On Diff #157310)

I would convert it to a ConstString up front and then check whether the ConstString is empty. This way you don't have to worry about nullptr vs empty string.

[apolyakov]

source/API/SBTarget.cpp

1468 ↗

(On Diff #157310)

Maybe then change method's signature from const char * to ConstString? I haven't done this since in other methods strings are represented as pointers to char, is it some rule for SB API or not?

[aprantl]

source/API/SBTarget.cpp

1468 ↗

(On Diff #157310)

In order to map this to Python properly I think we need to keep the signature using const char*.

[apolyakov]

Added converting from const char * to ConstString, documented new API.

[clayborg]

source/API/SBTarget.cpp

1476–1477 ↗

(On Diff #158319)

check if csFrom or csTo is empty and give an error message that states which one is invalid?

[apolyakov]

Made error handling more meaningful: added different error messages for cases - empty <from> path and empty <to> path.

[aprantl]

source/API/SBTarget.cpp

1467 ↗

(On Diff #158330)

personally I would write this as:

if (!csFrom)
  return error.SetErrorString("<from> path is empty");
if (!csTo)
  return error.SetErrorString("<to> path is empty");
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
if (log)
  log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
              static_cast<void *>(target_sp.get()),  __FUNCTION__,
              from, to);
target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);

[apolyakov]

source/API/SBTarget.cpp

1467 ↗

(On Diff #158330)

in my opinion, the branch if (csFrom && csTo) will be executed in most cases, so it is reasonable to make it executed first, as this will benefit the branch prediction of the processor. Is this a worthwhile argument?

[aprantl]

source/API/SBTarget.cpp

1467 ↗

(On Diff #158330)

Not really. First, you don't actually control the order in which the code is emitted by the compiler by rearranging the source code. Second, it doesn't make sense to think about branch prediction outside of a tight loop as the effect would not even be measurable. We should optimize for readability and compact code here.

[apolyakov]

source/API/SBTarget.cpp

1467 ↗

(On Diff #158330)

Yes, I should agree that thinking about branch prediction isn't so effective in our case. But the readability of our approaches is almost equal. At the same time, taking into consideration that in most cases we will have not empty string arguments, your approach will check 2 conditions and mine approach will check 1 condition.

[aprantl]

source/API/SBTarget.cpp

1467 ↗

(On Diff #158330)

It's the LLVM style to prefer early exits:

http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

[apolyakov]

Changed the order of if statements to follow llvm coding standards.

[aprantl]

sgtm.

[apolyakov]

Splitted the patch into two parts: this part with the new API and another one with re-implementing of target-select command.
The second part will be committed separately.

[jingham]

Could you add a short doc to this API - it's particularly useful to do this to the .i version. I know there are lots of API's that don't have docs, but that is not a happy situation. I'd like it if we all at least didn't make the problem worse by documenting all the new API's we add.

[apolyakov]

Sure, will do it.

[jingham]

Thanks! No need for review...

[apolyakov]

It seems that target-select-so-path.test hangs on macOS. Thanks to @t.p.northover for noting this.

The debugserver doesn't have --pipe option, so on macOS it fails to start and the test hangs waiting for output from debugserver. Is there any alternative for --pipe option to let the debugserver choose a tcp port itself? I took a quick look at debugserver's sources and didn't find it.

[jingham]

It looks like lots of options to debugserver were added without also adding them to the --help output. Probably because there are so few clients they already know the options (the same person added & used them...) But if you look at the actual options in debugserver.cpp, you will see:

{"unix-socket", required_argument, NULL,
 'u'}, // If we need to handshake with our parent process, an option will be
       // passed down that specifies a unix socket name to use
{"fd", required_argument, NULL,
 '2'}, // A file descriptor was passed to this process when spawned that
       // is already open and ready for communication
{"named-pipe", required_argument, NULL, 'P'},
{"reverse-connect", no_argument, NULL, 'R'},

Will one of those serve your purpose?

Jim

[apolyakov]

I think those options don't fit. I'm looking for behavior like this:

~/workspace/gsoc/build/bin/lldb-server gdbserver --pipe 1 localhost:0
36251

Here lldb-server prints out the port number it is listening on and continues running.

If debugserver can choose a tcp port in case of passed hostname:0 then probably all we need to do is to add analogue of --pipe option.

[labath]

In D49739#1194037, @apolyakov wrote:

I think those options don't fit. I'm looking for behavior like this:

~/workspace/gsoc/build/bin/lldb-server gdbserver --pipe 1 localhost:0
36251

Here lldb-server prints out the port number it is listening on and continues running.

If debugserver can choose a tcp port in case of passed hostname:0 then probably all we need to do is to add analogue of --pipe option.

That's what --unix-socket and --named-pipe do, except that they write to the port number, well.. to a unix socket or a named pipe. You should be able to use that the same way as the --pipe argument, just the setup will be slightly different. However, adding support for the --pipe option should not be a problem either, given that all the pieces are already in place.

[teemperor]

This test is timing out for me on Arch Linux. Any idea why?

bash -x ./tools/lldb/lit/tools/lldb-mi/target/Output/target-select-so-path.test.script
+ set -o pipefail
+ set -x
+ : 'RUN: at line 3'
+ /data/llvm/dbg-build/./bin/clang -o /data/llvm/dbg-build/tools/lldb/lit/tools/lldb-mi/target/Output/target-select-so-path.test.tmp /home/me/llvm/llvm/tools/lldb/lit/tools/lldb-mi/target/inputs/main.c -g
+ : 'RUN: at line 4'
+ python /home/me/llvm/llvm/tools/lldb/lit/tools/lldb-mi/target/inputs/target-select-so-path.py '/data/llvm/dbg-build/bin/lldb-server gdbserver' '/data/llvm/dbg-build/bin/lldb-mi --synchronous /data/llvm/dbg-build/tools/lldb/lit/tools/lldb-mi/target/Output/target-select-so-path.test.tmp' /home/me/llvm/llvm/tools/lldb/lit/tools/lldb-mi/target/target-select-so-path.test
failed to write to the unnamed pipe: timed out

[tatyana-krasnukha]

On CentOS this test hangs forever. If I manually kill lldb-mi and lldb-server subprocesses, the output is

--
Exit Code: 1

Command Output (stderr):
--
/lldb/lit/tools/lldb-mi/target/target-select-so-path.test:16:10: error: CHECK: expected string not found in input
# CHECK: ^done
         ^
<stdin>:4:1: note: scanning from here
(gdb)
^
<stdin>:5:37: note: possible intended match here
=library-loaded,id="/lldb/lit/tools/lldb-mi/target/Output/target-select-so-path.test.tmp",target-name="/lldb/lit/tools/lldb-mi/target/Output/target-select-so-path.test.tmp",host-name="/lldb/lit/tools/lldb-mi/target/Output/target-select-so-path.test.tmp",symbols-loaded="0",loaded_addr="-",size="0"

[tatyana-krasnukha]

lit/tools/lldb-mi/target/target-select-so-path.test

4 ↗

(On Diff #158377)

Could you run the same python that is used for all python tests? (%python)

[apolyakov]

lit/tools/lldb-mi/target/target-select-so-path.test

4 ↗

(On Diff #158377)

Done in https://reviews.llvm.org/D52498

lldb/trunk/include/lldb/API/SBTarget.h

public:
   /// @return
   ///      A process object for the connected process.
   //------------------------------------------------------------------
   lldb::SBProcess ConnectRemote(SBListener &listener, const char *url,
                                 const char *plugin_name, SBError &error);
 
   lldb::SBFileSpec GetExecutable();
 
+  void AppendImageSearchPath(const char *from, const char *to,
+                             lldb::SBError &error);
+
   bool AddModule(lldb::SBModule &module);
 
   lldb::SBModule AddModule(const char *path, const char *triple,
                            const char *uuid);
 
   lldb::SBModule AddModule(const char *path, const char *triple,
                            const char *uuid_cstr, const char *symfile);
 

lldb/trunk/scripts/interface/SBTarget.i

public:
     ConnectRemote (SBListener &listener,
                    const char *url,
                    const char *plugin_name,
                    SBError& error);
 
     lldb::SBFileSpec
     GetExecutable ();
 
+    void
+    AppendImageSearchPath (const char *from,
+                           const char *to,
+                           SBError &error);
+
     bool
     AddModule (lldb::SBModule &module);
 
     lldb::SBModule
     AddModule (const char *path,
                const char *triple,
                const char *uuid);
 

lldb/trunk/source/API/SBTarget.cpp

if (target_sp) {
     std::unique_lock<std::recursive_mutex> lock;
     target_sp->GetWatchpointList().GetListMutex(lock);
     target_sp->RemoveAllWatchpoints();
     return true;
   }
   return false;
 }
 
+void SBTarget::AppendImageSearchPath(const char *from, const char *to,
+                                     lldb::SBError &error) {
+  TargetSP target_sp(GetSP());
+  if (!target_sp)
+    return error.SetErrorString("invalid target");
+
+  const ConstString csFrom(from), csTo(to);
+  if (!csFrom)
+    return error.SetErrorString("<from> path can't be empty");
+  if (!csTo)
+    return error.SetErrorString("<to> path can't be empty");
+
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
+  if (log)
+    log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
+                static_cast<void *>(target_sp.get()),  __FUNCTION__,
+                from, to);
+  target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
+}
+
 lldb::SBModule SBTarget::AddModule(const char *path, const char *triple,
                                    const char *uuid_cstr) {
   return AddModule(path, triple, uuid_cstr, NULL);
 }
 
 lldb::SBModule SBTarget::AddModule(const char *path, const char *triple,
                                    const char *uuid_cstr, const char *symfile) {
   lldb::SBModule sb_module;