Commit Graph

123 Commits

Author SHA1 Message Date
Pavel Labath 7f815a9a42 Have Stream::PutCStringAsRawHex8 take llvm::StringRef
This enables the function to be called with a StringRef without jumping
through any hoops. I rename the function to "PutStringAsRawHex8" to
honor the extended interface. I also remove ".c_str()" from any calls to
this function I could find.

llvm-svn: 353841
2019-02-12 14:28:55 +00:00
Pavel Labath 841bea933b Breakpad: auto-detect path style of file entries
Summary:
This adds support for auto-detection of path style to SymbolFileBreakpad
(similar to how r351328 did the same for DWARF). We guess each file
entry separately, as we have no idea which file came from which compile
units (and different compile units can have different path styles). The
breakpad generates should have already converted the paths to absolute
ones, so this guess should be reasonable accurate, but as always with
these kinds of things, it is hard to give guarantees about anything.

In an attempt to bring some unity to the path guessing logic, I move the
guessing logic from inside SymbolFileDWARF into the FileSpec class and
have both symbol files use it to implent their desired behavior.

Reviewers: clayborg, lemo, JDevlieghere

Subscribers: aprantl, markmentovai, lldb-commits

Differential Revision: https://reviews.llvm.org/D57895

llvm-svn: 353702
2019-02-11 14:11:00 +00:00
Jonas Devlieghere 58947cf854 [Reproducers] SBReproducer framework: Capture & Replay
This is part two of the reproducer instrumentation framework. It
contains the code to capture and replay function calls. The main user of
this framework will be the SB API layer.

For all the details refer to the RFC on the mailing list:
http://lists.llvm.org/pipermail/lldb-dev/2019-January/014530.html

Differential revision: https://reviews.llvm.org/D56322

llvm-svn: 353324
2019-02-06 18:57:42 +00:00
Jonas Devlieghere 494fd8f84f [Reproducers] Instrumentation Framework: Serialization
This is the is serialization/deserialization part of the reproducer
instrumentation framework.

For all the details refer to the RFC on the mailing list:
http://lists.llvm.org/pipermail/lldb-dev/2019-January/014530.html

Differential revision: https://reviews.llvm.org/D57714

llvm-svn: 353195
2019-02-05 18:46:36 +00:00
Jonas Devlieghere 043ff33362 [unittest] Fix scalar unit test.
The test was using ASSERT_EQ instead of ASSERT_STREQ which meant we were
comparing string addresses instead of the actual string. This caused the
test to fail with with the sanitizers enabled.

llvm-svn: 352780
2019-01-31 18:48:17 +00:00
Davide Italiano 51d46bd4d2 [Scalar] Implement support for 512-bit values.
(useful, e.g. when reading 512-bits registers, a-la AVX-512).

<rdar://problem/46886288>

llvm-svn: 352639
2019-01-30 18:05:36 +00:00
Jonas Devlieghere 7d2192ca7b [test] Use correct macro in FileCollectorTest.cpp
This test was supposed to be skipped on Windows but was using the wrong
macro name.

llvm-svn: 352556
2019-01-29 22:53:47 +00:00
Jonas Devlieghere 46575176e9 [Reproducers] Add file provider
This patch adds the file provider which is responsible for capturing
files used by LLDB.

When capturing a reproducer, we use a file collector that is very
similar to the one used in clang. For every file that we touch, we add
an entry with a mapping from its virtual to its real path. When we
decide to generate a reproducer we copy over the files and their
permission into to reproducer folder.

When replaying a reproducer, we load the VFS mapping and instantiate a
RedirectingFileSystem. The latter will transparently use the files
available in the reproducer.

I've tested this on two macOS machines with an artificial example.
Still, it is very likely that I missed some places where we (still) use
native file system calls. I'm hoping to flesh those out while testing
with more advanced examples. However, I will fix those things in
separate patches.

Differential revision: https://reviews.llvm.org/D54617

llvm-svn: 352538
2019-01-29 20:36:38 +00:00
Jim Ingham f3ecbfc164 Add UUID::SetFromOptionalStringRef, use it in DynamicLoaderDarwin
We use UUID::fromOptionalData to read UUID's from the Mach-O files, so UUID's
of all 0's are invalid UUID's.
We also get uuid's from debugserver, which need to match the file UUID's.  So
we need an API that treats "000000000" as invalid as well.  Added that and use it.

Differential Revision: https://reviews.llvm.org/D57195

llvm-svn: 352122
2019-01-24 22:43:44 +00:00
Davide Italiano 7fca260dc8 [Scalar] Clarify the constructor from APInt and document through a test.
I want to add 512-bits support but I first want to make sure I'm
not breaking anything obvious. This is the first of a series of commit
adding tests. The first oddity found is that Scalar from APInt(s)
always constructed signed. Maybe at some point we want to revisit
this, but at least now we have a test to document how the API behaves.

<rdar://problem/46886288>

llvm-svn: 352103
2019-01-24 20:55:12 +00:00
Chandler Carruth 2946cd7010 Update the file headers across all of the LLVM projects in the monorepo
to reflect the new license.

We understand that people may be surprised that we're moving the header
entirely to discuss the new license. We checked this carefully with the
Foundation's lawyer and we believe this is the correct approach.

Essentially, all code in the project is now made available by the LLVM
project under our new license, so you will see that the license headers
include that license only. Some of our contributors have contributed
code under our old license, and accordingly, we have retained a copy of
our old license notice in the top-level files in each project and
repository.

llvm-svn: 351636
2019-01-19 08:50:56 +00:00
Jonas Devlieghere 17f277d754 [Reproducers] Fix reproducers unittest.
Fix fallout from r351501 in the reproducer unittest.

llvm-svn: 351524
2019-01-18 07:17:05 +00:00
Davide Italiano ff92a1a7cf [Scalar] Simplify comparison operators and add coverage.
llvm-svn: 350428
2019-01-04 19:23:52 +00:00
Pavel Labath f760f5aef4 Simplify ObjectFile::GetArchitecture
Summary:
instead of returning the architecture through by-ref argument and a
boolean value indicating success, we can just return the ArchSpec
directly. Since the ArchSpec already has an invalid state, it can be
used to denote the failure without the additional bool.

Reviewers: clayborg, zturner, espindola

Subscribers: emaste, arichardson, JDevlieghere, lldb-commits

Differential Revision: https://reviews.llvm.org/D56129

llvm-svn: 350291
2019-01-03 10:37:19 +00:00
Davide Italiano b429637168 [Scalar] Simplify as Jonas suggested. NFCI.
llvm-svn: 349971
2018-12-21 22:45:07 +00:00
Davide Italiano 18a0ce9813 [Scalar] Implement operator!= using operator==.
Summary: Adding some test coverage while I'm around.

Reviewers: JDevlieghere, aprantl, zturner, clayborg, jingham

Differential Revision: https://reviews.llvm.org/D56017

llvm-svn: 349970
2018-12-21 22:42:00 +00:00
Pavel Labath 181b823b04 Move Broadcaster+Listener+Event combo from Core into Utility
Summary:
These are general purpose "utility" classes, whose functionality is not
debugger-specific in any way. As such, I believe they belong in the
Utility module.

This doesn't break any particular dependency (yet), but it reduces the
number of Core dependencies across the board.

Reviewers: zturner, jingham, teemperor, clayborg

Subscribers: mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D55361

llvm-svn: 349157
2018-12-14 15:59:49 +00:00
Adrian Prantl 96f6963708 Add another ArchSpec unit test.
llvm-svn: 348502
2018-12-06 17:11:45 +00:00
Adrian Prantl 7b8a03751c Add a unit test for ArchSpec matching to document how it behaves (and test it).
llvm-svn: 348440
2018-12-06 00:43:55 +00:00
Jonas Devlieghere 15eacd741f [Reproducers] Change how reproducers are initialized.
This patch changes the way the reproducer is initialized. Rather than
making changes at run time we now do everything at initialization time.
To make this happen we had to introduce initializer options and their SB
variant. This allows us to tell the initializer that we're running in
reproducer capture/replay mode.

Because of this change we also had to alter our testing strategy. We
cannot reinitialize LLDB when using the dotest infrastructure. Instead
we use lit and invoke two instances of the driver.

Another consequence is that we can no longer enable capture or replay
through commands. This was bound to go away form the beginning, but I
had something in mind where you could enable/disable specific providers.
However this seems like it adds very little value right now so the
corresponding commands were removed.

Finally this change also means you now have to control this through the
driver, for which I replaced --reproducer with --capture and --replay to
differentiate between the two modes.

Differential revision: https://reviews.llvm.org/D55038

llvm-svn: 348152
2018-12-03 17:28:29 +00:00
Jonas Devlieghere 68ed93d252 [Reproducers] Improve reproducer API and add unit tests.
When I landed the initial reproducer framework I knew there were some
things that needed improvement. Rather than bundling it with a patch
that adds more functionality I split it off into this patch. I also
think the API is stable enough to add unit testing, which is included in
this patch as well.

Other improvements include:

 - Refactor how we initialize the loader and generator.
 - Improve naming consistency: capture and replay seems the least ambiguous.
 - Index providers by name and make sure there's only one of each.
 - Add convenience methods for creating and accessing providers.

Differential revision: https://reviews.llvm.org/D54616

llvm-svn: 347716
2018-11-27 22:11:02 +00:00
Pavel Labath 07e93c57a3 Move DataExtractorTest to unittests/Utility
The DataExtractor class itself was moved to Utility some time ago, but
it seems this was not reflected in the location of the test code. Fix
that.

llvm-svn: 346867
2018-11-14 14:58:36 +00:00
Jonas Devlieghere 72787ac661 Revert "[FileSystem] Make use of FS in TildeExpressionResolver"
The whole point of this change was making it possible to resolve paths
without depending on the FileSystem, which is not what I did here. Not
sure what I was thinking...

llvm-svn: 346466
2018-11-09 01:59:28 +00:00
Jonas Devlieghere 9560f353ed [FileSystem] Make use of FS in TildeExpressionResolver
In order to call real_path from the TildeExpressionResolver we need
access to the FileSystem. Since the resolver lives under utility we have
to pass in the FS.

llvm-svn: 346457
2018-11-09 00:50:50 +00:00
Jonas Devlieghere 8f3be7a32b [FileSystem] Move path resolution logic out of FileSpec
This patch removes the logic for resolving paths out of FileSpec and
updates call sites to rely on the FileSystem class instead.

Differential revision: https://reviews.llvm.org/D53915

llvm-svn: 345890
2018-11-01 21:05:36 +00:00
Aaron Smith c3d447fe26 [lldb] Add support in Status::AsCString to retrieve win32 system error strings
Reviewers: rnk, zturner, aleksandr.urakov

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D53092

llvm-svn: 344798
2018-10-19 18:58:24 +00:00
Raphael Isemann 7f88829cea Add support for descriptions with command completions.
Summary:
This patch adds a framework for adding descriptions to the command completions we provide.
It also adds descriptions for completed top-level commands so that we can test this code.

Completions are in general supposed to be displayed alongside the completion itself. The descriptions
can be used to provide additional information about the completion to the user. Examples for descriptions
are function signatures when completing function calls in the expression command or the binary name
when providing completion for a symbol.

There is still some boilerplate code from the old completion API left in LLDB (mostly because the respective
APIs are reused for non-completion related purposes, so the CompletionRequest doesn't make sense to be
used), so that's why I still had to change some function signatures. Also, as the old API only passes around a
list of matches, and the descriptions are for these functions just another list, I had to add some code that
essentially just ensures that both lists are always the same side (e.g. all the manual calls to
`descriptions->AddString(X)` below a `matches->AddString(Y)` call).

The initial command descriptions that come with this patch are just reusing the existing
short help that is already added in LLDB.

An example completion with descriptions looks like this:
```
(lldb) pl
Available completions:
        platform -- Commands to manage and create platforms.
        plugin   -- Commands for managing LLDB plugins.
```

Reviewers: #lldb, jingham

Reviewed By: #lldb, jingham

Subscribers: jingham, JDevlieghere, lldb-commits

Differential Revision: https://reviews.llvm.org/D51175

llvm-svn: 342181
2018-09-13 21:26:00 +00:00
Shafik Yaghmour e2da5c525e Remove undefined behavior around the use of StateType
rdar://problem/43530233

Patch by Shafik Yaghmour.

Differential Revision: https://reviews.llvm.org/D51445

llvm-svn: 341957
2018-09-11 16:08:05 +00:00
Raphael Isemann 7fae4932ad Move Predicate.h from Host to Utility
Summary:
This class was initially in Host because its implementation used to be
very OS-specific. However, with C++11, it has become a very simple
std::condition_variable wrapper, with no host-specific code.

It is also a general purpose utility class, so it makes sense for it to
live in a place where it can be used by everyone.

This has no effect on the layering right now, but it enables me to later
move the Listener+Broadcaster+Event combo to a lower layer, which is
important, as these are used in a lot of places (notably for launching a
process in Host code).

Reviewers: jingham, zturner, teemperor

Reviewed By: zturner

Subscribers: xiaobai, mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D50384

llvm-svn: 341089
2018-08-30 17:51:10 +00:00
Raphael Isemann b238b8dcf0 Add unit test for StringLexer
Reviewers: labath, #lldb

Reviewed By: labath

Subscribers: jloser, mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D50298

llvm-svn: 340448
2018-08-22 20:22:34 +00:00
Stefan Granitz 2397a2b6e2 Fix: ConstString::GetConstCStringAndSetMangledCounterPart() should update the value if the key exists already
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
2018-08-14 11:07:18 +00:00
Stefan Granitz 4c01eccba2 Add ConstString test FromMidOfBufferStringRef
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
2018-08-08 21:57:42 +00:00
Pavel Labath 87730a6391 Move ScalarTest to follow the class being tested
This should have been a part of r339127, but I missed it somehow.

llvm-svn: 339136
2018-08-07 13:10:16 +00:00
Pavel Labath d821c997aa Move RegisterValue,Scalar,State from Core to Utility
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
2018-08-07 11:07:21 +00:00
Stefan Granitz 4af5a83a48 Add ConstString::IsNull() to tell between null vs. empty strings and fix usage in Mangled::GetDemangledName()
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
2018-08-06 14:15:17 +00:00
Pavel Labath 19a357adf8 Change ConstString::SetCStringWithMangledCounterpart to use StringRef
This should simplify the upcoming demangling patch (D50071). While I was
in there, I also added a quick test for the function.

llvm-svn: 338995
2018-08-06 08:27:59 +00:00
Raphael Isemann 13ff0d8d58 Added unit test for StringList
Reviewers: labath

Reviewed By: labath

Subscribers: mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D50293

llvm-svn: 338961
2018-08-04 17:28:21 +00:00
Raphael Isemann 92b16738a1 Add byte counting mechanism to LLDB's Stream class.
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
2018-08-02 16:38:34 +00:00
Raphael Isemann f4590de992 Fix out-of-bounds read in Stream::PutCStringAsRawHex8
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
2018-08-01 21:07:18 +00:00
Raphael Isemann 0bb8d83c89 Don't ignore byte_order in Stream::PutMaxHex64
Reviewers: labath

Reviewed By: labath

Subscribers: zturner, lldb-commits

Differential Revision: https://reviews.llvm.org/D50025

llvm-svn: 338591
2018-08-01 17:12:58 +00:00
Raphael Isemann 7dd0aba0b2 Removed failing StreamTest case
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
2018-08-01 06:35:27 +00:00
Raphael Isemann b1dfad0601 Added initial unit test for LLDB's Stream class.
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
2018-08-01 06:04:48 +00:00
Raphael Isemann 1a6d7ab55d Narrow the CompletionRequest API to being append-only.
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
2018-07-27 18:42:46 +00:00
Raphael Isemann bae7367cd9 Add unit tests for VMRange
Subscribers: clayborg, labath, mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D49415

llvm-svn: 337873
2018-07-24 23:52:39 +00:00
Raphael Isemann 6188a42592 Added unit test for StreamTee
Reviewers: davide

Reviewed By: davide

Subscribers: davide, mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D49708

llvm-svn: 337778
2018-07-24 00:01:32 +00:00
Raphael Isemann 3914833099 Added unit tests for Flags
Reviewers: labath

Reviewed By: labath

Subscribers: labath, mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D49435

llvm-svn: 337475
2018-07-19 17:45:51 +00:00
Raphael Isemann 2d42579886 Fix some crashes and deadlocks in FormatAnsiTerminalCodes
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
2018-07-16 16:38:30 +00:00
Raphael Isemann a2e76c0bfc Replaced more boilerplate code with CompletionRequest (NFC)
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
2018-07-13 18:28:14 +00:00
Raphael Isemann 3a0e12700b Refactor parsing of option lists with a raw string suffix.
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
2018-07-10 20:17:38 +00:00
Raphael Isemann 2443bbd4aa Refactoring for for the internal command line completion API (NFC)
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
2018-07-02 21:29:56 +00:00