Summary:
CompletionTest.DirCompletionAbsolute had a random failure on a CI node
(in the failure, the completion count was 0, while we expected it to be 1),
but there seems no good reason for it to fail. The sanitizers don't complain
about the test when it's run, so I think we don't have some uninitialized
memory that we access here.
My best bet is that the unique directory selection randomly failed on the CI
node because maybe the FS there doesn't actually guarantee the atomic fopen
assumptions we make in the LLVM code (or some other funny race condition).
In this case a different test run could get the same directory and clean its contents
which would lead to 0 results.
The other possible explanation is that someone changed the CI configuration
on the node and changed the working dir to something very long, which would
make our PATH_MAX test fail (which also leads to 0 results), but I think that case
is unlikely.
This patch is just a stab in the dark that (hopefully) fixes this random failure by
giving each test a (more) unique working directory by appending the unique
test name to the temp-dir prefix. Also adds one more ASSERT_NO_ERROR to
one of our chdir calls just in case that is the reason for failing.
The good thing is that this refactor gets rid of most of the static variables
and files that we previously had as shared state between the different tests.
Potentially fixes rdar://problem/43150260
Reviewers: aprantl
Reviewed By: aprantl
Subscribers: jfb, lldb-commits
Differential Revision: https://reviews.llvm.org/D50722
llvm-svn: 339715
Summary:
This issue came up because it caused problems in our unit tests. The StringPool did connect counterparts only once and silently ignored the values passed in subsequent calls.
The simplest solution for the unit tests would be silent overwrite. In practice, however, it seems useful to assert that we never overwrite a different mangled counterpart.
If we ever have mangled counterparts for other languages than C++, this makes it more likely to notice collisions.
I added an assertion that allows the following cases:
* inserting a new value
* overwriting the empty string
* overwriting with an identical value
I fixed the unit tests, which used "random" strings and thus produced collisions.
It would be even better if there was a way to reset or isolate the StringPool, but that's a different story.
Reviewers: jingham, friss, labath
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D50536
llvm-svn: 339669
Summary:
We can optimize and refactor some of the classes in RangeMap.h, but first
we should have some tests for all the data structures in there. This adds a first
batch of tests for the Range class itself.
There are some unexpected results happening when mixing invalid and valid ranges, so
I added some FIXME's for that in the tests.
Reviewers: vsk
Reviewed By: vsk
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D50620
llvm-svn: 339611
Summary:
Removing FastDemangle will greatly reduce maintenance efforts. This patch replaces the last point of use in LLDB. Semantics should be kept intact.
Once this is agreed upon, we can:
* Remove the FastDemangle sources
* Add more features e.g. substitutions in template parameters, considering all variations, etc.
Depends on LLVM patch https://reviews.llvm.org/D50586
Reviewers: erik.pilkington, friss, jingham, JDevlieghere
Subscribers: kristof.beyls, chrib, lldb-commits
Differential Revision: https://reviews.llvm.org/D50587
llvm-svn: 339583
Summary: It was not immediately clear to me whether or not non-null-terminated StringRef's are supported in ConstString and/or the counterpart mechanism. From this test it seems to be fine. Maybe useful to keep?
Reviewers: labath
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D50334
llvm-svn: 339292
Summary:
I set up a new review, because not all the code I touched was marked as a change in old one anymore.
In preparation for this review, there were two earlier ones:
* https://reviews.llvm.org/D49612 introduced the ItaniumPartialDemangler to LLDB demangling without conceptual changes
* https://reviews.llvm.org/D49909 added a unit test that covers all relevant code paths in the InitNameIndexes() function
Primary goals for this patch are:
(1) Use ItaniumPartialDemangler's rich mangling info for building LLDB's name index.
(2) Provide a uniform interface.
(3) Improve indexing performance.
The central implementation in this patch is our new function for explicit demangling:
```
const RichManglingInfo *
Mangled::DemangleWithRichManglingInfo(RichManglingContext &, SkipMangledNameFn *)
```
It takes a context object and a filter function and provides read-only access to the rich mangling info on success, or otherwise returns null. The two new classes are:
* `RichManglingInfo` offers a uniform interface to query symbol properties like `getFunctionDeclContextName()` or `isCtorOrDtor()` that are forwarded to the respective provider internally (`llvm::ItaniumPartialDemangler` or `lldb_private::CPlusPlusLanguage::MethodName`).
* `RichManglingContext` works a bit like `LLVMContext`, it the actual `RichManglingInfo` returned from `DemangleWithRichManglingInfo()` and handles lifetime and configuration. It is likely stack-allocated and can be reused for multiple queries during batch processing.
The idea here is that `DemangleWithRichManglingInfo()` acts like a gate keeper. It only provides access to `RichManglingInfo` on success, which in turn avoids the need to handle a `NoInfo` state in every single one of its getters. Having it stored within the context, avoids extra heap allocations and aids (3). As instantiations of the IPD the are considered expensive, the context is the ideal place to store it too. An efficient filtering function `SkipMangledNameFn` is another piece in the performance puzzle and it helps to mimic the original behavior of `InitNameIndexes`.
Future potential:
* `DemangleWithRichManglingInfo()` is thread-safe, IFF using different contexts in different threads. This may be exploited in the future. (It's another thing that it has in common with `LLVMContext`.)
* The old implementation only parsed and indexed Itanium mangled names. The new `RichManglingInfo` can be extended for various mangling schemes and languages.
One problem with the implementation of RichManglingInfo is the inaccessibility of class `CPlusPlusLanguage::MethodName` (defined in source/Plugins/Language/..), from within any header in the Core components of LLDB. The rather hacky solution is to store a type erased reference and cast it to the correct type on access in the cpp - see `RichManglingInfo::get<ParserT>()`. At the moment there seems to be no better way to do it. IMHO `CPlusPlusLanguage::MethodName` should be a top-level class in order to enable forward delcarations (but that is a rather big change I guess).
First simple profiling shows a good speedup. `target create clang` now takes 0.64s on average. Before the change I observed runtimes between 0.76s an 1.01s. This is still no bulletproof data (I only ran it on one machine!), but it's a promising indicator I think.
Reviewers: labath, jingham, JDevlieghere, erik.pilkington
Subscribers: zturner, clayborg, mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D50071
llvm-svn: 339291
These three classes have no external dependencies, but they are used
from various low-level APIs. Moving them down to Utility improves
overall code layering (although it still does not break any particular
dependency completely).
The XCode project will need to be updated after this change.
Differential Revision: https://reviews.llvm.org/D49740
llvm-svn: 339127
Summary: `IsEmpty()` and `operator bool() == false` have equal semantics. Usage in Mangled::GetDemangledName() was incorrect. What it actually wants is a check for null-string. Split this off of D50071 and added a test to clarify usage.
Reviewers: labath, jingham
Subscribers: erik.pilkington, lldb-commits
Differential Revision: https://reviews.llvm.org/D50327
llvm-svn: 339014
Summary:
This patch allows LLDB's Stream class to count the bytes it has written to so far.
There are two major motivations for this patch:
The first one is that this will allow us to get rid of all the handwritten byte counting code
we have in LLDB so far. Examples for this are pretty much all functions in LLDB that
take a Stream to write to and return a size_t, which usually represents the bytes written.
By moving to this centralized byte counting mechanism, we hopefully can avoid some
tricky errors that happen when some code forgets to count the written bytes while
writing something to a stream.
The second motivation is that this is needed for the migration away from LLDB's `Stream`
and towards LLVM's `raw_ostream`. My current plan is to start offering a fake raw_ostream
class that just forwards to a LLDB Stream.
However, for this raw_ostream wrapper we need to fulfill the raw_ostream interface with
LLDB's Stream, which currently lacks the ability to count the bytes written so far (which
raw_ostream exposes by it's `tell()` method). By adding this functionality it is trivial to start
rolling out our raw_ostream wrapper (and then eventually completely move to raw_ostream).
Also, once this fake raw_ostream is available, we can start replacing our own code writing
to LLDB's Stream by LLVM code writing to raw_ostream. The best example for this is the
LEB128 encoding we currently ship, which can be replaced with by LLVM's version which
accepts an raw_ostream.
From the point of view of the pure source changes this test does, we essentially just renamed
the Write implementation in Stream to `WriteImpl` while the `Write` method everyone is using
to write its raw bytes is now just forwarding and counting the written bytes.
Reviewers: labath, davide
Reviewed By: labath
Subscribers: JDevlieghere, lldb-commits
Differential Revision: https://reviews.llvm.org/D50159
llvm-svn: 338733
Summary: In order to exploit the potential of LLVM's new ItaniumPartialDemangler for indexing in LLDB, we expect conceptual changes in the implementation of the InitNameIndexes function. Here is a unit test that aims at covering all relevant code paths in that function.
Reviewers: labath, jingham, JDevlieghere
Reviewed By: JDevlieghere
Subscribers: friss, teemperor, davide, clayborg, erik.pilkington, lldb-commits, mgorny
Differential Revision: https://reviews.llvm.org/D49909
llvm-svn: 338695
Summary:
This patch adds syntax highlighting support to LLDB. When enabled (and lldb is allowed
to use colors), printed source code is annotated with the ANSI color escape sequences.
So far we have only one highlighter which is based on Clang and is responsible for all
languages that are supported by Clang. It essentially just runs the raw lexer over the input
and then surrounds the specific tokens with the configured escape sequences.
Reviewers: zturner, davide
Reviewed By: davide
Subscribers: labath, teemperor, llvm-commits, mgorny, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D49334
llvm-svn: 338662
Summary:
When I added the Stream unit test (r338488), the build bots failed due to an out-of-
bound reads when passing an empty string to the PutCStringAsRawHex8 method.
In r338491 I removed the test case to fix the bots.
This patch fixes this in PutCStringAsRawHex8 by always checking for the terminating
null character in the given string (instead of skipping it the first time). It also re-adds the
test case I removed.
Reviewers: vsk
Reviewed By: vsk
Subscribers: vsk, lldb-commits
Differential Revision: https://reviews.llvm.org/D50149
llvm-svn: 338637
The suspicious behavior is obviously because this method reads
OOB memory, so I'll remove it for now and re-add the test alongside
the fix later.
llvm-svn: 338491
Summary:
This adds an initial small unit test for LLDB's Stream class, which should at least cover
most of the functions in the Stream class. StreamString is always in big endian
mode, so that's the only stream byte order path this test covers as of now. Also,
the binary mode still needs to be tested for all print methods.
Also adds some FIXMEs for wrong/strange result values of the Stream class that we hit
while testing those functions.
Reviewers: labath
Reviewed By: labath
Subscribers: probinson, labath, mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D50027
llvm-svn: 338488
Summary:
We currently allow any completion handler to read and manipulate the list of matches we
calculated so far. This leads to a few problems:
Firstly, a completion handler's logic can now depend on previously calculated results
by another handlers. No completion handler should have such an implicit dependency,
but the current API makes it likely that this could happen (or already happens). Especially
the fact that some completion handler deleted all previously calculated results can mess
things up right now.
Secondly, all completion handlers have knowledge about our internal data structures with
this API. This makes refactoring this internal data structure much harder than it should be.
Especially planned changes like the support of descriptions for completions are currently
giant patches because we have to refactor every single completion handler.
This patch narrows the contract the CompletionRequest has with the different handlers to:
1. A handler can suggest a completion.
2. A handler can ask how many suggestions we already have.
Point 2 obviously means we still have a dependency left between the different handlers, but
getting rid of this is too large to just append it to this patch.
Otherwise this patch just completely hides the internal StringList to the different handlers.
The CompletionRequest API now also ensures that the list of completions is unique and we
don't suggest the same value multiple times to the user. This property has been so far only
been ensured by the `Option` handler, but is now applied globally. This is part of this patch
as the OptionHandler is no longer able to implement this functionality itself.
Reviewers: jingham, davide, labath
Reviewed By: davide
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D49322
llvm-svn: 338151
Summary:
Replace the existing combination of FastDemangle and the fallback to llvm::itaniumDemangle() with LLVM's new ItaniumPartialDemangler. It slightly reduces complexity and slightly improves performance, but doesn't introduce conceptual changes. This patch is preparing for more fundamental improvements on LLDB's demangling approach.
Reviewers: friss, jingham, erik.pilkington, labath, clayborg, mgorny, davide, JDevlieghere
Reviewed By: JDevlieghere
Subscribers: teemperor, JDevlieghere, labath, clayborg, davide, lldb-commits, mgorny, erik.pilkington
Differential Revision: https://reviews.llvm.org/D49612
llvm-svn: 337931
The new test checks that we are actually able to read data from these
kinds of elf headers correctly instead of just that we read the section
number correctly. It is also easier to figure out what's going on in the
test.
llvm-svn: 337459
Summary:
This patch fixes a few problems with the FormatAnsiTerminalCodes function:
* It does an infinite loop on an unknown color value.
* It crashes when the color value is at the end of the string.
* It deletes the first character behind the color token.
Also added a few tests that reproduce those problems (and test some other corner cases).
Reviewers: davide, labath
Reviewed By: labath
Subscribers: labath, lldb-commits, mgorny
Differential Revision: https://reviews.llvm.org/D49307
llvm-svn: 337189
Summary:
As suggested in D48796, this patch replaces even more internal calls that were using the old
completion API style with a single CompletionRequest. In some cases we also pass an option
vector/index, but as we don't always have this information, it currently is not part of the
CompletionRequest class.
The constructor of the CompletionRequest is now also more sensible. You only pass the
user input, cursor position and your list of matches to the request and the rest will be
inferred (using the same code we used before to calculate this). You also have to pass these
match window parameters to it, even though they are unused right now.
The patch shouldn't change any behavior.
Reviewers: jingham
Reviewed By: jingham
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D48976
llvm-svn: 337031
Corrupted minidumps was leading to unpredictable behavior.
This change adds explicit consistency checks for the minidump early on. The
checks are not comprehensive but they should catch obvious structural violations:
streams with type == 0
duplicate streams (same type)
overlapping streams
truncated minidumps
Another early check is to make sure we actually support the minidump architecture
instead of crashing at a random place deep inside LLDB.
Differential Revision: https://reviews.llvm.org/D49202
llvm-svn: 336918
Summary:
A subset of the LLDB commands follows this command line interface style:
<command name> [arguments] -- <string suffix>
The parsing code for this interface has been so far been duplicated into the different
command objects which makes it hard to maintain and reuse elsewhere.
This patches improves the situation by adding a OptionsWithRaw class that centralizes
the parsing logic and allows easier testing. The different commands now just call this class to
extract the arguments and the raw suffix from the provided user input.
Reviewers: jingham
Reviewed By: jingham
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D49106
llvm-svn: 336723
Summary:
This patch refactors the internal completion API. It now takes (as far as possible) a single
CompletionRequest object instead o half a dozen in/out/in-out parameters. The CompletionRequest
contains a common superset of the different parameters as far as it makes sense. This includes
the raw command line string and raw cursor position, which should make the `expr` command
possible to implement (at least without hacks that reconstruct the command line from the args).
This patch is not intended to change the observable behavior of lldb in any way. It's also as
minimal as possible and doesn't attempt to fix all the problems the API has.
Some Q&A:
Q: Why is this not fixing all the problems in the completion API?
A: Because is a blocker for the expr command completion which I want to get in ASAP. This is the
smallest patch that unblocks the expr completion patch and which allows trivial refactoring in the future.
The patch also doesn't really change the internal information flow in the API, so that hopefully
saves us from ever having to revert and resubmit this humongous patch.
Q: Can we merge all the copy-pasted code in the completion methods
(like computing the current incomplete arg) into CompletionRequest class?
A: Yes, but it's out of scope for this patch.
Q: Why the `word_complete = request.GetWordComplete(); ... ` pattern?
A: I don't want to add a getter that returns a reference to the internal integer. So we have
to use a temporary variable and the Getter/Setter instead. We don't throw exceptions
from what I can tell, so the behavior doesn't change.
Q: Why are we not owning the list of matches?
A: Because that's how the previous API works. But that should be fixed too (in another patch).
Q: Can we make the constructor simpler and compute some of the values from the plain command?
A: I think this works, but I rather want to have this in a follow up commit. Especially when making nested
request it's a bit awkward that the parsed arguments behave as both input/output (as we should in theory
propagate the changes on the nested request back to the parent request if we don't want to change the
behavior too much).
Q: Can't we pass one const request object and then just return another result object instead of mixing
them together in one in/out parameter?
A: It's hard to get keep the same behavior with that pattern, but I think we can also get a nice API with just
a single request object. If we make all input parameters read-only, we have a clear separation between what
is actually an input and what an output parameter (and hopefully we get rid of the in-out parameters).
Q: Can we throw out the 'match' variables that are not implemented according to the comment?
A: We currently just forward them as in the old code to the different methods, even though I think
they are really not used. We can easily remove and readd them once every single completion method just
takes a CompletionRequest, but for now I prefer NFC behavior from the perspective of the API user.
Reviewers: davide, jingham, labath
Reviewed By: jingham
Subscribers: mgorny, friss, lldb-commits
Differential Revision: https://reviews.llvm.org/D48796
llvm-svn: 336146
Summary:
The data structure is optimized for the case where the UUID size is <=
20 bytes (standard length emitted by the GNU linkers), but larger sizes
are also possible.
I've modified the string conversion function to support the new sizes as
well. For standard UUIDs it maintains the traditional formatting
(4-2-2-2-6). If a UUID is shorter, we just cut this sequence short, and
for longer UUIDs it will just repeat the last 6-byte block as long as
necessary.
I've also modified ObjectFileELF to take advantage of the new UUIDs and
avoid manually padding the UUID to 16 bytes. While there, I also made
sure the computed UUID does not depend on host endianness.
Reviewers: clayborg, lemo, sas, davide, espindola
Subscribers: emaste, arichardson, lldb-commits
Differential Revision: https://reviews.llvm.org/D48633
llvm-svn: 335963
Now that we have gmock, we can use its matchers to better express the
test assertions. The main advantage of this is that when things fail,
the test will now print the expected and actual lists of completed
strings instead of just a not-very-helpful "false is not true" message.
llvm-svn: 335955
Summary:
The test fails because we don't rewrite the slash behind `foo` to the OS specific
separator (as the completion API doesn't support this kind of rewriting). However,
we assume that this part of the string is rewritten in the test case, which broke
on Windows.
Reviewers: stella.stamenova
Reviewed By: stella.stamenova
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D48746
llvm-svn: 335934
Summary:
During the previous attempt to generalize the UUID class, it was
suggested that we represent invalid UUIDs as length zero (previously, we
used an all-zero UUID for that). This meant that some valid build-ids
could not be represented (it's possible however unlikely that a checksum of
some file would be zero) and complicated adding support for variable
length build-ids (should a 16-byte empty UUID compare equal to a 20-byte
empty UUID?).
This patch resolves these issues by introducing a canonical
representation for an invalid UUID. The slight complication here is that
some clients (MachO) actually use the all-zero notation to mean "no UUID
has been set". To keep this use case working (while making it very
explicit about which construction semantices are wanted), replaced the
UUID constructors and the SetBytes functions with named factory methods.
- "fromData" creates a UUID from the given data, and it treats all bytes
equally.
- "fromOptionalData" first checks the data contents - if all bytes are
zero, it treats this as an invalid/empty UUID.
Reviewers: clayborg, sas, lemo, davide, espindola
Subscribers: emaste, lldb-commits, arichardson
Differential Revision: https://reviews.llvm.org/D48479
llvm-svn: 335612
Instead of a separate GetBytes + GetByteSize methods I introduce a
single GetBytes method returning an ArrayRef.
This is NFC cleanup now, but it should make handling arbitrarily-sized
UUIDs cleaner, should we choose to go that way. I also took the
opportunity to add some unit tests for this class.
llvm-svn: 335244
StringConvert was the only non-Utility dependency of this class. Getting
rid of it means it will be easy to move this class to a lower layer.
While I was in there, I also added a couple of unit tests for the Scalar
string conversion function.
llvm-svn: 335060
Summary:
Instead of a function taking an enum value determining which path to
return, we now have a suite of functions, each returning a single path
kind. This makes it easy to move the python-path function into a
specific plugin in a follow-up commit.
All the users of GetLLDBPath were converted to call specific functions
instead. Most of them were hard-coding the enum value anyway, so this
conversion was simple. The only exception was SBHostOS, which I've
changed to use a switch on the incoming enum value.
Reviewers: clayborg, zturner
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D48272
llvm-svn: 335052
Summary:
This has multiple advantages:
- we need only one function argument/instance variable instead of three
- no need to default initialize variables
- no custom parsing code
- VersionTuple has comparison operators, which makes version comparisons much
simpler
Reviewers: zturner, friss, clayborg, jingham
Subscribers: emaste, lldb-commits
Differential Revision: https://reviews.llvm.org/D47889
llvm-svn: 334950
r334615 changed the the value of FileSpec.IsRelative("/") for windows
path syntax. We previously considered it absolute but now it is
considered relative (I guess because it's interpretation depends on the
current drive).
This cause a failure in PathMappingList test, which assumed that "/"
will not get remapped as it is an absolute path. As this is no longer
true on windows, I replace "/" with a really absolute path.
llvm-svn: 334702
With the recent changes in FileSpec to use LLVM's path style, it is
possible to delegate a bunch of common path operations to LLVM's path
helpers. This means we only have to maintain a single implementation and
at the same time can benefit from the efforts made by the rest of the
LLVM community.
This is part one of a set of patches. There was no obvious way to split
this so I just worked from top to bottom.
Differential revision: https://reviews.llvm.org/D48084
llvm-svn: 334615
This simplifies some code which had StringRefs to begin with, and
makes other code more complicated which had const char* to begin
with.
In the end, I think this makes for a more idiomatic and platform
agnostic API. Not all platforms launch process with null terminated
c-string arrays for the environment pointer and argv, but the api
was designed that way because it allowed easy pass-through for
posix-based platforms. There's a little additional overhead now
since on posix based platforms we'll be takign StringRefs which
were constructed from null terminated strings and then copying
them to null terminate them again, but from a readability and
usability standpoint of the API user, I think this API signature
is strictly better.
llvm-svn: 334518
Summary:
The patch adds support of splitted functions (when MSVC is used with PGO) and function-level linking feature.
SymbolFilePDB::ParseCompileUnitLineTable function relies on fact that ranges of compiled source files in the binary are continuous and don't intersect each other. The function creates LineSequence for each file and inserts it into LineTable, and implementation of last one relies on continuity of the sequence. But it's not always true when function-level linking enabled, e.g. in added input test file test-pdb-function-level-linking.exe there is xstring's std__basic_string_char_std__char_traits_char__std__allocator_char_____max_size (.00454820) between test-pdb-function-level-linking.cpp's foo (.00454770) and main (.004548F0).
To fix the problem this patch renews the sequence on each address gap.
Reviewers: asmith, zturner
Reviewed By: asmith
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D47708
llvm-svn: 334030
Raphael Isemann