[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

2021-06-18 Thread walter erquinigo via Phabricator via lldb-commits 
wallace added inline comments.



Comment at: lldb/include/lldb/Target/Trace.h:367
 
+  /// We use 1 as a default stop id for post portem processes.
+  uint32_t m_stop_id = 1;

clayborg wrote:
> We really shouldn't have to set this manually. Probably best to start it at 
> zero. It might even be better to create a LLDB_INVALID_STOPID define next to 
> the LLDB_INVALID_ADDRESS define and use it. 
this is a pretty good idea



Comment at: lldb/source/Target/Trace.cpp:478
+uint32_t Trace::GetStopID() {
+  RefreshLiveProcessState();
+  return m_stop_id;

clayborg wrote:
> I would just allow the core file to set the process state like any other and 
> start the m_stop_id at zero, the invalid value.
makes sense, i'll do the core file stuff in a different diff 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422

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

[Lldb-commits] [PATCH] D104578: Clarify the "env" launch configuration setting.

2021-06-18 Thread walter erquinigo via Phabricator via lldb-commits 
wallace accepted this revision.
wallace added a comment.
This revision is now accepted and ready to land.

thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104578

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

[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

2021-06-18 Thread walter erquinigo via Phabricator via lldb-commits 
wallace updated this revision to Diff 353135.
wallace marked 3 inline comments as done.
wallace added a comment.

per comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-defines.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceCursor.cpp

Index: lldb/source/Target/TraceCursor.cpp
===
--- /dev/null
+++ lldb/source/Target/TraceCursor.cpp
@@ -0,0 +1,15 @@
+//===-- TraceCursor.cpp -*- C++ -*-===//
+//
+// 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/Target/TraceCursor.h"
+
+#include "lldb/Target/Trace.h"
+
+using namespace lldb_private;
+
+bool TraceCursor::IsStale() { return m_stop_id != m_trace_sp->GetStopID(); }
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -473,3 +473,8 @@
 
   DoRefreshLiveProcessState(std::move(live_process_state));
 }
+
+uint32_t Trace::GetStopID() {
+  RefreshLiveProcessState();
+  return m_stop_id;
+}
Index: lldb/source/Target/CMakeLists.txt
===
--- lldb/source/Target/CMakeLists.txt
+++ lldb/source/Target/CMakeLists.txt
@@ -68,6 +68,7 @@
   ThreadSpec.cpp
   ThreadPostMortemTrace.cpp
   Trace.cpp
+  TraceCursor.cpp
   TraceSessionFileParser.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
Index: lldb/include/lldb/lldb-forward.h
===
--- lldb/include/lldb/lldb-forward.h
+++ lldb/include/lldb/lldb-forward.h
@@ -229,6 +229,7 @@
 class ThreadSpec;
 class ThreadPostMortemTrace;
 class Trace;
+class TraceCursor;
 class TraceSessionFileParser;
 class Type;
 class TypeAndOrName;
@@ -441,6 +442,7 @@
 typedef std::weak_ptr ThreadPlanWP;
 typedef std::shared_ptr ThreadPlanTracerSP;
 typedef std::shared_ptr TraceSP;
+typedef std::unique_ptr TraceCursorUP;
 typedef std::shared_ptr TypeSP;
 typedef std::weak_ptr TypeWP;
 typedef std::shared_ptr TypeCategoryImplSP;
Index: lldb/include/lldb/lldb-enumerations.h
===
--- lldb/include/lldb/lldb-enumerations.h
+++ lldb/include/lldb/lldb-enumerations.h
@@ -958,6 +958,25 @@
   eExpressionEvaluationComplete
 };
 
+/// Architecture-agnostic categorization of instructions for traversing the
+/// control flow of a trace.
+///
+/// A single instruction can match one or more of these categories.
+FLAGS_ENUM(TraceInstructionControlFlowType){
+/// Any instruction.
+eTraceInstructionControlFlowTypeInstruction = (1u << 1),
+/// A conditional or unconditional branch/jump.
+eTraceInstructionControlFlowTypeBranch = (1u << 2),
+/// A conditional or unconditional branch/jump that changed
+/// the control flow of the program.
+eTraceInstructionControlFlowTypeTakenBranch = (1u << 3),
+/// A call to a function.
+eTraceInstructionControlFlowTypeCall = (1u << 4),
+/// A return from a function.
+eTraceInstructionControlFlowTypeReturn = (1u << 5)};
+
+LLDB_MARK_AS_BITMASK_ENUM(TraceInstructionControlFlowType)
+
 /// Watchpoint Kind.
 ///
 /// Indicates what types of events cause the watchpoint to fire. Used by Native
Index: lldb/include/lldb/lldb-defines.h
===
--- lldb/include/lldb/lldb-defines.h
+++ lldb/include/lldb/lldb-defines.h
@@ -82,6 +82,7 @@
 #define LLDB_REGNUM_GENERIC_ARG8   \
   12 // The register that would contain pointer size or less argument 8 (if any)
 /// Invalid value definitions
+#define LLDB_INVALID_STOP_ID 0
 #define LLDB_INVALID_ADDRESS UINT64_MAX
 #define LLDB_INVALID_INDEX32 UINT32_MAX
 #define LLDB_INVALID_IVAR_OFFSET UINT32_MAX
Index: lldb/include/lldb/Target/TraceCursor.h
===
--- /dev/null
+++ lldb/include/lldb/Target/TraceCursor.h
@@ -0,0 +1,139 @@
+//===-- TraceCursor.h ---*- C++ -*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef

[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just a few nits!




Comment at: lldb/include/lldb/Target/Trace.h:292-293
+  /// \return
+  /// The stop ID of the live process being traced, or \b 1 if this is a
+  /// post-mortem trace session.
+  uint32_t GetStopID();

No need to differentiate between line and post mortem stop IDs. Just say "The 
stop ID of the process being traced, or an invalid stop ID of zero if the trace 
is in an error state."



Comment at: lldb/include/lldb/Target/Trace.h:367
 
+  /// We use 1 as a default stop id for post portem processes.
+  uint32_t m_stop_id = 1;

We really shouldn't have to set this manually. Probably best to start it at 
zero. It might even be better to create a LLDB_INVALID_STOPID define next to 
the LLDB_INVALID_ADDRESS define and use it. 



Comment at: lldb/source/Target/Trace.cpp:478
+uint32_t Trace::GetStopID() {
+  RefreshLiveProcessState();
+  return m_stop_id;

I would just allow the core file to set the process state like any other and 
start the m_stop_id at zero, the invalid value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422

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

[Lldb-commits] [PATCH] D104578: Clarify the "env" launch configuration setting.

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg created this revision.
clayborg added reviewers: labath, aprantl, JDevlieghere, wallace.
clayborg requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

A few users recently were trying to set environment values when using 
lldb-vscode and were unsure of the format of the "env" launch configuration 
setting. Clarify the exact format as when users add the "env" launch config 
setting, they can see this help string in the IDE.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104578

Files:
  lldb/tools/lldb-vscode/package.json


Index: lldb/tools/lldb-vscode/package.json
===
--- lldb/tools/lldb-vscode/package.json
+++ lldb/tools/lldb-vscode/package.json
@@ -102,7 +102,7 @@
},
"env": {
"type": "array",
-   "description": 
"Additional environment variables.",
+   "description": 
"Additional environment variables to set when launching the program. This is an 
array of strings that contains the variable name followed by an optional '=' 
character and the environment variable's value. Example:  [\"FOO=BAR\", 
\"BAZ\"]",
"default": []
},
"stopOnEntry": {


Index: lldb/tools/lldb-vscode/package.json
===
--- lldb/tools/lldb-vscode/package.json
+++ lldb/tools/lldb-vscode/package.json
@@ -102,7 +102,7 @@
 			},
 			"env": {
 "type": "array",
-"description": "Additional environment variables.",
+"description": "Additional environment variables to set when launching the program. This is an array of strings that contains the variable name followed by an optional '=' character and the environment variable's value. Example:  [\"FOO=BAR\", \"BAZ\"]",
 "default": []
 			},
 			"stopOnEntry": {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D104488: Create synthetic symbol names on demand to improve memory consumption and startup times.

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg updated this revision to Diff 353120.
clayborg added a comment.

Fixed a case where the accessor was being used twice in the same function and 
fixed a typo in a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104488

Files:
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/include/lldb/Symbol/Symbol.h
  lldb/include/lldb/Symbol/Symtab.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Symbol/ObjectFile.cpp
  lldb/source/Symbol/Symbol.cpp
  lldb/source/Symbol/Symtab.cpp

Index: lldb/source/Symbol/Symtab.cpp
===
--- lldb/source/Symbol/Symtab.cpp
+++ lldb/source/Symbol/Symtab.cpp
@@ -291,7 +291,7 @@
   // the trampoline symbols to be searchable by name we can remove this and
   // then possibly add a new bool to any of the Symtab functions that
   // lookup symbols by name to indicate if they want trampolines.
-  if (symbol->IsTrampoline())
+  if (symbol->IsTrampoline() || symbol->IsSynthetic())
 continue;
 
   // If the symbol's name string matched a Mangled::ManglingScheme, it is
@@ -614,6 +614,36 @@
   }
 }
 
+uint32_t Symtab::GetNameIndexes(ConstString symbol_name,
+std::vector ) {
+  auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  const uint32_t count = name_to_index.GetValues(symbol_name, indexes);
+  if (count)
+return count;
+  // Synthetic symbol names are not added to the name indexes, but they start
+  // with a prefix and end with a the symbol UserID, so allow users to find
+  // these without having to add them to the name indexes. These queries will
+  // not happen very often since the names don't mean anything, so performance
+  // is not paramount in this case.
+  llvm::StringRef name = symbol_name.GetStringRef();
+  // String the synthetic prefix if the name starts with it.
+  if (!name.consume_front(Symbol::GetSyntheticSymbolPrefix()))
+return 0; // Not a synthetic symbol name
+
+  // Extract the user ID from the symbol name
+  user_id_t uid = 0;
+  if (getAsUnsignedInteger(name, 10 /* Radix */, uid))
+return 0; // Failed to extract the user ID as an integer
+  Symbol *symbol = FindSymbolByID(uid);
+  if (symbol == nullptr)
+return 0;
+  const uint32_t symbol_idx = GetIndexForSymbol(symbol);
+  if (symbol_idx == UINT32_MAX)
+return 0;
+  indexes.push_back(symbol_idx);
+  return 1;
+}
+
 uint32_t Symtab::AppendSymbolIndexesWithName(ConstString symbol_name,
  std::vector ) {
   std::lock_guard guard(m_mutex);
@@ -623,8 +653,7 @@
 if (!m_name_indexes_computed)
   InitNameIndexes();
 
-auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
-return name_to_index.GetValues(symbol_name, indexes);
+return GetNameIndexes(symbol_name, indexes);
   }
   return 0;
 }
@@ -641,10 +670,9 @@
 if (!m_name_indexes_computed)
   InitNameIndexes();
 
-auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
 std::vector all_name_indexes;
 const size_t name_match_count =
-name_to_index.GetValues(symbol_name, all_name_indexes);
+GetNameIndexes(symbol_name, all_name_indexes);
 for (size_t i = 0; i < name_match_count; ++i) {
   if (CheckSymbolAtIndex(all_name_indexes[i], symbol_debug_type,
  symbol_visibility))
Index: lldb/source/Symbol/Symbol.cpp
===
--- lldb/source/Symbol/Symbol.cpp
+++ lldb/source/Symbol/Symbol.cpp
@@ -56,8 +56,8 @@
   m_size_is_synthesized(false),
   m_size_is_valid(size_is_valid || range.GetByteSize() > 0),
   m_demangled_is_synthesized(false),
-  m_contains_linker_annotations(contains_linker_annotations), 
-  m_is_weak(false), m_type(type), m_mangled(mangled), m_addr_range(range), 
+  m_contains_linker_annotations(contains_linker_annotations),
+  m_is_weak(false), m_type(type), m_mangled(mangled), m_addr_range(range),
   m_flags(flags) {}
 
 Symbol::Symbol(const Symbol )
@@ -119,7 +119,7 @@
 }
 
 ConstString Symbol::GetDisplayName() const {
-  return m_mangled.GetDisplayDemangledName();
+  return GetMangled().GetDisplayDemangledName();
 }
 
 ConstString Symbol::GetReExportedSymbolName() const {
@@ -202,7 +202,7 @@
   s->Printf(", value = 0x%16.16" PRIx64,
 m_addr_range.GetBaseAddress().GetOffset());
   }
-  ConstString demangled = m_mangled.GetDemangledName();
+  ConstString demangled = GetMangled().GetDemangledName();
   if (demangled)
 s->Printf(", name=\"%s\"", demangled.AsCString());
   if (m_mangled.GetMangledName())
@@ -218,7 +218,7 @@
   // Make sure the size of the symbol is up to date before dumping
   GetByteSize();
 
-  ConstString name =

[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

2021-06-18 Thread walter erquinigo via Phabricator via lldb-commits 
wallace updated this revision to Diff 353117.
wallace marked an inline comment as done.
wallace added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceCursor.cpp

Index: lldb/source/Target/TraceCursor.cpp
===
--- /dev/null
+++ lldb/source/Target/TraceCursor.cpp
@@ -0,0 +1,15 @@
+//===-- TraceCursor.cpp -*- C++ -*-===//
+//
+// 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/Target/TraceCursor.h"
+
+#include "lldb/Target/Trace.h"
+
+using namespace lldb_private;
+
+bool TraceCursor::IsStale() { return m_stop_id != m_trace_sp->GetStopID(); }
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -473,3 +473,8 @@
 
   DoRefreshLiveProcessState(std::move(live_process_state));
 }
+
+uint32_t Trace::GetStopID() {
+  RefreshLiveProcessState();
+  return m_stop_id;
+}
Index: lldb/source/Target/CMakeLists.txt
===
--- lldb/source/Target/CMakeLists.txt
+++ lldb/source/Target/CMakeLists.txt
@@ -68,6 +68,7 @@
   ThreadSpec.cpp
   ThreadPostMortemTrace.cpp
   Trace.cpp
+  TraceCursor.cpp
   TraceSessionFileParser.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
Index: lldb/include/lldb/lldb-forward.h
===
--- lldb/include/lldb/lldb-forward.h
+++ lldb/include/lldb/lldb-forward.h
@@ -229,6 +229,7 @@
 class ThreadSpec;
 class ThreadPostMortemTrace;
 class Trace;
+class TraceCursor;
 class TraceSessionFileParser;
 class Type;
 class TypeAndOrName;
@@ -441,6 +442,7 @@
 typedef std::weak_ptr ThreadPlanWP;
 typedef std::shared_ptr ThreadPlanTracerSP;
 typedef std::shared_ptr TraceSP;
+typedef std::unique_ptr TraceCursorUP;
 typedef std::shared_ptr TypeSP;
 typedef std::weak_ptr TypeWP;
 typedef std::shared_ptr TypeCategoryImplSP;
Index: lldb/include/lldb/lldb-enumerations.h
===
--- lldb/include/lldb/lldb-enumerations.h
+++ lldb/include/lldb/lldb-enumerations.h
@@ -958,6 +958,25 @@
   eExpressionEvaluationComplete
 };
 
+/// Architecture-agnostic categorization of instructions for traversing the
+/// control flow of a trace.
+///
+/// A single instruction can match one or more of these categories.
+FLAGS_ENUM(TraceInstructionControlFlowType){
+/// Any instruction.
+eTraceInstructionControlFlowTypeInstruction = (1u << 1),
+/// A conditional or unconditional branch/jump.
+eTraceInstructionControlFlowTypeBranch = (1u << 2),
+/// A conditional or unconditional branch/jump that changed
+/// the control flow of the program.
+eTraceInstructionControlFlowTypeTakenBranch = (1u << 3),
+/// A call to a function.
+eTraceInstructionControlFlowTypeCall = (1u << 4),
+/// A return from a function.
+eTraceInstructionControlFlowTypeReturn = (1u << 5)};
+
+LLDB_MARK_AS_BITMASK_ENUM(TraceInstructionControlFlowType)
+
 /// Watchpoint Kind.
 ///
 /// Indicates what types of events cause the watchpoint to fire. Used by Native
Index: lldb/include/lldb/Target/TraceCursor.h
===
--- /dev/null
+++ lldb/include/lldb/Target/TraceCursor.h
@@ -0,0 +1,139 @@
+//===-- TraceCursor.h ---*- C++ -*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef LLDB_TARGET_TRACE_CURSOR_H
+#define LLDB_TARGET_TRACE_CURSOR_H
+
+#include "lldb/lldb-private.h"
+
+namespace lldb_private {
+
+/// Class used for iterating over the instructions of a thread's trace.
+///
+/// This class attempts to be a generic interface for accessing the instructions
+/// of the trace so that each Trace plug-in can reconstruct, represent and store
+/// the instruction data in an flexible way that is efficient for the given
+/// technology.
+///
+/// Live processes:
+///  In the case of a live process trace, an instance of a \a TraceCursor should
+///  point to the trace at the moment

[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

2021-06-18 Thread walter erquinigo via Phabricator via lldb-commits 
wallace marked 2 inline comments as done.
wallace added inline comments.



Comment at: lldb/include/lldb/Target/Trace.h:292
+  /// \return
+  /// The stop ID of the live process being traced, or \b 0 if this is a
+  /// post-mortem trace session.

clayborg wrote:
> zero is invalid. We should just have post mortem use a valid stop ID like 1
ah, good idea



Comment at: lldb/source/Target/Trace.cpp:478
+uint32_t Trace::GetStopID() {
+  RefreshLiveProcessState();
+  return m_stop_id;

clayborg wrote:
> We should make sure for core file that the m_stop_id is set correctly somehow
for a corefile the stop id is now set as 1 in Trace.h, which should be fine. 
RefreshLiveProcessState does nothing for a corefile, btw



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422

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

[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

In D104395#2828065 , @OmarEmaraDev 
wrote:

> @clayborg I tried implementing scrolling mechanisms as suggested. My first 
> trial essentially defined a "visible area" rectangle which gets updated with 
> every time the selection changes, then when it comes to drawing, each field 
> checks if it is completely contained in the visible area and draws itself 
> with an offset that we get from from the visible area origin. This worked, 
> but fields that spans multiple columns can completely disappear leaving 
> mostly no fields on the window, so it worked in most cases, but not all. My 
> second trial was about drawing to an ncurses pad that is large enough to fit 
> all contents, then the window is refreshed from that pad. This used manual 
> ncurses calls because we don't support pads at the moment, so I scratched 
> that for now. I think support for pads would be good for those kind of 
> applications in the future. I plan to work on a proposal that would include 
> support for pads and lightweight subwindows, I will detail that later.

Yeah, that is why I was suggesting that we use the list view for fields as it 
makes scrolling much easier as I really like your graphical approach that you 
have, it just means scrolling logic is much more complicated.

> I eventually opted to create "pages" for the forms. Basically, during form 
> construction, if the number of fields is large and can't fit in the window, 
> the user can call `NewPage()` to create a new page, all fields after that 
> call gets added to the new page. As the user selects fields, the form can 
> scroll to different pages. This is similar to the ncurses form scrolling 
> functionality. Visual indicator for the current pages and the active page is 
> added to the form if there is more than one page. I hope this solution is 
> satisfactory.

That will work as long as you are wanting to implement all of the required 
features. I personally like to keep things simple, but I'm not against seeing a 
nicer solution.

It it were me I would probably make a ListDelegate that can manage a list of 
delegates where each field would be a delegate and each field would display on 
one or more line kind of like I detailed in ASCII art. This would make 
scrolling really easy to implement in an abtract ListDelegate kind of like how 
we did for the TreeDelegate. For example the "Path" for the executable would 
take up one line in the list window part of the form. A "Arguments" field for a 
Process launch dialog could take up one or more lines depending on how many 
entries the user typed. Same things for the environment variables field we will 
need. So think about all of the fields types we will need and think about how 
easy they all will be to implement as we add new things. The fields I can think 
of include:

- file path (for either a file or directory where constructor of the field 
specifies which one and also constructor would say if the path needs to exist 
(which would be true for an executable for a target, but not for a path 
substitution path)
  - target executable as a file that must exist
  - current working directory as a directory that must exist
  - script path to a file that must exist
- list field (for things like arguments or environment variables where the user 
specifies one or more items. Each item in the list could use a FieldDelegate so 
the list can be a list of anything)
  - arguments
  - environment variables
- string field (with optional validation on the string when it loses focus 
(This could be used to implement the "file path" above where the user types 
something and then hits tab to select another field, but we don't let the focus 
change because the path isn't valid or it is expecting a file and a path to a 
directory is in the field at the moement)
- pulldown to select something. If this was done on a single line, then we can 
pop up a menu from the menu bar to allow selection
- boolean value
- integer
  - for pid selection for attach dialog where we need to validate the process 
ID that the user enters is valid
  - for  value for a setting that uses sockets

So just think about all the ways we will want to use each of these items and 
make sure you are willing to support all that is needed to make these happen. 
If you prefer the more GUI like approach, it will be good for you to understand 
how much work or support this will involve as new things are added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

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

[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

2021-06-18 Thread Omar Emara via Phabricator via lldb-commits 
OmarEmaraDev added a comment.

@clayborg I tried implementing scrolling mechanisms as suggested. My first 
trial essentially defined a "visible area" rectangle which gets updated with 
every time the selection changes, then when it comes to drawing, each field 
checks if it is completely contained in the visible area and draws itself with 
an offset that we get from from the visible area origin. This worked, but 
fields that spans multiple columns can completely disappear leaving mostly no 
fields on the window, so it worked in most cases, but not all. My second trial 
was about drawing to an ncurses pad that is large enough to fit all contents, 
then the window is refreshed from that pad. This used manual ncurses calls 
because we don't support pads at the moment, so I scratched that for now. I 
think support for pads would be good for those kind of applications in the 
future. I plan to work on a proposal that would include support for pads and 
lightweight subwindows, I will detail that later.

I eventually opted to create "pages" for the forms. Basically, during form 
construction, if the number of fields is large and can't fit in the window, the 
user can call `NewPage()` to create a new page, all fields after that call gets 
added to the new page. As the user selects fields, the form can scroll to 
different pages. This is similar to the ncurses form scrolling functionality. 
Visual indicator for the current pages and the active page is added to the form 
if there is more than one page. I hope this solution is satisfactory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

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

[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

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

[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

2021-06-18 Thread Omar Emara via Phabricator via lldb-commits 
OmarEmaraDev updated this revision to Diff 353087.
OmarEmaraDev added a comment.

- Add form pages.
- Handle review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -392,6 +392,12 @@
   void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
 ::box(m_window, v_char, h_char);
   }
+  void VerticalLine(int n, chtype v_char = ACS_VLINE) {
+::wvline(m_window, v_char, n);
+  }
+  void HorizontalLine(int n, chtype h_char = ACS_HLINE) {
+::whline(m_window, h_char, n);
+  }
   void Clear() { ::wclear(m_window); }
   void Erase() { ::werase(m_window); }
   Rect GetBounds() const {
@@ -674,6 +680,36 @@
   AttributeOff(attr);
   }
 
+  void DrawBox(const Rect , chtype v_char = ACS_VLINE,
+   chtype h_char = ACS_HLINE) {
+MoveCursor(bounds.origin.x, bounds.origin.y);
+VerticalLine(bounds.size.height);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_ULCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1, bounds.origin.y);
+VerticalLine(bounds.size.height);
+PutChar(ACS_URCORNER);
+
+MoveCursor(bounds.origin.x, bounds.origin.y + bounds.size.height - 1);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_LLCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1,
+   bounds.origin.y + bounds.size.height - 1);
+PutChar(ACS_LRCORNER);
+  }
+
+  void DrawTitledBox(const Rect , const char *title,
+ chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
+DrawBox(bounds, v_char, h_char);
+int title_offset = 2;
+MoveCursor(bounds.origin.x + title_offset, bounds.origin.y);
+PutChar('[');
+PutCString(title, bounds.size.width - title_offset);
+PutChar(']');
+  }
+
   virtual void Draw(bool force) {
 if (m_delegate_sp && m_delegate_sp->WindowDelegateDraw(*this, force))
   return;
@@ -869,6 +905,636 @@
   const Window =(const Window &) = delete;
 };
 
+/
+// Forms
+/
+
+class FieldDelegate {
+public:
+  virtual ~FieldDelegate() = default;
+
+  virtual Rect FieldDelegateGetBounds() = 0;
+
+  virtual void FieldDelegateDraw(Window , bool is_active) = 0;
+
+  virtual HandleCharResult FieldDelegateHandleChar(int key) {
+return eKeyNotHandled;
+  }
+};
+
+typedef std::shared_ptr FieldDelegateSP;
+
+class TextFieldDelegate : public FieldDelegate {
+public:
+  TextFieldDelegate(const char *label, int width, Point origin,
+const char *content)
+  : m_label(label), m_width(width), m_origin(origin), m_cursor_position(0),
+m_first_visibile_char(0) {
+if (content)
+  m_content = content;
+assert(m_width > 2);
+  }
+
+  // Get the bounding box of the field. The text field has a height of 3, 2
+  // lines for borders and 1 for the content.
+  Rect FieldDelegateGetBounds() override {
+return Rect(m_origin, Size(m_width, 3));
+  }
+
+  // Get the start X position of the content in window space, without the
+  // borders.
+  int GetX() { return m_origin.x + 1; }
+
+  // Get the start Y position of the content in window space, without the
+  // borders.
+  int GetY() { return m_origin.y + 1; }
+
+  // Get the effective width of the field, without the borders.
+  int GetEffectiveWidth() { return m_width - 2; }
+
+  // Get the cursor position in window space.
+  int GetCursorWindowXPosition() {
+return GetX() + m_cursor_position - m_first_visibile_char;
+  }
+
+  int GetContentLength() { return m_content.length(); }
+
+  void FieldDelegateDraw(Window , bool is_active) override {
+// Draw label box.
+window.DrawTitledBox(FieldDelegateGetBounds(), m_label.c_str());
+
+// Draw content.
+window.MoveCursor(GetX(), GetY());
+const char *text = m_content.c_str() + m_first_visibile_char;
+window.PutCString(text, GetEffectiveWidth());
+
+// Highlight the cursor.
+window.MoveCursor(GetCursorWindowXPosition(), GetY());
+if (is_active)
+  window.AttributeOn(A_REVERSE);
+if (m_cursor_position == GetContentLength())
+  // Cursor is past the last character. Highlight an empty space.
+  window.PutChar(' ');
+else
+  window.PutChar(m_content[m_cursor_position]);
+if (is_active)
+  window.AttributeOff(A_REVERSE);
+  }
+
+  // The cursor is allowed to move one character past the string.
+  // m_cursor_position is in range [0, GetContentLength()].
+  void MoveCursorRight() {
+if (m_cursor_position < GetContentLength())
+  m_cursor_position++;
+  }
+
+  void MoveCursorLeft() {
+if (m_cursor_position > 0)
+  m_cursor_position--;
+  }
+
+  // If the cursor moved past the last visible character, scroll right by one
+

[Lldb-commits] [PATCH] D104054: [lldb] Enable Rust v0 symbol demangling

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

Yes, good to go!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104054

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

[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Lets try the diamond character for the boolean stuff unless anyone has any 
objections. Maybe handle a few more keys for the boolean field as suggested in 
the comments. This will be good to go after these changes!




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:960
+
+  int GetContentLength() { return (int)m_content.length(); }
+

It is fine to leave cast to int if this is causing compiler warnings. Many 
things are integers in the curses API.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1104
+  }
+
+  // [X] Label  or [ ] Label

I like the diamond one personally. Looks nice and clean



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1121
+
+  HandleCharResult FieldDelegateHandleChar(int key) override {
+switch (key) {

maybe handle '1' to set m_content to true and '0' to set to false? Could also 
handle 't' for true and 'f' for false?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

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

[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

2021-06-18 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just one issue regarding the stop ID for post mortem. Once this is resolved, we 
will be good to go!




Comment at: lldb/include/lldb/Target/Trace.h:292
+  /// \return
+  /// The stop ID of the live process being traced, or \b 0 if this is a
+  /// post-mortem trace session.

zero is invalid. We should just have post mortem use a valid stop ID like 1



Comment at: lldb/include/lldb/Target/Trace.h:367
 
+  uint32_t m_stop_id = 0;
   /// Process traced by this object if doing live tracing. Otherwise it's null.

As per my comment above, we will need to set this to 1 for post mortem, or 
maybe the process in post mortem already has its stop ID set to 1 so this will 
fix itself?



Comment at: lldb/include/lldb/Target/TraceCursor.h:77
+  /// \return
+  /// \b true if the cursor moved, \b false otherwise.
+  virtual bool Next(lldb::TraceInstructionControlFlowType granularity =

Is the cursor in an error state in the case where we return false? It should be 
and we should document it



Comment at: lldb/source/Target/Trace.cpp:478
+uint32_t Trace::GetStopID() {
+  RefreshLiveProcessState();
+  return m_stop_id;

We should make sure for core file that the m_stop_id is set correctly somehow


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422

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

[Lldb-commits] [PATCH] D104054: [lldb] Enable Rust v0 symbol demangling

2021-06-18 Thread Alexander via Phabricator via lldb-commits 
asm added a comment.

@teemperor @clayborg Is this good to go? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104054

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

[Lldb-commits] [PATCH] D104380: [lldb] Set return object failed status even if error string is empty

2021-06-18 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie added a comment.

In D104380#2826465 , @DavidSpickett 
wrote:

> This change would have been part of https://reviews.llvm.org/D103701 if I had 
> realised that this was in fact how it worked. I separated it from the follow 
> ons because of that and to not bury 3 lines of change that apply to all 
> callers of these functions, in 300 lines of change to callers of 
> `SetStatus(eReturnStatusFailed)`. If I were bisecting a failure caused by 
> these changes I'd appreciate the separation but there's probably not much 
> difference.

Yeah, if it's a ton of call sites that are going to be updated - maybe this is 
simple enough that the interface semantic change + all the call sites being 
updated in one patch isn't the worst thing. But if it's more complicated I'd 
potentially do the interface semantic change/generalization + 1 caller to 
demonstrate the value, then the rest in subsequent cleanup commits.

Though sometimes I'll refactor an API (generalize it, split it into two 
functions, etc) without having changed any callers - perhaps because the new 
callers are coming in a subsequent more complicated patch.

No really hard rules here - just some thoughts. :)

> I agree about the assert, if you're not meant to pass empty strings we should 
> enforce that. I'll get something into review.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104380

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

[Lldb-commits] [PATCH] D104525: [lldb] Assert that CommandResultObject error messages are not empty

2021-06-18 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds good, if this already passes all tests/etc (ie: there actually aren't 
any callers passing empty strings/non-failed errors/etc)




Comment at: lldb/source/Interpreter/CommandReturnObject.cpp:122
   SetStatus(eReturnStatusFailed);
-  if (in_string.empty())
-return;
+  assert(in_string.size() && "Expected a non-empty error message");
   GetErrorStream() << in_string;

I'd prefer to write that as `!in_string.empty()` - it's a somewhat more direct 
statement of the intent (& good habit to use empty on containers rather than 
size != 0 - some containers can have more expensive size operations).

Similarly above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104525

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

[Lldb-commits] [lldb] f2c009d - [lldb-vscode] attempt to fix flakiness

2021-06-18 Thread Walter Erquinigo via lldb-commits 

Author: Walter Erquinigo
Date: 2021-06-18T10:12:46-07:00
New Revision: f2c009dbcfd11fd1e8941513dcf49fffe43565a1

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

LOG: [lldb-vscode] attempt to fix flakiness

There are many tests failing intermittently for lldb-vscode after
https://reviews.llvm.org/rGaa4685c0fb3aab5acb90be5fd3eb5ba8bf1e3211. I'm
unsure if this actually the culprit, so I'm softly removing that feature
to see if that fixes the issue.

Added: 


Modified: 
lldb/tools/lldb-vscode/lldb-vscode.cpp

Removed: 




diff  --git a/lldb/tools/lldb-vscode/lldb-vscode.cpp 
b/lldb/tools/lldb-vscode/lldb-vscode.cpp
index 4ac96166aa24..e61996678b0d 100644
--- a/lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ b/lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1411,7 +1411,8 @@ void request_modules(const llvm::json::Object ) {
 // }
 void request_initialize(const llvm::json::Object ) {
   g_vsc.debugger = lldb::SBDebugger::Create(true /*source_init_files*/);
-  g_vsc.progress_event_thread = std::thread(ProgressEventThreadFunction);
+  // TODO: reenable once confirmed that this doesn't make the buildbots flaky
+  // g_vsc.progress_event_thread = std::thread(ProgressEventThreadFunction);
 
   // Create an empty target right away since we might get breakpoint requests
   // before we are given an executable to launch in a "launch" request, or a



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

[Lldb-commits] [PATCH] D104525: [lldb] Assert that CommandResultObject error messages are not empty

2021-06-18 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett created this revision.
Herald added a subscriber: inglorion.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The intention is now that AppendError/SetError/AppendRawError only
be called with some message to show. This enforces that.

For SetError with a Status and a fallback string first assert
that the Status is a failure Status. Then it calls SetError(StringRef)
which checks the message is valid. (which could be the fallback
or the Status')


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104525

Files:
  lldb/source/Interpreter/CommandReturnObject.cpp


Index: lldb/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -99,24 +99,18 @@
 
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);
-  if (in_string.empty())
-return;
+  assert(in_string.size() && "Expected a non-empty error message");
   error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::SetError(const Status ,
const char *fallback_error_cstr) {
-  const char *error_cstr = error.AsCString();
-  if (error_cstr == nullptr)
-error_cstr = fallback_error_cstr;
-  SetError(error_cstr);
+  assert(error.Fail() && "Expected a failed Status");
+  SetError(error.AsCString(fallback_error_cstr));
 }
 
 void CommandReturnObject::SetError(llvm::StringRef error_str) {
   SetStatus(eReturnStatusFailed);
-  if (error_str.empty())
-return;
-
   AppendError(error_str);
 }
 
@@ -125,8 +119,7 @@
 
 void CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);
-  if (in_string.empty())
-return;
+  assert(in_string.size() && "Expected a non-empty error message");
   GetErrorStream() << in_string;
 }
 


Index: lldb/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -99,24 +99,18 @@
 
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);
-  if (in_string.empty())
-return;
+  assert(in_string.size() && "Expected a non-empty error message");
   error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::SetError(const Status ,
const char *fallback_error_cstr) {
-  const char *error_cstr = error.AsCString();
-  if (error_cstr == nullptr)
-error_cstr = fallback_error_cstr;
-  SetError(error_cstr);
+  assert(error.Fail() && "Expected a failed Status");
+  SetError(error.AsCString(fallback_error_cstr));
 }
 
 void CommandReturnObject::SetError(llvm::StringRef error_str) {
   SetStatus(eReturnStatusFailed);
-  if (error_str.empty())
-return;
-
   AppendError(error_str);
 }
 
@@ -125,8 +119,7 @@
 
 void CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);
-  if (in_string.empty())
-return;
+  assert(in_string.size() && "Expected a non-empty error message");
   GetErrorStream() << in_string;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

2021-06-18 Thread Levon Ter-Grigoryan via Phabricator via lldb-commits 
PatriosTheGreat updated this revision to Diff 352966.
PatriosTheGreat added a comment.

Thanks for the review feedback.
Looks like moving frame recognizer call to StackFrameList::GetFramesUpTo also 
fixes infinity recursion issue.

Regarding the zeroth frame issue: I'm actually using the lldb-server compiled 
with followed cmake command:
cmake -G Ninja -DLLVM_ENABLE_PROJECTS='lldb;clang;libcxx'  ../llvm

I see a performance improvement even on a local run, however it's not as huge:
In the sample I provided previously with this patch the execution time will be 
around 4 seconds instead of 6 seconds without it during the local run.

I see at the profiler that asking 0th frame triggers 
UnwindLLDB::DoGetFrameInfoAtIndex -> UnwindLLDB::AddFirstFrame -> 
UnwindLLDB::UpdateUnwindPlanForFirstFrameIfInvalid -> 
UnwindLLDB::AddOneMoreFrame -> UnwindLLDB::GetOneMoreFrame -> 
RegisterContextUnwind::RegisterContextUnwind -> 
RegisterContextUnwind::InitializeNonZerothFrame

- which is costly.

If I understand correctly the LLDB won't do any of this for background threads 
by default.

Could it be that accelerated stop-reply packets you said previously is turned 
off by default in lldb-server and I need to set a special setting to turn them 
on?


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

https://reviews.llvm.org/D103271

Files:
  lldb/include/lldb/Target/Thread.h
  lldb/source/Target/StackFrameList.cpp
  lldb/source/Target/Thread.cpp


Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -578,15 +578,9 @@
   return raw_stop_description;
 }
 
-void Thread::SelectMostRelevantFrame() {
+void Thread::SelectMostRelevantFrame(lldb::StackFrameSP first_frame) {
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD);
-
-  auto frames_list_sp = GetStackFrameList();
-
-  // Only the top frame should be recognized.
-  auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
-  auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
+  auto recognized_frame_sp = first_frame->GetRecognizedFrame();
 
   if (!recognized_frame_sp) {
 LLDB_LOG(log, "Frame #0 not recognized");
@@ -606,8 +600,6 @@
 void Thread::WillStop() {
   ThreadPlan *current_plan = GetCurrentPlan();
 
-  SelectMostRelevantFrame();
-
   // FIXME: I may decide to disallow threads with no plans.  In which
   // case this should go to an assert.
 
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -547,6 +547,9 @@
 curr_frame_address = next_frame_address;
   }
 }
+
+if (idx == 0)
+  m_thread.SelectMostRelevantFrame(unwind_frame_sp);
   } while (m_frames.size() - 1 < end_idx);
 
   // Don't try to merge till you've calculated all the frames in this stack.
Index: lldb/include/lldb/Target/Thread.h
===
--- lldb/include/lldb/Target/Thread.h
+++ lldb/include/lldb/Target/Thread.h
@@ -218,7 +218,7 @@
 
   virtual void RefreshStateAfterStop() = 0;
 
-  void SelectMostRelevantFrame();
+  void SelectMostRelevantFrame(lldb::StackFrameSP first_frame);
 
   std::string GetStopDescription();
 


Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -578,15 +578,9 @@
   return raw_stop_description;
 }
 
-void Thread::SelectMostRelevantFrame() {
+void Thread::SelectMostRelevantFrame(lldb::StackFrameSP first_frame) {
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD);
-
-  auto frames_list_sp = GetStackFrameList();
-
-  // Only the top frame should be recognized.
-  auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
-  auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
+  auto recognized_frame_sp = first_frame->GetRecognizedFrame();
 
   if (!recognized_frame_sp) {
 LLDB_LOG(log, "Frame #0 not recognized");
@@ -606,8 +600,6 @@
 void Thread::WillStop() {
   ThreadPlan *current_plan = GetCurrentPlan();
 
-  SelectMostRelevantFrame();
-
   // FIXME: I may decide to disallow threads with no plans.  In which
   // case this should go to an assert.
 
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -547,6 +547,9 @@
 curr_frame_address = next_frame_address;
   }
 }
+
+if (idx == 0)
+  m_thread.SelectMostRelevantFrame(unwind_frame_sp);
   } while (m_frames.size() - 1 < end_idx);
 
   // Don't try to merge till you've calculated all the frames in this stack.
Index: lldb/include/lldb/Target/Thread.h
===
---

[Lldb-commits] [PATCH] D103626: [lldb][AArch64] Remove non address bits from memory read arguments

2021-06-18 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett marked an inline comment as done.
DavidSpickett added inline comments.



Comment at: 
lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py:23
+def test_mte_regions(self):
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')

omjavaid wrote:
> This condition will always be true because we havent yet connected to remote 
> platform.
Runs fine for me using dotest:
```
$ ./bin/lldb-dotest --platform-name remote-linux <...> -p 
TestAArch64LinuxTaggedMemoryRead.py --arch aarch64
```

And this is how it's used elsewhere. Perhaps you're running the tests in a 
different way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103626

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

[Lldb-commits] [PATCH] D103626: [lldb][AArch64] Remove non address bits from memory read arguments

2021-06-18 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett updated this revision to Diff 352964.
DavidSpickett added a comment.

- Add missing include
- Rename test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103626

Files:
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/test/API/linux/aarch64/tagged_memory_read/Makefile
  
lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py
  lldb/test/API/linux/aarch64/tagged_memory_read/main.c

Index: lldb/test/API/linux/aarch64/tagged_memory_read/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_read/main.c
@@ -0,0 +1,18 @@
+#include 
+
+static char *set_non_address_bits(char *ptr, size_t tag) {
+  // Set top byte tag
+  ptr = (char *)((size_t)ptr | (tag << 56));
+  // Sign it
+  __asm__ __volatile__("pacdzb %0" : "=r"(ptr) : "r"(ptr));
+  return ptr;
+}
+
+int main(int argc, char const *argv[]) {
+  char buf[32];
+
+  char *ptr1 = set_non_address_bits(buf, 0x34);
+  char *ptr2 = set_non_address_bits(buf, 0x56);
+
+  return 0; // Set break point at this line.
+}
Index: lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py
@@ -0,0 +1,55 @@
+"""
+Test that "memory read" removes the top byte and pointer
+authentication signature of addresses before using them.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxTaggedMemoryReadTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+def test_mte_tagged_memory_read(self):
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Set break point at this line.'),
+num_expected_locations=1)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# If we do not remove non address bits, this can fail in two ways.
+# 1. We attempt to read much more than 16 bytes, probably more than
+#the default 1024 byte read size. (which is an error)
+# 2. We error because end address is < start address. Likely
+#because end's tag is < the beginning's tag.
+#
+# Each time we check that the printed line addresses do not include
+# either of the tags we set.
+tagged_addr_pattern = "0x(34|46)[0-9A-Fa-f]{14}:.*"
+self.expect("memory read ptr1 ptr2+16", patterns=[tagged_addr_pattern], matching=False)
+# Check that the stored previous end address is stripped
+self.expect("memory read", patterns=[tagged_addr_pattern], matching=False)
+self.expect("memory read ptr2 ptr1+16", patterns=[tagged_addr_pattern], matching=False)
+self.expect("memory read", patterns=[tagged_addr_pattern], matching=False)
Index: lldb/test/API/linux/aarch64/tagged_memory_read/Makefile
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_read/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -march=armv8.3-a
+
+include Makefile.rules
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -22,6 +22,7 @@
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/MemoryHistory.h"
 #include "lldb/Target/MemoryRegionInfo.h"
@@ -589,9 +590,16 @@
   return false;
 }
 
+ABISP abi = m_exe_ctx.GetProcessPtr()->GetABI();
+if (abi)
+  addr = abi->FixDataAddress(addr);
+
 if (argc == 2) {
   lldb::addr_t end_addr = OptionArgParser::ToAddress(
   _exe_ctx, command[1].ref(), LLDB_INVALID_ADDRESS, nullptr);
+  if (end_addr != LLDB_INVALID_ADDRESS && abi)
+end_addr = abi->FixDataAddress(end_addr);
+
   if (end_addr == LLDB_INVALID_ADDRESS) {
 result.AppendError("invalid end address expression.");

[Lldb-commits] [PATCH] D102757: [lldb] Remove non address bits when looking up memory regions

2021-06-18 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett added a comment.

Do you think this needs a test with a core file as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102757

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

[Lldb-commits] [PATCH] D102757: [lldb] Remove non address bits when looking up memory regions

2021-06-18 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett updated this revision to Diff 352959.
DavidSpickett added a comment.

- Move DoGetMemoryRegionInfo into protected section, GetMemoryRegionInfo is the 
public part.
- Fix a comment typo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102757

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.h
  lldb/source/Target/Process.cpp
  lldb/test/API/linux/aarch64/tagged_memory_region/Makefile
  
lldb/test/API/linux/aarch64/tagged_memory_region/TestAArch64LinuxTaggedMemoryRegion.py
  lldb/test/API/linux/aarch64/tagged_memory_region/main.c

Index: lldb/test/API/linux/aarch64/tagged_memory_region/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_region/main.c
@@ -0,0 +1,19 @@
+#include 
+#include 
+#include 
+#include 
+
+int main(int argc, char const *argv[]) {
+  void *the_page = mmap(0, sysconf(_SC_PAGESIZE), PROT_READ | PROT_EXEC,
+MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (the_page == MAP_FAILED)
+return 1;
+
+  // Put something in the top byte
+  the_page = (void *)((size_t)the_page | ((size_t)0x34 << 56));
+
+  // Then sign it
+  __asm__ __volatile__("pacdzb %0" : "=r"(the_page) : "r"(the_page));
+
+  return 0; // Set break point at this line.
+}
Index: lldb/test/API/linux/aarch64/tagged_memory_region/TestAArch64LinuxTaggedMemoryRegion.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_region/TestAArch64LinuxTaggedMemoryRegion.py
@@ -0,0 +1,44 @@
+"""
+Test that "memory region" lookup removes the top byte and pointer
+authentication signatures of the address given.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxTaggedMemoryRegionTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+def test_mte_regions(self):
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Set break point at this line.'),
+num_expected_locations=1)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# Despite the non address bits we should find a region
+self.expect("memory region the_page", patterns=[
+"\[0x[0-9A-Fa-f]+-0x[0-9A-Fa-f]+\) r-x"])
Index: lldb/test/API/linux/aarch64/tagged_memory_region/Makefile
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_region/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -march=armv8.3-a
+
+include Makefile.rules
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -5846,6 +5846,13 @@
   return retval;
 }
 
+Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
+MemoryRegionInfo _info) {
+  if (auto abi = GetABI())
+load_addr = abi->FixDataAddress(load_addr);
+  return DoGetMemoryRegionInfo(load_addr, range_info);
+}
+
 Status
 Process::GetMemoryRegions(lldb_private::MemoryRegionInfos _list) {
 
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.h
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.h
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.h
@@ -77,9 +77,6 @@
 
   ArchSpec GetArchitecture();
 
-  Status GetMemoryRegionInfo(lldb::addr_t load_addr,
- MemoryRegionInfo _info) override;
-
   Status GetMemoryRegions(

[Lldb-commits] [PATCH] D102757: [lldb] Remove non address bits when looking up memory regions

2021-06-18 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett updated this revision to Diff 352954.
DavidSpickett added a comment.

Remove TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102757

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.h
  lldb/source/Target/Process.cpp
  lldb/test/API/linux/aarch64/tagged_memory_region/Makefile
  
lldb/test/API/linux/aarch64/tagged_memory_region/TestAArch64LinuxTaggedMemoryRegion.py
  lldb/test/API/linux/aarch64/tagged_memory_region/main.c

Index: lldb/test/API/linux/aarch64/tagged_memory_region/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_region/main.c
@@ -0,0 +1,19 @@
+#include 
+#include 
+#include 
+#include 
+
+int main(int argc, char const *argv[]) {
+  void *the_page = mmap(0, sysconf(_SC_PAGESIZE), PROT_READ | PROT_EXEC,
+MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (the_page == MAP_FAILED)
+return 1;
+
+  // Put something in the top byte
+  the_page = (void *)((size_t)the_page | ((size_t)0x34 << 56));
+
+  // Then sign it
+  __asm__ __volatile__("pacdzb %0" : "=r"(the_page) : "r"(the_page));
+
+  return 0; // Set break point at this line.
+}
Index: lldb/test/API/linux/aarch64/tagged_memory_region/TestAArch64LinuxTaggedMemoryRegion.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_region/TestAArch64LinuxTaggedMemoryRegion.py
@@ -0,0 +1,44 @@
+"""
+Test that "memory region" lookup removes the top byte and pointer
+authentication signatures of the address given.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxTaggedMemoryRegionTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+def test_mte_regions(self):
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Set break point at this line.'),
+num_expected_locations=1)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# Despite the non address bits we should find a region
+self.expect("memory region the_page", patterns=[
+"\[0x[0-9A-Fa-f]+-0x[0-9A-Fa-f]+\) r-x"])
Index: lldb/test/API/linux/aarch64/tagged_memory_region/Makefile
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/tagged_memory_region/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -march=armv8.3-a
+
+include Makefile.rules
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -5846,6 +5846,13 @@
   return retval;
 }
 
+Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
+MemoryRegionInfo _info) {
+  if (auto abi = GetABI())
+load_addr = abi->FixDataAddress(load_addr);
+  return DoGetMemoryRegionInfo(load_addr, range_info);
+}
+
 Status
 Process::GetMemoryRegions(lldb_private::MemoryRegionInfos _list) {
 
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.h
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.h
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.h
@@ -77,8 +77,8 @@
 
   ArchSpec GetArchitecture();
 
-  Status GetMemoryRegionInfo(lldb::addr_t load_addr,
- MemoryRegionInfo _info) override;
+  Status DoGetMemoryRegionInfo(lldb::addr_t load_addr,
+   MemoryRegionInfo _info) override;
 
   Status GetMemoryRegions(

[Lldb-commits] [PATCH] D104380: [lldb] Set return object failed status even if error string is empty

2021-06-18 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett added a comment.

This change would have been part of https://reviews.llvm.org/D103701 if I had 
realised that this was in fact how it worked. I separated it from the follow 
ons because of that and to not bury 3 lines of change that apply to all callers 
of these functions, in 300 lines of change to callers of 
`SetStatus(eReturnStatusFailed)`. If I were bisecting a failure caused by these 
changes I'd appreciate the separation but there's probably not much difference.

I agree about the assert, if you're not meant to pass empty strings we should 
enforce that. I'll get something into review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104380

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