7ad49aec12 added a __memory subdirectory to libc++
but the code we use to find libc++ from the debug info support files wasn't
prepared to encounter unknown subdirectories within libc++. The import-std-module
tests automatically fell back to not importing the std module which caused
them to fail.
This patch removes our hardcoded exception for the 'experimental' subdirectory
and instead just ignores all subdirectories of c++/vX/ when searching the
support files.
Right now we have one large AST for all types in LLDB. All ODR violations in
types we reconstruct are resolved by just letting the ASTImporter handle the
conflicts (either by merging types or somehow trying to introduce a duplicated
declaration in the AST). This works ok for the normal types we build from debug
information as most of them are just simple CXXRecordDecls or empty template
declarations.
However, with a loaded `std` C++ module we have alternative versions of pretty
much all declarations in the `std` namespace that are much more fleshed out than
the debug information declarations. They have all the information that is lost
when converting to DWARF, such as default arguments, template default arguments,
the actual uninstantiated template declarations and so on.
When we merge these C++ module types into the big scratch AST (that might
already contain debug information types) we give the ASTImporter the tricky task
of somehow creating a consistent AST out of all these declarations. Usually this
ends in a messy AST that contains a mostly broken mix of both module and debug
info declarations. The ASTImporter in LLDB is also importing types with the
MinimalImport setting, which usually means the only information we have when
merging two types is often just the name of the declaration and the information
that it contains some child declarations. This makes it pretty much impossible
to even implement a better merging logic (as the names of C++ module
declarations and debug info declarations are identical).
This patch works around this whole merging problem by separating C++ module
types from debug information types. This is done by splitting up the single
scratch AST into two: One default AST for debug information and a dedicated AST
for C++ module types.
The C++ module AST is implemented as a 'specialised AST' that lives within the
default ScratchTypeSystemClang. When we select the scratch AST we can explicitly
request that we want such a isolated sub-AST of the scratch AST. I kept the
infrastructure more general as we probably can use the same mechanism for other
features that introduce conflicting types (such as programs that are compiled
with a custom -wchar-size= option).
There are just two places where we explicitly have request the C++ module AST:
When we export persistent declarations (`$mytype`) and when we create our
persistent result variable (`$0`, `$1`, ...). There are a few formatters that
were previously assuming that there is only one scratch AST which I cleaned up
in a preparation revision here (D92757).
Reviewed By: aprantl
Differential Revision: https://reviews.llvm.org/D92759
By now LLDB can import the 'std' C++ module to improve expression evaluation,
but there are still a few problems to solve before we can do this by default.
One is that importing the C++ module is slightly slower than normal expression
evaluation (mostly because the disk access and loading the initial lookup data
is quite slow in comparison to the barebone Clang setup the rest of the LLDB
expression evaluator is usually doing). Another problem is that some complicated
types in the standard library aren't fully supported yet by the ASTImporter, so
we end up types that fail to import (which usually appears to the user as if the
type is empty or there is just no result variable).
To still allow people to adopt this mode in their daily debugging, this patch
adds a setting that allows LLDB to automatically retry failed expression with a
loaded C++ module. All success expressions will behave exactly as they would do
before this patch. Failed expressions get a another parse attempt if we find a
usable C++ module in the current execution context. This way we shouldn't have
any performance/parsing regressions in normal debugging workflows, while the
debugging workflows involving STL containers benefit from the C++ module type
info.
This setting is off by default for now with the intention to enable it by
default on macOS soon-ish.
The implementation is mostly just extracting the existing parse logic into its
own function and then calling the parse function again if the first evaluation
failed and we have a C++ module to retry the parsing with.
Reviewed By: shafik, JDevlieghere, aprantl
Differential Revision: https://reviews.llvm.org/D92784
LLDB is currently always activating C++ when parsing expressions as LLDB itself
is using C++ features when creating the final AST that will be codegen'd
(specifically, references to variables, namespaces and using declarations are
used).
This is causing problems for users that have variables in non-C++ programs (e.g.
plain C or Objective-C) that have names which are keywords in C++. Expressions
referencing those variables fail to parse as LLDB's Clang parser thinks those
identifiers are C++ keywords and not identifiers that may belong to a
declaration.
We can't just disable C++ in the expression parser for those situations as
replacing the functionality of the injected C++ code isn't trivial. So this
patch is just disabling most keywords that are exclusive to C++ in LLDB's Clang
parser when we are in a non-C++ expression. There are a few keywords we can't
disable for now:
* `using` as that's currently used in some situations to inject variables into the expression function.
* `__null` as that's used by LLDB to define `NULL`/`Nil`/`nil`.
Getting rid of these last two keywords is possible but is a large enough change
that this will be handled in follow up patches.
Note that this only changes the keyword status of those tokens but this patch
does not remove any C++ functionality from the expression parser. The type
system still follows C++ rules and so does the rest of the expression parser.
There is another small change that gives the hardcoded macro definitions in LLDB
a higher precedence than the macros imported from the Objective-C modules. The
reason for this is that the Objective-C modules in LLDB are actually parsed in
Objective-C++ mode and they end up providing the C++ definitions of certain
system macros (like `NULL` being defined as `nullptr`). So we have to move the
LLDB definition forward and surround the definition from the module with an
`#ifdef` to make sure that we use the correct LLDB definition that doesn't
reference C++ keywords. Or to give an example, this is how the expression source
code changes:
Before:
```
#define NULL (nullptr) // injected module definition
#ifndef NULL
#define NULL (__null) // hardcoded LLDB definition
#endif
```
After:
```
#ifndef NULL
#define NULL (__null) // hardcoded LLDB definition
#endif
#ifndef NULL
#define NULL (nullptr) // injected module definition
#endif
```
Fixes rdar://10356912
Reviewed By: shafik
Differential Revision: https://reviews.llvm.org/D82770
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
This patch redesigns the Target::GetUtilityFunctionForLanguage API:
- Use a unique_ptr instead of a raw pointer for the return type.
- Wrap the result in an llvm::Expected instead of using a Status object as an I/O parameter.
- Combine the action of "getting" and "installing" the UtilityFunction as they always get called together.
- Pass std::strings instead of const char* and std::move them where appropriate.
There's more room for improvement but I think this tackles the most
prevalent issues with the current API.
Differential revision: https://reviews.llvm.org/D90011
The UtilityFunction ctor was dropping the text argument. Probably for
that reason ClangUtilityFunction was setting the parent's member
directly instead of deferring to the parent ctor. Also change the
signatures to take strings which are std::moved in place.
Currently one can redefine a persistent variable and LLDB will just silently
ignore the second definition:
```
(lldb) expr int $i = 1
(lldb) expr int $i = 2
(lldb) expr $i
(int) $i = 1
```
This patch makes this an error and rejects the expression with the second
definition.
A nice follow up would be to refactor LLDB's persistent variables to not just be
a pair of type and name, but also contain some way to obtain the original
declaration and source code that declared the variable. That way we could
actually make a full diagnostic as we would get from redefining a variable twice
in the same expression.
Reviewed By: labath, shafik, JDevlieghere
Differential Revision: https://reviews.llvm.org/D89310
While debugging another bug I found out that we currently don't set any limit
for the number of diagnostics Clang emits. If a user does something that
generates a lot of errors (like including some long header file from within the
expression function), then we currently spam the LLDB output with potentially
thousands of Clang error diagnostics.
Clang sets a default limit of 20 errors, but given that LLDB is often used
interactively for small expressions I would say a limit of 5 is enough. The
limit is implemented as a setting, so if a user cares about seeing having a
million errors printed to their terminal then they can just increase the
settings value.
Reviewed By: shafik, mib
Differential Revision: https://reviews.llvm.org/D88889
Both of the if-clauses modify the raw_path variable and only one of them
was resetting the variable for the fallback. Avoid future bugs like that
by always resetting the variable.
Differential revision: https://reviews.llvm.org/D88704
This adds support for substituting std::pair instantiations with enabled
import-std-module.
With the fixes in parent revisions we can currently substitute a single pair
(however, a result that returns a second pair currently causes LLDB to crash
while importing the second template instantiation).
Reviewed By: aprantl
Differential Revision: https://reviews.llvm.org/D85141
The ASTImporter has an `Imported(From, To)` callback that notifies subclasses
that a declaration has been imported in some way. LLDB uses this in the
`CompleteTagDeclsScope` to see which records have been imported into the scratch
context. If the record was declared inside the expression, then the
`CompleteTagDeclsScope` will forcibly import the full definition of that record
to the scratch context so that the expression AST can safely be disposed later
(otherwise we might end up going back to the deleted AST to complete the
minimally imported record). The way this is implemented is that there is a list
of decls that need to be imported (`m_decls_to_complete`) and we keep completing
the declarations inside that list until the list is empty. Every `To` Decl we
get via the `Imported` callback will be added to the list of Decls to be
completed.
There are some situations where the ASTImporter will actually give us two
`Imported` calls with the same `To` Decl. One way where this happens is if the
ASTImporter decides to merge an imported definition into an already imported
one. Another way is that the ASTImporter just happens to get two calls to
`ASTImporter::Import` for the same Decl. This for example happens when importing
the DeclContext of a Decl requires importing the Decl itself, such as when
importing a RecordDecl that was declared inside a function.
The bug addressed in this patch is that when we end up getting two `Imported`
calls for the same `To` Decl, then we would crash in the
`CompleteTagDeclsScope`. That's because the first time we complete the Decl we
remove the Origin tracking information (that maps the Decl back to from where it
came from). The next time we try to complete the same `To` Decl the Origin
tracking information is gone and we hit the `to_context_md->getOrigin(decl).ctx
== m_src_ctx` assert (`getOrigin(decl).ctx` is a nullptr the second time as the
Origin was deleted).
This is actually a regression coming from D72495. Before D72495
`m_decls_to_complete` was actually a set so every declaration in there could
only be queued once to be completed. The set was changed to a vector to make the
iteration over it deterministic, but that also causes that we now potentially
end up trying to complete a Decl twice.
This patch essentially just reverts D72495 and makes the `CompleteTagDeclsScope`
use a SetVector for the list of declarations to be completed. The SetVector
should filter out the duplicates (as the original `set` did) and also ensure that
the completion order is deterministic. I actually couldn't find any way to cause
LLDB to reproduce this bug by merging declarations (this would require that we
for example declare two namespaces in a non-top-level expression which isn't
possible). But the bug reproduces very easily by just declaring a class in an
expression, so that's what the test is doing.
Reviewed By: shafik
Differential Revision: https://reviews.llvm.org/D85648
SemaSourceWithPriorities is a special SemaSource that wraps our normal LLDB
ExternalASTSource and the ASTReader (which is used for the C++ module loading).
It's only active when the `import-std-module` setting is turned on.
The `CompleteType` function there in `SemaSourceWithPriorities` is looping over
all ExternalASTSources and asks each to complete the type. However, that loop is
in another loop that keeps doing that until the type is complete. If that
function is ever called on a type that is a forward decl then that causes LLDB
to go into an infinite loop.
I remember I added that second loop and the comment because I thought I saw a
similar pattern in some other Clang code, but after some grepping I can't find
that code anywhere and it seems the rest of the code base only calls
CompleteType once (It would also be kinda silly to have calling it multiple
times). So it seems that's just a silly mistake.
The is implicitly tested by importing `std::pair`, but I also added a simpler
dedicated test that creates a dummy libc++ module with some forward declarations
and then imports them into the scratch AST context. At some point the
ASTImporter will check if one of the forward decls could be completed by the
ExternalASTSource, which will cause the `SemaSourceWithPriorities` to go into an
infinite loop once it receives the `CompleteType` call.
Reviewed By: shafik
Differential Revision: https://reviews.llvm.org/D87289
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.
The search for the complete class definition can also produce entries
which are not of the expected type. This can happen for instance when
there is a function with the same name as the class we're looking up
(which means that the class needs to be disambiguated with the
struct/class tag in most contexts).
Previously we were just picking the first Decl that the lookup returned,
which later caused crashes or assertion failures if it was not of the
correct type. This patch changes that to search for an entry of the
correct type.
Differential Revision: https://reviews.llvm.org/D85904
This patch has no effect for C and C++. In more dynamic languages,
such as Objective-C and Swift GetByteSize() needs to call into the
language runtime, so it's important to pass one in where possible. My
primary motivation for this is some work I'm doing on the Swift
branch, however, it looks like we are also seeing warnings in
Objective-C that this may resolve. Everything in the SymbolFile
hierarchy still passes in nullptrs, because we don't have an execution
context in SymbolFile, since SymbolFile transcends processes.
Differential Revision: https://reviews.llvm.org/D84267
The `intrinsics_gen` target exists in the CMake exports since r309389
(see LLVMConfig.cmake.in), hence projects can depend on `intrinsics_gen`
even it they are built separately from LLVM.
Reviewed By: MaskRay, JDevlieghere
Differential Revision: https://reviews.llvm.org/D83454
The passthrough DiagnosticConsumer is an implementation detail of
ClangDiagnosticManagerAdapter and we can just hide it behind the normal
DiagnosticConsumer interface that ClangDiagnosticManagerAdapter is supposed
to implement.
Summary:
This patch adds support for evaluation of expressions referring to types
which were compiled in -flimit-debug-info (a.k.a -fno-standalone-debug)
in clang. In this mode it's possible that the debug information needed
to fully describe a c++ type is not present in a single shared library
-- for example debug info for a base class or a member of a type can
only be found in another shared library. This situation is not
currently handled well within lldb as we are limited to searching within
a single shared library (lldb_private::Module) when searching for the
definition of these types.
The way that this patch gets around this limitation is by doing the
search at a later stage -- during the construction of the expression ast
context. This works by having the parser (currently SymbolFileDWARF, but
a similar approach is probably needed for PDBs too) mark a type as
"forcefully completed". What this means is that the parser has marked
the type as "complete" in the module ast context (as this is necessary
to e.g. derive classes from it), but its definition is not really there.
This is done via a new field on the ClangASTMetadata struct.
Later, when we are importing such a type into the expression ast, we
check this flag. If the flag is set, we try to find a better definition
for the type in other shared libraries. We do this by initiating a
new lookup for the "forcefully completed" classes, which then imports the
type from a module with a full definition.
This patch only implements this handling for base classes, but other
cases (members, array element types, etc.). The changes for that should
be fairly simple and mostly revolve around marking these types as
"forcefully completed" at an approriate time -- the importing logic is
generic already.
Another aspect, which is also not handled by this patch is viewing these
types via the "frame variable" command. This does not use the AST
importer and so it will need to handle these types on its own -- that
will be the subject of another patch.
Differential Revision: https://reviews.llvm.org/D81561
Summary:
The ClangASTSource has a lock that globally disables all lookups into the
external AST source when we explicitly "guarded" copy a type. It's not used for
anything else, so importing declarations or importing types that are
dependencies of a declaration actually won't activate that lock. The lookups it
is supposed to prevent also don't actually happen in our test suite. The check
in `ClangExpressionDeclMap::FindExternalVisibleDecls` is never executed and the
check in the `ClangASTSource::FindExternalVisibleDeclsByName` is only ever
reached by the `Import-std-module` tests (which explicitly do a lookup into the
expression context on purpose).
This lock was added in 6abfabff61 as a replacement
for a list of types we already looked up which appeared to be an optimisation
strategy. I assume back then this lock had a purpose but these days the
ASTImporter and LLDB seem to be smart enough to avoid whatever lookups this
tried to prevent.
I would say we remove it from LLDB. The main reason is that it blocks D81561
(which explicitly does a specific lookup to resolve placeholder types produced
by `-flimit-debug-info`) but it's semantics are also very confusing. The naming
implies it's a flag to indicate when we import something at the moment which is
practically never true as described above. Also the fact that it makes our
ExternalASTSource alternate between doing lookups into the debug info and
pretending it doesn't know any external decls could really break our lookup in
some weird way if Clang decides to cache a fake empty lookup result that was
generated while the lock was active.
Reviewers: labath, shafik, JDevlieghere, aprantl
Reviewed By: labath, JDevlieghere, aprantl
Subscribers: aprantl, abidh
Differential Revision: https://reviews.llvm.org/D81749
Move the part of the code which is responsible for finding a complete
definition of the type into a separate function (FindCompleteType). This
is split off from D81561, as it's a generally useful cleanup.
No functional change.
Summary:
When we get an error back from IRForTarget we directly print that error to the
debugger output stream instead of putting it in the result object. The result
object only gets a vague "The expression could not be prepared to run in the
target" error message that doesn't actually tell the user what went wrong.
This patch just puts the IRForTarget errors into the status object that is
returned to the caller instead of directly printing it to the debugger. Also
updates one test that now can actually check for the error message it is
supposed to check for (instead of the default error which is all we had before).
Reviewers: JDevlieghere
Reviewed By: JDevlieghere
Differential Revision: https://reviews.llvm.org/D81654
Summary:
ClangExpressionSourceCode has different ways to wrap the user expression based on
which context the expression is executed in. For example, if we're in a C++ member
function we put the expression inside a fake member function of a fake class to make the
evaluation possible. Similar things are done for Objective-C instance/static methods.
There is also a default wrapping where we put the expression in a normal function
just to make it possible to execute it.
The way we currently define which kind of wrapping the expression needs is based on
the `wrapping_language` we keep passing to the ClangExpressionSourceCode
instance. We repurposed the language type enum for that variable to distinguish the
cases above with the following mapping:
* language = C_plus_plus -> member function wrapping
* language = ObjC -> instance/static method wrapping (`is_static` distinguished between those two).
* language = C -> normal function wrapping
* all other cases like C_plus_plus11, Haskell etc. make our class a no-op that does mostly nothing.
That mapping is currently not documented and just confusing as the `language`
is unrelated to the expression language (and in the ClangUserExpression we even pretend
that it *is* the actual language, but luckily never used it for anything). Some of the code
in ClangExpressionSourceCode is also obviously thinking that this is the actual language of
the expression as it checks for non-existent cases such as `ObjC_plus_plus` which is
not part of the mapping.
This patch makes a new enum to describe the four cases above (with instance/static Objective-C
methods now being their own case). It also make that enum just a member of
ClangExpressionSourceCode instead of having to pass the same value to the class repeatedly.
This gets also rid of all the switch-case-checks for 'unknown' language such as C_plus_plus11 as this
is no longer necessary.
Reviewers: labath, JDevlieghere
Reviewed By: labath
Subscribers: abidh
Differential Revision: https://reviews.llvm.org/D80793
Summary:
It turns out that the order in which we provide completions for expressions is
nondeterministic. This leads to confusing user experience and also breaks the
reproducer tests (as two LLDB tests can go out of sync due to the
non-determinism in the completion lists)
The reason for the non-determinism is that the CompletionConsumer informs us
about decls in the order in which it finds declarations in the lookup store of
the DeclContexts it visits (mainly this snippet in SemaLookup.cpp):
``` lang=c++
// Enumerate all of the results in this context.
for (DeclContextLookupResult R :
Load ? Ctx->lookups()
: Ctx->noload_lookups(/*PreserveInternalState=*/false)) {
[...]
```
This storage of the lookup is sorted by pointer values (see the hash of
`DeclarationName`) and can therefore be non-deterministic. The LLDB code
completion consumer that receives these calls originally expected that the order
of declarations is defined by Clang, but it seems the API expects the client to
provide an order to the completions.
This patch fixes the issue as follows:
* We sort the completions we get from Clang alphabetically and also by the
priority value we get from Clang (with priority value sorting having precedence
over the alphabetical sorting)
* We make all the functions/variables that touch a completion before the sorting
const-qualified. The idea is that this should prevent that we never have
observable side-effect from touching these declarations in a non-deterministic
order (e.g., we don't try to complete the type by accident).
This way we behave like the other parts of Clang which also sort the results by
some deterministic value (usually the name or something computed from a name,
e.g., edit distance to a given string).
We most likely also need to fix the Clang code to make the loop I listed above
deterministic to prevent these issues in the future (tracked in rdar://63442513
). This wouldn't replace the functionality provided in this patch though as we
would still need the priority and overall alphabetical sorting.
Note: I had to increase the lldb-vscode completion limit to 100 as the tests
look for strings that aren't in the first 50 results anymore due to variable
names starting with letters like 'v' (which are now always shown much further
down in the list due to the alphabetical sorting).
Fixes rdar://63200995
Reviewers: JDevlieghere, clayborg
Reviewed By: JDevlieghere
Subscribers: mgrang, abidh
Differential Revision: https://reviews.llvm.org/D80292
Summary:
For ObjCInterfaceDecls, LLDB iterates over the `methods` of the interface in FindExternalVisibleDeclsByName
since commit ef423a3ba5 .
However, when LLDB calls `oid->methods()` in that function, Clang will pull in all declarations in the current
DeclContext from the current ExternalASTSource (which is again, `ClangExternalASTSourceCallbacks`). The
reason for that is that `methods()` is just a wrapper for `decls()` which is supposed to provide a list of *all*
(both currently loaded and external) decls in the DeclContext.
However, `ClangExternalASTSourceCallbacks::FindExternalLexicalDecls` doesn't implement support for ObjCInterfaceDecl,
so we don't actually add any declarations and just mark the ObjCInterfaceDecl as having no ExternalLexicalStorage.
As LLDB uses the ExternalLexicalStorage to see if it can complete a type with the ExternalASTSource, this causes
that LLDB thinks our class can't be completed any further by the ExternalASTSource
and will from on no longer make any CompleteType/FindExternalLexicalDecls calls to that decl. This essentially
renders those types unusable in the expression parser as they will always be considered incomplete.
This patch just changes the call to `methods` (which is just a `decls()` wrapper), to some ad-hoc `noload_methods`
call which is wrapping `noload_decls()`. `noload_decls()` won't trigger any calls to the ExternalASTSource, so
this prevents that ExternalLexicalStorage will be set to false.
The test for this is just adding a method to an ObjC interface. Before this patch, this unset the ExternalLexicalStorage
flag and put the interface into the state described above.
In a normal user session this situation was triggered by setting a breakpoint in a method of some ObjC class. This
caused LLDB to create the MethodDecl for that specific method and put it into the the ObjCInterfaceDecl.
Also `ObjCLanguageRuntime::LookupInCompleteClassCache` needs to be unable to resolve the type do
an actual definition when the breakpoint is set (I'm not sure how exactly this can happen, but we just
found no Type instance that had the `TypePayloadClang::IsCompleteObjCClass` flag set in its payload in
the situation where this happens. This however doesn't seem to be a regression as logic wasn't changed
from what I can see).
The module-ownership.mm test had to be changed as the only reason why the ObjC interface in that test had
it's ExternalLexicalStorage flag set to false was because of this unintended side effect. What actually happens
in the test is that ExternalLexicalStorage is first set to false in `DWARFASTParserClang::CompleteTypeFromDWARF`
when we try to complete the `SomeClass` interface, but is then the flag is set back to true once we add
the last ivar of `SomeClass` (see `SetMemberOwningModule` in `TypeSystemClang.cpp` which is called
when we add the ivar). I'll fix the code for that in a follow-up patch.
I think some of the code here needs some rethinking. LLDB and Clang shouldn't infer anything about the ExternalASTSource
and its ability to complete the current type form the `ExternalLexicalStorage` flag. We probably should
also actually provide any declarations when we get asked for the lexical decls of an ObjCInterfaceDecl. But both of those
changes are bigger (and most likely would cause us to eagerly complete more types), so those will be follow up patches
and this patch just brings us back to the state before commit ef423a3ba5 .
Fixes rdar://63584164
Reviewers: aprantl, friss, shafik
Reviewed By: aprantl, shafik
Subscribers: arphaman, abidh, JDevlieghere
Differential Revision: https://reviews.llvm.org/D80556
This reverts commit 5f88f39ab8. It broke these
three tests on the Window bot:
lldb-api :: commands/expression/completion/TestExprCompletion.py
lldb-api :: lang/cpp/scope/TestCppScope.py
lldb-api :: lang/cpp/standards/cpp11/TestCPP11Standard.py
Raphael Isemann