Since quite a while Apple's LLDB fork (that contains the Swift debugging
support) is randomly crashing in `CommandLineParser::addOption` with an error
such as `CommandLine Error: Option 'h' registered more than once!`
The backtrace of the crashing thread is shown below. There are also usually many
other threads also performing similar clang::FrontendActions which are all
trying to generate (usually outdated) Clang modules which are used by Swift for
various reasons.
```
[ 6] LLDB`CommandLineParser::addOption(llvm:🆑:Option*, llvm:🆑:SubCommand*) + 856
[ 7] LLDB`CommandLineParser::addOption(llvm:🆑:Option*, llvm:🆑:SubCommand*) + 733
[ 8] LLDB`CommandLineParser::addOption(llvm:🆑:Option*, bool) + 184
[ 9] LLDB`llvm:🆑:ParseCommandLineOptions(...) [inlined] ::CommandLineParser::ParseCommandLineOptions(... + 1279
[ 9] LLDB`llvm:🆑:ParseCommandLineOptions(...) + 497
[ 10] LLDB`setCommandLineOpts(clang::CodeGenOptions const&) + 416
[ 11] LLDB`EmitAssemblyHelper::EmitAssemblyWithNewPassManager(...) + 98
[ 12] LLDB`clang::EmitBackendOutput(...) + 4580
[ 13] LLDB`PCHContainerGenerator::HandleTranslationUnit(clang::ASTContext&) + 871
[ 14] LLDB`clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) + 43
[ 15] LLDB`clang::ParseAST(clang::Sema&, bool, bool) + 579
[ 16] LLDB`clang::FrontendAction::Execute() + 74
[ 17] LLDB`clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 1808
```
The underlying reason for the crash is that the CommandLine code in LLVM isn't
thread-safe and will never be thread-safe with its current architecture. The way
LLVM's CommandLine logic works is that all parts of the LLVM can provide command
line arguments by defining `cl::opt` global variables and their constructors
(which are invoked during static initialisation) register the variable in LLVM's
CommandLineParser (which is also just a global variable). At some later point
after static initialization we actually try to parse command line arguments and
we ask the CommandLineParser to parse our `argv`. The CommandLineParser then
lazily constructs it's internal parsing state in a non-thread-safe way (this is
where the crash happens), parses the provided command line and then goes back to
the respective `cl::opt` global variables and sets their values according to the
parse result.
As all of this is based on global state, this whole mechanism isn't thread-safe
so the only time to ever use it is when we know we only have one active thread
dealing with LLVM logic. That's why nearly all callers of
`llvm:🆑:ParseCommandLineOptions` are at the top of the `main` function of the
some LLVM-based tool. One of the few exceptions to this rule is in the
`setCommandLineOpts` function in `BackendUtil.cpp` which is in our backtrace:
```
static void setCommandLineOpts(const CodeGenOptions &CodeGenOpts) {
SmallVector<const char *, 16> BackendArgs;
BackendArgs.push_back("clang"); // Fake program name.
if (!CodeGenOpts.DebugPass.empty()) {
BackendArgs.push_back("-debug-pass");
BackendArgs.push_back(CodeGenOpts.DebugPass.c_str());
}
if (!CodeGenOpts.LimitFloatPrecision.empty()) {
BackendArgs.push_back("-limit-float-precision");
BackendArgs.push_back(CodeGenOpts.LimitFloatPrecision.c_str());
}
BackendArgs.push_back(nullptr);
llvm:🆑:ParseCommandLineOptions(BackendArgs.size() - 1,
BackendArgs.data());
}
```
This is trying to set `cl::opt` variables in the LLVM backend to their right
value as the passed via CodeGenOptions by invoking the CommandLine parser. As
this is just in some generic Clang CodeGen code (where we allow having multiple
threads) this is code is clearly wrong. If we're unlucky it either overwrites
the value of the global variables or it causes the CommandLine parser to crash.
So the next question is why is this only crashing in LLDB? The main reason seems
to be that easiest way to crash this code is to concurrently enter the initial
CommandLineParser construction where it tries to collect all the registered
`cl::opt` options and checks for sanity:
```
// If it's a DefaultOption, check to make sure it isn't already there.
if (O->isDefaultOption() &&
SC->OptionsMap.find(O->ArgStr) != SC->OptionsMap.end())
return;
// Add argument to the argument map!
if (!SC->OptionsMap.insert(std::make_pair(O->ArgStr, O)).second) {
errs() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
<< "' registered more than once!\n";
HadErrors = true;
}
```
The `OptionsMap` here is global variable and if we end up in this code with two
threads at once then two threads at the same time can register an option (such
as 'h') when they pass the first `if` and then we fail with the sanity check in
the second `if`.
After this sanity check and initial setup code the only remaining work is just
parsing the provided CommandLine which isn't thread-safe but at least doesn't
crash in all my attempts at breaking it (as it's usually just reading from the
already generated parser state but not further modifying it). The exception to
this is probably that once people actually specify the options in the code
snippet above we might run into some new interesting ways to crash everything.
To go back to why it's only affecting LLDB: Nearly all LLVM tools I could find
(even if they are using threads) seem to call the CommandLine parser at the
start so they all execute the initial parser setup at a point where there is
only one thread. So once the code above is executed they are mostly safe from
the sanity check crashes. We even have some shady code for the gtest `main` in
`TestMain.cpp` which is why this also doesn't affect unit tests.
The only exception to this rule is ... *drum roll* ... LLDB! it's not using that
CommandLine library for parsing options so it also never ends up calling it in
`main`. So when we end up in the `FrontendAction` code from the backtrace we are
already very deep in some LLDB logic and usually already have several threads.
In a situation where Swift decides to compile a large amount of Clang modules in
parallel we then end up entering this code via several threads. If several
threads reach this code at the same time we end up in the situation where the
sanity-checking code of CommandLine crashes. I have a very reliable way of
demonstrating the whole thing in D99650 (just run the unit test several times,
it usually crashes after 3-4 attempts).
We have several ways to fix this:
1. Make the whole CommandLine mechanism in LLVM thread-safe.
2. Get rid of `setCommandLineOpts` in `BackendUtil.cpp` and other callers of the
command line parsing in generic Clang code.
3. Initialise the CommandLine library in a safe point in LLDB.
Option 1 is just a lot of work and I'm not even sure where to start. The whole
mechanism is based on global variables and global state and this seems like a
humongous task.
Option 2 is probably the best thing we can do in the near future. There are only
two callers of the command line parser in generic Clang code. The one in
`BackendUtils.cpp` looks like it can be replaced with some reasonable
refactoring (as it only deals with two specific options). There is another one
in `ExecuteCompilerInvocation` which deals with forwarding the generic `-mllvm`
options to the backend which seems like it will just end up requiring us to do
Option 1.
Option 3 is what this patch is doing. We just parse some dummy command line
invocation in a point of the LLDB execution where we only have one thread that
is dealing with LLVM/Clang stuff. This way we are at least prevent the frequent
crashes for users as parsing the dummy command line invocation will set up the
initial parser state safely.
Fixes rdar://70989856
Reviewed By: mib, JDevlieghere
Differential Revision: https://reviews.llvm.org/D99652
This implements the interactive trace start and stop methods.
This diff ended up being much larger than I anticipated because, by doing it, I found that I had implemented in the beginning many things in a non optimal way. In any case, the code is much better now.
There's a lot of boilerplate code due to the gdb-remote protocol, but the main changes are:
- New tracing packets: jLLDBTraceStop, jLLDBTraceStart, jLLDBTraceGetBinaryData. The gdb-remote packet definitions are quite comprehensive.
- Implementation of the "process trace start|stop" and "thread trace start|stop" commands.
- Implementaiton of an API in Trace.h to interact with live traces.
- Created an IntelPTDecoder for live threads, that use the debugger's stop id as checkpoint for its internal cache.
- Added a functionality to stop the process in case "process tracing" is enabled and a new thread can't traced.
- Added tests
I have some ideas to unify the code paths for post mortem and live threads, but I'll do that in another diff.
Differential Revision: https://reviews.llvm.org/D91679
LLDB can often appear deadlocked to users that use IDEs when it is indexing DWARF, or parsing symbol tables. These long running operations can make a debug session appear to be doing nothing even though a lot of work is going on inside LLDB. This patch adds a public API to allow clients to listen to debugger events that report progress and will allow UI to create an activity window or display that can show users what is going on and keep them informed of expensive operations that are going on inside LLDB.
Differential Revision: https://reviews.llvm.org/D97739
This patch adds a new command options to the CommandObjectProcessLaunch
for scripted processes.
Among the options, the user need to specify the class name managing the
scripted process. The user can also use a key-value dictionary holding
arbitrary data that will be passed to the managing class.
This patch also adds getters and setters to `SBLaunchInfo` for the
class name managing the scripted process and the dictionary.
rdar://65508855
Differential Review: https://reviews.llvm.org/D95710
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
This patch exposes the getter and setter methods for the command
interpreter `print_errors` run option.
rdar://74816984
Differential Revision: https://reviews.llvm.org/D98001
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
This patch adds a new command options to the CommandObjectProcessLaunch
for scripted processes.
Among the options, the user need to specify the class name managing the
scripted process. The user can also use a key-value dictionary holding
arbitrary data that will be passed to the managing class.
This patch also adds getters and setters to `SBLaunchInfo` for the
class name managing the scripted process and the dictionary.
rdar://65508855
Differential Review: https://reviews.llvm.org/D95710
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
Our code for locating the shared library directory works via dladdr (or
the windows equivalent) to locate the path of an address known to reside
in liblldb. This works great for C++ programs, but there's a catch.
When (lib)lldb is used from python (like in our test suite), this dladdr
call will return a path to the _lldb.so (or such) file in the python
directory. To compensate for this, we have code which attempts to
resolve this symlink, to ensure we get the canonical location. However,
here's the second catch.
On windows, this file is not a symlink (but a copy), so this logic
fails. Since most of our other paths are derived from the liblldb
location, all of these paths will be wrong, when running the test suite.
One effect of this was the failure to find lldb-server in D96202.
To fix this issue, I add some windows-specific code to locate the
liblldb directory. Since it cannot rely on symlinks, it works by
manually walking the directory tree -- essentially doing the opposite of
what we do when computing the python directory.
To avoid python leaking back into the host code, I implement this with
the help of a callback which can be passed to HostInfo::Initialize in
order to assist with the directory location. The callback lives inside
the python plugin.
I also strenghten the existing path test to ensure the returned path is
the right one.
Differential Revision: https://reviews.llvm.org/D96779
It seems that recording fundamental return type is bogus.
This can trigger asserts when running a test with reproducers so this
patch updates the `SBTarget::IsLoaded` test to stop recording them.
Differential Revision: https://reviews.llvm.org/D95686
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
This reverts commit 754ab803b8.
As pointed out in https://reviews.llvm.org/D95761, this patch could lead to
having the wrong execution context in some situations (thanks Jim!).
D92164 is addressing the same issue and will replace this patch, so I'll
revert this one.
This patch adds an `SBTarget::IsLoaded(const SBModule&) const` endpoint
to lldb's Scripting Bridge API. As the name suggests, it will allow the
user to know if the module is loaded in a specific target.
rdar://37957625
Differential Review: https://reviews.llvm.org/D95686
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
Second try, handling both a bogus arch string and the "null file & arch" used
to create an empty but valid target.
Also check in that case before logging (previously the logging would have
crashed.)
Also revert "Follow on to: f05dc40c31d1883b46b8bb60547087db2f4c03e3"
After these changes, multiple lldb tests are failing. Calls to
CreateTargetWithFileAndArch(None, None) appear to fail after these
changes.
This reverts commit f05dc40c31 and
1fba21778f.
When you pass in a bogus ArchSpec, TargetList.CreateTarget
makes a target with the arch of the executable. That wasn't the
case with a bogus triple, so this change caused one of the bogus
input data tests to fail. So check that the ArchSpec is valid
before passing it to CreateTarget.
This patch introduces a LLDB_SCOPED_TIMER macro to hide the needlessly
repetitive creation of scoped timers in LLDB. It's similar to the
LLDB_LOG(F) macro.
Differential revision: https://reviews.llvm.org/D93663
This reverts commit a01b26fb51, because it
breaks the "finish" command in some way -- the command does not
terminate after it steps out, but continues running the target. The
exact blast radius is not clear, but it at least affects the usage of
the "finish" command in TestGuiBasicDebug.py. The error is *not*
gui-related, as the same issue can be reproduced by running the same
steps outside of the gui.
There is some kind of a race going on, as the test fails only 20% of the
time on the buildbot.
This patch exposes the Target::CreateBreakpoint overload with the
boolean argument to move to the neareast code to the SBAPI.
This is useful when creating column breakpoints to restrict lldb's
resolution to the pointed source location, preventing it to go to the next
line.
rdar://72196842
Differential Revision: https://reviews.llvm.org/D93266
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
This patch exposes the Target::CreateBreakpoint overload with the
boolean argument to move to the neareast code to the SBAPI.
This is useful when creating column breakpoints to restrict lldb's
resolution to the pointed source location, preventing it to go to the next
line.
rdar://72196842
Differential Revision: https://reviews.llvm.org/D93266
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
Currently, the interpreter's context is not updated until a command is executed.
This has resulted in the behavior of SB-interface functions and some commands
depends on previous user actions. The interpreter's context can stay uninitialized,
point to a currently selected target, or point to one of previously selected targets.
This patch removes any usages of CommandInterpreter::UpdateExecutionContext.
CommandInterpreter::HandleCommand* functions still may override context temporarily,
but now they always restore it before exiting. CommandInterpreter saves overriden
contexts to the stack, that makes nesting commands possible.
Added test reproduces one of the issues. Without this fix, the last assertion fails
because interpreter's execution context is empty until running "target list", so,
the value of the global property was updated instead of process's local instance.
Differential Revision: https://reviews.llvm.org/D92164
TargetList::CreateTarget automatically adds created target to the list, however,
CommandObjectTargetCreate does some additional preparation after creating a target
and which can fail. The command should remove created target if it failed. Since
the function has many ways to return, scope guard does this work safely.
Changes to the TargetList make target adding and selection more transparent.
Other changes remove unnecessary SetSelectedTarget after CreateTarget.
Differential Revision: https://reviews.llvm.org/D93052
Add a 'can_connect' parameter to Process plugin initialization, and use
it to filter plugins to these capable of remote connections. This is
used to prevent 'process connect' from picking up a plugin that can only
be used locally, e.g. the legacy FreeBSD plugin.
Differential Revision: https://reviews.llvm.org/D91810
Instead of having a custom error message, propagate the llvm::Error from
SystemInitializerCommon. I didn't realize we had this overload until
Pavel mentioned it in D90987 today.
During active replay, the ::Initialize call is replayed like any other
SB API call and the return value is ignored. Since we can't intercept
this, we terminate here before the uninitialized debugger inevitably
crashes.
Differential revision: https://reviews.llvm.org/D90987
SBType::GetArrayElementType should return the actual type, not the
canonical type (e.g. int32_t, not the underlying int).
Added a test case to validate the new behavior. I also ran all other
tests on Linux (ninja check-lldb), they all pass.
Differential revision: https://reviews.llvm.org/D90318
For performance reasons the reproducers don't copy the files captured by
the file collector eagerly, but wait until the reproducer needs to be
generated.
This is a problematic when LLDB crashes and we have to do all this
signal-unsafe work in the signal handler. This patch uses a similar
trick to clang, which has the driver invoke a new cc1 instance to do all
this work out-of-process.
This patch moves the writing of the mapping file as well as copying over
the reproducers into a separate process spawned when lldb crashes.
Differential revision: https://reviews.llvm.org/D89600
Renamed ThreadIntelPT to TreaceThread, making it a top-level class. I noticed that this class can and shuld work for any trace plugin and there's nothing intel-pt specific in it.
With that TraceThread change, I was able to move most of the json file parsing logic to the base class TraceSessionFileParser, which makes adding new plug-ins easier.
This originally was part of https://reviews.llvm.org/D89283
Differential Revision: https://reviews.llvm.org/D89408
When doing a standalone build (i.e., building just LLDB against an existing
LLVM/Clang installation), LLDB is currently unable to find any Clang resource
directory that contains all the builtin headers we need to parse real source
code. This causes several tests that actually parse source code on disk within
the expression parser to fail (most notably nearly all the import-std-module
tests).
The reason why LLDB can't find the resource directory is that we search based on
the path of the LLDB shared library path. We assumed that the Clang resource
directory is in the same prefix and has the same relative path to the LLDB
shared library (e.g., `../clang/10.0.0/include`). However for a standalone build
where the existing Clang can be anywhere on the disk, so we can't just rely on
the hardcoded relative paths to the LLDB shared library.
It seems we can either solve this by copying the resource directory to the LLDB
installation, symlinking it there or we pass the path to the Clang installation
to the code that is trying to find the resource directory. When building the
LLDB framework we currently copy the resource directory over to the framework
folder (this is why the import-std-module are not failing on the Green Dragon
standalone bot).
This patch symlinks the resource directory of Clang into the LLDB build
directory. The reason for that is simply that this is only needed when running
LLDB from the build directory. Once LLDB and Clang/LLVM are installed the
already existing logic can find the Clang resource directory by searching
relative to the LLDB shared library.
Reviewed By: kastiglione, JDevlieghere
Differential Revision: https://reviews.llvm.org/D88581
Every call to the protected SBAddress constructor and the SetAddress
method takes the address of a valid object which means we might as well
pass it as a const reference instead of a pointer and drop the null
check.
Differential revision: https://reviews.llvm.org/D88249
In MinGW world, UNIX like lib prefix is preferred for the libraries.
This patch adjusts CMake files to do that.
Differential Revision: https://reviews.llvm.org/D87517
This patch adds a way to fetch breakpoint metadatas as a serialized
`Structured` Data format (JSON). This can be used by IDEs to update
their UI when a breakpoint is set or modified from the console.
rdar://11013798
Differential Revision: https://reviews.llvm.org/D87491
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
Add a reproducer verifier that catches:
- Missing or invalid home directory
- Missing or invalid working directory
- Missing or invalid module/symbol paths
- Missing files from the VFS
The verifier is enabled by default during replay, but can be skipped by
passing --reproducer-no-verify.
Differential revision: https://reviews.llvm.org/D86497
This patch adds the ability to use a custom interpreter with the
`platform shell` command. If the user set the `-s|--shell` option
with the path to a binary, lldb passes it down to the platform's
`RunShellProcess` method and set it as the shell to use in
`ProcessLaunchInfo to run commands.
Note that not all the Platforms support running shell commands with
custom interpreters (i.e. RemoteGDBServer is only expected to use the
default shell).
This patch also makes some refactoring and cleanups, like swapping
CString for StringRef when possible and updating `SBPlatformShellCommand`
with new methods and a new constructor.
rdar://67759256
Differential Revision: https://reviews.llvm.org/D86667
Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
Add a reproducer verifier that catches:
- Missing or invalid home directory
- Missing or invalid working directory
- Missing or invalid module/symbol paths
- Missing files from the VFS
The verifier is enabled by default during replay, but can be skipped by
passing --reproducer-no-verify.
Differential revision: https://reviews.llvm.org/D86497
Extract all the provider related logic from Reproducer.h and move it
into its own header ReproducerProvider.h. These classes are seeing most
of the development these days and this reorganization reduces
incremental compilation from ~520 to ~110 files when making changes to
the new header.
Raphael Isemann