Summary:
I often struggle to understand what exactly LLDB is doing by looking at our expression evaluation logging as our messages look like this:
```
CompleteTagDecl[2] on (ASTContext*)0x7ff31f01d240 Completing (TagDecl*)0x7ff31f01d568 named DeclName1
```
From the log messages it's unclear what this ASTContext is. Is it the scratch context, the expression context, some decl vendor context or a context from a module?
The pointer value isn't helpful for anyone unless I'm in a debugger where I could inspect the memory at the address. But even with a debugger it's not easy to
figure out what this ASTContext is without having deeper understanding about all the different ASTContext instances in LLDB (e.g., valid SourceLocation
from the file system usually means that this is the Objective-C decl vendor, a file name from multiple expressions is probably the scratch context, etc.).
This patch adds a name field to ClangASTContext instances that we can use to store a name which can be used for logging and debugging. With this
our log messages now look like this:
```
CompleteTagDecl[2] on scratch ASTContext. Completing (TagDecl*)0x7ff31f01d568 named Foo
```
We can now also just print a ClangASTContext from the debugger and see a useful name in the `m_display_name` field, e.g.
```
m_display_name = "AST for /Users/user/test/main.o";
```
Reviewers: shafik, labath, JDevlieghere, mib
Reviewed By: shafik
Subscribers: clayborg, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D72391
Summary:
CXXRecordDecls that have a move constructor but no copy constructor need to
have their implicit copy constructor marked as deleted (see C++11 [class.copy]p7, p18)
Currently we don't do that when building an AST with ClangASTContext which causes
Sema to realise that the AST is malformed and asserting when trying to create an implicit
copy constructor for us in the expression:
```
Assertion failed: ((data().DefaultedCopyConstructorIsDeleted || needsOverloadResolutionForCopyConstructor())
&& "Copy constructor should not be deleted"), function setImplicitCopyConstructorIsDeleted, file include/clang/AST/DeclCXX.h, line 828.
```
In the test case there is a class `NoCopyCstr` that should have its copy constructor marked as
deleted (as it has a move constructor). When we end up trying to tab complete in the
`IndirectlyDeletedCopyCstr` constructor, Sema realises that the `IndirectlyDeletedCopyCstr`
has no implicit copy constructor and tries to create one for us. It then realises that
`NoCopyCstr` also has no copy constructor it could find via lookup. However because we
haven't marked the FieldDecl as having a deleted copy constructor the
`needsOverloadResolutionForCopyConstructor()` returns false and the assert fails.
`needsOverloadResolutionForCopyConstructor()` would return true if during the time we
added the `NoCopyCstr` FieldDecl to `IndirectlyDeletedCopyCstr` we would have actually marked
it as having a deleted copy constructor (which would then mark the copy constructor of
`IndirectlyDeletedCopyCstr ` as needing overload resolution and Sema is happy).
This patch sets the correct mark when we complete our CXXRecordDecls (which is the time when
we know whether a copy constructor has been declared). In theory we don't have to do this if
we had a Sema around when building our debug info AST but at the moment we don't have this
so this has to do the job for now.
Reviewers: shafik
Reviewed By: shafik
Subscribers: aprantl, JDevlieghere, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D72694
and document the shortcomings of LLDB's partially defined DW_OP_piece
handling.
This would manifest as "DW_OP_piece for offset foo but top of stack is
of size bar".
rdar://problem/46262998
Differential Revision: https://reviews.llvm.org/D72880
By switching to Scalars that are backed by explicitly-sized APInts we
can avoid a bug that increases the buffer reserved for a small piece
to the next-largest host integer type.
This manifests as "DW_OP_piece for offset foo but top of stack is of size bar".
Differential Revision: https://reviews.llvm.org/D72879
Summary:
In Debug builds we call VerifyDecl in ClangASTContext::CreateFunctionDeclaration which in turn
calls `getAccess` on the created FunctionDecl. As we passed in a RecordDecl as the DeclContext
for the FunctionDecl, we end up hitting the assert in `getAccess` that checks that we never have
a Decl inside a Record without a valid AccessSpecifier. FunctionDecls are never in RecordDecls
(that would be a CXXMethodDecl) so setting a access specifier would not be the correct way to
fix this.
Instead this patch does the same thing that DWARFASTParserClang::ParseSubroutine is doing:
We pass in the FunctionDecl with the TranslationUnit as the DeclContext. That's not ideal but
it is how we currently do it when creating our debug info AST, so the unit test should do
the same.
Reviewers: shafik
Reviewed By: shafik
Subscribers: aprantl, JDevlieghere, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D72359
As suggested by @labath extended RangeDataVector so that user can provide
custom sorting of the Entry's `data' field for D63540.
https://reviews.llvm.org/D63540
RangeData functions were used just by RangeDataVector (=after I removed them
LLDB still builds fine) which no longer uses them so I removed them.
Differential revision: https://reviews.llvm.org/D72460
Creating an ASTContext with an unknown triple is rarely a good idea (as usually
all our ASTs have a valid triple that is either from the host or the target) and the
default argument makes it far to easy to implicitly create such an AST. Let's
remove it and force people to pass a triple.
The only place where we don't pass a triple is a DWARFASTParserClangTests
where we now just pass the host triple instead (the test doesn't depend on any
triple so this shouldn't change anything).
This constructor is supposed to take a string representing an llvm::Triple.
We might as well take a llvm::Triple here which saves us all the string
conversions in the call sites and we make this more type safe.
Summary:
We currently don't set access specifiers for function template declarations. This seems to be fine
as long as the function template is not declared inside any record in which case Clang asserts
with the following once we try to query it's access:
```
Assertion failed: (Access != AS_none && "Access specifier is AS_none inside a record decl"), function AccessDeclContextSanity,
```
This patch just marks these function template declarations as public to make Clang happy.
Reviewers: shafik, teemperor
Reviewed By: teemperor
Subscribers: JDevlieghere, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D71909
This adds a check that the ClangASTContext actually fits to the
DeclContext that we want to create a CompilerDeclContext for. If
the ClangASTContext (and its associated ASTContext) does not fit
to the DeclContext (that is, the DeclContext wasn't created by the
ASTContext), all computations using this malformed CompilerDeclContext
will yield unpredictable results.
Also fixes the only place that actually hits this assert which is the
construction of a CompilerDeclContext in ClangExpressionDeclMap
where we pass an unrelated ASTContext instead of the ASTContext
of the current expression.
I had to revert my previous change to DWARFASTParserClangTests.cpp
back to using the unsafe direct construction of CompilerDeclContext
as this assert won't work if the DeclContext we pass isn't a valid
DeclContext in the first place.
Summary:
Many of our tests need to initialize certain subsystems/plugins of LLDB such as
`FileSystem` or `HostInfo` by calling their static `Initialize` functions before the
test starts and then calling `::Terminate` after the test is done (in reverse order).
This adds a lot of error-prone boilerplate code to our testing code.
This patch adds a RAII called SubsystemRAII that ensures that we always call
::Initialize and then call ::Terminate after the test is done (and that the Terminate
calls are always in the reverse order of the ::Initialize calls). It also gets rid of
all of the boilerplate that we had for these calls.
Per-fixture initialization is still not very nice with this approach as it would
require some kind of static unique_ptr that gets manually assigned/reseted
from the gtest SetUpTestCase/TearDownTestCase functions. Because of that
I changed all per-fixture setup to now do per-test setup which can be done
by just having the SubsystemRAII as a member of the test fixture. This change doesn't
influence our normal test runtime as LIT anyway runs each test case separately
(and the Initialize/Terminate calls are anyway not very expensive). It will however
make running all tests in a single executable slightly slower.
Reviewers: labath, JDevlieghere, martong, espindola, shafik
Reviewed By: labath
Subscribers: mgorny, rnkovacs, emaste, MaskRay, abidh, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D71630
The CompilerDeclContext constructor takes a void* pointer which
means that all callers of this constructor need to first explicitly
convert all pointers to clang::DeclContext*. This causes that we
for example can't just pass a TranslationUnitDecl* to the constructor without
first casting it to its parent class (as it inherits from both
Decl and DeclContext so the void* pointer is actually a Decl*).
This patch introduces a utility function in the ClangASTContext
which gets rid of the requirement to cast all pointers to
clang::DeclContext. Also moves all constructor calls to use this
function instead which is NFC (beside the change in
DWARFASTParserClangTests.cpp).
ClangASTContext::getASTContext() currently returns a ptr but we have an assert there since a
while that the ASTContext is not a nullptr. This causes that we still have a lot of code
that is doing nullptr checks on the result of getASTContext() which is all unreachable code.
This patch changes the return value to a reference to make it clear this can't be a nullptr
and deletes all the nullptr checks.
Their naming is misleading as they only return the
ClangASTContext-owned variables. For ClangASTContext instances constructed
for a given clang::ASTContext they silently generated duplicated instances
(e.g., a second IdentifierTable) that were essentially unusable.
This removes all these getters as they are anyway not very useful in comparison
to just calling the clang::ASTContext getters. The initialization
code has been moved to the CreateASTContext initialization method so that all
code for making our own clang::ASTContext is in one place.
This implements a very elementary Lua script interpreter. It supports
running a single command as well as running interactively. It uses
editline if available. It's still missing a bunch of stuff though. Some
things that I intentionally ingored for now are that I/O isn't properly
hooked up (so every print goes to stdout) and the non-editline support
which is not handling a bunch of corner cases. The latter is a matter of
reusing existing code in the Python interpreter.
Discussion on the mailing list:
http://lists.llvm.org/pipermail/lldb-dev/2019-December/015812.html
Differential revision: https://reviews.llvm.org/D71234
We already pass a Decl here and the additional ASTContext needs to
match the Decl. We might as well just pass the Decl and then extract
the ASTContext from that.
This adds a unit test for looking up persistent declarations in the scratch AST
context. Also adds the `GetPersistentDecl` hook to the ClangExpressionDeclMap
that this unit test can emulate looking up persistent variables without having
a lldb_private::Target.
The ClangExpressionDeclMap should be testable from a unit test. This is currently
impossible as they have both dependencies on Target/ExecutionContext from their
constructor. This patch allows constructing these classes without an active Target
and adds the missing tests for running without a target that we can do at least
a basic lookup test without crashing.
Summary:
These types were handled in some places, but not others. This resulted
in (for example) not being able to display members of structs whose
types were defined using these constructs.
Using getLocallyUnqualifiedSingleStepDesugaredType for these types is
not fully equivalent, as it will only desugar them if the types are not
instantiation-dependent, whereas previously we did that unconditionally.
It's not clear to me which behavior is correct here, but the test suite
does not seem to care either way.
Reviewers: teemperor, shafik
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D71405
As suggested by Pavel in a code review:
> Can we replace this (and maybe python too, while at it) with a
> Host/Config.h entry? A global definition means that one has to
> recompile everything when these change in any way, whereas in
> practice only a handful of files need this..
Differential revision: https://reviews.llvm.org/D71280
GetMaxU64Bitfield(...) uses the ul suffix but we require a 64 bit unsigned integer and ul could be 32 bit. So this replacing it with a explicit cast and refactors the code around it to use an early exit.
Differential Revision: https://reviews.llvm.org/D70992
Summary:
Yet another step on the long road towards getting rid of lldb's Stream class.
We probably should just make this some kind of member of Address/AddressRange, but it seems quite often we just push
in random integers in there and this is just about getting rid of Stream and not improving arbitrary APIs.
I had to rename another `DumpAddress` function in FormatEntity that is dumping the content of an address to make Clang happy.
Reviewers: labath
Reviewed By: labath
Subscribers: JDevlieghere, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D71052
Summary:
This patch adds code which will substitute references to the full object
constructors/destructors with their base object versions.
Like all substitutions in this category, this operation is not really
sound, but doing this in a more precise way allows us to get rid of a
much larger hack -- matching function according to their demangled
names, which effectively does the same thing, but also much more.
This is a (very late) follow-up to D54074.
Background: clang has an optimization which can eliminate full object
structors completely, if they are found to be equivalent to their base
object versions. It does this because it assumes they can be regenerated
on demand in the compile unit that needs them (e.g., because they are
declared inline). However, this doesn't work for the debugging scenario,
where we don't have the structor bodies available -- we pretend all
constructors are defined out-of-line as far as clang is concerned. This
causes clang to emit references to the (nonexisting) full object
structors during expression evaluation.
Fun fact: This is not a problem on darwin, because the relevant
optimization is disabled to work around a linker bug.
Reviewers: teemperor, JDevlieghere
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D70721
Summary:
This patch fixes a bug where when target triple created from elf information
is arm-*-linux-eabihf and platform triple is armv8l-*-linux-gnueabihf. Merging
both triple results in armv8l--unknown-unknown.
This happens because we order a triple update while calling CoreUpdated and
CoreUpdated creates a new triple with no vendor or environment information.
Making sure we do not update triple and just update to more specific core
fixes the issue.
Reviewers: labath, jasonmolenda, clayborg
Reviewed By: jasonmolenda
Subscribers: jankratochvil, kristof.beyls, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D70155
Summary:
The FileSpec class is often used as a sort of a pattern -- one specifies
a bare file name to search, and we check if in matches the full file
name of an existing module (for example).
These comparisons used FileSpec::Equal, which had some support for it
(via the full=false argument), but it was not a good fit for this job.
For one, it did a symmetric comparison, which makes sense for a function
called "equal", but not for typical searches (when searching for
"/foo/bar.so", we don't want to find a module whose name is just
"bar.so"). This resulted in patterns like:
if (FileSpec::Equal(pattern, file, pattern.GetDirectory()))
which would request a "full" match only if the pattern really contained
a directory. This worked, but the intended behavior was very unobvious.
On top of that, a lot of the code wanted to handle the case of an
"empty" pattern, and treat it as matching everything. This resulted in
conditions like:
if (pattern && !FileSpec::Equal(pattern, file, pattern.GetDirectory())
which are nearly impossible to decipher.
This patch introduces a FileSpec::Match function, which does exactly
what most of FileSpec::Equal callers want, an asymmetric match between a
"pattern" FileSpec and a an actual FileSpec. Empty paterns match
everything, filename-only patterns match only the filename component.
I've tried to update all callers of FileSpec::Equal to use a simpler
interface. Those that hardcoded full=true have been changed to use
operator==. Those passing full=pattern.GetDirectory() have been changed
to use FileSpec::Match.
There was also a handful of places which hardcoded full=false. I've
changed these to use FileSpec::Match too. This is a slight change in
semantics, but it does not look like that was ever intended, and it was
more likely a result of a misunderstanding of the "proper" way to use
FileSpec::Equal.
[In an ideal world a "FileSpec" and a "FileSpec pattern" would be two
different types, but given how widespread FileSpec is, it is unlikely
we'll get there in one go. This at least provides a good starting point
by centralizing all matching behavior.]
Reviewers: teemperor, JDevlieghere, jdoerfert
Subscribers: emaste, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D70851
Summary:
A most of these tests create FileSpecs with a hardcoded style. Add
utility functions which create a file spec of a given style to simplify
things.
While in there add SCOPED_TRACE messages to tests which loop over
multiple inputs to ensure it's clear which of the inputs failed.
Reviewers: teemperor
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D70814
Summary:
I recently re-discovered that the unsinged stream operators of the
lldb_private::Stream class have a surprising behavior in that they print
the number in hex. This is all the more confusing because the "signed"
versions of those operators behave normally.
Now that, thanks to Raphael, each Stream class has a llvm::raw_ostream
wrapper, I think we should delete most of our formatting capabilities
and just delegate to that. This patch tests the water by just deleting
the operators with the most surprising behavior.
Most of the code using these operators was printing user_id_t values. It
wasn't fully consistent about prefixing them with "0x", but I've tried
to consistenly print it without that prefix, to make it more obviously
different from pointer values.
Reviewers: teemperor, JDevlieghere, jdoerfert
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D70241
Raphael Isemann