[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D97739#2608510 , @friss wrote:

> In D97739#2607961 , @clayborg wrote:
>
>> In D97739#2607869 , @jingham wrote:
>>
>>> This way of doing progress is going to look odd in anything that uses 
>>> multiple debuggers.  If I'm reading the code aright, if I have two 
>>> debuggers, and a target in Debugger A starts doing something that would 
>>> cause progress traffic, both debuggers will show activity.
>>
>> that is true, but it is a global module repository that benefits both 
>> debuggers. And I very rarely debug two things at the same time, so most of 
>> the time for most people this will be beneficial and shouldn't cause too 
>> much confusion.
>
> Just one tidbit here. Most users are actually routinely running tens of 
> debuggers at the same time, because tests run in parallel and they have a 
> debugger attached by default. Now if you have a long running operation kick 
> in in your unit tests, you might already have a different kind of issue, but 
> I’d like to avoid a world where the IDE displays spurious and wrong 
> information because of this.

But you wouldn't actually hookup any progress callbacks on any of these 
debuggers right? You don't make a window for each test, IIRC you are only 
running it under the debugger so you can report issues by using the debugger. 
Am I remember this correctly? What happens when a test crashes? If you are 
running 100 tests in parallel and 10 of them crash, do you open a window for 
each one that does crash? Or do you manually have to debug the test in order to 
stop at breakpoints or at a crash?

If we truly need debugger specific task progress, that is a lot more work and 
we have no great solution. One idea is we could end up having progress items 
take a SymbolContextScope pointer as an optional parameter that would allow us 
to grab the module pointer from it and then ask the debugger if any targets 
contain this module prior to reporting progress. This would be a bit expensive 
code for a lot of quick progress updates as we would need to iterate over each 
target and each target's module list and we would still have many system 
libraries reporting progress to all debuggers when targets contain the same 
system libraries.

The threading overhead and expense of delivering SBEvents also seems like 
overkill as threading itself and waiting using a mutex + condition will slow 
down progress delivery especially if we have a progress that does a bunch of 
updates. And once we receive the event we will need to make a static function 
call to extract all progress variables (total, completed, progress_id, baton, 
etc).

In D97739#2608510 , @friss wrote:

> In D97739#2607961 , @clayborg wrote:
>
>> In D97739#2607869 , @jingham wrote:
>>
>>> This way of doing progress is going to look odd in anything that uses 
>>> multiple debuggers.  If I'm reading the code aright, if I have two 
>>> debuggers, and a target in Debugger A starts doing something that would 
>>> cause progress traffic, both debuggers will show activity.
>>
>> that is true, but it is a global module repository that benefits both 
>> debuggers. And I very rarely debug two things at the same time, so most of 
>> the time for most people this will be beneficial and shouldn't cause too 
>> much confusion.
>
> Just one tidbit here. Most users are actually routinely running tens of 
> debuggers at the same time, because tests run in parallel and they have a 
> debugger attached by default. Now if you have a long running operation kick 
> in in your unit tests, you might already have a different kind of issue, but 
> I’d like to avoid a world where the IDE displays spurious and wrong 
> information because of this.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97739/new/

https://reviews.llvm.org/D97739

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 328917.
nealsid added a comment.
Herald added a subscriber: JDevlieghere.

Reuploading patch after I incorrectly set project repo.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98153/new/

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,172 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionAllParameters) {
+  Definition d("foo", "bar", FormatEntity::Entry::Type::Invalid, 0, 0, nullptr,
+   false);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "bar");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "string");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 33UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  ASSERT_EQ(parent.name, "parent");
+  ASSERT_EQ(parent.string, nullptr);
+  ASSERT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.num_children, 1UL);
+  ASSERT_EQ(parent.children, &d);
+  ASSERT_FALSE(parent.keep_separator);
+
+  ASSERT_EQ(parent.children[0].name, "foo");
+  ASSERT_EQ(parent.children[0].string, nullptr);
+  ASSERT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.children[0].data, 33UL);
+  ASSERT_EQ(parent.children[0].num_children, 0UL);
+  ASSERT_EQ(parent.children[0].children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[Caroline Tice via Phabricator via lldb-commits]

cmtice added a comment.

In D97786#2607275 , @labath wrote:

> In D97786#2603656 , @cmtice wrote:
>
>> I'm not sure about using target.debug-file-search-paths, and what the 
>> changes Pavel is suggesting would entail.
>
> I imagine it would involve calling Symbols::LocateExecutableSymbolFile to 
> locate the dwo file, similar to how we do that for dwp files (see 
> `SymbolFileDWARF::GetDwpSymbolFile`). (Disclaimer: I have not tried doing 
> this, so I don't know if that function would work out-of-the-box.)
> Note that I myself am not convinced that this is the right solution (that 
> function is rather heavy), but it does solve the problem of "how do we let 
> the user specify/choose the location of the dwo files" (answer: put the path 
> in the target.debug-file-search-paths setting), and it would search the cwd 
> and the module directory automatically. And the "heavy-ness" of the function 
> is moderated by the fact that it is a no-op for absolute paths to existing 
> files.
>
>> But the real question I am trying to resolve here is not "what are all the 
>> places we should be searching for the .dwo files?" but "When we're given a 
>> *relative path* in the DWARF for finding the files, what should it be 
>> relative TO?".
>
> I'm sorry, but what's the difference between those two questions? Is it about 
> the fact that the second question sort of implies that there should only be 
> one correct location where we should be searching?

I think I'm saying this poorly (I apologize).  Let me try a bit differently.  
It's a matter of defaults.  If the user builds with split debug and relative 
paths, and does not ever move the built binary:  1). it seems wrong to me that 
the debugger will sometimes find the .dwo files and sometimes will not, 
depending on where the debugger was launched from.  It ought, at the very 
least, to behave consistently no matter where the debugger was launched from.  
2).It is not possible that the compiler could ever have intended that paths to 
be relative to the location from where the debugger was launched because the 
compiler could have absolutely no idea where that would be.  So when the 
compiler generated the relative paths, whatever it was truly intended to be 
relative to, the location from which the debugger was launched could not be it. 
 3).  Given statements 1 & 2, it seems reasonable that the *default* behavior 
of the debugger is to search relative to the location of the binary.

Yes, the user could always use debugger settings to add or update search paths, 
but I don't think that should be required as part of the default behavior for 
finding the .dwo files.

>> I still think the most correct answer to that question is "relative to the 
>> location of the binary" (I know, this is assuming that the binary has not 
>> been moved since it was built, or all bets are off).  If you like, I can 
>> update this patch so that it continues to search relative to the cwd (where 
>> the debugger was launched), and IN ADDITION, will search relative to where 
>> the binary is.
>
> I don't really have a strong opinion either way -- the thing I'm mostly 
> interested in is some sort of consistency between lldb & gdb. Have you 
> already discussed this with the gdb folks? If they want to move to the 
> binary-relative mechanism, then I don't see a problem with us doing the same. 
> If they want to keep the existing behavior, then I think it would be good to 
> preserve that in lldb as well (but I don't have a problem with searching in 
> other places too; and `LocateExecutableSymbolFile` is one way to achieve 
> that).

I have submitted a patch for this to GDB; it is currently under review.  I am 
ok with waiting for this (LLDB) patch to be resolved until the GDB one is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97786/new/

https://reviews.llvm.org/D97786

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits