Revert "[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute." and follow-ups

This reverts commit 9429b67b8e300e638d7828bbcb95585f85c4df4d.

It broke

Revert "[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute." and follow-ups

This reverts commit 9429b67b8e300e638d7828bbcb95585f85c4df4d.

It broke the build on Windows, see comments on https://reviews.llvm.org/D130309

It also reverts these follow-ups:

Revert "Fix buildbot breakage after https://reviews.llvm.org/D130309."
This reverts commit f959d815f4637890ebbacca379f1c38ab47e4e14.

Revert "Fix buildbot breakage after https://reviews.llvm.org/D130309."
This reverts commit 0bbce7a4c2d2bff622bdadd4323f93f5d90e6d24.

Revert "Cache the value for absolute path in FileSpec."
This reverts commit dabe877248b85b34878e75d5510339325ee087d0.

show more ...

[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.

The FileSpect APIs allow users to modify instance variables directly by getting a non const ref

[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.

The FileSpect APIs allow users to modify instance variables directly by getting a non const reference to the directory and filename instance variables. This makes it impossibly to control all of the times the FileSpec object is modified so we can clear the cache. This patch modifies the APIs of FileSpec so no one can modify the directory or filename directly by adding set accessors and by removing the get accessors that are non const.

Many clients were using FileSpec::GetCString(...) which returned a unique C string from a ConstString'ified version of the result of GetPath() which returned a std::string. This caused many locations to use this convenient function incorrectly and could cause many strings to be added to the constant string pool that didn't need to. Most clients were converted to using FileSpec::GetPath().c_str() when possible. Other clients were modified to use the newly renamed version of this function which returns an actualy ConstString:
ConstString FileSpec::GetPathAsConstString(bool denormalize = true) const;

This avoids the issue where people were getting an already uniqued "const char *" that came from a ConstString only to put the "const char *" back into a "ConstString" object. By returning the ConstString instead of a "const char *" clients can be more efficient with the result.

The patch:
- Removes the non const GetDirectory() and GetFilename() get accessors
- Adds set accessors to replace the above functions: SetDirectory() and SetFilename().
- Adds ClearDirectory() and ClearFilename() to replace usage of the FileSpec::GetDirectory().Clear()/FileSpec::GetFilename().Clear() call sites
- Fixed all incorrect usage of FileSpec::GetCString() to use FileSpec::GetPath().c_str() where appropriate, and updated other call sites that wanted a ConstString to use the newly returned ConstString appropriately and efficiently.

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

show more ...

Use llvm::is_contained (NFC)

[lldb] Resolve exe location for `target create`

This fixes an issue that, when you start lldb or use `target create`
with a program name which is on $PATH, or not specify the .exe suffix of
a progra

[lldb] Resolve exe location for `target create`

This fixes an issue that, when you start lldb or use `target create`
with a program name which is on $PATH, or not specify the .exe suffix of
a program in the working directory on Windows, you get a confusing
error, for example:

(lldb) target create notepad
error: 'C:\WINDOWS\SYSTEM32\notepad.exe' doesn't contain any 'host'
platform architectures: i686, x86_64, i386, i386

Fixes https://github.com/mstorsjo/llvm-mingw/issues/265

Reviewed By: DavidSpickett

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

show more ...

[lldb] Remove the global platform list

This patch moves the platform creation and selection logic into the
per-debugger platform lists. I've tried to keep functional changes to a
minimum -- the main

[lldb] Remove the global platform list

This patch moves the platform creation and selection logic into the
per-debugger platform lists. I've tried to keep functional changes to a
minimum -- the main (only) observable difference in this change is that
APIs, which select a platform by name (e.g.,
Debugger::SetCurrentPlatform) will not automatically pick up a platform
associated with another debugger (or no debugger at all).

I've also added several tests for this functionality -- one of the
pleasant consequences of the debugger isolation is that it is now
possible to test the platform selection and creation logic.

This is a product of the discussion at
<https://discourse.llvm.org/t/multiple-platforms-with-the-same-name/59594>.

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

show more ...

[lldb] Change the way we pick a platform for fat binaries

Currently, when creating a target for a fat binary, we error out if more
than one platforms can support the different architectures in the
b

[lldb] Change the way we pick a platform for fat binaries

Currently, when creating a target for a fat binary, we error out if more
than one platforms can support the different architectures in the
binary. There are situations where it makes sense for multiple platforms
to support the same architectures: for example the host and
remote-macosx platform on Darwin.

The only way to currently disambiguate between them is to specify the
architecture. This patch changes that to take into account the selected
and host platform. The new algorithm works a follows:

1. Pick the selected platform if it matches any of the architectures.
2. Pick the host platform if it matches any of the architectures.
3. If there's one platform that works for all architectures, pick that.

If none of the above apply then we either have no platform supporting
the architectures in the fat binary or multiple platforms with no good
way to disambiguate between them.

I've added a bunch of unit tests to codify this new behavior.

rdar://90360204

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

show more ...

[lldb] Plumb process host architecture through platform selection

To allow us to select a different platform based on where the process is
running, plumb the process host architecture through platfo

[lldb] Plumb process host architecture through platform selection

To allow us to select a different platform based on where the process is
running, plumb the process host architecture through platform selection.

This patch is in preparation for D121444 which needs this functionality
to tell apart iOS binaries running on Apple Silicon vs on a remote iOS
device.

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

show more ...

Revert "[lldb] Remove the global platform list"

It makes module dependencies loopier.

This reverts commits 49cffe3c7fab74252d4b6a073303c803dc1659f0 and
ffb9429b6f3c29ab4687b96fd85374924c98ad16.

[lldb] Remove the global platform list

This patch moves the platform creation and selection logic into the
per-debugger platform lists. I've tried to keep functional changes to a
minimum -- the main

[lldb] Remove the global platform list

This patch moves the platform creation and selection logic into the
per-debugger platform lists. I've tried to keep functional changes to a
minimum -- the main (only) observable difference in this change is that
APIs, which select a platform by name (e.g.,
Debugger::SetCurrentPlatform) will not automatically pick up a platform
associated with another debugger (or no debugger at all).

I've also added several tests for this functionality -- one of the
pleasant consequences of the debugger isolation is that it is now
possible to test the platform selection and creation logic.

This is a product of the discussion at
<https://discourse.llvm.org/t/multiple-platforms-with-the-same-name/59594>.

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

show more ...

[lldb/Platform] Prepare decouple instance and plugin names

This patch changes the return value of Platform::GetName() to a
StringRef, and uses the opportunity (compile errors) to change some
callsit

[lldb/Platform] Prepare decouple instance and plugin names

This patch changes the return value of Platform::GetName() to a
StringRef, and uses the opportunity (compile errors) to change some
callsites to use GetPluginName() instead. The two methods still remain
hardwired to return the same thing, but this will change once the ideas
in
<https://discourse.llvm.org/t/multiple-platforms-with-the-same-name/59594>
are implemented.

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

show more ...

[lldb] Add missing mutex guards to TargetList::CreateTarget

TestMultipleTargets is randomly failing on the bots. The reason for that is that
the test is calling `SBDebugger::CreateTarget` from multi

[lldb] Add missing mutex guards to TargetList::CreateTarget

TestMultipleTargets is randomly failing on the bots. The reason for that is that
the test is calling `SBDebugger::CreateTarget` from multiple threads.
`TargetList::CreateTarget` is curiously missing the guard that all of its other
member functions have, so all the threads in the test end up changing the
internal TargetList state at the same time and end up corrupting it.

Reviewed By: vsk, JDevlieghere

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

show more ...

[lldb] Abstract scoped timer logic behind LLDB_SCOPED_TIMER (NFC)

This patch introduces a LLDB_SCOPED_TIMER macro to hide the needlessly
repetitive creation of scoped timers in LLDB. It's similar to

[lldb] Abstract scoped timer logic behind LLDB_SCOPED_TIMER (NFC)

This patch introduces a LLDB_SCOPED_TIMER macro to hide the needlessly
repetitive creation of scoped timers in LLDB. It's similar to the
LLDB_LOG(F) macro.

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

show more ...

[lldb] Modernize TargetList for-loops, NFC

Replace loops with standard algorithms or range-based loops.

[lldb] "target create" shouldn't save target if the command failed

TargetList::CreateTarget automatically adds created target to the list, however,
CommandObjectTargetCreate does some additional pre

[lldb] "target create" shouldn't save target if the command failed

TargetList::CreateTarget automatically adds created target to the list, however,
CommandObjectTargetCreate does some additional preparation after creating a target
and which can fail. The command should remove created target if it failed. Since
the function has many ways to return, scope guard does this work safely.

Changes to the TargetList make target adding and selection more transparent.

Other changes remove unnecessary SetSelectedTarget after CreateTarget.

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

show more ...

[TargetList] Delete the destructor

AFAICT, ~TargetList simply implements the default destructor, plus some
locking.

The history is murky, so I'm not sure why we do this locking. Perhaps,
at some po

[TargetList] Delete the destructor

AFAICT, ~TargetList simply implements the default destructor, plus some
locking.

The history is murky, so I'm not sure why we do this locking. Perhaps,
at some point, it was possible to delete the same TargetList instance
from two different threads, setting up a race. If that were true, then
the locking would protect against the race.

Since TargetList is uniquely owned by Debugger (m_target_list), no such
race is possible today.

Testing: check-lldb

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

show more ...

[TargetList] Simplify dummy target creation

Factor out dummy target creation from CreateTargetInternal.

This makes it impossible for dummy target creation to accidentally fail
due to too-strict che

[TargetList] Simplify dummy target creation

Factor out dummy target creation from CreateTargetInternal.

This makes it impossible for dummy target creation to accidentally fail
due to too-strict checking in one of the CreateTargetInternal overloads.

Testing: check-lldb

rdar://70630655

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

show more ...

Unify the code that updates the ArchSpec after finding a fat binary
with how it is done for a lean binary

In particular this affects how target create --arch is handled — it
allowed us to override t

Unify the code that updates the ArchSpec after finding a fat binary
with how it is done for a lean binary

In particular this affects how target create --arch is handled — it
allowed us to override the deployment target (a useful feature for the
expression evaluator), but the fat binary case didn't.

rdar://problem/66024437

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

(cherry picked from commit 470bdd3caaab0b6e0ffed4da304244be40b78668)

show more ...

Convert to early exit (NFC)

[lldb] Improve platform handling in CreateTargetInternal

Currently, `target create` has no --platform option. However,
TargetList::CreateTargetInternal which is called under the hood, will
return an

[lldb] Improve platform handling in CreateTargetInternal

Currently, `target create` has no --platform option. However,
TargetList::CreateTargetInternal which is called under the hood, will
return an error when either no platform or multiple matching platforms
are found, saying that a platform should be specified with --platform.

This patch adds the platform option, but that doesn't solve either of
these errors.

- If more than one platform matches, specifying the platform isn't
going to fix that. The current code will only look at the
architecture instead. I've updated the error message to ask the user
to specify an architecture.

- If no architecture is found, specifying a new one via platform isn't
going to change that either because we already try to find one that
matches the given architecture.

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

show more ...

Moving executable module symbols parsing to target creation method.

Summary:
In our project we are using remote client-server LLDB configuration.
We want to parse as much debugging symbols as we can

Moving executable module symbols parsing to target creation method.

Summary:
In our project we are using remote client-server LLDB configuration.
We want to parse as much debugging symbols as we can before debugger starts attachment to the remote process.
To do that we are passing the path of the local executable module to CreateTarget method at the client.
But, it seems that this method are not parsing the executable module symbols.
To fix this I added PreloadSymbols call for executable module to target creation method.

This patch also fixes a problem where the DynamicLoader would reset a
module when launching the target. We fix it by making sure
Platform::ResolveExecutable returns the module object obtained from the
remote platform.

Reviewed By: labath

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

show more ...

[lldb][NFC] Fix all formatting errors in .cpp file headers

Summary:
A *.cpp file header in LLDB (and in LLDB) should like this:
```
//===-- TestUtilities.cpp ----------------------------------------

[lldb][NFC] Fix all formatting errors in .cpp file headers

Summary:
A *.cpp file header in LLDB (and in LLDB) should like this:
```
//===-- TestUtilities.cpp -------------------------------------------------===//
```
However in LLDB most of our source files have arbitrary changes to this format and
these changes are spreading through LLDB as folks usually just use the existing
source files as templates for their new files (most notably the unnecessary
editor language indicator `-*- C++ -*-` is spreading and in every review
someone is pointing out that this is wrong, resulting in people pointing out that this
is done in the same way in other files).

This patch removes most of these inconsistencies including the editor language indicators,
all the different missing/additional '-' characters, files that center the file name, missing
trailing `===//` (mostly caused by clang-format breaking the line).

Reviewers: aprantl, espindola, jfb, shafik, JDevlieghere

Reviewed By: JDevlieghere

Subscribers: dexonsmith, wuzish, emaste, sdardis, nemanjai, kbarton, MaskRay, atanasyan, arphaman, jfb, abidh, jsji, JDevlieghere, usaxena95, lldb-commits

Tags: #lldb

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

show more ...

[lldb] s/FileSpec::Equal/FileSpec::Match

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 o

[lldb] s/FileSpec::Equal/FileSpec::Match

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

show more ...

[lldb][NFC] Migrate to raw_ostream in ArchSpec::DumpTriple

Reviewers: labath, davide

Reviewed By: davide

Subscribers: clayborg, JDevlieghere, lldb-commits

Tags: #lldb

Differential Revision: http

[lldb][NFC] Migrate to raw_ostream in ArchSpec::DumpTriple

Reviewers: labath, davide

Reviewed By: davide

Subscribers: clayborg, JDevlieghere, lldb-commits

Tags: #lldb

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

show more ...

Lock accesses to OptionValueFileSpecList objects

Before a Debugger gets a Target, target settings are routed to a global set
of settings. Even without this, some part of the LLDB which exist indepen

Lock accesses to OptionValueFileSpecList objects

Before a Debugger gets a Target, target settings are routed to a global set
of settings. Even without this, some part of the LLDB which exist independently
of the Debugger object (the Module cache, the Symbol vendors, ...) access
directly the global default store for those settings.

Of course, if you modify one of those global settings while they are being read,
bad things happen. We see this quite a bit with FileSpecList settings. In
particular, we see many cases where one debug session changes
target.exec-search-paths while another session starts up and it crashes when
one of those accesses invalid FileSpecs.

This patch addresses the specific FileSpecList issue by adding locking to
OptionValueFileSpecList and never returning by reference.

Reviewers: clayborg

Subscribers: lldb-commits

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

llvm-svn: 359028

show more ...

[NFC] Remove ASCII lines from comments

A lot of comments in LLDB are surrounded by an ASCII line to delimit the
begging and end of the comment.

Its use is not really consistent across the code base

[NFC] Remove ASCII lines from comments

A lot of comments in LLDB are surrounded by an ASCII line to delimit the
begging and end of the comment.

Its use is not really consistent across the code base, sometimes the
lines are longer, sometimes they are shorter and sometimes they are
omitted. Furthermore, it looks kind of weird with the 80 column limit,
where the comment actually extends past the line, but not by much.
Furthermore, when /// is used for Doxygen comments, it looks
particularly odd. And when // is used, it incorrectly gives the
impression that it's actually a Doxygen comment.

I assume these lines were added to improve distinguishing between
comments and code. However, given that todays editors and IDEs do a
great job at highlighting comments, I think it's worth to drop this for
the sake of consistency. The alternative is fixing all the
inconsistencies, which would create a lot more churn.

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

llvm-svn: 358135

show more ...