[Lldb-commits] [PATCH] D125860: [clang] Only use major version in resource dir

2022-11-08 Thread Timm Bäder via Phabricator via lldb-commits 
tbaeder updated this revision to Diff 473951.
tbaeder added a comment.

Add a release note


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

https://reviews.llvm.org/D125860

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Version.inc.in
  clang/lib/Driver/Driver.cpp
  clang/lib/Headers/CMakeLists.txt
  clang/lib/Tooling/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  compiler-rt/cmake/base-config-ix.cmake
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/cmake/modules/LLVMExternalProjectUtils.cmake
  polly/lib/External/isl/interface/extract_interface.cc

Index: polly/lib/External/isl/interface/extract_interface.cc
===
--- polly/lib/External/isl/interface/extract_interface.cc
+++ polly/lib/External/isl/interface/extract_interface.cc
@@ -109,7 +109,7 @@
 	llvm::cl::value_desc("name"));
 
 static const char *ResourceDir =
-	CLANG_PREFIX "/lib/clang/" CLANG_VERSION_STRING;
+CLANG_PREFIX "/lib/clang/" CLANG_VERSION_MAJOR_STRING;
 
 /* Does decl have an attribute of the following form?
  *
Index: llvm/cmake/modules/LLVMExternalProjectUtils.cmake
===
--- llvm/cmake/modules/LLVMExternalProjectUtils.cmake
+++ llvm/cmake/modules/LLVMExternalProjectUtils.cmake
@@ -255,9 +255,9 @@
 set(llvm_config_path ${LLVM_CONFIG_PATH})
 
 if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-  string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION
+  string(REGEX MATCH "^[0-9]+" CLANG_VERSION_MAJOR
  ${PACKAGE_VERSION})
-  set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION}")
+  set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION_MAJOR}")
   set(flag_types ASM C CXX MODULE_LINKER SHARED_LINKER EXE_LINKER)
   foreach(type ${flag_types})
 set(${type}_flag -DCMAKE_${type}_FLAGS=-resource-dir=${resource_dir})
Index: lldb/unittests/Expression/ClangParserTest.cpp
===
--- lldb/unittests/Expression/ClangParserTest.cpp
+++ lldb/unittests/Expression/ClangParserTest.cpp
@@ -38,10 +38,11 @@
 #if !defined(_WIN32)
   std::string path_to_liblldb = "/foo/bar/lib/";
   std::string path_to_clang_dir =
-  "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_STRING;
+  "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING;
 #else
   std::string path_to_liblldb = "C:\\foo\\bar\\lib";
-  std::string path_to_clang_dir = "C:\\foo\\bar\\lib\\clang\\" CLANG_VERSION_STRING;
+  std::string path_to_clang_dir =
+  "C:\\foo\\bar\\lib\\clang\\" CLANG_VERSION_MAJOR_STRING;
 #endif
   EXPECT_EQ(ComputeClangResourceDir(path_to_liblldb), path_to_clang_dir);
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -55,7 +55,7 @@
   static const llvm::StringRef kResourceDirSuffixes[] = {
   // LLVM.org's build of LLDB uses the clang resource directory placed
   // in $install_dir/lib{,64}/clang/$clang_version.
-  CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_STRING,
+  CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING,
   // swift-lldb uses the clang resource directory copied from swift, which
   // by default is placed in $install_dir/lib{,64}/lldb/clang. LLDB places
   // it there, so we use LLDB_INSTALL_LIBDIR_BASENAME.
Index: compiler-rt/cmake/base-config-ix.cmake
===
--- compiler-rt/cmake/base-config-ix.cmake
+++ compiler-rt/cmake/base-config-ix.cmake
@@ -38,14 +38,14 @@
 
 if (LLVM_TREE_AVAILABLE)
   # Compute the Clang version from the LLVM version.
-  # FIXME: We should be able to reuse CLANG_VERSION variable calculated
+  # FIXME: We should be able to reuse CLANG_VERSION_MAJOR variable calculated
   #in Clang cmake files, instead of copying the rules here.
-  string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION
+  string(REGEX MATCH "^[0-9]+" CLANG_VERSION_MAJOR
  ${PACKAGE_VERSION})
   # Setup the paths where compiler-rt runtimes and headers should be stored.
-  set(COMPILER_RT_OUTPUT_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION})
+  set(COMPILER_RT_OUTPUT_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION_MAJOR})
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
-  set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
+  set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION_MAJOR})
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests."
  ${LLVM_INCLUDE_TESTS})
   option(COMPILER_RT_ENABLE_WERROR "Fail and

[Lldb-commits] [PATCH] D136565: [clang] Instantiate alias templates with sugar

2022-11-08 Thread Alexander Kornienko via Phabricator via lldb-commits 
alexfh added a comment.

In D136565#3913932 , @mizvekov wrote:

> @alexfh Thanks!
>
> While there is a huge increase in the amount of UsingTypes, it seems the 
> total amount is still reasonable and does not explain the perf hit.
>
> Perhaps this is a case of bad hashing and they are all falling into the same 
> bucket?
>
> cc @sam.mcall for awareness of UsingType issue.
> It may be some very simple problem, I just can't even look at the code right 
> now.

I've reduced the test case to an initializer with a few thousand std::variant 
elements (F25231339: q2.cc ), which is 
compiled around 2 times slower with the clang with this patch vs clang before 
the patch:

  $ ../clang-base -fsyntax-only q2.cc  -ftime-report ; ../clang-exp 
-fsyntax-only q2.cc  -ftime-report
  
===-===
Clang front-end time report
  
===-===
Total Execution Time: 7.4495 seconds (7.4498 wall clock)
  
 ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- 
Name ---
 7.2215 (100.0%)   0.2280 (100.0%)   7.4495 (100.0%)   7.4498 (100.0%)  
Clang front-end timer
 7.2215 (100.0%)   0.2280 (100.0%)   7.4495 (100.0%)   7.4498 (100.0%)  
Total
  
  
===-===
Clang front-end time report
  
===-===
Total Execution Time: 13.7677 seconds (13.7686 wall clock)
  
 ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- 
Name ---
13.5666 (100.0%)   0.2011 (100.0%)  13.7677 (100.0%)  13.7686 (100.0%)  
Clang front-end timer
13.5666 (100.0%)   0.2011 (100.0%)  13.7677 (100.0%)  13.7686 (100.0%)  
Total

When I duplicate the number of array elements, the parsing time after the patch 
grows by a larger factor:

  $ ../clang-base -fsyntax-only q2.cc  -ftime-report ; ../clang-exp 
-fsyntax-only q2.cc  -ftime-report
  
===-===
Clang front-end time report
  
===-===
Total Execution Time: 14.1165 seconds (14.1173 wall clock)
  
 ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- 
Name ---
13.7642 (100.0%)   0.3523 (100.0%)  14.1165 (100.0%)  14.1173 (100.0%)  
Clang front-end timer
13.7642 (100.0%)   0.3523 (100.0%)  14.1165 (100.0%)  14.1173 (100.0%)  
Total
  
  
===-===
Clang front-end time report
  
===-===
Total Execution Time: 41.6697 seconds (41.6729 wall clock)
  
 ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- 
Name ---
41.2583 (100.0%)   0.4114 (100.0%)  41.6697 (100.0%)  41.6729 (100.0%)  
Clang front-end timer
41.2583 (100.0%)   0.4114 (100.0%)  41.6697 (100.0%)  41.6729 (100.0%)  
Total

Thus, the patch introduces non-linear dependency of compilation times from the 
number of certain elements in the AST.

I'm going to revert the patch for now and let you figure out this when it's 
convenient to you. Have a nice vacation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136565

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

[Lldb-commits] [PATCH] D125860: [clang] Only use major version in resource dir

2022-11-08 Thread Timm Bäder via Phabricator via lldb-commits 
tbaeder added a comment.

I'm worried that this is gonna break build bots. Has anyone seen the 
ThreadSanitizer timeout reported by the precommit-ci before?


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

https://reviews.llvm.org/D125860

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

[Lldb-commits] [lldb] df766fb - [NFC][intelpt] Improve IntelPT trace bundle documentation

2022-11-08 Thread Jakob Johnson via lldb-commits 

Author: Jakob Johnson
Date: 2022-11-08T05:16:28-08:00
New Revision: df766fb65cc939bd88c25cb8e478b07f7b4ce123

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

LOG: [NFC][intelpt] Improve IntelPT trace bundle documentation

Mention that the LLVM/clang triple must be provided if the trace will be
consumed via `SBTraceCursor`

Test Plan:

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

Added: 


Modified: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp 
b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
index cac91e801c926..0e664fd91d80f 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
@@ -241,6 +241,8 @@ StringRef TraceIntelPTBundleLoader::GetSchema() {
   "pid": integer,
   "triple"?: string,
   // Optional clang/llvm target triple.
+  // This must be provided if the trace will be created not using the
+  // CLI or on a machine other than where the target was traced.
   "threads": [
   // A list of known threads for the given process. When context switch
   // data is provided, LLDB will automatically create threads for the



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

[Lldb-commits] [PATCH] D137509: [NFC][intelpt] Improve IntelPT trace bundle documentation

2022-11-08 Thread Jakob Johnson via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdf766fb65cc9: [NFC][intelpt] Improve IntelPT trace bundle 
documentation (authored by jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137509

Files:
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp


Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
@@ -241,6 +241,8 @@
   "pid": integer,
   "triple"?: string,
   // Optional clang/llvm target triple.
+  // This must be provided if the trace will be created not using the
+  // CLI or on a machine other than where the target was traced.
   "threads": [
   // A list of known threads for the given process. When context switch
   // data is provided, LLDB will automatically create threads for the


Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
@@ -241,6 +241,8 @@
   "pid": integer,
   "triple"?: string,
   // Optional clang/llvm target triple.
+  // This must be provided if the trace will be created not using the
+  // CLI or on a machine other than where the target was traced.
   "threads": [
   // A list of known threads for the given process. When context switch
   // data is provided, LLDB will automatically create threads for the
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

2022-11-08 Thread Jakob Johnson via Phabricator via lldb-commits 
jj10306 created this revision.
jj10306 added reviewers: wallace, persona0220.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Test Plan:


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137645

Files:
  lldb/bindings/interface/SBTraceCursor.i
  lldb/include/lldb/API/SBTraceCursor.h
  lldb/source/API/SBTraceCursor.cpp


Index: lldb/source/API/SBTraceCursor.cpp
===
--- lldb/source/API/SBTraceCursor.cpp
+++ lldb/source/API/SBTraceCursor.cpp
@@ -124,6 +124,13 @@
   return m_opaque_sp->GetCPU();
 }
 
+double SBTraceCursor::GetWallClockTime() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;
+}
+
 bool SBTraceCursor::IsValid() const {
   LLDB_INSTRUMENT_VA(this);
 
Index: lldb/include/lldb/API/SBTraceCursor.h
===
--- lldb/include/lldb/API/SBTraceCursor.h
+++ lldb/include/lldb/API/SBTraceCursor.h
@@ -169,6 +169,12 @@
   ///not available for the current item.
   lldb::cpu_id_t GetCPU() const;
 
+  /// \return
+  /// The approximate wall clock time for the trace item, or \a -1.0
+  /// if not available.
+  double GetWallClockTime() const;
+  /// \}
+
   bool IsValid() const;
 
   explicit operator bool() const;
Index: lldb/bindings/interface/SBTraceCursor.i
===
--- lldb/bindings/interface/SBTraceCursor.i
+++ lldb/bindings/interface/SBTraceCursor.i
@@ -51,6 +51,8 @@
 
   lldb::cpu_id_t GetCPU() const;
 
+  double GetWallClockTime() const;
+
   bool IsValid() const;
 
   explicit operator bool() const;


Index: lldb/source/API/SBTraceCursor.cpp
===
--- lldb/source/API/SBTraceCursor.cpp
+++ lldb/source/API/SBTraceCursor.cpp
@@ -124,6 +124,13 @@
   return m_opaque_sp->GetCPU();
 }
 
+double SBTraceCursor::GetWallClockTime() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;
+}
+
 bool SBTraceCursor::IsValid() const {
   LLDB_INSTRUMENT_VA(this);
 
Index: lldb/include/lldb/API/SBTraceCursor.h
===
--- lldb/include/lldb/API/SBTraceCursor.h
+++ lldb/include/lldb/API/SBTraceCursor.h
@@ -169,6 +169,12 @@
   ///not available for the current item.
   lldb::cpu_id_t GetCPU() const;
 
+  /// \return
+  /// The approximate wall clock time for the trace item, or \a -1.0
+  /// if not available.
+  double GetWallClockTime() const;
+  /// \}
+
   bool IsValid() const;
 
   explicit operator bool() const;
Index: lldb/bindings/interface/SBTraceCursor.i
===
--- lldb/bindings/interface/SBTraceCursor.i
+++ lldb/bindings/interface/SBTraceCursor.i
@@ -51,6 +51,8 @@
 
   lldb::cpu_id_t GetCPU() const;
 
+  double GetWallClockTime() const;
+
   bool IsValid() const;
 
   explicit operator bool() const;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

2022-11-08 Thread Jakob Johnson via Phabricator via lldb-commits 
jj10306 added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: lldb/source/API/SBTraceCursor.cpp:131
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;
+}

open to suggestions on the best way to indicate that the current item doesn't 
have a timestamp associated with it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

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

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

2022-11-08 Thread Will Hawkins via Phabricator via lldb-commits 
hawkinsw added inline comments.



Comment at: lldb/source/API/SBTraceCursor.cpp:131
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;
+}

jj10306 wrote:
> open to suggestions on the best way to indicate that the current item doesn't 
> have a timestamp associated with it
Could we use `llvm::Optional`? Sorry if that is a silly suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

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

[Lldb-commits] [PATCH] D125860: [clang] Only use major version in resource dir

2022-11-08 Thread Tom Stellard via Phabricator via lldb-commits 
tstellar added a comment.

@tbaeder That CI failure seems unrelated.  Maybe just commit early in the day 
so you have time to deal with the bot failures (if any).


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

https://reviews.llvm.org/D125860

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

[Lldb-commits] [PATCH] D137614: [trace] Add a new call graph reconstructor

2022-11-08 Thread Jakob Johnson via Phabricator via lldb-commits 
jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:296
   std::vector m_item_data;
+  std::vector m_insn_extra_info;
   /// The TraceItemKind for each trace item encoded as uint8_t. We don't 
include

do we need to store this or can this information be calculated on demand? just 
thinking about massive traces and want to make sure we're intentional about 
only storing what we absolutely need to for each instruction



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:317
  events_stats.total_count);
-for (const auto &event_to_count : events_stats.events_counts) {
-  s.Format("  {0}: {1}\n",
-   TraceCursor::EventKindToString(event_to_count.first),
-   event_to_count.second);
+for (auto it = events_stats.events_counts.begin();
+ it != events_stats.events_counts.end(); ++it) {

what's the reason for this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137614

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

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

2022-11-08 Thread walter erquinigo via Phabricator via lldb-commits 
wallace requested changes to this revision.
wallace added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/include/lldb/API/SBTraceCursor.h:175
+  /// if not available.
+  double GetWallClockTime() const;
+  /// \}

mention here that the trace plugin decides how to estimate wall clock time it 
needed, and that not all trace items are guaranteed to have wall clock time, as 
it depends on the trace plug-in capabilities



Comment at: lldb/source/API/SBTraceCursor.cpp:127-131
+double SBTraceCursor::GetWallClockTime() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;

we don't have optionals in the SB bridge, so you could do the following

  bool SBTraceCursor::GetWallClockTime(double &time) {
if (const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime()) {
  time = *maybe_wall_clock_time;
  return true;
}
return false;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

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

[Lldb-commits] [PATCH] D137614: [trace] Add a new call graph reconstructor

2022-11-08 Thread walter erquinigo via Phabricator via lldb-commits 
wallace added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:296
   std::vector m_item_data;
+  std::vector m_insn_extra_info;
   /// The TraceItemKind for each trace item encoded as uint8_t. We don't 
include

jj10306 wrote:
> do we need to store this or can this information be calculated on demand? 
> just thinking about massive traces and want to make sure we're intentional 
> about only storing what we absolutely need to for each instruction
I thought about it and it will help us have more correctness for now and a 
little bit more speed with little memory cost. The total size per instruction 
went from 9 bytes to 11, which is very decent. 

Now some details: the Disassembler fails with some instructions. I was tracing 
dynolog and found many instances of the disassembler failing, but libipt 
doesn't fail. In other words, we can't get the control flow kind from LLDB but 
we can from libipt. Same for the byte size. That means that we need to fix the 
disassembler, but that will require a lot of work that I can hardly prioritize, 
so I'd rather use libipt for now.




Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:317
  events_stats.total_count);
-for (const auto &event_to_count : events_stats.events_counts) {
-  s.Format("  {0}: {1}\n",
-   TraceCursor::EventKindToString(event_to_count.first),
-   event_to_count.second);
+for (auto it = events_stats.events_counts.begin();
+ it != events_stats.events_counts.end(); ++it) {

jj10306 wrote:
> what's the reason for this change?
wtf, i think this is from a previous commit i never intended to publish. I was 
playing with some stuff. I need to revert this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137614

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

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

2022-11-08 Thread Will Hawkins via Phabricator via lldb-commits 
hawkinsw added inline comments.



Comment at: lldb/source/API/SBTraceCursor.cpp:127-131
+double SBTraceCursor::GetWallClockTime() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;

wallace wrote:
> we don't have optionals in the SB bridge, so you could do the following
> 
>   bool SBTraceCursor::GetWallClockTime(double &time) {
> if (const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime()) {
>   time = *maybe_wall_clock_time;
>   return true;
> }
> return false;
>   }
Not that it matters, but *that* was going to be my other suggestion! @wallace 
is smarter than I am!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

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

[Lldb-commits] [PATCH] D137662: Make aliases from a raw command that isn't a top-level command work

2022-11-08 Thread Jim Ingham via Phabricator via lldb-commits 
jingham created this revision.
jingham added reviewers: JDevlieghere, kastiglione, clayborg.
Herald added a subscriber: jeroen.dobbelaere.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When we went to alias a raw command, we look up the command and pass that to 
HandleAliasingRawCommand.  But then there is a check to make sure that we 
aren't using a nested alias, which we do by re-looking up the command object by 
name.  Note, the command object's name is just its individual name, not the 
full path.  That's appropriate for checking aliases since at present you can 
only alias TO a top-level command.

But if that lookup failed we return an error.  That lookup will fail for 
anything that's not a top-level command, since this is just the node name not 
the path.  So we should then look up the full command, but fortunately we don't 
have to do that because we already have the command passed in.  So if it isn't 
an alias to be resolved, we can just use it directly.

I had to add enable_shared_from_this to CommandObject.  We pass CommandObject 
*'s around in the command parsing code when we probably should be passing 
Shared Pointers, but trying to fix that ended up being a lot of change for not 
much benefit, and shared_from_this solves this neatly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137662

Files:
  lldb/include/lldb/Interpreter/CommandObject.h
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/test/API/commands/command/container/TestContainerCommands.py


Index: lldb/test/API/commands/command/container/TestContainerCommands.py
===
--- lldb/test/API/commands/command/container/TestContainerCommands.py
+++ lldb/test/API/commands/command/container/TestContainerCommands.py
@@ -55,6 +55,12 @@
 self.expect("test-multi test-multi-sub welcome friend", "Test command 
works",
 substrs=["Hello friend, welcome to LLDB"])
 
+# Make sure we can make an alias to this:
+self.runCmd("command alias my-welcome test-multi test-multi-sub 
welcome", "We can make an alias to multi-word")
+self.expect("my-welcome friend", "Test command works",
+substrs=["Hello friend, welcome to LLDB"])
+self.runCmd("command unalias my-welcome")
+
 # Make sure overwriting works on the leaf command.  First using the
 # explicit option so we should not be able to remove extant commands 
by default:
 
Index: lldb/source/Commands/CommandObjectCommands.cpp
===
--- lldb/source/Commands/CommandObjectCommands.cpp
+++ lldb/source/Commands/CommandObjectCommands.cpp
@@ -485,29 +485,31 @@
 OptionArgVectorSP(new OptionArgVector);
 
 const bool include_aliases = true;
-if (CommandObjectSP cmd_obj_sp = m_interpreter.GetCommandSPExact(
-cmd_obj.GetCommandName(), include_aliases)) {
-  if (m_interpreter.AliasExists(alias_command) ||
-  m_interpreter.UserCommandExists(alias_command)) {
-result.AppendWarningWithFormat(
-"Overwriting existing definition for '%s'.\n",
-alias_command.str().c_str());
-  }
-  if (CommandAlias *alias = m_interpreter.AddAlias(
-  alias_command, cmd_obj_sp, raw_command_string)) {
-if (m_command_options.m_help.OptionWasSet())
-  alias->SetHelp(m_command_options.m_help.GetCurrentValue());
-if (m_command_options.m_long_help.OptionWasSet())
-  alias->SetHelpLong(m_command_options.m_long_help.GetCurrentValue());
-result.SetStatus(eReturnStatusSuccessFinishNoResult);
-  } else {
-result.AppendError("Unable to create requested alias.\n");
-  }
+// Look up the command using command's name first.  This is to resolve
+// aliases when you are making nested aliases.  But if you don't find
+// it that way, then it wasn't an alias and we can just use the object
+// we were passed in.
+CommandObjectSP cmd_obj_sp = m_interpreter.GetCommandSPExact(
+cmd_obj.GetCommandName(), include_aliases);
+if (!cmd_obj_sp)
+  cmd_obj_sp = cmd_obj.shared_from_this();
 
+if (m_interpreter.AliasExists(alias_command) ||
+m_interpreter.UserCommandExists(alias_command)) {
+  result.AppendWarningWithFormat(
+  "Overwriting existing definition for '%s'.\n",
+  alias_command.str().c_str());
+}
+if (CommandAlias *alias = m_interpreter.AddAlias(
+alias_command, cmd_obj_sp, raw_command_string)) {
+  if (m_command_options.m_help.OptionWasSet())
+alias->SetHelp(m_command_options.m_help.GetCurrentValue());
+  if (m_command_options.m_long_help.OptionWasSet())
+alias->SetHelpLong(m_command_options.m_long_help.GetCurrentValue());
+  result.SetStatus(eReturnStatusSu

[Lldb-commits] [PATCH] D137000: [lldb] Make callback-based formatter matching available from the CLI.

2022-11-08 Thread Jim Ingham via Phabricator via lldb-commits 
jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

It is a bit odd to follow the what the help summary says is a valid option set 
and then get an error.   But this is a corner case, and since you say 
explicitly that you can't provide both that's probably good enough.  Would you 
mind changing "Incompatible with" to: "Cannot be specified at the same time 
as".  Incompatible isn't precise - does it mean you can't use them both, or 
that if you do something weird will happen?

Other than that, this is fine.  Thanks for working on this.


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

https://reviews.llvm.org/D137000

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

[Lldb-commits] [PATCH] D137000: [lldb] Make callback-based formatter matching available from the CLI.

2022-11-08 Thread Jim Ingham via Phabricator via lldb-commits 
jingham added a comment.

In D137000#3902217 , @labath wrote:

> In D137000#3897878 , @jgorbe wrote:
>
>> I'm looking at the option of using a non-printable character for the short 
>> flag, and at the same time make `--regex` and `--recognizer-function` 
>> mutually exclusive using option groups. One problem I see is that the 
>> command help gets pretty confusing. Using a non-printable character as the 
>> short option name makes the option completely disappear from the "Command 
>> Options Usage" summary part of the help.
>>
>> For example, for `disassemble` the `--force` option is defined with 
>> `Groups[2,3,4,5,7]` but there's no indication in the usage summary that 
>> `--force` is only usable in some of the groups:
>
> Hmm.. you're right, I didn't know that.  I'll leave it up to you, but 
> personally I wouldn't consider it a blocker, as I think that the way we print 
> our option group help needs an overhaul anyway. I mean, the idea is kinda 
> nice, but after a certain number of options (and groups) it just becomes 
> ridiculous. For example, I'd like to meet the person who can make sense of 
> this:
>
>   Command Options Usage:
> breakpoint set [-DHd] -l  [-G ] [-C ] [-c 
> ] [-i ] [-o ] [-q ] [-t ] [-x 
> ] [-T ] [-R ] [-N ] [-u 
> ] [-f ] [-m ] [-s ] [-K ]
> breakpoint set [-DHd] -a  [-G ] [-C 
> ] [-c ] [-i ] [-o ] [-q ] [-t 
> ] [-x ] [-T ] [-N ] 
> [-s ]
> breakpoint set [-DHd] -n  [-G ] [-C ] 
> [-c ] [-i ] [-o ] [-q ] [-t ] 
> [-x ] [-T ] [-R ] [-N ] 
> [-f ] [-L ] [-s ] [-K ]
> breakpoint set [-DHd] -F  [-G ] [-C ] [-c 
> ] [-i ] [-o ] [-q ] [-t ] [-x 
> ] [-T ] [-R ] [-N ] [-f 
> ] [-L ] [-s ] [-K ]
> breakpoint set [-DHd] -S  [-G ] [-C ] [-c 
> ] [-i ] [-o ] [-q ] [-t ] [-x 
> ] [-T ] [-R ] [-N ] [-f 
> ] [-L ] [-s ] [-K ]
> breakpoint set [-DHd] -M  [-G ] [-C ] [-c 
> ] [-i ] [-o ] [-q ] [-t ] [-x 
> ] [-T ] [-R ] [-N ] [-f 
> ] [-L ] [-s ] [-K ]
> breakpoint set [-DHd] -r  [-G ] [-C 
> ] [-c ] [-i ] [-o ] [-q ] [-t 
> ] [-x ] [-T ] [-R ] [-N 
> ] [-f ] [-L ] [-s ] 
> [-K ]
> breakpoint set [-DHd] -b  [-G ] [-C ] 
> [-c ] [-i ] [-o ] [-q ] [-t ] 
> [-x ] [-T ] [-R ] [-N ] 
> [-f ] [-L ] [-s ] [-K ]
> breakpoint set [-ADHd] -p  [-G ] [-C 
> ] [-c ] [-i ] [-o ] [-q ] [-t 
> ] [-x ] [-T ] [-N ] 
> [-f ] [-m ] [-s ] [-X ]
> breakpoint set [-DHd] -E  [-G ] [-C ] 
> [-c ] [-i ] [-o ] [-q ] [-t ] 
> [-x ] [-T ] [-N ] [-h ] 
> [-w ]
> breakpoint set [-DHd] -P  [-k ] [-v ] [-G 
> ] [-C ] [-c ] [-i ] [-o ] [-q 
> ] [-t ] [-x ] [-T ] [-N 
> ] [-f ] [-s ]
> breakpoint set [-DHd] -y  [-G ] [-C ] [-c 
> ] [-i ] [-o ] [-q ] [-t ] [-x 
> ] [-T ] [-R ] [-N ] [-m 
> ] [-s ] [-K ]

If I were starting over, I would make "break set" be a multiword command with 
sub-commands for all the primary specification operations (`break set file`, 
`break set name` etc.)  We could use unique first letters for all the actual 
operations, so this would actually be LESS typing by one dash, and would allow 
for better documentation of the shared options.  But I don't think we can get 
away with that at this point.

This display while noisy is the only way you can see all the kinds of 
breakpoint setting operations, so we really have to have something like this, 
and it should be fairly reliable...  It would also be nice if help also 
understood options, so for instance:`help break set -a` should only show you 
the variants that include that option.


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

https://reviews.llvm.org/D137000

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

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

2022-11-08 Thread Jakob Johnson via Phabricator via lldb-commits 
jj10306 updated this revision to Diff 474092.
jj10306 added a comment.

update the way items with no timestamps are handled


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

Files:
  lldb/bindings/interface/SBTraceCursor.i
  lldb/include/lldb/API/SBTraceCursor.h
  lldb/source/API/SBTraceCursor.cpp


Index: lldb/source/API/SBTraceCursor.cpp
===
--- lldb/source/API/SBTraceCursor.cpp
+++ lldb/source/API/SBTraceCursor.cpp
@@ -124,6 +124,16 @@
   return m_opaque_sp->GetCPU();
 }
 
+bool SBTraceCursor::GetWallClockTime(double &estimated_time) const {
+  LLDB_INSTRUMENT_VA(this);
+
+  if (const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime()) {
+estimated_time = *maybe_wall_clock_time;
+return true;
+  }
+  return false;
+}
+
 bool SBTraceCursor::IsValid() const {
   LLDB_INSTRUMENT_VA(this);
 
Index: lldb/include/lldb/API/SBTraceCursor.h
===
--- lldb/include/lldb/API/SBTraceCursor.h
+++ lldb/include/lldb/API/SBTraceCursor.h
@@ -169,6 +169,19 @@
   ///not available for the current item.
   lldb::cpu_id_t GetCPU() const;
 
+  /// Get the approximate wall clock time in nanoseconds at which the current
+  /// trace item was executed. Each trace plug-in has a different definition 
for
+  /// what time 0 means.
+  ///
+  /// \param[out] estimated_time
+  /// The trace item's estimtated timestamp, if available.
+  ///
+  /// \return
+  ///   true if the current trace item has an estimated timestamp associated
+  ///   with it, false otherwise.
+  bool GetWallClockTime(double &estimated_time) const;
+  /// \}
+
   bool IsValid() const;
 
   explicit operator bool() const;
Index: lldb/bindings/interface/SBTraceCursor.i
===
--- lldb/bindings/interface/SBTraceCursor.i
+++ lldb/bindings/interface/SBTraceCursor.i
@@ -51,6 +51,8 @@
 
   lldb::cpu_id_t GetCPU() const;
 
+  bool GetWallClockTime(double &estimated_time) const;
+
   bool IsValid() const;
 
   explicit operator bool() const;


Index: lldb/source/API/SBTraceCursor.cpp
===
--- lldb/source/API/SBTraceCursor.cpp
+++ lldb/source/API/SBTraceCursor.cpp
@@ -124,6 +124,16 @@
   return m_opaque_sp->GetCPU();
 }
 
+bool SBTraceCursor::GetWallClockTime(double &estimated_time) const {
+  LLDB_INSTRUMENT_VA(this);
+
+  if (const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime()) {
+estimated_time = *maybe_wall_clock_time;
+return true;
+  }
+  return false;
+}
+
 bool SBTraceCursor::IsValid() const {
   LLDB_INSTRUMENT_VA(this);
 
Index: lldb/include/lldb/API/SBTraceCursor.h
===
--- lldb/include/lldb/API/SBTraceCursor.h
+++ lldb/include/lldb/API/SBTraceCursor.h
@@ -169,6 +169,19 @@
   ///not available for the current item.
   lldb::cpu_id_t GetCPU() const;
 
+  /// Get the approximate wall clock time in nanoseconds at which the current
+  /// trace item was executed. Each trace plug-in has a different definition for
+  /// what time 0 means.
+  ///
+  /// \param[out] estimated_time
+  /// The trace item's estimtated timestamp, if available.
+  ///
+  /// \return
+  ///   true if the current trace item has an estimated timestamp associated
+  ///   with it, false otherwise.
+  bool GetWallClockTime(double &estimated_time) const;
+  /// \}
+
   bool IsValid() const;
 
   explicit operator bool() const;
Index: lldb/bindings/interface/SBTraceCursor.i
===
--- lldb/bindings/interface/SBTraceCursor.i
+++ lldb/bindings/interface/SBTraceCursor.i
@@ -51,6 +51,8 @@
 
   lldb::cpu_id_t GetCPU() const;
 
+  bool GetWallClockTime(double &estimated_time) const;
+
   bool IsValid() const;
 
   explicit operator bool() const;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 3b70e8b - Move the second instance of TestUniqueTypes.py to a unique file

2022-11-08 Thread Jim Ingham via lldb-commits 

Author: Jim Ingham
Date: 2022-11-08T16:59:02-08:00
New Revision: 3b70e8b012f0f51eb0e2abeb38ee6c8914b90f9c

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

LOG: Move the second instance of TestUniqueTypes.py to a unique file
name.  lldb-dotest.py errors out if two tests have the same filename.

Added: 
lldb/test/API/lang/cpp/unique-types/TestUniqueTypes2.py

Modified: 


Removed: 
lldb/test/API/lang/cpp/unique-types/TestUniqueTypes.py



diff  --git a/lldb/test/API/lang/cpp/unique-types/TestUniqueTypes.py 
b/lldb/test/API/lang/cpp/unique-types/TestUniqueTypes2.py
similarity index 100%
rename from lldb/test/API/lang/cpp/unique-types/TestUniqueTypes.py
rename to lldb/test/API/lang/cpp/unique-types/TestUniqueTypes2.py



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

[Lldb-commits] [PATCH] D137682: Change IRMemoryMap's last-resort magic address to an inaddressable address so it doesn't conflict

2022-11-08 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda created this revision.
jasonmolenda added reviewers: JDevlieghere, jingham.
jasonmolenda added a project: LLDB.
Herald added a subscriber: Michael137.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

When the IRInterpreter runs, and needs to store things in inferior memory, and 
it cannot allocate a memory region in the inferior, IRMemoryMap::FindSpace will 
create a buffer in lldb memory with a target address, and that will be used. If 
the target address overlaps with the address of an actual memory region in the 
inferior process, attempts to access the genuine memory there in IRInterpreted 
expressions will instead access lldb's local scratch memory.  The Process may 
not support any way to query memory regions, so the solution Sean has picked in 
years past was to pick an especially unlikely address.

Obviously, we have a program that puts things at this address.

Given that 64-bit systems do not genuinely have 64 bits of address space, we 
can pick a better address.  This patch changes the current unlikely address, 
0x, to 0xdead0fff.  Still in high memory, but because 
bit 62 is now 0 with this address, it cannot overlap with an actual virtual 
address unless there was a system alling for 62 bits of addressable address 
space.  This is unlikely to happen soon.

I wrote a test which loads a binary into lldb, and slides the DATA segment 
around to different addresses, testing that the global variable in that DATA 
segment can still be read, as well as testing that if the DATA segment is slid 
to this impossible address, we get the incorrect value for the global.

This patch also fixes an incorrect way of getting the Target in 
`DWARFExpression::Evaluate` which gets a StackFrame from the ExecutionContext 
passed in, and uses that to get the Target, instead of simply using the Target, 
when calculating the load address of the address just read.  This had the 
effect of always returning the original file address, even when I loaded the 
segment at different addresses, in this test where there's no running process.  
I looked through DWARFExpression briefly and didn't see any other code doing 
this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137682

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/source/Expression/IRMemoryMap.cpp
  lldb/test/API/lang/c/high-mem-global/Makefile
  lldb/test/API/lang/c/high-mem-global/TestHighMemGlobal.py
  lldb/test/API/lang/c/high-mem-global/main.c

Index: lldb/test/API/lang/c/high-mem-global/main.c
===
--- /dev/null
+++ lldb/test/API/lang/c/high-mem-global/main.c
@@ -0,0 +1,9 @@
+
+struct mystruct {
+  int c, d, e;
+} global = {1, 2, 3};
+
+int main ()
+{
+  return global.c; // break here
+}
Index: lldb/test/API/lang/c/high-mem-global/TestHighMemGlobal.py
===
--- /dev/null
+++ lldb/test/API/lang/c/high-mem-global/TestHighMemGlobal.py
@@ -0,0 +1,52 @@
+"""Look that lldb can display a global loaded in high memory at an addressable address."""
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
+
+class TestHighMemGlobal(TestBase):
+
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipUnlessDarwin  # hardcoding of __DATA segment name
+def test_command_line(self):
+"""Test that we can display a global variable loaded in high memory."""
+self.build()
+
+exe = self.getBuildArtifact("a.out")
+err = lldb.SBError()
+
+target = self.dbg.CreateTarget(exe, '', '', False, err)
+self.assertTrue(target.IsValid())
+module = target.GetModuleAtIndex(0)
+self.assertTrue(module.IsValid())
+data_segment = module.FindSection("__DATA")
+self.assertTrue(data_segment.IsValid())
+err.Clear()
+
+self.expect("p global.c", substrs=[' = 1'])
+
+err = target.SetSectionLoadAddress(data_segment, 0x)
+self.assertTrue(err.Success())
+self.expect("p global.c", substrs=[' = 1'])
+
+err = target.SetSectionLoadAddress(data_segment, 0x08814000)
+self.assertTrue(err.Success())
+self.expect("p global.c", substrs=[' = 1'])
+
+# This is an address in IRMemoryMap::FindSpace where it has an 
+# lldb-side buffer of memory that's used in IR interpreters when
+# memory cannot be allocated in the inferior / functions cannot
+# be jitted.
+err = target.SetSectionLoadAddress(data_segment, 0xdead0fff)
+self.assertTrue(err.Success())
+
+# The global variable `global` is now overlayed by this 
+# IRMemoryMap special buffer, and now we cannot see the variable.
+# If the IRInterpreter some day is able to distinguish between
+

[Lldb-commits] [PATCH] D137682: Change IRMemoryMap's last-resort magic address to an inaddressable address so it doesn't conflict

2022-11-08 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added a comment.

Probably the laziest bit is at the end of the test case where I want to test 
that the variable isn't '1', I didn't use a substring because it's 
uninitialized memory could come back as '10' or something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137682

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

[Lldb-commits] [PATCH] D137684: Make sure we are stopped before we try to install functions into the target

2022-11-08 Thread Jim Ingham via Phabricator via lldb-commits 
jingham created this revision.
jingham added reviewers: JDevlieghere, jasonmolenda, labath, clayborg.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

It's actually fairly hard to get lldb to do this, since most ways you can 
access the expression evaluator already assert that you have to be stopped.  
But if you are unlucky you might end up doing that, for instance 
SBTarget::FindFirstType when called the first time for something that requires 
types from the ObjC runtime metadata can end up installing and calling the 
"metadata introspection" UtilityFunction.  This fails, mostly in harmless ways, 
but if you happen to hit a breakpoint and stop while in the middle of making up 
the expression, you can mess up internal state.  I really don't think it's 
worth the effort to make this work.  It's easier to reason about if we just 
assert that you have to be stopped if you want to cons up a UtilityExpression.

The only subtle bit of this patch is that if code is running on the Internal 
State Thread, it is governed by the m_private_state, not the m_public_state.  
So I made Process::GetState behave the same way the target API lock does, with 
a separate entity for the private & public running code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137684

Files:
  lldb/source/Expression/FunctionCaller.cpp
  lldb/source/Expression/UserExpression.cpp
  lldb/source/Expression/UtilityFunction.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -1293,7 +1293,10 @@
 }
 
 StateType Process::GetState() {
-  return m_public_state.GetValue();
+  if (CurrentThreadIsPrivateStateThread())
+return m_private_state.GetValue();
+  else
+return m_public_state.GetValue();
 }
 
 void Process::SetPublicState(StateType new_state, bool restarted) {
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
@@ -99,6 +99,12 @@
 return false;
   }
 
+  // Since we might need to call allocate memory and maybe call code to make
+  // the caller, we need to be stopped.
+  if (process->GetState() != lldb::eStateStopped) {
+diagnostic_manager.PutString(eDiagnosticSeverityError, "process running");
+return false;
+  }
   //
   // Parse the expression
   //
Index: lldb/source/Expression/UtilityFunction.cpp
===
--- lldb/source/Expression/UtilityFunction.cpp
+++ lldb/source/Expression/UtilityFunction.cpp
@@ -64,6 +64,13 @@
 error.SetErrorString("Can't make a function caller without a process.");
 return nullptr;
   }
+  // Since we might need to call allocate memory and maybe call code to make
+  // the caller, we need to be stopped.
+  if (process_sp->GetState() != lldb::eStateStopped) {
+error.SetErrorString("Can't make a function caller while the process is " 
+ "running");
+return nullptr;
+  }
 
   Address impl_code_address;
   impl_code_address.SetOffset(StartAddress());
Index: lldb/source/Expression/UserExpression.cpp
===
--- lldb/source/Expression/UserExpression.cpp
+++ lldb/source/Expression/UserExpression.cpp
@@ -194,16 +194,22 @@
 
   Process *process = exe_ctx.GetProcessPtr();
 
-  if (process == nullptr || process->GetState() != lldb::eStateStopped) {
-if (execution_policy == eExecutionPolicyAlways) {
-  LLDB_LOG(log, "== [UserExpression::Evaluate] Expression may not run, but "
-"is not constant ==");
+  if (process == nullptr && execution_policy == eExecutionPolicyAlways) {
+LLDB_LOG(log, "== [UserExpression::Evaluate] No process, but the policy is "
+  "eExecutionPolicyAlways");
 
-  error.SetErrorString("expression needed to run but couldn't");
+error.SetErrorString("expression needed to run but couldn't: no process");
 
-  return execution_results;
-}
+return execution_results;
   }
+  // Since we might need to call allocate memory and maybe call code to make
+  // the caller, we need to be stopped.
+  if (process != nullptr && process->GetState() != lldb::eStateStopped) {
+error.SetErrorString("Can't make a function caller while the process is " 
+  "running");
+return execution_results;
+  }
+
 
   // Explicitly force the IR interpreter to evaluate the expression when the
   // there is no process that supports ru

[Lldb-commits] [PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

2022-11-08 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie added a subscriber: aaron.ballman.
dblaikie added a comment.

@aaron.ballman does this seem OK? (this was based on my suggestion in the 
related review linked in the description)

It probably needs tests in clang too - not sure if there's an opportunity to 
use a unit test to simplify access to the printing policy & exercise this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137583

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

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

2022-11-08 Thread walter erquinigo via Phabricator via lldb-commits 
wallace accepted this revision.
wallace added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/API/SBTraceCursor.cpp:127-131
+double SBTraceCursor::GetWallClockTime() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;

hawkinsw wrote:
> wallace wrote:
> > we don't have optionals in the SB bridge, so you could do the following
> > 
> >   bool SBTraceCursor::GetWallClockTime(double &time) {
> > if (const auto &maybe_wall_clock_time = 
> > m_opaque_sp->GetWallClockTime()) {
> >   time = *maybe_wall_clock_time;
> >   return true;
> > }
> > return false;
> >   }
> Not that it matters, but *that* was going to be my other suggestion! @wallace 
> is smarter than I am!
<3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

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