This reverts commit d657519838 as it
breaks two dozen tests. The breakages are related to variable path
expression parsing and summary string parsing (possibly the same code).
As I worked through changes to another PR
(https://github.com/llvm/llvm-project/pull/74912), I couldn't help but
rewrite a few methods for readability, maintainability, and possibly
some behavior correctness too.
1. Exiting early instead of nested `if`-statements, which:
- Reduces indentation levels for all subsequent lines
- Treats missing pre-conditions similar to an error
- Clearly indicates that the full length of the method is the "happy
path".
2. Explicitly return empty Value Object shared pointers for those error
(like) situations, which
- Reduces the time it takes a maintainer to figure out what the method
actually returns based on those conditions.
3. Converting a mix of `if` and `if`-`else`-statements around an enum
into one `switch` statement, which:
- Consolidates the former branching logic
- Lets the compiler warn you of a (future) missing enum case
- This one may actually change behavior slightly, because what was an
early test for one enum case, now happens later on in the `switch`.
4. Consolidating near-identical, "copy-pasta" logic into one place,
which:
- Separates the common code to the diverging paths.
- Highlights the differences between the code paths.
rdar://119833526
I've been working on more/better configuration for improving DEBUGINFOD
support. This is the first (and easiest) slice of the work.
I've added `timeout` and `cache-path` settings that can override the
DEBUGINFOD library defaults (and environment variables.) I also renamed
the `plugin.symbol-locator.debuginfod.server_urls` setting to
`server-urls` to be more consistent with the rest of LLDB's settings
(the underscore switch is switched to a hyphen)
I've got a few tests that validate the cache-path setting (as a
side-effect), but they've exposed a few bugs that I'll be putting up a
separate PR for (which will include the tests).
---------
Co-authored-by: Kevin Frei <freik@meta.com>
BroadcastEvent currently takes its EventData* param and shoves it into
an Event object, which takes ownership of the pointer and places it into
a shared_ptr to manage the lifetime.
Instead of relying on `new` and passing raw pointers around, I think it
would make more sense to create the shared_ptr up front.
Follow-up to #69422.
This PR puts all the highlighting settings into a single struct for
easier handling
Co-authored-by: Talha Tahir <talha.tahir@10xengineers.ai>
Previously committed as 9e08e51a20, and
reverted because a dependency commit was reverted, then committed again
as 4b574008ae and reverted again because
"dependency commit" 5a391d38ac was
reverted. But it doesn't seem that 5a391d38ac was a real dependency
for this.
This commit incorporates 4b574008ae and
18e093faf7 by Richard Smith (@zygoloid),
with some minor fixes, most notably:
- `UncommonValue` renamed to `StructuralValue`
- `VK_PRValue` instead of `VK_RValue` as default kind in lvalue and
member pointer handling branch in
`BuildExpressionFromNonTypeTemplateArgumentValue`;
- handling of `StructuralValue` in `IsTypeDeclaredInsideVisitor`;
- filling in `SugaredConverted` along with `CanonicalConverted`
parameter in `Sema::CheckTemplateArgument`;
- minor cleanup in
`TemplateInstantiator::transformNonTypeTemplateParmRef`;
- `TemplateArgument` constructors refactored;
- `ODRHash` calculation for `UncommonValue`;
- USR generation for `UncommonValue`;
- more correct MS compatibility mangling algorithm (tested on MSVC ver.
19.35; toolset ver. 143);
- IR emitting fixed on using a subobject as a template argument when the
corresponding template parameter is used in an lvalue context;
- `noundef` attribute and opaque pointers in `template-arguments` test;
- analysis for C++17 mode is turned off for templates in
`warn-bool-conversion` test; in C++17 and C++20 mode, array reference
used as a template argument of pointer type produces template argument
of UncommonValue type, and
`BuildExpressionFromNonTypeTemplateArgumentValue` makes
`OpaqueValueExpr` for it, and `DiagnoseAlwaysNonNullPointer` cannot see
through it; despite of "These cases should not warn" comment, I'm not
sure about correct behavior; I'd expect a suggestion to replace `if` by
`if constexpr`;
- `temp.arg.nontype/p1.cpp` and `dr18xx.cpp` tests fixed.
PlatformDarwinKernel::GetSharedModule, which can find a kernel or kext
from a local filesystem scan, needed a little cleanup. The method which
finds kernels was (1) not looking for the SymbolFileSpec when creating a
Module, and (2) adding that newly created Module to a Target, which
GetSharedModule should not be doing - after auditing many other subclass
implementations of this method, I haven't found any others doing it.
Platform::GetSharedModule didn't have a headerdoc so it took a little
work to piece together the intended behaviors.
This is addressing a bug where
PlatformDarwinKernel::GetSharedModuleKernel would find the ObjectFile
for a kernel, create a Module, and add it to the Target. Then up in
DynamicLoaderDarwinKernel, it would check if the Module had a SymbolFile
FileSpec, and because it did not, it would do its own search for a
binary & dSYM, find them, and then add that to the Target. Now we have
two copies of the Module in the Target, one with a dSYM and the other
without, and only one of them has its load addresses set.
GetSharedModule should not be adding binaries to the Target, and it
should set the SymbolFile FileSpec when it is creating the Module.
rdar://120895951
The lifetime of these BreakpointEventData objects is difficult to reason
about. These BreakpointEventData pointers are created and passed along
to `Event` which takes the raw pointer and sticks them in a shared
pointer. Instead of manually managing the lifetime and memory, it would
be simpler to have them be shared pointers from the start.
For example, the following message has the severity string "error: "
twice.
> "error: <EXPR>:3:1: error: cannot find 'bogus' in scope
This method already appends the severity string in the beginning, but
with this fix, it also removes a secondary instance, if applicable.
Note that this change only removes the *first* redundant substring. I
considered putting the removal logic in a loop, but I decided that if
something is generating more than one redundant severity substring, then
that's a problem the message's source should probably fix.
rdar://114203423
Fixes:
```
[3465/3822] Building CXX object tools\lldb\source\Plugins\SymbolFile\CTF\CMakeFiles\lldbPluginSymbolFileCTF.dir\SymbolFileCTF.cpp.obj
C:\git\llvm-project\lldb\source\Plugins\SymbolFile\CTF\SymbolFileCTF.cpp(606) : warning C4715: 'lldb_private::SymbolFileCTF::CreateType': not all control paths return a value
```
This fixes missing inlined function names when formatting frame and the
`Block` in `SymbolContext` is a lexical block (e.g.
`DW_TAG_lexical_block` in Dwarf).
The TLS implementation on apple platforms has changed. Instead of
invoking pthread_getspecific with a pthread_key_t, we instead perform a
virtual function call.
Note: Some versions of Apple's new linker do not emit debug symbols for
TLS symbols. This causes the TLS tests to fail because LLDB and dsymutil
expects there to be debug symbols to resolve the relevant TLS block. You
may work around this by switching to the older linker (ld-classic) or by
disabling the TLS tests until you have a newer version of the new
linker.
rdar://120676969
This fixes:
```
[6083/7449] Building CXX object tools\lldb\source\Commands\CMakeFiles\lldbCommands.dir\CommandObjectFrame.cpp.obj
C:\git\llvm-project\lldb\source\Commands\CommandObjectFrame.cpp(497) : warning C4715: 'CommandObjectFrameVariable::ScopeRequested': not all control paths return a value
```
In https://reviews.llvm.org/D151292 I added the ability to track address
masks separately for high and low memory addresses, a capability of
AArch64. I did my testing with manual address mask settings (via
target.process.highmem-virtual-addressable-bits) but didn't have a real
corefile that included this metadata and required it.
My intention is that when the high address mask isn't specified, by the
user (via the setting) or the Process plugin, we fall back to using the
low address mask. The low and high address mask is the same for almost
all environments.
But the patch I wrote never uses the Process plugin high address mask if
it was set, e.g. from corefile metadata. This patch corrects that.
I also have an old patch in Phabractor that was approved to add
FixAddress methods to SBProcess; I need to pick that patch up and finish
it (I wanted to add an enum to specify which mask is being requested
iirc), so I can do address masks tests in API tests.
rdar://120926000
I have my editor configured to remove trailing whitespace and every time
I touch this file I end up with a bunch of clang-format changes to lines
that were modified because of it. Nobody likes trailing whitespace so
this cleans up the file.
Store a SupportFile, rather than a FileSpec, in CompileUnit. This commit
works towards having the SourceManager operate on SupportFiles so that
it can (1) validate the Checksum and (2) materialize the content of
inline source information.
Store a SupportFile, rather than a FileSpec, in LineEntry. This commit
works towards having the SourceManageroperate on SupportFiles so that it
can (1) validate the Checksum and (2) materialize the content of inline
source information.
We claim in a couple places that the zeroth element of the module list
for a target is the main executable, but we don't actually enforce that
in the ModuleList class. As we saw, for instance, in
32dd5b2097
it's not all that hard to get this to be off. This patch ensures that
the first object file of type Executable added to it is moved to the
front of the ModuleList. I also added a test for this.
In the normal course of operation, where the executable is added first,
this only adds a check for whether the first element in the module list
is an executable. If that's true, we just append as normal.
Note, the code in Target::GetExecutableModule doesn't actually agree
that the zeroth element must be the executable, it instead returns the
first Module of type Executable. But I can't tell whether that was a
change in intention or just working around the bug that we don't always
maintain this ordering. But given we've said this in scripting as well
as internally, I think we shouldn't change our minds about this.
With lldb build fix.
Original message:
EnumConstantDecl is allocated by the ASTContext allocator so the
destructor is never called.
This patch takes a similar approach to IntegerLiteral by using
APIntStorage to allocate large APSInts using the ASTContext allocator as
well.
The downside is that an additional heap allocation and copy of the data
needs to be made when calling getInitValue if the APSInt is large.
Fixes#78160.
Per this RFC:
https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717
on improving progress reports, this commit separates the title field and
details field so that the title specifies the category that the progress
report falls under. The details field is added as a part of the
constructor for progress reports and by default is an empty string. In addition, changes the total amount of progress completed into a std::optional. Also
updates the test to check for details being correctly reported from the
event structured data dictionary.
We only ever call this function once, without relying on the defaulted
`honor_array` parameter, so make it non-defaulted. Also `max_length` is
always set to `0`, so remove it entirely.
This simplifies some upcoming refactoring.
This is a followup of #76983 and adds the libc++ data formatters for
- weekday,
- weekday_indexed,
- weekday_last,
- month_weekday,
- month_weekday_last,
- year_month,
- year_month_day_last
- year_month_weekday, and
- year_month_weekday_last.
BreakpointIDList does not need to know about CommandReturnObject.
BreakpointIDList::FindAndReplaceIDRanges is the last place that uses it
in BreakpointIDList.
Instead of passing in a CommandReturnObject, it now returns an
llvm::Error. The callsite uses the Error to populate the
CommandReturnObject as needed.
When I added the MD5 checksum I was on the fence between storing it in
FileSpec or creating a new SupportFile abstraction. The latter was
deemed overkill for just the MD5 hashes, but support for inline sources
in the DWARF 5 line table tipped the scales. This patch moves the MD5
checksum into the new SupportFile class.
There seems to be a regression since
6f8b33f6df.
`Max String Summary Length` target property is not read properly and the
default value (1024) is being used instead.
16.0.6:
```
(lldb) settings set target.max-string-summary-length 16
(lldb) var
(std::string) longStdString = "0123456789101112131415161718192021222324252627282930313233343536"
(const char *) longCharPointer = 0x000055555556f310 "0123456789101112131415161718192021222324252627282930313233343536"
```
17.0.4:
```
(lldb) settings set target.max-string-summary-length 16
(lldb) var
(std::string) longStdString = "0123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221222223224225226227228229230231232233234235236237238239240241242243244245246247248249250251252253254255256257258259260261262263264265266267268269270271272273274275276277278279280281282283284285286287288289290291292293294295296297298299300301302303304305306307308309310311312313314315316317318319320321322323324325326327328329330331332333334335336337338339340341342343344345346347348349350351352353354355356357358359360361362363364365366367368369370371372373374375376377"...
(const char *) longCharPointer = 0x000055555556f310 "*same as line above*"...
```
Comparison fails here:
9cb1673fa5/lldb/source/Interpreter/OptionValue.cpp (L256)
Due to the type difference:
9cb1673fa5/lldb/source/Target/Target.cpp (L4611)9cb1673fa5/lldb/source/Target/TargetProperties.td (L98)
This change just adds a `bool colors` parameter to the `StreamString`
class's constructor, which it passes up to its superclass’s constructor.
I'm working on another patch that prints out error messages using a
`StreamString` but I wasn't getting colorized text because of this
missing implementation detail.
rdar://120671168
The "kern ver str" LC_NOTE gives lldb a kernel version string -- with a
UUID and/or a load address (stext) to load it at. The LC_NOTE specifies
a size of the identifier string in bytes. In
ObjectFileMachO::GetIdentifierString, I copy that number of bytes into a
std::string, and in case there were additional nul characters at the end
of the sting for padding reasons, I tried to shrink the std::string to
not include these extra nul's.
However, I did this resizing without handling the case of an empty
identifier string. I don't know why any corefile creator would do that,
but of course at least one does. This patch removes the resizing
altogether; I was solving something that hasn't ever shown to be a
problem. I also added a test case for this, to check that lldb doesn't
crash when given one of these corefiles.
rdar://120390199
This patch fixes:
lldb/source/Target/ProcessTrace.cpp:23:33: error: extra ';' outside
of a function is incompatible with C++98
[-Werror,-Wc++98-compat-extra-semi]
…ntext
Following the specification chain seems to be clearly the expected
behavior of GetDeclContext(). Otherwise C++ methods have an empty
CompilerContext instead of being nested in their struct/class.
Theprimary motivation for this functionality is the Swift plugin. In
order to test the change I added a proof-of-concept implementation of a
Module::FindFunction() variant that takes a CompilerContext, expesed via
lldb-test.
rdar://120553412
This adds a subset of the C++20 calendar data formatters:
- day,
- month,
- year,
- month_day,
- month_day_last, and
- year_month_day.
A followup patch will add the missing calendar data formatters:
- weekday,
- weekday_indexed,
- weekday_last,
- month_weekday,
- month_weekday_last,
- year_month,
- year_month_day_last
- year_month_weekday, and
- year_month_weekday_last.
This is required for users of `TypeQuery` that limit the set of
languages of the query using APIs such as
`GetSupportedLanguagesForTypes` or
`GetSupportedLanguagesForExpressions`.
Example usage: https://github.com/apple/llvm-project/pull/7885
There are 2 motivations here:
1.) There is no need to hand out constant references to BreakpointIDs,
they are only 8 bytes big. In addition, every use of this method already
makes a copy anyway.
2.) Each BreakpointIDList held onto an invalid BreakpointID specifically
to
prevent lifetime issues. Returning a value means you can return an
invalid BreakpointID instead of needing to allocate storage for an
invalid BreakpointID.
This abstraction is leaky and BreakpointIDList does not need to know
about CommandReturnObject.
Additionally, setting the CommandReturnObject inout param to a success
state does very little. The function returns immediately if the input
ArrayRef is empty, and reading
CommandObjectMultiwordBreakpoint::VerifyIDs more closely, the input is
always empty if the previous call to
BreakpointIDList::FindAndReplaceIDRanges failed. If the call was
successful, then the CommandReturnObject is already in a success state.
I have opted to remove the function altogether and inline the
functionality where it was used.
This moves the functionally of finding a DIE based on a fully qualified
name from SymbolFileDWARF into DWARFIndex itself, so that
specializations of DWARFIndex can implement faster versions of this
query.
With DWARFv5, C++ static data members are represented as
`DW_TAG_variable`s (see `faa3a5ea9ae481da757dab1c95c589e2d5645982`).
In GetClangDeclForDIE, when trying to parse the `DW_AT_specification`
that a static data member's CU-level `DW_TAG_variable` points to, we
would try to `CreateVariableDeclaration`. Whereas previously it was a
no-op (for `DW_TAG_member`s). However, adding `VarDecls` to RecordDecls
for static data members should always be done in
`CreateStaticMemberVariable`. The test-case is an exapmle where we would
crash if we tried to create a `VarDecl` from within `GetClangDeclForDIE`
for a static data member.
This patch simply checks whether the `DW_TAG_variable` being parsed is a
static data member, and if so, trivially returns from
`GetClangDeclForDIE` (as we previously did for `DW_TAG_member`s).
