Revert "[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute." and follow-ups

This reverts commit 9429b67b8e300e638d7828bbcb95585f85c4df4d.

It broke

Revert "[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute." and follow-ups

This reverts commit 9429b67b8e300e638d7828bbcb95585f85c4df4d.

It broke the build on Windows, see comments on https://reviews.llvm.org/D130309

It also reverts these follow-ups:

Revert "Fix buildbot breakage after https://reviews.llvm.org/D130309."
This reverts commit f959d815f4637890ebbacca379f1c38ab47e4e14.

Revert "Fix buildbot breakage after https://reviews.llvm.org/D130309."
This reverts commit 0bbce7a4c2d2bff622bdadd4323f93f5d90e6d24.

Revert "Cache the value for absolute path in FileSpec."
This reverts commit dabe877248b85b34878e75d5510339325ee087d0.

show more ...

[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.

The FileSpect APIs allow users to modify instance variables directly by getting a non const ref

[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.

The FileSpect APIs allow users to modify instance variables directly by getting a non const reference to the directory and filename instance variables. This makes it impossibly to control all of the times the FileSpec object is modified so we can clear the cache. This patch modifies the APIs of FileSpec so no one can modify the directory or filename directly by adding set accessors and by removing the get accessors that are non const.

Many clients were using FileSpec::GetCString(...) which returned a unique C string from a ConstString'ified version of the result of GetPath() which returned a std::string. This caused many locations to use this convenient function incorrectly and could cause many strings to be added to the constant string pool that didn't need to. Most clients were converted to using FileSpec::GetPath().c_str() when possible. Other clients were modified to use the newly renamed version of this function which returns an actualy ConstString:
ConstString FileSpec::GetPathAsConstString(bool denormalize = true) const;

This avoids the issue where people were getting an already uniqued "const char *" that came from a ConstString only to put the "const char *" back into a "ConstString" object. By returning the ConstString instead of a "const char *" clients can be more efficient with the result.

The patch:
- Removes the non const GetDirectory() and GetFilename() get accessors
- Adds set accessors to replace the above functions: SetDirectory() and SetFilename().
- Adds ClearDirectory() and ClearFilename() to replace usage of the FileSpec::GetDirectory().Clear()/FileSpec::GetFilename().Clear() call sites
- Fixed all incorrect usage of FileSpec::GetCString() to use FileSpec::GetPath().c_str() where appropriate, and updated other call sites that wanted a ConstString to use the newly returned ConstString appropriately and efficiently.

Differential Revision: https://reviews.llvm.org/D130309

show more ...

[lldb] Fix race condition between lldb-vscode and stop hooks executor

The race is between these two pieces of code that are executed in two separate
lldb-vscode threads (the first is in the main thr

[lldb] Fix race condition between lldb-vscode and stop hooks executor

The race is between these two pieces of code that are executed in two separate
lldb-vscode threads (the first is in the main thread and another is in the
event-handling thread):

```
// lldb-vscode.cpp
g_vsc.debugger.SetAsync(false);
g_vsc.target.Launch(launch_info, error);
g_vsc.debugger.SetAsync(true);
```

```
// Target.cpp
bool old_async = debugger.GetAsyncExecution();
debugger.SetAsyncExecution(true);
debugger.GetCommandInterpreter().HandleCommands(GetCommands(), exc_ctx,
options, result);
debugger.SetAsyncExecution(old_async);
```

The sequence that leads to the bug is this one:
1. Main thread enables synchronous mode and launches the process.
2. When the process is launched, it generates the first stop event.
3. This stop event is catched by the event-handling thread and DoOnRemoval
is invoked.
4. Inside DoOnRemoval, this thread runs stop hooks. And before running stop
hooks, the current synchronization mode is stored into old_async (and
right now it is equal to "false").
5. The main thread finishes the launch and returns to lldb-vscode, the
synchronization mode is restored to asynchronous by lldb-vscode.
6. Event-handling thread finishes stop hooks processing and restores the
synchronization mode according to old_async (i.e. makes the mode synchronous)
7. And now the mode is synchronous while lldb-vscode expects it to be
asynchronous. Synchronous mode forbids the process to broadcast public stop
events, so, VS Code just hangs because lldb-vscode doesn't notify it about
stops.

So, this diff makes the target intercept the first stop event if the process is
launched in the synchronous mode, thus preventing stop hooks execution.

The bug is only present on Windows because other platforms already
intercept this event using their own hijacking listeners.

So, this diff also fixes some problems with lldb-vscode tests on Windows to make
it possible to run the related test. Other tests still can't be enabled because
the debugged program prints something into stdout and LLDB can't intercept this
output and redirect it to lldb-vscode properly.

Reviewed By: jingham

Differential Revision: https://reviews.llvm.org/D119548

show more ...

[lldb/Platform] s/m_name/m_hostname

m_name is confusing, as there is a getter called GetName, but it returns
a completely different thing.

[lldb] Rename Logging.h to LLDBLog.h and clean up includes

Most of our code was including Log.h even though that is not where the
"lldb" log channel is defined (Log.h defines the generic logging
inf

[lldb] Rename Logging.h to LLDBLog.h and clean up includes

Most of our code was including Log.h even though that is not where the
"lldb" log channel is defined (Log.h defines the generic logging
infrastructure). This worked because Log.h included Logging.h, even
though it should.

After the recent refactor, it became impossible the two files include
each other in this direction (the opposite inclusion is needed), so this
patch removes the workaround that was put in place and cleans up all
files to include the right thing. It also renames the file to LLDBLog to
better reflect its purpose.

show more ...

[lldb] Convert "LLDB" log channel to the new API

[lldb/platform-gdb] Clear cached protocol state upon disconnection

Previously we would persist the flags indicating whether the remote side
supports a particular feature across reconnects, which is

[lldb/platform-gdb] Clear cached protocol state upon disconnection

Previously we would persist the flags indicating whether the remote side
supports a particular feature across reconnects, which is obviously not
a good idea.

I implement the clearing by nuking (its the only way to be sure :) the
entire GDBRemoteCommunication object in the disconnect operation and
creating a new one upon connection. This allows us to maintain a nice
invariant that the GDBRemoteCommunication object (which is now a
pointer) exists only if it is connected. The downside to that is that a
lot of functions now needs to check the validity of the pointer instead
of blindly accessing the object.

The process communication does not suffer from the same issue because we
always destroy the entire Process object for a relaunch.

Differential Revision: https://reviews.llvm.org/D116539

show more ...

Remove redundant string initialization (NFC)

Identified with readability-redundant-string-init.

[lldb] Remove reproducer replay functionality

This is part of a bigger rework of the reproducer feature. See [1] for
more details.

[1] https://lists.llvm.org/pipermail/lldb-dev/2021-September/01704

[lldb] Remove reproducer replay functionality

This is part of a bigger rework of the reproducer feature. See [1] for
more details.

[1] https://lists.llvm.org/pipermail/lldb-dev/2021-September/017045.html

show more ...

[lldb] Port PlatformWindows, PlatformOpenBSD and PlatformRemoteGDBServer to GetSupportedArchitectures

[lldb] Refactor Platform::ResolveExecutable

Module resolution is probably the most complex piece of lldb [citation
needed], with numerous levels of abstraction, each one implementing
various retry a

[lldb] Refactor Platform::ResolveExecutable

Module resolution is probably the most complex piece of lldb [citation
needed], with numerous levels of abstraction, each one implementing
various retry and fallback strategies.

It is also a very repetitive, with only small differences between
"host", "remote-and-connected" and "remote-but-not-(yet)-connected"
scenarios.

The goal of this patch (first in series) is to reduce the number of
abstractions, and deduplicate the code.

One of the reasons for this complexity is the tension between the desire
to offload the process of module resolution to the remote platform
instance (that's how most other platform methods work), and the desire
to keep it local to the outer platform class (its easier to subclass the
outer class, and it generally makes more sense).

This patch resolves that conflict in favour of doing everything in the
outer class. The gdb-remote (our only remote platform) implementation of
ResolveExecutable was not doing anything gdb-specific, and was rather
similar to the other implementations of that method (any divergence is
most likely the result of fixes not being applied everywhere rather than
intentional).

It does this by excising the remote platform out of the resolution
codepath. The gdb-remote implementation of ResolveExecutable is moved to
Platform::ResolveRemoteExecutable, and the (only) call site is
redirected to that. On its own, this does not achieve (much), but it
creates new opportunities for layer peeling and code sharing, since all
of the code now lives closer together.

Differential Revision: https://reviews.llvm.org/D113487

show more ...

Modernize Platform::GetOSKernelDescription

[lldb] Remove ConstString from Platform plugin names

[lldb] Modernize Platform::GetOSBuildString

[lldb] [Utility/UriParser] Return results as 'struct URI'

Return results of URI parsing as 'struct URI' instead of assigning them
via output parameters.

Differential Revision: https://reviews.llvm.

[lldb] [Utility/UriParser] Return results as 'struct URI'

Return results of URI parsing as 'struct URI' instead of assigning them
via output parameters.

Differential Revision: https://reviews.llvm.org/D112314

show more ...

[lldb] [Utility/UriParser] Replace port==-1 with llvm::None

Use llvm::Optional<uint16_t> instead of int for port number
in UriParser::Parse(), and use llvm::None to indicate missing port
instead of

[lldb] [Utility/UriParser] Replace port==-1 with llvm::None

Use llvm::Optional<uint16_t> instead of int for port number
in UriParser::Parse(), and use llvm::None to indicate missing port
instead of a magic value of -1.

Differential Revision: https://reviews.llvm.org/D112309

show more ...

[lldb] Return StringRef from PluginInterface::GetPluginName

There is no reason why this function should be returning a ConstString.

While modifying these files, I also fixed several instances where

[lldb] Return StringRef from PluginInterface::GetPluginName

There is no reason why this function should be returning a ConstString.

While modifying these files, I also fixed several instances where
GetPluginName and GetPluginNameStatic were returning different strings.

I am not changing the return type of GetPluginNameStatic in this patch, as that
would necessitate additional changes, and this patch is big enough as it is.

Differential Revision: https://reviews.llvm.org/D111877

show more ...

[lldb] Make Platform::DebugProcess take a Target reference

instead of a pointer. There are just two callers of this function, and
both of them have a valid target pointer, so there's no need for all

[lldb] Make Platform::DebugProcess take a Target reference

instead of a pointer. There are just two callers of this function, and
both of them have a valid target pointer, so there's no need for all
implementations to concern themselves with whether the pointer is null.

show more ...

Revert "Revert "Reset the wakeup timeout when we re-enter the continue wait.""

This reverts commit 82a38837150099288a1262391ef43e1fd69ffde4.

The original version had a copy-paste error: using the I

Revert "Revert "Reset the wakeup timeout when we re-enter the continue wait.""

This reverts commit 82a38837150099288a1262391ef43e1fd69ffde4.

The original version had a copy-paste error: using the Interrupt timeout
for the ResumeSynchronous wait, which is clearly wrong. This error would
have been evident with real use, but the interrupt is long enough that it
only caused one testsuite failure (in the Swift fork).

Anyway, I found that mistake and fixed it and checked all the other places
where I had to plumb through a timeout, and added a test with a short
interrupt timeout stepping over a function that takes 3x the interrupt timeout
to complete, so that should detect a similar mistake in the future.

show more ...

[lldb] Replace default bodies of special member functions with = default;

Replace default bodies of special member functions with = default;

$ run-clang-tidy.py -header-filter='lldb' -checks='-*,mo

[lldb] Replace default bodies of special member functions with = default;

Replace default bodies of special member functions with = default;

$ run-clang-tidy.py -header-filter='lldb' -checks='-*,modernize-use-equals-default' -fix ,

https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html

Differential revision: https://reviews.llvm.org/D104041

show more ...

Revert "Reset the wakeup timeout when we re-enter the continue wait."

This reverts commit bd5751f3d249ec0798060bd98c07272174c52af0.
This patch series is causing us to every so often miss switching
t

Revert "Reset the wakeup timeout when we re-enter the continue wait."

This reverts commit bd5751f3d249ec0798060bd98c07272174c52af0.
This patch series is causing us to every so often miss switching
the state from eStateRunning to eStateStopped when we get the stop
packet from the debug server.

Reverting till I can figure out how that could be happening.

show more ...

Add an "interrupt timeout" to Process, and pipe that through the
ProcessGDBRemote plugin layers.

Also fix a bug where if we tried to interrupt, but the ReadPacket
wakeup timer woke us up just after

Add an "interrupt timeout" to Process, and pipe that through the
ProcessGDBRemote plugin layers.

Also fix a bug where if we tried to interrupt, but the ReadPacket
wakeup timer woke us up just after the timeout, we would break out
the switch, but then since we immediately check if the response is
empty & fail if it is, we could end up actually only giving a
small interval to the interrupt.

Differential Revision: https://reviews.llvm.org/D102085

show more ...

[lldb] Move PlatformPOSIX::ConnectToWaitingProcesses to RemoteAwarePlatform

The functionality is not posix specific. Also force the usage of the
gdb-remote process plugin in the gdb platform class.

[lldb] Move PlatformPOSIX::ConnectToWaitingProcesses to RemoteAwarePlatform

The functionality is not posix specific. Also force the usage of the
gdb-remote process plugin in the gdb platform class.

This is not sufficient to make TestPlatformConnect pass on windows (it
seems it suffers from module loading issues, unrelated to this test),
but it at least makes it shut down correctly, so I change the skip to an
xfail.

show more ...

[lldb] Apply gdb-remote timeout to platform connections as well

We have a plugin.process.gdb-remote.packet-timeout setting, which can be
used to control how long the lldb client is willing to wait b

[lldb] Apply gdb-remote timeout to platform connections as well

We have a plugin.process.gdb-remote.packet-timeout setting, which can be
used to control how long the lldb client is willing to wait before
declaring the server side dead. Our test suite makes use of this
feature, and sets the setting value fairly high, as the low default
value can cause flaky tests, particularly on slower bots.

After fixing TestPlatformConnect (one of the few tests exercising the
remote platform capabilities of lldb) in 4b284b9ca, it immediately
started being flaky on the arm bots. It turns out this is because the
packet-timeout setting is not being applied to platform connections.

This patch makes the platform connections also respect the value of this
setting. It also adds a test which checks that the timeout value is
being honored.

Differential Revision: https://reviews.llvm.org/D97769

show more ...

[lldb] Fix TestPlatformProcessConnect.py

The test was marked as remote-only, which means it was run ~never, and
accumulated various problems. This commit modifies the test to run
locally and include

[lldb] Fix TestPlatformProcessConnect.py

The test was marked as remote-only, which means it was run ~never, and
accumulated various problems. This commit modifies the test to run
locally and includes a couple of other fixes necessary to make it run:
- moves the "invoke" method into the "Base" test class
- adds []'s around the IP address in a couple more places to make things
work with IPv6

The test is now marked as skipped when running the remote test suite. It
would be possible to make it run both locally and remotely, but this
would require writing a lot special logic for the remote case, and that
is not worth it.

show more ...