Page MenuHomePhabricator

[vscode] Improve runInTerminal and support linux
ClosedPublic

Authored by wallace on Dec 30 2020, 3:03 PM.

Details

Summary

Depends on D93874.

runInTerminal was using --wait-for, but it was some problems because it uses process polling looking for a single instance of the debuggee:

  • it gets to know of the target late, which renders breakpoints in the main function almost impossible
  • polling might fail if there are already other processes with the same name
  • polling might also fail on some linux machine, as it's implemented with the ps command, and the ps command's args and output are not standard everywhere

As a better way to implement this so that it works well on Darwin and Linux, I'm using now the following process:

  • lldb-vscode notices the runInTerminal, so it spawns lldb-vscode with a special flag --launch-target <target>. This flags tells lldb-vscode to wait to be attached and then it execs the target program. I'm using lldb-vscode itself to do this, because it makes finding the launcher program easier. Also no CMAKE INSTALL scripts are needed.
  • Besides this, the debugger creates a temporary FIFO file where the launcher program will write its pid to. That way the debugger will be sure of which program to attach.
  • Once attach happend, the debugger creates a second temporary file to notify the launcher program that it has been attached, so that it can then exec. I'm using this instead of using a signal or a similar mechanism because I don't want the launcher program to wait indefinitely to be attached in case the debugger crashed. That would pollute the process list with a lot of hanging processes. Instead, I'm setting a 20 seconds timeout (that's an overkill) and the launcher program seeks in intervals the second tepmorary file.

Some notes:

  • I also fixed a small issue preventing preRunCommands to be executed with runInTerminal
  • stopOnEntry true and false values work correctly as well
  • I preferred not to use sockets because it requires a lot of code and I only need a pid. It would also require a lot of code when windows support is implemented.
  • I didn't add Windows support, as I don't have a windows machine, but adding support for it should be easy, as the FIFO file can be implemented with a named pipe, which is standard on Windows and works pretty much the same way.

The existing test which didn't pass on Linux, now passes.

Diff Detail

Event Timeline

wallace created this revision.Dec 30 2020, 3:03 PM
wallace requested review of this revision.Dec 30 2020, 3:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2020, 3:03 PM
wallace edited the summary of this revision. (Show Details)Dec 30 2020, 3:08 PM
clayborg requested changes to this revision.Jan 4 2021, 12:49 PM

Great stuff! See inline comments.

A few new tests need to be added:

  • test running lldb-vscode with invalid "--launch-target" executable path so things fail to exec and verify we get as nice of an error message as possible
  • test running lldb-vscode with valid "--launch-target" and no "--pid-file" option to make sure lldb-vscode fails gracefully with an error and appropriate message
  • test running lldb-vscode with valid "--launch-target" and an invalid "--pid-file" path with an invalid directory
  • test running lldb-vscode with valid "--launch-target" and an invalid "--pid-file" path that points somewhere we don't have permissions

All these tests will ensure we don't end up with a deadlock situation when trying to launch in terminal.

lldb/tools/lldb-vscode/lldb-vscode.cpp
1465

If windows is not supported, should we avoid calling this function (CreateRunInTerminalPidFile) in the first place?

1477–1480

Check for invalid LLDB_INVALID_PROCESS_ID here in case something went wrong when trying to read the "pid" and return an error

1483

What happens if the pid never gets written to the stream? Deadlock forever?

Also, what happens if an error happens during the read? Can we check if the stream has an error after readind "pid" just in case? If there is error, (like what if the other process wrote "hello" to the stream instead of "123"), what ends up in "pid"? Is it set to zero? Or just left as a random number? If it is left alone, we should initialize "pid" with the LLDB_INVALID_PROCESS_ID on line 1470

1483–1484

Do we need more to this comment? Aren't we continuing the lldb-vscode that will exec into our program here? If we are, we should clearly explain what is happening here.

2981

This path can be relative, so we might need to resolve it using a llvm path call? I can't remember if execv() can use a relative path or not.

2989

We need a nice beefy comment here as to what is going on explaining everything.

2990

Does the llvm argument parser already guarantee that we have a valid value in pid_file? If not, we need to make sure "pid_file" is not empty here and return an error if it wasn't specified

2991

error check that the creation of the ofstream succeeded and return an error if it fails and exit with EXIT_FAILURE

3000

Should we verify that "target" is a valid file that exists prior to just calling execv? We might be able to return a better error message. Or we can check if the file exists after the "execv(...)" call and return a better error message than "Couldn't launch target" like "Failed to launch '/path/that/doesnt/exists/a.out'".

3003

So is the code calling lldb-vscode with "--launch-target ... --pid-file ..." watching for a non zero return value? If something goes wrong does this show up in the terminal for the process?

This revision now requires changes to proceed.Jan 4 2021, 12:49 PM
wallace updated this revision to Diff 315015.EditedJan 6 2021, 4:03 PM
wallace marked 10 inline comments as done.

Address all comments.

  • Moved all the communication logic between the debug adaptor and the launcher to classes, simplifying the code in lldb-vscode.
  • Added more comments and tests
  • Created a file where lldb-vscode can write errors to.
  • Now returning any error messages to the IDE, so that the user can see what's going on. No exit() calls are using in the debug adaptor side.

Errors in the IDE are displayed like this:

clayborg requested changes to this revision.Jan 6 2021, 5:34 PM

So I like that we are using files in the file system for things, but we are using 3 of them and that seems excessive. See inlined comments and see if we can get down to using just one? Also, what is stopping us from being able to support windows here? Just mkfifo?

lldb/tools/lldb-vscode/RunInTerminal.cpp
30

These permissions seem bad, probably should be 0600 to only allow current user to access for security purposes.

Also, any reason we are using mkfifo over just a normal temp file? Is this the only thing that keeps us from using this on windows? Could we have windows support if just create a normal file like std::ofstream and std::ifstream?

lldb/tools/lldb-vscode/RunInTerminal.h
33–39

Can we avoid using more than one file here? Might be nice to require just one file that gets a JSON dictionary as the contents?

{ "pid": 123 }

or

{ "error" : "<error message>" }

Then we need to read data until we get correctly formed JSON or timeout.

lldb/tools/lldb-vscode/lldb-vscode.cpp
1394–1396

Reverse this to avoid the negate?

1456–1460

We shouldn't need any #if define(_WIN32) since we didn't enable this feature in the response to "initialize" right?

2982

Might be worth checking if there is a llvm file system call to determine the program path already?

2990

Don't use auto

2991

don't use auto

3001

Is "[[noreturn]]" supported by all compilers that compile LLDB? There might be some LLVM define that we should use?

3013

Why are we using env vars for some things and options values for others? We should probably pass this in as an argument. We are also polluting the env vars of the new process we will exec if we leave this in the environment

3020

I assume env vars are being inherited from this process?

This revision now requires changes to proceed.Jan 6 2021, 5:34 PM

We should also add a test that makes sure that env vars set in the launch config are received in the program that runs in the terminal

And a test for current working directory

I'll follow your recommendations.

Regarding Windows, mkfifo can be replaced with CreateNamedPipe and should lead to the same behavior. Sadly, I don't have a Windows machine where I could even compile it... So this should be left for someone else or for when I can get my laptop from the office.

And why are fifo files needed? Well, when you open a normal text file, c++ doesn't just know when the file gets updated. Its pointer to the end of the file is set when you open the file. So if you want to get updates, you have to reopen the file in a polling fashion. Fifo files, on the other hand, which are the same as pipes, behave the same way as stdin, so c++ keeps reading it until it's empty and closed at OS level, aware of new data being written to it

wallace marked an inline comment as done.Jan 7 2021, 10:00 AM

I'll think about just writing this as tcp sockets. That would for sure be cross platform

wallace marked 5 inline comments as done.Jan 7 2021, 10:18 AM
wallace added inline comments.
lldb/tools/lldb-vscode/lldb-vscode.cpp
2982

it seems there isn't

3013

I'm only using this for testing, so that the python tests doesn't take 10 seconds waiting for the timeout to happen. The user shouldn't need to change this for any reason.

3020

yes, that's the default behavior

wallace updated this revision to Diff 315527.Jan 8 2021, 3:08 PM

Followed all the suggestions:

  • This is POSIX-only, but making it work for windows should only require deleting a few #ifdef blocks and using the CreateNamedPipe API instead of mkfifo. I'll do it later in another diff.
  • Added more tests covering different possible failures and making sure that environment variables are passed correctly.
  • Now using JSON for the messages. That will make more robust if in the future we need to send more information back to the debugger.
  • I tried to keep it as simple as possible, but still created a few abstractions to keep each piece very readable.

Can we still try to use just one file? Isn't this file both read and write? The flow would be:

  • create 1 file using mkfifo(...) and pass it down as argument
  • launcher lldb-vscode will write pid or error JSON back into the stream, and will start to wait for some data from the same fifo file
  • normal lldb-vscode will write some JSON to fifo file to indicate it has attached after it has attached
  • launcher lldb-vscode reads that it attached and then does the exec

It would really simplify everything if we can use just one file

lldb/tools/lldb-vscode/RunInTerminal.cpp
94

Need to use the llvm file system appending stuff so this will work on windows.

llvm::SmallString<64> current_path = comm_dir.str(); // this might not compile, but you get the idea...
style = ...; // Set this correctly with #ifdef for windows
llvm::sys::path::append(comm_dir, style, "to_adaptor");
98

ditto

lldb/tools/lldb-vscode/lldb-vscode.cpp
437–438

Is this intentional??

1500

Do we want to set async to true prior to doing the continue?

wallace updated this revision to Diff 315652.Jan 9 2021, 10:29 PM

Addressed all comments:

  • Now using a single file for all the communication. Did indeed simplified a lot of code
  • Now adding timeouts to Send operations. This will help prevent the adaptor from undefinitely waiting for the launcher to read the data.
  • I added more comments, specially in the part where the process attaches and resumes
wallace updated this revision to Diff 315655.Jan 9 2021, 11:19 PM

improve tests

clayborg accepted this revision.Jan 11 2021, 2:53 PM

Just one rename of an instance variable and this is good to go! Much cleaner without the directory and multiple files.

lldb/tools/lldb-vscode/FifoFiles.h
27

rename to "m_path" since this is an instance variable.

This revision is now accepted and ready to land.Jan 11 2021, 2:53 PM
This revision was landed with ongoing or failed builds.Jan 25 2021, 12:30 PM
This revision was automatically updated to reflect the committed changes.

It looks like this broke the windows lldb bot:

lab.llvm.org:8011/%23/builders/83/builds/3021

I've already submitted a fix, let's see if the buildbot gets fixed

Il giorno lun 25 gen 2021 alle ore 14:18 Stella Stamenova via Phabricator <
reviews@reviews.llvm.org> ha scritto:

stella.stamenova added a comment.

It looks like this broke the windows lldb bot:

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D93951/new/

https://reviews.llvm.org/D93951

I am seeing this consistently fail on our internal Ubuntu bot:

ERROR: test_missingArgInRunInTerminalLauncher (TestVSCode_runInTerminal.TestVSCode_runInTerminal)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/_work/5/s/lldb/packages/Python/lldbsuite/test/decorators.py", line 135, in wrapper
    return func(*args, **kwargs)
  File "/mnt/_work/5/s/lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py", line 92, in test_missingArgInRunInTerminalLauncher
    capture_output=True, universal_newlines=True)
  File "/usr/lib/python3.6/subprocess.py", line 423, in run
    with Popen(*popenargs, **kwargs) as process:
TypeError: __init__() got an unexpected keyword argument 'capture_output'
Config=x86_64-/mnt/_work/5/b/llvm/bin/clang
======================================================================
FAIL: test_runInTerminal (TestVSCode_runInTerminal.TestVSCode_runInTerminal)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/_work/5/s/lldb/packages/Python/lldbsuite/test/decorators.py", line 135, in wrapper
    return func(*args, **kwargs)
  File "/mnt/_work/5/s/lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py", line 54, in test_runInTerminal
    env=["FOO=bar"])
  File "/mnt/_work/5/s/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py", line 353, in build_and_launch
    terminateCommands, sourcePath, debuggerRoot, runInTerminal=runInTerminal)
  File "/mnt/_work/5/s/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py", line 329, in launch
    'launch failed (%s)' % (response['message']))
AssertionError: False is not True : launch failed (Failed to attach to the target process. Timed out trying to get messages from the runInTerminal launcher)
Config=x86_64-/mnt/_work/5/b/llvm/bin/clang
----------------------------------------------------------------------
Ran 7 tests in 2.447s

RESULT: FAILED (5 passes, 1 failures, 1 errors, 0 skipped, 0 expected failures, 0 unexpected successes)
Timed out trying to get messages from the debug adaptor

@stella.stamenova , is that build a Debug or Release build? I think that will help me determine why it's failing

@stella.stamenova , is that build a Debug or Release build? I think that will help me determine why it's failing

Release. It passed in Debug.

I'll send a diff tomorrow morning

lldb/tools/lldb-vscode/FifoFiles.cpp
9

Also, this is no good. It works if you are targeting windows on windows, but not if you are targeting something else and building on windows. There are a few different ways this is done in LLVM/clang and they are generally not based on defined(WIN32). Here are a couple of examples:

From llvm\lib\Support\Path.cpp:

#if !defined(_MSC_VER) && !defined(__MINGW32__)

From clang\lib\Driver\Driver.cpp:

#if LLVM_ON_UNIX

From llvm\lib\Support\ErrorHandling.cpp:

#if defined(HAVE_UNISTD_H)

I suggest browsing through the code and finding the most appropriate way to manage your includes. In the mean time, this is breaking our internal builds that run on Windows but do not target Windows.

Good to know. I'll work on that right now. Thanks!

Il giorno mer 27 gen 2021 alle ore 11:41 Stella Stamenova via Phabricator <
reviews@reviews.llvm.org> ha scritto:

stella.stamenova added inline comments.

Comment at: lldb/tools/lldb-vscode/FifoFiles.cpp:9
+
+#if !defined(WIN32)

+#include <sys/stat.h>

Also, this is no good. It works if you are targeting windows on windows,
but not if you are targeting something else and building on windows. There
are a few different ways this is done in LLVM/clang and they are generally
not based on defined(WIN32). Here are a couple of examples:

From llvm\lib\Support\Path.cpp:

#if !defined(_MSC_VER) && !defined(__MINGW32__)

From clang\lib\Driver\Driver.cpp:

#if LLVM_ON_UNIX

From llvm\lib\Support\ErrorHandling.cpp:

#if defined(HAVE_UNISTD_H)

I suggest browsing through the code and finding the most appropriate way
to manage your includes. In the mean time, this is breaking our internal
builds that run on Windows but do not target Windows.

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D93951/new/

https://reviews.llvm.org/D93951

@stella.stamenova , I've just pushed a fix. Let me know if it actually works.

mstorsjo added inline comments.
lldb/tools/lldb-vscode/FifoFiles.cpp
9

@stella.stamenova defined(WIN32) shouldn't be used (as that isn't predefined by the compiler), but defined(_WIN32) should be just fine (and is used a lot within LLDB already, 176 occurrances). If cross compiling on windows, targeting another os, neither of them should be defined, so it should be just fine, no?

Or is this a case of cygwin, where the lines are blurred even more?

Does the current main branch work fine for you?

lldb/tools/lldb-vscode/FifoFiles.cpp
9

defined(_WIN32) is fine for us, since we build on Windows (and not cygwin). I have not tried the lldb-vscode build on cygwin, so that may or may not work with the current setup.

In general, there are too many ways in LLVM to achieve the same excludes/includes right now and the lack of unity is only creating issues and confusion.