[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The patch description could definitely use more details about the motivation 
for the change, and a description of how it works. Apart from the fact that it 
uses the raSearch feature I introduced a while back, I don't know much about 
how it works (and given the planned changes, I am not going to try to 
understand that), but I think I can provide some details/thoughts about the 
motivation.

(IIUC), the motivating case is some code in kernel32.dll, which ends up setting 
rbp to zero. I don't know why it does that, but I suspect it may have something 
to do with the windows syscall convention (I know that ebp is used in i386 
linux, for instance).  While it would be definitely interesting to understand 
why it does that, I don't think it really matters -- one can never really 
exclude the possibility that some code (inline asm perhaps) will be messing 
with the frame pointer. So, scanning the stack for return addresses as a *last* 
last resort (the architectural default plan is already a kind of a last resort 
option) seems like it could be useful, regardless of the architecture (although 
the actual algorithm may differ somewhat for x86, which stores the return 
address on the stack directly, and say arm, which uses a link register). And 
given that the choice is between "showing nothing" and "showing something 
potentially incorrect", I don't think we need to be extra careful about vetting 
this address. All the checks Greg mentioned seem fine, but I don't see a 
compelling reason to implement them (all of them), especially given that they 
would not be useful in this case -- all we have is a minidump core file with 
some stack memory and a list of loaded modules (not necessarily the modules 
themselves).  In this case, all we can do is to check whether the pointer 
points to some known code region (which is already done IIRC).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [PATCH] D124110: Refactor protected virtual functions from SymbolFile into new SymbolFileActual class.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Yes, that's pretty much what I had in mind. Thanks.




Comment at: lldb/include/lldb/Symbol/SymbolFile.h:76
   // Constructors and Destructors
-  SymbolFile(lldb::ObjectFileSP objfile_sp)
-  : m_objfile_sp(std::move(objfile_sp)) {}
+  SymbolFile() {}
 

`= default` (or just delete altogether)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124110

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

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks, for your patience. Overall, I'd say this looks pretty clean now -- much 
cleaner than I originally thought it could be. I don't have any further 
comments here.

Does anyone else have any thoughts?




Comment at: lldb/source/Core/Module.cpp:474
+ eSymbolContextLineEntry | eSymbolContextVariable))
+  symfile->SetLoadDebugInfoEnabled();
+

Can this be moved into the if block below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D123204: [lldb] Fix initialization of LazyBool/bool variables m_overwrite/m_overwrite_lazy. NFCI.

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

Ping @jingham


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123204

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

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Bunch of random nits.

Looks like a great addition. I know debug load time is a common annoyance.




Comment at: lldb/docs/use/ondemand.rst:1
+On Demand Symbols
+=

If you can find a logical place for it, maybe define what "hydration" means in 
this context.

Developers can find the meaning from the source code but if "hydration" appears 
in logs it would be good for users to have an idea what that means.

Something like "the process of  (referred to 
internally as "hydration")".



Comment at: lldb/docs/use/ondemand.rst:92
+symbol tables. This can cause breakpoint setting by function name to fail when
+previosly it wouldn't fail.
+

"previously"



Comment at: lldb/include/lldb/Symbol/SymbolFileOnDemand.h:31
+/// symbols. Any client can on demand hydrate the underlying
+/// SymbolFile via SetForceLoad() API.
+class SymbolFileOnDemand : public lldb_private::SymbolFile {

Where is this call defined, and by API do you mean API like 
https://lldb.llvm.org/design/sbapi.html or API as in API of the SymbolFile 
class?



Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:47
+lldb::LanguageType SymbolFileOnDemand::ParseLanguage(CompileUnit &comp_unit) {
+  if (!this->m_debug_info_enabled) {
+Log *log = GetLog();

Is there a specific reason to use "this" explicitly instead of just 
`!m_debug_info_enabled`?

Something to do with the wrapping of the underlying SymbolFile perhaps.



Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:54
+LLDB_LOG(log, "Language {0} would return if hydrated.",
+ eLanguageTypeUnknown);
+}

This should be `langType` shouldn't it?



Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:110
+ __FUNCTION__);
+// Not early exit.
+return false;

What's the meaning of this comment?



Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:133
+  if (optimized) {
+LLDB_LOG(log, "Optimized would return if hydrated.");
+  }

Would return  if hydrated?



Comment at: lldb/test/Shell/SymbolFile/OnDemand/symbolic-breakpoint.test:1
+
+# Test shows that symbolic function breakpoint works with LLDB on demand 
symbol loading.

Stray newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Thanks Pavel for taking the time to review this thoroughly. I am fine with this 
as I have been working with Jeffrey on this, so anyone else please chime in if 
you have any comments!

A few follow up patches I would love to see after this goes in:

- add a command that allows users to load debug info manually using a new or 
existing command when this feature is enabled
- add settings for "never load any of these executables in Symbols On Demand" 
that takes a list of globs
- add settings for "always load the the debug info for executables in Symbols 
On Demand" that takes a list of globs
- add a new column in "image list" that shows up by default when Symbols On 
Demand is enable to show the status for each shlib like "not enabled for this", 
"debug info off" and "debug info on" (with a single character to short string, 
not the ones I just typed)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D124198#3466889 , @labath wrote:

> The patch description could definitely use more details about the motivation 
> for the change, and a description of how it works. Apart from the fact that 
> it uses the raSearch feature I introduced a while back, I don't know much 
> about how it works (and given the planned changes, I am not going to try to 
> understand that), but I think I can provide some details/thoughts about the 
> motivation.
>
> (IIUC), the motivating case is some code in kernel32.dll, which ends up 
> setting rbp to zero. I don't know why it does that, but I suspect it may have 
> something to do with the windows syscall convention (I know that ebp is used 
> in i386 linux, for instance).  While it would be definitely interesting to 
> understand why it does that, I don't think it really matters -- one can never 
> really exclude the possibility that some code (inline asm perhaps) will be 
> messing with the frame pointer. So, scanning the stack for return addresses 
> as a *last* last resort (the architectural default plan is already a kind of 
> a last resort option) seems like it could be useful, regardless of the 
> architecture (although the actual algorithm may differ somewhat for x86, 
> which stores the return address on the stack directly, and say arm, which 
> uses a link register). And given that the choice is between "showing nothing" 
> and "showing something potentially incorrect", I don't think we need to be 
> extra careful about vetting this address. All the checks Greg mentioned seem 
> fine, but I don't see a compelling reason to implement them (all of them), 
> especially given that they would not be useful in this case -- all we have is 
> a minidump core file with some stack memory and a list of loaded modules (not 
> necessarily the modules themselves).  In this case, all we can do is to check 
> whether the pointer points to some known code region (which is already done 
> IIRC).

We wrote a command in python called "sbt" and we run this when stacks are 
truncated when loading minidumps. It often gives us pretty bad results and you 
would be surprised how many things on the stack look like code pointers. So if 
we do have the ability to do the checks when we do have the code and can try to 
verify that the thing on the stack is a return address, then it would be great 
to do so to avoid the noise. And if we are in code that has no symbols and we 
don't know the start address of the function, it would be VERY helpful to 
correctly backtrace if we _can_ determine the start address of the current 
frame's function if we don't know it as then we can correctly parse the 
prologue. So adding these checks might actually get us back on track and allow 
us not to have to use the RA search. The flow would be:

- enable RA search in a frame that has no function bounds info from the object 
files or runtime info
- find the right thing on the stack that is the correct RA by finding the 
instruction before the RA and figuring out what address it calls
- use the new function address to create a real stack frame that uses opcode 
parsing

So there is real benefit to doing these checks IMHO as it can help us get back 
on track in live debug sessions. Core file sessions results will vary depending 
on it we actually have the original object files or not, many times we do not 
have this info, in which case we would just enable RA search and we might be 
able to unwind the PC, but not other registers if we can't actually find the 
prologue or if we don't have stack frame details from breakpad


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

If anyone wants to see the "sbt" python command, I am happy to share it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

"sbt" == stack back trace

Just looks at the stack for things that look like code and tries to make 
something that looks like a backtrace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I also wonder if this shouldn't require a separate flag to turn on this search, 
or be a separate command in the way Greg did.  People rely on backtraces being 
accurate.  It's fine to add something more like a desperation play - something 
is sometimes better than nothing, but if we are presenting a view that we don't 
have full confidence in, we really should tell users that so they can factor it 
into their analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [lldb] fd14646 - [LLDB] Applying clang-tidy modernize-use-override over LLDB

[Shafik Yaghmour via lldb-commits]

Author: Shafik Yaghmour
Date: 2022-04-22T13:29:47-07:00
New Revision: fd1464604367f2259614b66c886db16598b5be6b

URL: 
https://github.com/llvm/llvm-project/commit/fd1464604367f2259614b66c886db16598b5be6b
DIFF: 
https://github.com/llvm/llvm-project/commit/fd1464604367f2259614b66c886db16598b5be6b.diff

LOG: [LLDB] Applying clang-tidy modernize-use-override over LLDB

Applied clang-tidy modernize-use-override over LLDB and added it to the LLDB 
.clang-tidy config.

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

Added: 


Modified: 
lldb/.clang-tidy
lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
lldb/unittests/API/SBCommandInterpreterTest.cpp
lldb/unittests/Interpreter/TestCommandPaths.cpp
lldb/unittests/Interpreter/TestOptionValue.cpp
lldb/unittests/Target/RemoteAwarePlatformTest.cpp

Removed: 




diff  --git a/lldb/.clang-tidy b/lldb/.clang-tidy
index 2cde63668d048..499170a49c07a 100644
--- a/lldb/.clang-tidy
+++ b/lldb/.clang-tidy
@@ -1,4 +1,4 @@
-Checks: 
'-readability-identifier-naming,modernize-use-default-member-init,modernize-use-equals-default'
+Checks: 
'-readability-identifier-naming,modernize-use-default-member-init,modernize-use-equals-default,modernize-use-override'
 InheritParentConfig: true
 CheckOptions:
   - key: modernize-use-default-member-init.IgnoreMacros

diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 160740da31b4f..9ef3c3671abf6 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -240,7 +240,7 @@ class CompleteTagDeclsScope : public 
ClangASTImporter::NewDeclListener {
 m_delegate->SetImportListener(this);
   }
 
-  virtual ~CompleteTagDeclsScope() {
+  ~CompleteTagDeclsScope() override {
 ClangASTImporter::ASTContextMetadataSP to_context_md =
 importer.GetContextMetadata(m_dst_ctx);
 

diff  --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp 
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
index 180ac3ea7f1b2..b3eafecedd76e 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -193,7 +193,7 @@ class PlatformDarwinKernelProperties : public Properties {
 m_collection_sp->Initialize(g_platformdarwinkernel_properties);
   }
 
-  virtual ~PlatformDarwinKernelProperties() = default;
+  ~PlatformDarwinKernelProperties() override = default;
 
   FileSpecList GetKextDirectories() const {
 const uint32_t idx = ePropertyKextDirectories;

diff  --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp 
b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
index f58adcafcd018..b77cf8012b5ea 100644
--- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
+++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
@@ -74,7 +74,7 @@ class PluginProperties : public Properties {
 m_collection_sp->Initialize(g_processkdp_properties);
   }
 
-  virtual ~PluginProperties() = default;
+  ~PluginProperties() override = default;
 
   uint64_t GetPacketTimeout() {
 const uint32_t idx = ePropertyKDPPacketTimeout;
@@ -880,7 +880,7 @@ class CommandObjectProcessKDPPacketSend : public 
CommandObjectParsed {
 m_option_group.Finalize();
   }
 
-  ~CommandObjectProcessKDPPacketSend() = default;
+  ~CommandObjectProcessKDPPacketSend() override = default;
 
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 const size_t argc = command.GetArgumentCount();
@@ -981,7 +981,7 @@ class CommandObjectProcessKDPPacket : public 
CommandObjectMultiword {
 CommandObjectSP(new CommandObjectProcessKDPPacketSend(interpreter)));
   }
 
-  ~CommandObjectProcessKDPPacket() = default;
+  ~CommandObjectProcessKDPPacket() override = default;
 };
 
 class CommandObjectMultiwordProcessKDP : public CommandObjectMultiword {
@@ -995,7 +995,7 @@ class CommandObjectMultiwordProcessKDP : public 
CommandObjectMultiword {
  interpreter)));
   }
 
-  ~CommandObjectMultiwordProcessKDP() = default;
+  ~CommandObjectMultiwordProcessKDP() override = default;
 };
 
 CommandObject *ProcessKDP::GetPluginCommandObject() {

diff  --git a/lldb/unittests/API/SBCommandInterpreterTest.cpp 
b/lldb/unittests/API/SBCommandInterpreterTest.cpp
index d117c08c0bf46..0ddfa51fb6a70 100644
--- a/lldb/unittests/API/SBCommandInterpreterTest.cpp
+++ b/lldb/unittests/API/SBCommandInterpreterTest.cpp
@@ -34,7 +34,7 @@ class DummyCommand : public SBCommandPluginInterface {
   DummyCommand(const char *message) : m_message(message) {}
 
   bool DoExecute(SBDebugger dbg

[Lldb-commits] [PATCH] D123340: Applying clang-tidy modernize-use-override over LLDB

[Shafik Yaghmour via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfd1464604367: [LLDB] Applying clang-tidy 
modernize-use-override over LLDB (authored by shafik).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123340

Files:
  lldb/.clang-tidy
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/unittests/API/SBCommandInterpreterTest.cpp
  lldb/unittests/Interpreter/TestCommandPaths.cpp
  lldb/unittests/Interpreter/TestOptionValue.cpp
  lldb/unittests/Target/RemoteAwarePlatformTest.cpp

Index: lldb/unittests/Target/RemoteAwarePlatformTest.cpp
===
--- lldb/unittests/Target/RemoteAwarePlatformTest.cpp
+++ lldb/unittests/Target/RemoteAwarePlatformTest.cpp
@@ -37,7 +37,8 @@
const FileSpecList *));
   Status ResolveRemoteExecutable(
   const ModuleSpec &module_spec, lldb::ModuleSP &exe_module_sp,
-  const FileSpecList *module_search_paths_ptr) /*override*/ {
+  const FileSpecList *module_search_paths_ptr) /*override*/
+  { // NOLINT(modernize-use-override)
 auto pair = ResolveRemoteExecutable(module_spec, module_search_paths_ptr);
 exe_module_sp = pair.second;
 return pair.first;
Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- lldb/unittests/Interpreter/TestOptionValue.cpp
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -111,7 +111,7 @@
   }
 
 private:
-  lldb::OptionValueSP Clone() const {
+  lldb::OptionValueSP Clone() const override {
 return std::make_shared(*this);
   }
 
Index: lldb/unittests/Interpreter/TestCommandPaths.cpp
===
--- lldb/unittests/Interpreter/TestCommandPaths.cpp
+++ lldb/unittests/Interpreter/TestCommandPaths.cpp
@@ -51,7 +51,7 @@
   }
 
 protected:
-  virtual bool DoExecute(Args &command, CommandReturnObject &result) {
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
 result.SetStatus(eReturnStatusSuccessFinishResult);
 result.AppendMessage("I did nothing");
 return true;
Index: lldb/unittests/API/SBCommandInterpreterTest.cpp
===
--- lldb/unittests/API/SBCommandInterpreterTest.cpp
+++ lldb/unittests/API/SBCommandInterpreterTest.cpp
@@ -34,7 +34,7 @@
   DummyCommand(const char *message) : m_message(message) {}
 
   bool DoExecute(SBDebugger dbg, char **command,
- SBCommandReturnObject &result) {
+ SBCommandReturnObject &result) override {
 result.PutCString(m_message.c_str());
 result.SetStatus(eReturnStatusSuccessFinishResult);
 return result.Succeeded();
Index: lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
===
--- lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
+++ lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
@@ -74,7 +74,7 @@
 m_collection_sp->Initialize(g_processkdp_properties);
   }
 
-  virtual ~PluginProperties() = default;
+  ~PluginProperties() override = default;
 
   uint64_t GetPacketTimeout() {
 const uint32_t idx = ePropertyKDPPacketTimeout;
@@ -880,7 +880,7 @@
 m_option_group.Finalize();
   }
 
-  ~CommandObjectProcessKDPPacketSend() = default;
+  ~CommandObjectProcessKDPPacketSend() override = default;
 
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 const size_t argc = command.GetArgumentCount();
@@ -981,7 +981,7 @@
 CommandObjectSP(new CommandObjectProcessKDPPacketSend(interpreter)));
   }
 
-  ~CommandObjectProcessKDPPacket() = default;
+  ~CommandObjectProcessKDPPacket() override = default;
 };
 
 class CommandObjectMultiwordProcessKDP : public CommandObjectMultiword {
@@ -995,7 +995,7 @@
  interpreter)));
   }
 
-  ~CommandObjectMultiwordProcessKDP() = default;
+  ~CommandObjectMultiwordProcessKDP() override = default;
 };
 
 CommandObject *ProcessKDP::GetPluginCommandObject() {
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -193,7 +193,7 @@
 m_collection_sp->Initialize(g_platformdarwinkernel_properties);
   }
 
-  virtual ~PlatformDarwinKernelProperties() = default;
+  ~PlatformDarwinKernelProperties() override = default;
 
   FileSpecList GetKextDirectories() const {
 const uint32_t idx = ePropertyKextDirectories;
Index: lldb/source/Plugins/ExpressionParser/Clang/

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 13 inline comments as done.
wallace added inline comments.



Comment at: lldb/include/lldb/Target/TraceCursor.h:265
 
+class TraceEventsUtils {
+public:

jj10306 wrote:
> nit: maybe this will change in the future to where this will have data and 
> instance members, but if not, consider just using a namespace here to house 
> the utility functions since the class isn't really providing any state?
great idea



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:75
+
+  /// \return \b true iff this struct holds a libipt error.
+  explicit operator bool() const;

jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > 
> > oh, i wanted to mean if and only if. Or is that too mathematical and less 
> > colloquial? 
> Lol my first thought actually was "I wonder if he meant if and only if, so 
> this isn't a typo".
> Up to you if you want to keep if or leave it as iff 
i'll use if and only if



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.

jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > jj10306 wrote:
> > > > I know this isn't related to these changes, but this should be updated 
> > > > to be consistent with the other `instruction_index -> XXX` maps in this 
> > > > class.
> > > 
> > in this case we can't do that, because when doing random accesses in the 
> > trace, we need to quickly find the most recent TSC for the new instruction, 
> > which is done with binary search using a map. This is impossible in a hash 
> > map like DenseMap
> I know this is a pedantic discussion, but isn't lookup in hash tables 
> amortized O(1) (assuming a good hash that doesn't take too long to execute or 
> produce the same hash for different values) whereas in an ordered map it's 
> O(logn)?
> IIUC, you should only use an ordered map when you rely on the order property 
> of it.
> 
> In any case, we should use `uint64_t` here instead of `size_t` to be 
> consistent with the type for instruction index used in the other maps.
> In any case, we should use uint64_t here instead of size_t to be consistent 
> with the type for instruction index used in the other maps.

you are right with this one.

> IIUC, you should only use an ordered map when you rely on the order property 
> of it.

yes, and that's what we use for the timestamps. As there are a small number of 
unique timestamps, we are keeping this map 'insn_index -> timestamp'. We need 
its order property because when we do GoToId(insn_id) we need to look for most 
recent entry in the map that has a timestamp, and we use that timestamp for our 
new instruction. We use map.lower_bound for that. With a hashmap we couldn't do 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982

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

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 424658.
wallace marked 3 inline comments as done.
wallace added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/ThreadDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInfo.py
  lldb/test/API/commands/trace/TestTraceEvents.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -40,12 +40,17 @@
 
   Memory usage:
 Raw trace size: 4 KiB
-Total approximate memory usage (excluding raw trace): 0.27 KiB
-Average memory usage per instruction (excluding raw trace): 13.00 bytes
+Total approximate memory usage (excluding raw trace): 1.27 KiB
+Average memory usage per instruction (excluding raw trace): 61.76 bytes
 
   Timing:
 Decoding instructions: ''', '''s
 
+  Events:
+Number of instructions with events: 1
+Number of individual events: 1
+  paused: 1
+
   Errors:
 Number of TSC decoding errors: 0'''])
 
Index: lldb/test/API/commands/trace/TestTraceEvents.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceEvents.py
@@ -0,0 +1,82 @@
+import lldb
+from intelpt_testcase import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceEvents(TraceIntelPTTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@testSBAPIAndCommands
+def testPauseEvents(self):
+  '''
+Everytime the target stops running on the CPU, a 'disabled' event will
+be emitted, which is represented by the TraceCursor API as a 'paused'
+event.
+  '''
+  self.expect("target create " +
+os.path.join(self.getSourceDir(), "intelpt-trace-multi-file", "a.out"))
+  self.expect("b 12")
+  self.expect("r")
+  self.traceStartThread()
+  self.expect("n")
+  self.expect("n")
+  self.expect("si")
+  self.expect("si")
+  self.expect("si")
+  # We ensure that the paused events are printed correctly forward
+  self.expect("thread trace dump instructions -e -f",
+patterns=[f'''thread #1: tid = .*
+  a.out`main \+ 23 at main.cpp:12
+0: {ADDRESS_REGEX}movl .*
+  \[paused\]
+1: {ADDRESS_REGEX}addl .*
+2: {ADDRESS_REGEX}movl .*
+  \[paused\]
+  a.out`main \+ 34 \[inlined\] inline_function\(\) at main.cpp:4
+3: {ADDRESS_REGEX}movl .*
+  a.out`main \+ 41 \[inlined\] inline_function\(\) \+ 7 at main.cpp:5
+4: {ADDRESS_REGEX}movl .*
+5: {ADDRESS_REGEX}addl .*
+6: {ADDRESS_REGEX}movl .*
+  a.out`main \+ 52 \[inlined\] inline_function\(\) \+ 18 at main.cpp:6
+7: {ADDRESS_REGEX}movl .*
+  a.out`main \+ 55 at main.cpp:14
+8: {ADDRESS_REGEX}movl .*
+9: {ADDRESS_REGEX}addl .*
+10: {ADDRESS_REGEX}movl .*
+  \[paused\]
+  a.out`main \+ 63 at main.cpp:16
+11: {ADDRESS_REGEX}callq  .* ; symbol stub for: foo\(\)
+  \[paused\]
+  a.out`symbol stub for: foo\(\)
+12: {ADDRESS_REGEX}jmpq'''])
+
+  # We ensure that the paused events are printed correctly backward
+  self.expect("thread trace dump instructions -e --id 12",
+patterns=[f'''thread #1: tid = .*
+  a.out`symbol stub for: foo\(\)
+12: {ADDRESS_REGEX}jmpq .*
+  \[paused\]
+  a.out`main \+ 63 at main.cpp:16
+11: {ADDRESS_REGEX}callq  .* ; symbol stub for: foo\(\)
+  \[paused\]
+  a.out`main \+ 60 at main.cpp:14
+10: {ADDRESS_REGEX}movl .*
+9: {ADDRESS_REGEX}addl .*
+8: {ADDRESS_REGEX}movl .*
+  a.out`main \+ 52 \[inlined\] inline_function\(\) \+ 18 at main.cpp:6
+7: {ADDRESS_REGEX}movl .*
+  a.out`main \+ 49 \[inlined\] inline_function\(\) \+ 15 at main.cpp:5
+6: {ADDRESS_REGEX}movl .*
+5: {ADDRESS_REGEX}addl .*
+4: {ADDRESS_REGEX}movl .*
+  a.out`main \+ 34 \[inlined\] inline_function\(\) at main.cpp:4
+3: {ADDRESS_REGEX}movl .*
+  \[paused\]
+  a.out`main \+ 31 at main.cpp:12
+2: 

[Lldb-commits] [PATCH] D124314: lldb: Disable unittests if llvm_gtest target does not exist

[Tom Stellard via Phabricator via lldb-commits]

tstellar created this revision.
Herald added a subscriber: mgorny.
Herald added a project: All.
tstellar requested review of this revision.
Herald added a project: LLDB.

This fixes the stand-alone build configuration where LLVM_MAIN_SRC_DIR
does not exist.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124314

Files:
  lldb/CMakeLists.txt
  lldb/unittests/CMakeLists.txt


Index: lldb/unittests/CMakeLists.txt
===
--- lldb/unittests/CMakeLists.txt
+++ lldb/unittests/CMakeLists.txt
@@ -17,20 +17,6 @@
   list(APPEND LLVM_COMPILE_FLAGS -include ${LLDB_GTEST_COMMON_INCLUDE})
 endif ()
 
-if (LLDB_BUILT_STANDALONE)
-  # Build the gtest library needed for unittests, if we have LLVM sources
-  # handy.
-  if (EXISTS ${LLVM_MAIN_SRC_DIR}/utils/unittest AND NOT TARGET llvm_gtest)
-add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/unittest utils/unittest)
-  endif()
-  # LLVMTestingSupport library is needed for Process/gdb-remote.
-  if (EXISTS ${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
-  AND NOT TARGET LLVMTestingSupport)
-add_subdirectory(${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
-  lib/Testing/Support)
-  endif()
-endif()
-
 function(add_lldb_unittest test_name)
   cmake_parse_arguments(ARG
 ""
Index: lldb/CMakeLists.txt
===
--- lldb/CMakeLists.txt
+++ lldb/CMakeLists.txt
@@ -124,10 +124,31 @@
   finish_swig_lua("lldb-lua" "${lldb_lua_bindings_dir}" 
"${lldb_lua_target_dir}")
 endif()
 
+if (LLDB_BUILT_STANDALONE)
+  # Build the gtest library needed for unittests, if we have LLVM sources
+  # handy.
+  if (EXISTS ${LLVM_MAIN_SRC_DIR}/utils/unittest AND NOT TARGET llvm_gtest)
+add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/unittest utils/unittest)
+  endif()
+  # LLVMTestingSupport library is needed for Process/gdb-remote.
+  if (EXISTS ${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
+  AND NOT TARGET LLVMTestingSupport)
+add_subdirectory(${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
+  lib/Testing/Support)
+  endif()
+endif()
+
+set(LLDB_INCLUDE_UNITTESTS ON)
+if (NOT TARGET llvm_gtest)
+  set(LLDB_INCLUDE_UNITTESTS OFF)
+endif()
+
 option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." 
${LLVM_INCLUDE_TESTS})
 if(LLDB_INCLUDE_TESTS)
   add_subdirectory(test)
-  add_subdirectory(unittests)
+  if (LLDB_INCLUDE_UNITTESTS)
+add_subdirectory(unittests)
+  endif()
   add_subdirectory(utils)
 endif()
 


Index: lldb/unittests/CMakeLists.txt
===
--- lldb/unittests/CMakeLists.txt
+++ lldb/unittests/CMakeLists.txt
@@ -17,20 +17,6 @@
   list(APPEND LLVM_COMPILE_FLAGS -include ${LLDB_GTEST_COMMON_INCLUDE})
 endif ()
 
-if (LLDB_BUILT_STANDALONE)
-  # Build the gtest library needed for unittests, if we have LLVM sources
-  # handy.
-  if (EXISTS ${LLVM_MAIN_SRC_DIR}/utils/unittest AND NOT TARGET llvm_gtest)
-add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/unittest utils/unittest)
-  endif()
-  # LLVMTestingSupport library is needed for Process/gdb-remote.
-  if (EXISTS ${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
-  AND NOT TARGET LLVMTestingSupport)
-add_subdirectory(${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
-  lib/Testing/Support)
-  endif()
-endif()
-
 function(add_lldb_unittest test_name)
   cmake_parse_arguments(ARG
 ""
Index: lldb/CMakeLists.txt
===
--- lldb/CMakeLists.txt
+++ lldb/CMakeLists.txt
@@ -124,10 +124,31 @@
   finish_swig_lua("lldb-lua" "${lldb_lua_bindings_dir}" "${lldb_lua_target_dir}")
 endif()
 
+if (LLDB_BUILT_STANDALONE)
+  # Build the gtest library needed for unittests, if we have LLVM sources
+  # handy.
+  if (EXISTS ${LLVM_MAIN_SRC_DIR}/utils/unittest AND NOT TARGET llvm_gtest)
+add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/unittest utils/unittest)
+  endif()
+  # LLVMTestingSupport library is needed for Process/gdb-remote.
+  if (EXISTS ${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
+  AND NOT TARGET LLVMTestingSupport)
+add_subdirectory(${LLVM_MAIN_SRC_DIR}/lib/Testing/Support
+  lib/Testing/Support)
+  endif()
+endif()
+
+set(LLDB_INCLUDE_UNITTESTS ON)
+if (NOT TARGET llvm_gtest)
+  set(LLDB_INCLUDE_UNITTESTS OFF)
+endif()
+
 option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." ${LLVM_INCLUDE_TESTS})
 if(LLDB_INCLUDE_TESTS)
   add_subdirectory(test)
-  add_subdirectory(unittests)
+  if (LLDB_INCLUDE_UNITTESTS)
+add_subdirectory(unittests)
+  endif()
   add_subdirectory(utils)
 endif()
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits