Reverts llvm/llvm-project#132274
Broke a test on LLDB Widows on Arm:
https://lab.llvm.org/buildbot/#/builders/141/builds/7726
```
FAIL: test_dwarf (lldbsuite.test.lldbtest.TestExternCSymbols.test_dwarf)
<...>
self.assertTrue(self.res.Succeeded(), msg + output)
AssertionError: False is not true : Command 'expression -- foo()' did not return successfully
Error output:
error: Couldn't look up symbols:
int foo(void)
Hint: The expression tried to call a function that is not present in the target, perhaps because it was optimized out by the compiler.
```
Add the Data Inspection Language (DIL) implementation pieces for
handling plain local and global variable names.
See https://discourse.llvm.org/t/rfc-data-inspection-language/69893 for
information about DIL.
This change includes the basic AST, Lexer, Parser and Evaluator pieces,
as well as some tests.
This is an attempt to fix a test failure from #133794 when running on
windows builds. I suspect we are running into a case where the
[ICF](https://learn.microsoft.com/en-us/cpp/build/reference/opt-optimizations?view=msvc-170)
optimization kicks in and combines the CreateSystemRuntimePlugin*
functions into a single address. This means that we cannot uniquely
unregister the plugin based on its create function address.
The fix is have each create function return a different (bogus) value.
This commit adds support for enabling and disabling plugins by name. The
changes are made generically in the `PluginInstances` class, but
currently we only expose the ability to SystemRuntime plugins. Other
plugins types can be added easily.
We had a few design goals for how disabled plugins should work
1. Plugins that are disabled should still be visible to the system. This
allows us to dynamically enable and disable plugins and report their
state to the user.
2. Plugin order should be stable across disable and enable changes. We
want avoid changing the order of plugin lookup. When a plugin is
re-enabled it should return to its original slot in the creation order.
3. Disabled plugins should not appear in PluginManager operations.
Clients should be able to assume that only enabled plugins will be
returned from the PluginManager.
For the implementation we modify the plugin instance to maintain a bool
of its enabled state. Existing clients external to the Instances class
expect to iterate over only enabled instance so we skip over disabed
instances in the query and snapshot apis. This way the client does not
have to manually check which instances are enabled.
Hoist UUID generation into the UUID class and add a trivial unit test.
This also changes the telemetry code to drop the double underscore if we
failed to generate a UUID and subsequently logs to the Host instead of
Object log channel.
The tests for the
[intel-pt](3483740289/lldb/docs/use/intel_pt.rst)
trace plugin were failing for multiple reasons.
On machines where tracing is supported many of the tests were crashing
because of a nullptr dereference. It looks like the `core_file`
parameter in `ProcessTrace::CreateInstance` was once ignored, but was
changed to always being dereferenced. This caused the tests to fail even
when tracing was supported.
On machines where tracing is not supported we would still run tests that
attempt to take a trace. These would obviously fail because the required
hardware is not present. Note that some of the tests simply read
serialized json as trace files which does not require any special
hardware.
This PR fixes these two issues by guarding the pointer dereference and
then skipping unsupported tests on machines. With these changes the
trace tests pass on both types of machines.
We also add a new unit test to validate that a process can be created
with a nullptr core_file through the generic process trace plugin path.
So the dSYM can be told what target it has been loaded into.
When lldb is loading modules, while creating a target, it will run
"command script import" on any Python modules in Resources/Python in the
dSYM. However, this happens WHILE the target is being created, so it is
not yet in the target list. That means that these scripts can't act on
the target that they a part of when they get loaded.
This patch adds a new python API that lldb will call:
__lldb_module_added_to_target
if it is defined in the module, passing in the Target the module was
being added to, so that code in these dSYM's don't have to guess.
The `locked` variable can be accessed from the asynchronous thread until
the call to f.wait() completes. However, the variable is scoped in a
lexical block that ends before that, leading to a use-after-free.
Expose u target API mutex through the SB API. This is motivated by
lldb-dap, which is built on top of the SB API and needs a way to execute
a series of SB API calls in an atomic manner (see #131242).
We can solve this problem by either introducing an additional layer of
locking at the DAP level or by exposing the existing locking at the SB
API level. This patch implements the second approach.
This was discussed in an RFC on Discourse [0]. The original
implementation exposed a move-only lock rather than a mutex [1] which
doesn't work well with SWIG 4.0 [2]. This implement the alternative
solution of exposing the mutex rather than the lock. The SBMutex
conforms to the BasicLockable requirement [3] (which is why the methods
are called `lock` and `unlock` rather than Lock and Unlock) so it can be
used as `std::lock_guard<lldb::SBMutex>` and
`std::unique_lock<lldb::SBMutex>`.
[0]: https://discourse.llvm.org/t/rfc-exposing-the-target-api-lock-through-the-sb-api/85215/6
[1]: https://github.com/llvm/llvm-project/pull/131404
[2]: https://discourse.llvm.org/t/rfc-bumping-the-minimum-swig-version-to-4-1-0/85377/9
[3]: https://en.cppreference.com/w/cpp/named_req/BasicLockable
Using argv[0] for this was incorrect. I'm ignoring LaunchInfo::SetArg0,
as that's what darwin and windows launchers do (they use the first
element of the args vector instead).
I picked up the funny unit test re-exec method from the llvm unit tests.
To be able to describe discontinuous functions, this patch changes the
UnwindPlan to accept more than one address range.
I've also squeezed in a couple improvements/modernizations, for example
using the lower_bound function instead of a linear scan.
This moves two functions from Platform to Host:
1. GetCurrentXcodeToolchainDirectory
2. GetCurrentCommandLineToolsDirectory.
These two functions caused a layering violation in the Swift fork, which
added a dependency from lldbHost to lldbPlatform. As show by this PR,
there's no need for these two functions to live in Platform, and we
already have similar functions in Host.
We have various layering violations but this one is particularly bad,
because lldb-dap started depending on lldbHost. On the Swift fork, this
library was depending on lldbPlatform which pulled in various Swift
files, which libLLDB needs, but lldb-dap itself does not. We were
missing RPATHs to resume them, so in the current nightly, lldb-dap
crashes because the dynamic loader can't find the missing Swift libs.
rdar://146537366
In most places, the rows are copied anyway (because they are generated
by cumulating modifications) immediately after adding them to the unwind
plans. In others, they can be moved into the unwind plan. This lets us
remove some backflip copies and make `const UnwindPlan` actually mean
something.
I've split this patch into two (and temporarily left both APIs) as this
patch was getting a bit big. This patch covers all the interesting
cases. Part two all about converting "architecture default" unwind plans
from ABI and InstructionEmulation plugins.
The class was taking ownership of the SBCommandPluginInterface pointer
it was passed in, by wrapping it in a shared pointer. This causes a
double free in the unit test when the object is destroyed and the same
pointer gets freed once when the SBCommandPluginInterface goes away and
then again when the shared pointer hits a zero refcount.
After https://reviews.llvm.org/D116539, when `m_gdb_client_up` in
`PlatformRemoteGDBServer` is not null, the connection to a server is
expected to exist. However,
`PlatformRemoteGDBServer::DisconnectRemote()` is not the only way to
close the connection;
`GDBRemoteCommunication::WaitForPacketNoLock()` can disconnect if the
server stops responding, and in this case `m_gdb_client_up` is not
cleared. The patch removes this assumption and checks the connection
status directly.
The `DW_AT_APPLE_sdk` should always be equal to the filename of the
`DW_AT_LLVM_sysroot`. We can use this property to simplify
`XcodeSDK::Merge` to no longer manually adjust the sysroot filename.
Instead we simply update the sysroot filename with merged SDK name.
This should be an NFC change.
This commit modifies the `DebuggerStats::ReportStatistics`
implementation to avoid loading symbol files for unloaded symbols. We
collect stats on debugger shutdown and without this change it can cause
the debugger to hang for a long while on shutdown if they symbols were
not previously loaded (e.g. `settings set target.preload-symbols
false`).
The implementation is done by adding an optional parameter to
`Module::GetSymtab` to control if the corresponding symbol file will be
loaded in the same way that can control it for `Module::GetSymbolFile`.
Calling the public API `SBLineEntry::SetColumn()` sets the row instead
of the column.
This probably should be backported as it has been since version 3.4.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
Remove support for coalescing progress reports in LLDB. This
functionality was motivated by Xcode, which wanted to listen for less
frequent, aggregated progress events at the cost of losing some detail.
See the original RFC [1] for more details. Since then, they've
reevaluated this trade-off and opted to listen for the regular, full
fidelity progress events and do any post processing on their end.
rdar://146425487
and collect telemetry about a command's execution.
*NOTE: Please consider this PR a DRAFT ( Waiting on PR/127696 to be
submitted. )
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes it
easier to diagnose problems thanks to more meaningful error messages
being available. GetBitSize() is often the first thing LLDB asks about a
type, so this method is particularly important for a better user
experience.
rdar://145667239
This reverts commit 6041c745f32e8fd60ed24e29e7d919d8d1c87ca6.
Relands the original patch with the test-case data fixed. Weirldy the PR CI
didn't seem to run the unit-tests? In any case, the problem was an
incorrect expectation in the test-case data. Since we have both public
and internal SDK in that test-case, we should `expect_mismatch` to be
`true`.
Reverts llvm/llvm-project#128712
```
******************** TEST 'lldb-unit :: SymbolFile/DWARF/./SymbolFileDWARFTests/10/14' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/SymbolFile/DWARF/./SymbolFileDWARFTests-lldb-unit-1021-10-14.json GTEST_SHUFFLE=1 GTEST_TOTAL_SHARDS=14 GTEST_SHARD_INDEX=10 GTEST_RANDOM_SEED=62233 /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/SymbolFile/DWARF/./SymbolFileDWARFTests
--
Script:
--
/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/SymbolFile/DWARF/./SymbolFileDWARFTests --gtest_filter=SDKPathParsingTests/SDKPathParsingMultiparamTests.TestSDKPathFromDebugInfo/6
--
/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp:265: Failure
Expected equality of these values:
found_mismatch
Which is: true
expect_mismatch
Which is: false
/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp:265
Expected equality of these values:
found_mismatch
Which is: true
expect_mismatch
Which is: false
```
`GetSDKRoot` uses `xcrun` to find an SDK root path for a given SDK
version string. But if the SDK doesn't exist in the Xcode installations,
but instead lives in the `CommandLineTools`, `xcrun` will fail to find
it. Negative searches for an SDK path cost a lot (a few seconds) each
time `xcrun` is invoked. We do cache negative results in
`find_cached_path` inside LLDB, but we would still pay the price on
every new debug session the first time we evaluate an expression. This
doesn't only cause a noticable delay in running the expression, but also
generates following error:
```
error: Error while searching for Xcode SDK: timed out waiting for shell command to complete
(int) $0 = 42
```
In this patch we avoid these possibly expensive calls to `xcrun` by
checking the `DW_AT_LLVM_sysroot`, and if it exists, using that as the
SDK path. We need an explicit check for the `CommandLineTools` path
before we call `RegisterXcodeSDK`, because that will try to call
`xcrun`. This won't prevent other uses of `GetSDKRoot` popping up that
cause us to make expensive `xcrun` calls, but for now this addresses the
regression in the expression evaluator. We also had to adjust the
`XcodeSDK::Merge` logic to update the sysroot. There is one case for
which this wouldn't make sense: if a CU was compiled with
`CommandLineTools` and a different one with an older internal SDK, in
that case we would update the `CommandLineTools` sysroot with a
`.Internal.sdk` prefix, which won't possibly exist for
`CommandLineTools`. I added a unit-test for this. Not sure if we want to
explicitly detect and disallow this, given it's quite a niche scenario.
rdar://113619904
rdar://113619723
This assertion was added to check that `RegisterXcodeSDK` will correctly
update the source mappings of the module. However, the source mapping
will only get updated if the `Host::RunShellCommand` call to `xcrun`
succeeded. Even if `xcrun` failed to find an SDK, the source mappings
would get an entry. But if the shell invocation itself failed, then the
mappings are not updated (see
f6212c1cd3/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm (L424-L444)).
This means depending on how slow `xcrun` is on a given host, this test
may fail. On my machine this happens consistently in debug and release
builds.
This patch removes this flakey assertion. We unfortunately lost some
test coverage here but I'm not sure there's great alternatives unless we
either:
1. Mock the `xcrun` call somehow (we could maybe pass a callable around
which defaults to `xcrun` in non-test code?)
2. Make a `xcrun` time-out not an error either?
This type of entry is used to collect data about the debugger
startup/exit
Also introduced a helper ScopedDispatcher
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
Co-authored-by: Pavel Labath <pavel@labath.sk>
The tests are flaky because the read/write calls return sooner than they
should (and #128719 does not fix them). Skip them until we figure the
best way to fix this.
The main motivation for this was the inconsistency in handling of
partial reads/writes between the windows and posix implementations
(windows was returning partial reads, posix was trying to fill the
buffer completely). I settle on the windows implementation, as that's
the more common behavior, and the "eager" version can be implemented on
top of that (in most cases, it isn't necessary, since we're writing just
a single byte).
Since this also required auditing the callers to make sure they're
handling partial reads/writes correctly, I used the opportunity to
modernize the function signatures as a forcing function. They now use
the `Timeout` class (basically an `optional<duration>`) to support both
polls (timeout=0) and blocking (timeout=nullopt) operations in a single
function, and use an `Expected` instead of a by-ref result to return the
number of bytes read/written.
As a drive-by, I also fix a problem with the windows implementation
where we were rounding the timeout value down, which meant that calls
could time out slightly sooner than expected.
Details:
- Previously, we used the LLVM_BUILD_TELEMETRY flag to control whether
any Telemetry code will be built. This has proven to cause more nuisance
to both users of the Telemetry and any further extension of it. (Eg., we
needed to put #ifdef around caller/user code)
- So the new approach is to:
+ Remove this flag and introduce LLVM_ENABLE_TELEMETRY which would be
true by default.
+ If LLVM_ENABLE_TELEMETRY is set to FALSE (at buildtime), the library
would still be built BUT Telemetry cannot be enabled. And no data can be
collected.
The benefit of this is that it simplifies user (and extension) code
since we just need to put the check on Config::EnableTelemetry. Besides,
the Telemetry library itself is very small, hence the additional code to
be built would not cause any difference in build performance.
---------
Co-authored-by: Pavel Labath <pavel@labath.sk>
The LLDB SB API headers should be -Wdocumentation clean as they might
get included by projects building with -Wdocumentation. Although I'd
love for all of LLDB to be clean, we're pretty far removed from that
goal. Until that changes, this PR will detect issues in the SB API
headers by including all the headers in the unittests (by including
LLDB/API.h) and building that with the warning, if the compiler supports
it.
rdar://143597614
The whole unwind plan is already stored in a shared pointer, and there's
no need to persist Rows individually. If there's ever a need to do that,
there are at least two options:
- copy the row (they're not that big, and they're being copied left and
right during construction already)
- use the shared_ptr subobject constructor to create a shared_ptr which
points to a Row but holds the entire unwind plan alive
This also changes all of the getter functions to return const Row
pointers, which is important for safety because all of these objects are
cached and potentially accessed from multiple threads. (Technically one
could hand out `shared_ptr<const Row>`s, but we don't have a habit of
doing that.)
As a next step, I'd like to remove the internal UnwindPlan usages of the
shared pointer, but I'm doing this separately to gauge feedback, and
also because the patch got rather big.
Function was merging equal data even if they weren't adjecant. This
caused a problem in command-disassemble.s test because the two ranges
describing the function would be merged and "swallow" the function
between them.
This PR copies/adapts the algorithm from
RangeVector::CombineConsecutiveEntries (which does not have the same
problem) and also adds a call to ComputeUpperBounds as moving entries
around invalidates the binary tree. (The lack of this call wasn't
noticed until now either because we were not calling methods which rely
on upper bounds (right now, it's only the ill-named FindEntryIndexes
method), or because we weren't merging anything.
After (too) much deliberation, I came to the conclusion that this isn't
the right abstraction, as it doesn't behave completely like the standard
upper_bound method. What I really wanted was to get the range of line
entries for a given address range -- so I implement just that.
lower_bound is still useful as a primitive for building other kinds of
lookups.
This patch improves the synchronization of the debugger's output and error
streams using two new abstractions: `LockableStreamFile` and
`LockedStreamFile`.
- `LockableStreamFile` is a wrapper around a `StreamFile` and a mutex. Client
cannot use the `StreamFile` without calling `Lock`, which returns a
`LockedStreamFile`.
- `LockedStreamFile` is an RAII object that locks the stream for the duration
of its existence. As long as you hold on to the returned object you are
permitted to write to the stream. The destruction of the object
automatically flush the output stream.
The motivation is #123622 and the fact that is hard to fine the last
line entry in a given range. `FindLineEntryByAddress(range_end-1)` is
the best we have, but it's not ideal because it has a magic -1 and that
it relies on there existing a line entry at that address (generally, it
should be there, but if for some case it isn't, we might end up ignoring
the entries that are there (or -- like my incorrect fix in #123622 did
-- iterating through the entire line table).
What we really want is to get the last entry that exists in the given
range. Or, equivalently (and more STL-like) the first entry after that
range. This is what these functions do. I've used the STL names since
they do pretty much exactly what the standard functions do (the main
head-scratcher comes from the fact that our entries represent ranges
rather than single values).
The functions can also be used to simplify the maze of `if` statements
in `FindLineEntryByAddress`, but I'm keeping that as a separate patch.
For now, I'm just adding some unit testing for that function to gain
more confidence that the patch does not change the function behavior.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
