[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:102
+std::string *not_a_string = (std::string *) 0x0;
+touch_string(*not_a_string);
 return 0;

shafik wrote:
> This is undefined behavior and I AFAICT this won't pass a sanitized build, 
> amazingly even if I use `__attribute__((no_sanitize("address", 
> "undefined")))` : https://godbolt.org/z/4TGPbrYhq
Definitely UB, but the sanitized bot builds LLDB with the sanitizers, not the 
test cases, so this should be "fine". 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108228

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

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:102
+std::string *not_a_string = (std::string *) 0x0;
+touch_string(*not_a_string);
 return 0;

This is undefined behavior and I AFAICT this won't pass a sanitized build, 
amazingly even if I use `__attribute__((no_sanitize("address", "undefined")))` 
: https://godbolt.org/z/4TGPbrYhq


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108228

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

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So I am trying this out. I tried the target creation form and I am not able to 
use arrow keys when entering the path to the main executable. Are you able to 
do this?

It seems like all text fields have this issue for me:

- left and right arrow keys don't work
- forward and backward delete don't work
- none of the editing like CTRL+A to go to the beginning work, CTRL+E to go to 
the end, etc.

Next I tried to launch the process with no args and got a error that was due to 
use filling in the shell if the user didn't specify it. We need to not specify 
a shell in the launch information if the user didn't specify it because this 
will cause us to try and launch the program inside of a shell. See inlined 
comments for fix.

There is also a big usability issue with this dialog if you check the show 
advanced settings. You must TAB many many times to get to the "Launch" button.

That being said, lets just fix this dialog for now with by removing the not 
getting the shell if it isn't specified and not setting the arch in the launch 
info it it wasn't specified, and this should be functional. I will suggest some 
usability fixes via email and we can work on getting those resolved with 
individual patches.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3356-3357
+target_sp ? target_sp->GetPlatform() : PlatformSP();
+launch_info.GetArchitecture() = Platform::GetAugmentedArchSpec(
+platform_sp.get(), m_arch_field->GetArchString());
+  }

Don't set the architecture if the user didn't specify it.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3363-3364
+  launch_info.SetShell(m_shell_field->GetResolvedFileSpec());
+else
+  launch_info.SetShell(HostInfo::GetDefaultShell());
+launch_info.SetShellExpandArguments(

We shouldn't fill this out by default. If you do, then all of your programs 
will be launched via a shell. On Mac, this doesn't work because we can't debug 
any Apple code signed binairies and "/bin/sh", "/bin/bash", "/bin/zsh" and all 
others are not allowed to be debugged. So just remove this else clause. After I 
did this I was able to debug my binary with this dialog.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

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

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM module formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108228

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
+
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;

MaskRay wrote:
> 
Can this be a StringRef?



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:149
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;
+bool result;





Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:155
+  // Directory iterator doesn't ensure any sequence.
+  for (llvm::sys::fs::directory_iterator iter(proc_fd_path, EC), FileEnd;
+   iter != FileEnd && !EC; iter.increment(EC)) {




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [lldb] c64d185 - [lldb] Include arm64 in affected_by_radar_34562999

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-08-17T19:45:45-07:00
New Revision: c64d1855b9a912cd6e3d0227ecd939c93c68fd9e

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

LOG: [lldb] Include arm64 in affected_by_radar_34562999

The same issue impacts arm64, both on-device and on Apple Silicon.

Added: 


Modified: 
lldb/test/API/functionalities/return-value/TestReturnValue.py

Removed: 




diff  --git a/lldb/test/API/functionalities/return-value/TestReturnValue.py 
b/lldb/test/API/functionalities/return-value/TestReturnValue.py
index fe79c46c30f01..54abfe46c5607 100644
--- a/lldb/test/API/functionalities/return-value/TestReturnValue.py
+++ b/lldb/test/API/functionalities/return-value/TestReturnValue.py
@@ -22,9 +22,10 @@ def affected_by_pr44132(self):
 return (self.getArchitecture() in ["aarch64", "arm"] and
 self.getPlatform() in ["freebsd", "linux"])
 
-# ABIMacOSX_arm can't fetch simple values inside a structure
+# ABIMacOSX_arm(64) can't fetch simple values inside a structure
 def affected_by_radar_34562999(self):
-return (self.getArchitecture() == 'armv7' or self.getArchitecture() == 
'armv7k') and self.platformIsDarwin()
+arch = self.getArchitecture().lower()
+return arch in ['arm64', 'arm64e', 'armv7', 'armv7k'] and 
self.platformIsDarwin()
 
 @expectedFailureAll(oslist=["freebsd"], archs=["i386"],
 bugnumber="llvm.org/pr48376")



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

[Lldb-commits] [lldb] a452ca4 - [lldb] Extend isAArch64 to arm64 and arm64e

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-08-17T19:45:45-07:00
New Revision: a452ca471c0e3d0bcd26d16a45b6bc43efee

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

LOG: [lldb] Extend isAArch64 to arm64 and arm64e

This fixes TestMemoryTag on Apple Silicon.

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/lldbtest.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 7e1fdce36ede6..a899b1b154fe2 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1293,7 +1293,8 @@ def getCPUInfo(self):
 
 def isAArch64(self):
 """Returns true if the architecture is AArch64."""
-return self.getArchitecture().lower() == "aarch64"
+arch = self.getArchitecture().lower()
+return arch in ["aarch64", "arm64", "arm64e"]
 
 def isAArch64SVE(self):
 return self.isAArch64() and "sve" in self.getCPUInfo()



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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
+
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.

LGTM. Some nits




Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:161
+// stdin/stdout/stderr
+if ((fd > 2) && !info.GetFileActionForFD(fd) && fd != error_fd)
+  files_to_close.push_back(fd);

drop parens around `fd > 2`



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:164
+  }
+  for (auto _to_close : files_to_close)
+close(file_to_close);

`auto &` -> int



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:168
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
+if (!info.GetFileActionForFD(fd) && fd != error_fd) {

Cache `sysconf(_SC_OPEN_MAX)` to avoid repeated calls.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:169
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
+if (!info.GetFileActionForFD(fd) && fd != error_fd) {
+  close(fd);

one-line simple statements don't need braces. 
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Greg Clayton via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb26e1efc424a: [LLDB][GUI] Add Breakpoints window (authored 
by OmarEmaraDev, committed by clayborg).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -45,6 +45,7 @@
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
+#include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/VariableList.h"
@@ -3764,9 +3765,14 @@
TreeItem *_item) {
 return;
   }
-  virtual bool TreeDelegateItemSelected(
-  TreeItem ) = 0; // Return true if we need to update views
+  // This is invoked when a tree item is selected. If true is returned, the
+  // views are updated.
+  virtual bool TreeDelegateItemSelected(TreeItem ) = 0;
   virtual bool TreeDelegateExpandRootByDefault() { return false; }
+  // This is mostly useful for root tree delegates. If false is returned,
+  // drawing will be skipped completely. This is needed, for instance, in
+  // skipping drawing of the threads tree if there is no running process.
+  virtual bool TreeDelegateShouldDraw() { return true; }
 };
 
 typedef std::shared_ptr TreeDelegateSP;
@@ -3956,6 +3962,16 @@
 
   void SetIdentifier(uint64_t identifier) { m_identifier = identifier; }
 
+  const std::string () const { return m_text; }
+
+  void SetText(const char *text) {
+if (text == nullptr) {
+  m_text.clear();
+  return;
+}
+m_text = text;
+  }
+
   void SetMightHaveChildren(bool b) { m_might_have_children = b; }
 
 protected:
@@ -3963,6 +3979,7 @@
   TreeDelegate _delegate;
   void *m_user_data;
   uint64_t m_identifier;
+  std::string m_text;
   int m_row_idx; // Zero based visible row index, -1 if not visible or for the
  // root item
   std::vector m_children;
@@ -3981,21 +3998,6 @@
   int NumVisibleRows() const { return m_max_y - m_min_y; }
 
   bool WindowDelegateDraw(Window , bool force) override {
-ExecutionContext exe_ctx(
-m_debugger.GetCommandInterpreter().GetExecutionContext());
-Process *process = exe_ctx.GetProcessPtr();
-
-bool display_content = false;
-if (process) {
-  StateType state = process->GetState();
-  if (StateIsStoppedState(state, true)) {
-// We are stopped, so it is ok to
-display_content = true;
-  } else if (StateIsRunningState(state)) {
-return true; // Don't do any updating when we are running
-  }
-}
-
 m_min_x = 2;
 m_min_y = 1;
 m_max_x = window.GetWidth() - 1;
@@ -4004,35 +4006,36 @@
 window.Erase();
 window.DrawTitleBox(window.GetName());
 
-if (display_content) {
-  const int num_visible_rows = NumVisibleRows();
-  m_num_rows = 0;
-  m_root.CalculateRowIndexes(m_num_rows);
-  m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
- m_selected_item);
-
-  // If we unexpanded while having something selected our total number of
-  // rows is less than the num visible rows, then make sure we show all the
-  // rows by setting the first visible row accordingly.
-  if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
-m_first_visible_row = 0;
-
-  // Make sure the selected row is always visible
-  if (m_selected_row_idx < m_first_visible_row)
-m_first_visible_row = m_selected_row_idx;
-  else if (m_first_visible_row + num_visible_rows <= m_selected_row_idx)
-m_first_visible_row = m_selected_row_idx - num_visible_rows + 1;
-
-  int row_idx = 0;
-  int num_rows_left = num_visible_rows;
-  m_root.Draw(window, m_first_visible_row, m_selected_row_idx, row_idx,
-  num_rows_left);
-  // Get the selected row
-  m_selected_item = m_root.GetItemForRowIndex(m_selected_row_idx);
-} else {
+if (!m_delegate_sp->TreeDelegateShouldDraw()) {
   m_selected_item = nullptr;
+  return true;
 }
 
+const int num_visible_rows = NumVisibleRows();
+m_num_rows = 0;
+m_root.CalculateRowIndexes(m_num_rows);
+m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
+   m_selected_item);
+
+// If we unexpanded while having something selected our total number of
+// rows is less than the num visible rows, then make sure we show all the
+// rows by setting the first visible row accordingly.
+if (m_first_visible_row > 0 && 

[Lldb-commits] [lldb] b26e1ef - [LLDB][GUI] Add Breakpoints window

[Greg Clayton via lldb-commits]

Author: Omar Emara
Date: 2021-08-17T17:38:16-07:00
New Revision: b26e1efc424ad840143f02a96246cc666ee99e72

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

LOG: [LLDB][GUI] Add Breakpoints window

This patch adds a breakpoints window that lists all breakpoints and
breakpoints locations. The window is implemented as a tree, where the
first level is the breakpoints and the second level is breakpoints
locations.

The tree delegate was hardcoded to only draw when there is a process,
which is not necessary for breakpoints, so the relevant logic was
abstracted in the TreeDelegateShouldDraw method.

Reviewed By: clayborg

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

Added: 


Modified: 
lldb/source/Core/IOHandlerCursesGUI.cpp

Removed: 




diff  --git a/lldb/source/Core/IOHandlerCursesGUI.cpp 
b/lldb/source/Core/IOHandlerCursesGUI.cpp
index f1872af65b92..0ae41dee7f9d 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -45,6 +45,7 @@
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
+#include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/VariableList.h"
@@ -3764,9 +3765,14 @@ class TreeDelegate {
TreeItem *_item) {
 return;
   }
-  virtual bool TreeDelegateItemSelected(
-  TreeItem ) = 0; // Return true if we need to update views
+  // This is invoked when a tree item is selected. If true is returned, the
+  // views are updated.
+  virtual bool TreeDelegateItemSelected(TreeItem ) = 0;
   virtual bool TreeDelegateExpandRootByDefault() { return false; }
+  // This is mostly useful for root tree delegates. If false is returned,
+  // drawing will be skipped completely. This is needed, for instance, in
+  // skipping drawing of the threads tree if there is no running process.
+  virtual bool TreeDelegateShouldDraw() { return true; }
 };
 
 typedef std::shared_ptr TreeDelegateSP;
@@ -3956,6 +3962,16 @@ class TreeItem {
 
   void SetIdentifier(uint64_t identifier) { m_identifier = identifier; }
 
+  const std::string () const { return m_text; }
+
+  void SetText(const char *text) {
+if (text == nullptr) {
+  m_text.clear();
+  return;
+}
+m_text = text;
+  }
+
   void SetMightHaveChildren(bool b) { m_might_have_children = b; }
 
 protected:
@@ -3963,6 +3979,7 @@ class TreeItem {
   TreeDelegate _delegate;
   void *m_user_data;
   uint64_t m_identifier;
+  std::string m_text;
   int m_row_idx; // Zero based visible row index, -1 if not visible or for the
  // root item
   std::vector m_children;
@@ -3981,21 +3998,6 @@ class TreeWindowDelegate : public WindowDelegate {
   int NumVisibleRows() const { return m_max_y - m_min_y; }
 
   bool WindowDelegateDraw(Window , bool force) override {
-ExecutionContext exe_ctx(
-m_debugger.GetCommandInterpreter().GetExecutionContext());
-Process *process = exe_ctx.GetProcessPtr();
-
-bool display_content = false;
-if (process) {
-  StateType state = process->GetState();
-  if (StateIsStoppedState(state, true)) {
-// We are stopped, so it is ok to
-display_content = true;
-  } else if (StateIsRunningState(state)) {
-return true; // Don't do any updating when we are running
-  }
-}
-
 m_min_x = 2;
 m_min_y = 1;
 m_max_x = window.GetWidth() - 1;
@@ -4004,35 +4006,36 @@ class TreeWindowDelegate : public WindowDelegate {
 window.Erase();
 window.DrawTitleBox(window.GetName());
 
-if (display_content) {
-  const int num_visible_rows = NumVisibleRows();
-  m_num_rows = 0;
-  m_root.CalculateRowIndexes(m_num_rows);
-  m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
- m_selected_item);
-
-  // If we unexpanded while having something selected our total number of
-  // rows is less than the num visible rows, then make sure we show all the
-  // rows by setting the first visible row accordingly.
-  if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
-m_first_visible_row = 0;
-
-  // Make sure the selected row is always visible
-  if (m_selected_row_idx < m_first_visible_row)
-m_first_visible_row = m_selected_row_idx;
-  else if (m_first_visible_row + num_visible_rows <= m_selected_row_idx)
-m_first_visible_row = m_selected_row_idx - num_visible_rows + 1;
-
-  int row_idx = 0;
-  int num_rows_left = num_visible_rows;
-  m_root.Draw(window, m_first_visible_row, m_selected_row_idx, row_idx,
-  num_rows_left);
-  // Get 

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D108061#2950858 , @rdhindsa wrote:

> Updated the test to check for the respective function name from the shared 
> library in the frames.
> I didn't add run_to_source_breakpoint since we can't set a breakpoint on main 
> initially before running the program.  
> Hence it would need to be run first and then we can check later to see if the 
> function name is available. Please let me know if you think there is any 
> concern with this way.

We need to figure out where the main program is loaded somehow and the dynamic 
loader needs to set the load address for "a.out" so that breakpoints can be 
resolved and hit. We _do_ need to make sure a breakpoint can be set and hit 
otherwise this patch will just show us a debug session that has the dynamic 
loader as the main executable, but we will never see where the program it will 
run is loaded. That is the tricky part of this patch and where I got stuck. GDB 
is able to do this, but I didn't look at how they did it. We will want our 
program to show up in the "image list" output with a valid load address.

Are you able to hit a breakpoint if one is set in the a.out program?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108061

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.
Herald added a subscriber: JDevlieghere.

LGTM. Anyone else have any issues?




Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
 
+std::string proc_fd_path = "/proc/self/fd";
+std::filesystem::path fp(proc_fd_path);

MaskRay wrote:
> /proc/self/fd is generally unavailable on non-Linux OSes.
which is why the "else" clause is still intact. If the file fails to open, we 
do what we did before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa updated this revision to Diff 367067.
rdhindsa added a comment.
Herald added a project: LLDB.

Updated it to use llvm's filesystem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105732

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // Close everything besides stdin, stdout, and stderr that has no file
 // action to avoid leaking. Only do this when debugging, as elsewhere we
 // actually rely on passing open descriptors to child processes.
-for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(fd);
+
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;
+bool result;
+EC = llvm::sys::fs::is_directory(proc_fd_path, result);
+if (result) {
+  std::vector files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (llvm::sys::fs::directory_iterator iter(proc_fd_path, EC), FileEnd;
+   iter != FileEnd && !EC; iter.increment(EC)) {
+int fd = std::stoi(iter->path().substr(proc_fd_path.size() + 1));
+
+// Don't close first three entries since they are
+// stdin/stdout/stderr
+if ((fd > 2) && !info.GetFileActionForFD(fd) && fd != error_fd)
+  files_to_close.push_back(fd);
+  }
+  for (auto _to_close : files_to_close)
+close(file_to_close);
+} else {
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
+if (!info.GetFileActionForFD(fd) && fd != error_fd) {
+  close(fd);
+}
+}
 
 // Start tracing this child that is about to exec.
 if (ptrace(PT_TRACE_ME, 0, nullptr, 0) == -1)


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/FileSystem.h"
 
 #include 
 #include 
@@ -143,9 +144,32 @@
 // Close everything besides stdin, stdout, and stderr that has no file
 // action to avoid leaking. Only do this when debugging, as elsewhere we
 // actually rely on passing open descriptors to child processes.
-for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
-  if (!info.GetFileActionForFD(fd) && fd != error_fd)
-close(fd);
+
+std::string proc_fd_path = "/proc/self/fd";
+std::error_code EC;
+bool result;
+EC = llvm::sys::fs::is_directory(proc_fd_path, result);
+if (result) {
+  std::vector files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (llvm::sys::fs::directory_iterator iter(proc_fd_path, EC), FileEnd;
+   iter != FileEnd && !EC; iter.increment(EC)) {
+int fd = std::stoi(iter->path().substr(proc_fd_path.size() + 1));
+
+// Don't close first three entries since they are
+// stdin/stdout/stderr
+if ((fd > 2) && !info.GetFileActionForFD(fd) && fd != error_fd)
+  files_to_close.push_back(fd);
+  }
+  for (auto _to_close : files_to_close)
+close(file_to_close);
+} else {
+  // /proc/self/fd didn't work - trying the slow way instead
+  for (int fd = 3; fd < sysconf(_SC_OPEN_MAX); ++fd)
+if (!info.GetFileActionForFD(fd) && fd != error_fd) {
+  close(fd);
+}
+}
 
 // Start tracing this child that is about to exec.
 if (ptrace(PT_TRACE_ME, 0, nullptr, 0) == -1)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108257: Add type to the output for FieldDecl when logging in ClangASTSource::layoutRecordType

[Adrian Prantl via Phabricator via lldb-commits]

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

If getType() doesn't trigger type completion then this LGTM!


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

https://reviews.llvm.org/D108257

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

[Lldb-commits] [PATCH] D108257: Add type to the output for FieldDecl when logging in ClangASTSource::layoutRecordType

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik created this revision.
shafik added reviewers: teemperor, aprantl.
shafik requested review of this revision.

I was debugging a problem and noticed that it would have been helpful to have 
the type of each `FieldDecl` when looking at the output from 
`ClangASTSource::layoutRecordType`


https://reviews.llvm.org/D108257

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -1571,8 +1571,8 @@
 fe = record->field_end();
  fi != fe; ++fi) {
   LLDB_LOG(log,
-   "LRT (FieldDecl*){0}, Name = '{1}', Offset = {2} bits",
-   *fi, fi->getName(), field_offsets[*fi]);
+   "LRT (FieldDecl*){0}, Name = '{1}', Type = '{2}', Offset = 
{3} bits",
+   *fi, fi->getName(), fi->getType().getAsString(), 
field_offsets[*fi]);
 }
 DeclFromParser parser_cxx_record =
 DynCast(parser_record);


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -1571,8 +1571,8 @@
 fe = record->field_end();
  fi != fe; ++fi) {
   LLDB_LOG(log,
-   "LRT (FieldDecl*){0}, Name = '{1}', Offset = {2} bits",
-   *fi, fi->getName(), field_offsets[*fi]);
+   "LRT (FieldDecl*){0}, Name = '{1}', Type = '{2}', Offset = {3} bits",
+   *fi, fi->getName(), fi->getType().getAsString(), field_offsets[*fi]);
 }
 DeclFromParser parser_cxx_record =
 DynCast(parser_record);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107761: [LLDB][GUI] Refactor form drawing using subsurfaces

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D107761#2947639 , @OmarEmaraDev 
wrote:

> @clayborg  Perhaps you missed this, it is essential the same as D107182 
>  but without the unsupported function. It 
> would be good to have this committed.

It is in!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107761

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

[Lldb-commits] [PATCH] D107761: [LLDB][GUI] Refactor form drawing using subsurfaces

[Greg Clayton via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG79e950a29e00: [LLDB][GUI] Refactor form drawing using 
subsurfaces (authored by OmarEmaraDev, committed by clayborg).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107761

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -343,15 +343,30 @@
 // A surface is an abstraction for something than can be drawn on. The surface
 // have a width, a height, a cursor position, and a multitude of drawing
 // operations. This type should be sub-classed to get an actually useful ncurses
-// object, such as a Window, SubWindow, Pad, or a SubPad.
+// object, such as a Window or a Pad.
 class Surface {
 public:
-  Surface() : m_window(nullptr) {}
+  enum class Type { Window, Pad };
+
+  Surface(Surface::Type type) : m_type(type), m_window(nullptr) {}
 
   WINDOW *get() { return m_window; }
 
   operator WINDOW *() { return m_window; }
 
+  Surface SubSurface(Rect bounds) {
+Surface subSurface(m_type);
+if (m_type == Type::Pad)
+  subSurface.m_window =
+  ::subpad(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+else
+  subSurface.m_window =
+  ::derwin(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+return subSurface;
+  }
+
   // Copy a region of the surface to another surface.
   void CopyToSurface(Surface , Point source_origin, Point target_origin,
  Size size) {
@@ -535,41 +550,32 @@
   }
 
 protected:
+  Type m_type;
   WINDOW *m_window;
 };
 
 class Pad : public Surface {
 public:
-  Pad(Size size) { m_window = ::newpad(size.height, size.width); }
-
-  ~Pad() { ::delwin(m_window); }
-};
-
-class SubPad : public Surface {
-public:
-  SubPad(Pad , Rect bounds) {
-m_window = ::subpad(pad.get(), bounds.size.height, bounds.size.width,
-bounds.origin.y, bounds.origin.x);
-  }
-  SubPad(SubPad , Rect bounds) {
-m_window = ::subpad(subpad.get(), bounds.size.height, bounds.size.width,
-bounds.origin.y, bounds.origin.x);
+  Pad(Size size) : Surface(Surface::Type::Pad) {
+m_window = ::newpad(size.height, size.width);
   }
 
-  ~SubPad() { ::delwin(m_window); }
+  ~Pad() { ::delwin(m_window); }
 };
 
 class Window : public Surface {
 public:
   Window(const char *name)
-  : m_name(name), m_panel(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_panel(nullptr),
+m_parent(nullptr), m_subwindows(), m_delegate_sp(),
+m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(false),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {}
 
   Window(const char *name, WINDOW *w, bool del = true)
-  : m_name(name), m_panel(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_panel(nullptr),
+m_parent(nullptr), m_subwindows(), m_delegate_sp(),
+m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(del),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
 if (w)
@@ -577,8 +583,8 @@
   }
 
   Window(const char *name, const Rect )
-  : m_name(name), m_parent(nullptr), m_subwindows(), m_delegate_sp(),
-m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_parent(nullptr),
+m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(true),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
 Reset(::newwin(bounds.size.height, bounds.size.width, bounds.origin.y,
@@ -970,20 +976,6 @@
   const Window =(const Window &) = delete;
 };
 
-class DerivedWindow : public Surface {
-public:
-  DerivedWindow(Window , Rect bounds) {
-m_window = ::derwin(window.get(), bounds.size.height, bounds.size.width,
-bounds.origin.y, bounds.origin.x);
-  }
-  DerivedWindow(DerivedWindow _window, Rect bounds) {
-m_window = ::derwin(derived_window.get(), bounds.size.height,
-bounds.size.width, bounds.origin.y, bounds.origin.x);
-  }
-
-  ~DerivedWindow() { ::delwin(m_window); }
-};
-
 /
 // Forms
 /
@@ -1025,7 +1017,7 

[Lldb-commits] [lldb] 79e950a - [LLDB][GUI] Refactor form drawing using subsurfaces

[Greg Clayton via lldb-commits]

Author: Omar Emara
Date: 2021-08-17T16:54:41-07:00
New Revision: 79e950a29e004f2f0ac590f6090b61b3043503e2

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

LOG: [LLDB][GUI] Refactor form drawing using subsurfaces

This patch adds a new method SubSurface to the Surface class. The method
returns another surface that is a subset of this surface. This is
important to further abstract away drawing from the ncurses objects. For
instance, fields could previously be drawn on subpads only but can now
be drawn on any surface. This is needed to create the file search
dialogs and similar functionalities.

There is an opportunity to refactor window drawing in general using
surfaces, but we shall consider this separately later.

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

Added: 


Modified: 
lldb/source/Core/IOHandlerCursesGUI.cpp

Removed: 




diff  --git a/lldb/source/Core/IOHandlerCursesGUI.cpp 
b/lldb/source/Core/IOHandlerCursesGUI.cpp
index 010f9300aa2e5..f1872af65b925 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -343,15 +343,30 @@ class HelpDialogDelegate : public WindowDelegate {
 // A surface is an abstraction for something than can be drawn on. The surface
 // have a width, a height, a cursor position, and a multitude of drawing
 // operations. This type should be sub-classed to get an actually useful 
ncurses
-// object, such as a Window, SubWindow, Pad, or a SubPad.
+// object, such as a Window or a Pad.
 class Surface {
 public:
-  Surface() : m_window(nullptr) {}
+  enum class Type { Window, Pad };
+
+  Surface(Surface::Type type) : m_type(type), m_window(nullptr) {}
 
   WINDOW *get() { return m_window; }
 
   operator WINDOW *() { return m_window; }
 
+  Surface SubSurface(Rect bounds) {
+Surface subSurface(m_type);
+if (m_type == Type::Pad)
+  subSurface.m_window =
+  ::subpad(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+else
+  subSurface.m_window =
+  ::derwin(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+return subSurface;
+  }
+
   // Copy a region of the surface to another surface.
   void CopyToSurface(Surface , Point source_origin, Point target_origin,
  Size size) {
@@ -535,41 +550,32 @@ class Surface {
   }
 
 protected:
+  Type m_type;
   WINDOW *m_window;
 };
 
 class Pad : public Surface {
 public:
-  Pad(Size size) { m_window = ::newpad(size.height, size.width); }
-
-  ~Pad() { ::delwin(m_window); }
-};
-
-class SubPad : public Surface {
-public:
-  SubPad(Pad , Rect bounds) {
-m_window = ::subpad(pad.get(), bounds.size.height, bounds.size.width,
-bounds.origin.y, bounds.origin.x);
-  }
-  SubPad(SubPad , Rect bounds) {
-m_window = ::subpad(subpad.get(), bounds.size.height, bounds.size.width,
-bounds.origin.y, bounds.origin.x);
+  Pad(Size size) : Surface(Surface::Type::Pad) {
+m_window = ::newpad(size.height, size.width);
   }
 
-  ~SubPad() { ::delwin(m_window); }
+  ~Pad() { ::delwin(m_window); }
 };
 
 class Window : public Surface {
 public:
   Window(const char *name)
-  : m_name(name), m_panel(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_panel(nullptr),
+m_parent(nullptr), m_subwindows(), m_delegate_sp(),
+m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(false),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {}
 
   Window(const char *name, WINDOW *w, bool del = true)
-  : m_name(name), m_panel(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_panel(nullptr),
+m_parent(nullptr), m_subwindows(), m_delegate_sp(),
+m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(del),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
 if (w)
@@ -577,8 +583,8 @@ class Window : public Surface {
   }
 
   Window(const char *name, const Rect )
-  : m_name(name), m_parent(nullptr), m_subwindows(), m_delegate_sp(),
-m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_parent(nullptr),
+m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(true),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
 

[Lldb-commits] [lldb] 0479afb - [LLDB] Fix off by one logging placeholders in ClangASTSource::layoutRecordType()

[Shafik Yaghmour via lldb-commits]

Author: Shafik Yaghmour
Date: 2021-08-17T16:47:46-07:00
New Revision: 0479afb3d6a31668631464653f07154d6850e4a1

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

LOG: [LLDB] Fix off by one logging placeholders in 
ClangASTSource::layoutRecordType()

D72391 Added some additional information to the logging but in this case 
instead of using
placeholder 2 and 3 they used 3 and 4.

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
index fad2f3f1a8638..58b6b625bfeca 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -1495,7 +1495,7 @@ bool ClangASTSource::layoutRecordType(const RecordDecl 
*record, uint64_t ,
 
   LLDB_LOG(log,
"LayoutRecordType on (ASTContext*){0} '{1}' for (RecordDecl*)"
-   "{3} [name = '{4}']",
+   "{2} [name = '{3}']",
m_ast_context, m_clang_ast_context->getDisplayName(), record,
record->getName());
 



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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148
 
+std::string proc_fd_path = "/proc/self/fd";
+std::filesystem::path fp(proc_fd_path);

/proc/self/fd is generally unavailable on non-Linux OSes.


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Drive-by: std::filesystem is unavailable for GCC<8. The minimum GCC version is 
5, so std::filesystem isn't suitable. You may 
use`include/llvm/Support/FileSystem.h`


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

https://reviews.llvm.org/D105732

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

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa marked an inline comment as done.
rdhindsa added a comment.

Updated the test to check for the respective function name from the shared 
library in the frames.
I didn't add run_to_source_breakpoint since we can't set a breakpoint on main 
initially before running the program.  Hence it would need to be run first and 
then we can check later to see if the function name is available. Please let me 
know if you think there is any concern with this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108061

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

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

[Rumeet Dhindsa via Phabricator via lldb-commits]

rdhindsa updated this revision to Diff 367047.
rdhindsa marked an inline comment as done.
rdhindsa added a comment.

Updated the test to check for the respective function name from the shared 
library in the frames.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108061

Files:
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/test/API/functionalities/dyld-launch-linux/Makefile
  lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py
  lldb/test/API/functionalities/dyld-launch-linux/main.cpp
  lldb/test/API/functionalities/dyld-launch-linux/signal_file.cpp
  lldb/test/API/functionalities/dyld-launch-linux/signal_file.h

Index: lldb/test/API/functionalities/dyld-launch-linux/signal_file.h
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/signal_file.h
@@ -0,0 +1 @@
+int get_signal_crash(void);
Index: lldb/test/API/functionalities/dyld-launch-linux/signal_file.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/signal_file.cpp
@@ -0,0 +1,7 @@
+#include "signal_file.h"
+#include 
+
+int get_signal_crash(void) {
+  raise(SIGSEGV);
+  return 0;
+}
Index: lldb/test/API/functionalities/dyld-launch-linux/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/main.cpp
@@ -0,0 +1,3 @@
+#include "signal_file.h"
+
+int main() { return get_signal_crash(); }
Index: lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py
@@ -0,0 +1,31 @@
+"""
+Test that LLDB can launch a linux executable through the dynamic loader and still hit a breakpoint.
+"""
+
+import lldb
+import os
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestLinux64LaunchingViaDynamicLoader(TestBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+"""Test we can launch and hit a breakpoint when we run our program through the dynamic loader"""
+self.build()
+exe = "/lib64/ld-linux-x86-64.so.2"
+
+if(os.path.exists(exe)):
+# Create a target by the debugger.
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+process = target.LaunchSimple(
+["--library-path",self.get_process_working_directory(),self.getBuildArtifact("a.out")], None, self.get_process_working_directory())
+
+self.assertEqual(process.GetState(), lldb.eStateStopped)
+thread = process.GetSelectedThread()
+self.assertEqual(thread.GetStopReason(), lldb.eStopReasonSignal)
+self.assertIn("raise",thread.GetFrameAtIndex(0).GetDisplayFunctionName())
+self.assertIn("get_signal_crash",thread.GetFrameAtIndex(1).GetDisplayFunctionName())
Index: lldb/test/API/functionalities/dyld-launch-linux/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/Makefile
@@ -0,0 +1,15 @@
+CXX_SOURCES := main.cpp
+LD_EXTRAS := -Wl,-rpath "-Wl,$(shell pwd)"
+USE_LIBDL := 1
+
+include Makefile.rules
+
+# The following shared library will be used to test breakpoints under dynamic loading
+libsignal_file.so:  signal_file.cpp
+	$(MAKE) -f $(MAKEFILE_RULES) \
+		DYLIB_ONLY=YES DYLIB_CXX_SOURCES=signal_file.cpp DYLIB_NAME=signal_file
+
+a.out: libsignal_file.so main.cpp
+	$(MAKE) -f $(MAKEFILE_RULES) \
+CXX_SOURCES=main.cpp LD_EXTRAS=libsignal_file.so
+
Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -333,28 +333,48 @@
 LLDB_LOG(log, "Rendezvous structure is not set up yet. "
   "Trying to locate rendezvous breakpoint in the interpreter "
   "by symbol name.");
-ModuleSP interpreter = LoadInterpreterModule();
-if (!interpreter) {
-  LLDB_LOG(log, "Can't find interpreter, rendezvous breakpoint isn't set.");
-  return false;
-}
-
-// Function names from different dynamic loaders that are known to be used
-// as rendezvous between the loader and debuggers.
+// Function names from different dynamic loaders that are known to be
+// used as rendezvous between the loader and debuggers.
 static std::vector 

[Lldb-commits] [PATCH] D108233: WIP: Add minidump save-core functionality to ELF object files

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

I like the idea of this! I have a minidump creation tool I wrote in python for 
making tests.

ELF files support core files in their native file format, so I think the 
ObjectFileELF::SaveCore(...) should actually save an ELF core file if it saves 
anything. So we should move the minidump creation to another location. I 
mention ideas below.

When running "process save-core" we can add a new option like 
"--plugin=", and if this option is specified we will look for a plug-in 
name that matches when iterating over the instances that support core file 
exporting. Right now the save core calls the plugin manager:

  Status PluginManager::SaveCore(const lldb::ProcessSP _sp,
 const FileSpec ,
 lldb::SaveCoreStyle _style) {
Status error;
auto  = GetObjectFileInstances().GetInstances();
for (auto  : instances) {
  if (instance.save_core &&
  instance.save_core(process_sp, outfile, core_style, error))
return error;
}
error.SetErrorString(
"no ObjectFile plugins were able to save a core for this process");
return error;
  }

Currently we are expecting only one ObjectFile class to return true for any 
given process. But since ObjectFileELF::SaveCore(...) could eventually be added 
to ObjectFileELF, we need a way to force a plug-in to be considered. If we add 
an extra "ConstString plugin_name" to the arguments, we can check the name and 
target a different plug-in. It should be possible to save a core file in 
different formats.

So I propose:

- remove code from ELF ObjectFile plug-in
- modify PluginManager::SaveCore(...) to take an extra "ConstString 
plugin_name" parameter.
  - If this parameter is valid, skip any "instance" whose name doesn't match
- Modify the ObjectFileSaveCore callback definition to take a "bool force" 
parameter which will make the plug-ins skip the triple detection stuff that 
will skip saving the core file if each SaveCore(...) function didn't like the 
triple it was passed. It is fine for a plug-in to return an error stating that 
saving a core file with the current process doesn't make sense since each core 
file has to have support for saving registers.
- make a new Minidump ObjectFile plug-in at 
"lldb/source/Plugins/ObjectFile/Minidump"
  - Create a lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp and 
lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
  - when you register the callbacks for this plug-in, only register the 
ObjectFileMinidump::SaveCore callback (see example code below)

  void ObjectFileMinidump::Initialize() {
PluginManager::RegisterPlugin(
GetPluginNameStatic(),
GetPluginDescriptionStatic(), 
/*create_callback=*/nullptr,
/*create_memory_callback=*/nullptr, 
/*get_module_specifications=*/nullptr,
SaveCore);
  }

Then hopefully you can just make a very ObjectFileMinidump with a few plug-in 
template functions and the SaveCore(...) function. Let me know if any of this 
is not clear.




Comment at: lldb/source/Plugins/ObjectFile/ELF/MinidumpFileBuilder.cpp:70
+  default:
+arch = ProcessorArchitecture::AMD64;
+break;

Seems like we should return an error here if the architecture isn't supported 
with a nice error string that gets displayed. We shouldn't default to AMD64 
right?



Comment at: lldb/source/Plugins/ObjectFile/ELF/MinidumpFileBuilder.cpp:80
+  GetCurrentDataEndOffset() + sizeof(llvm::minidump::SystemInfo));
+  sys_info.PlatformId = OSPlatform::Linux;
+  m_data.AppendData(_info, sizeof(llvm::minidump::SystemInfo));

We should be able to populate this correctly using the triple right? And error 
out if we don't support the OS?



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:725-727
+  if (error.Fail()) {
+return false;
+  }

remove braces for single statement if (LLVM coding guidelines)



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:730-732
+  if (error.Fail()) {
+return false;
+  }

remove braces for single statement if (LLVM coding guidelines)



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:738-740
+if (error.Fail()) {
+  return false;
+}

remove braces for single statement if (LLVM coding guidelines)



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:743-745
+if (error.Fail()) {
+  return false;
+}

remove braces for single statement if (LLVM coding guidelines)



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:748-750
+if (error.Fail()) {
+  return false;
+}

remove braces for 

[Lldb-commits] [PATCH] D101406: Rename human-readable name for DW_LANG_Mips_Assembler

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> This can't have been the intention of this commit? As far as I can see, there 
> *is* no plugin for plain assembler?

This commit changes the warning from

> This version of LLDB has no plugin for the mipsassem language.

to

> This version of LLDB has no plugin for the language "assembler".

Yes, there is no plugin for the language assembler. But that hasn't chnaged 
with this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101406

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

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe21a21a977b4: [lldb] Fix#2 of DW_AT_ranges 
DW_FORM_sec_offset not using DW_AT_rnglists_base… (authored by jankratochvil).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s


Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -29,7 +29,7 @@
 # RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 12), please file a bug and attach the file at the start of this error message
+# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
@@ -97,7 +97,7 @@
 .long   .Lrnglists_end-rnglists # DW_AT_high_pc
 .long   .Laddr_table_base0  # DW_AT_addr_base
 .ifdef RNGLISTBASE
-.long   .Ldebug_ranges0 # DW_AT_rnglists_base
+.long   .Ldebug_ranges1 # DW_AT_rnglists_base
 .endif
 .byte   2   # Abbrev [2] 0x2b:0x37 
DW_TAG_subprogram
 .quad   rnglists# DW_AT_low_pc
@@ -105,7 +105,7 @@
 .asciz  "rnglists"  # DW_AT_name
 .byte   5   # Abbrev [5] 0x52:0xf 
DW_TAG_lexical_block
 .ifndef RNGLISTX
-.long   .Ldebug_ranges0 # DW_AT_ranges DW_FORM_sec_offset
+.long   .Ldebug_ranges1 # DW_AT_ranges DW_FORM_sec_offset
 .else
 .uleb128 0  # DW_AT_ranges DW_FORM_rnglistx
 .endif
@@ -130,9 +130,17 @@
 .byte   8   # Address size
 .byte   0   # Segment selector size
 .long   0   # Offset entry count
-.Ldebug_ranges0:
+.Ldebug_rnglist_table_end0:
+
+.long   .Ldebug_rnglist_table_end1-.Ldebug_rnglist_table_start1 # 
Length
+.Ldebug_rnglist_table_start1:
+.short  5   # Version
+.byte   8   # Address size
+.byte   0   # Segment selector size
+.long   0   # Offset entry count
+.Ldebug_ranges1:
 .byte   4   # DW_RLE_offset_pair
 .uleb128 .Lblock1_begin-rnglists  #   starting offset
 .uleb128 .Lblock1_end-rnglists#   ending offset
 .byte   0   # DW_RLE_end_of_list
-.Ldebug_rnglist_table_end0:
+.Ldebug_rnglist_table_end1:
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -437,15 +437,20 @@
   // We are expected to be called with Offset 0 or pointing just past the table
   // header. Correct Offset in the latter case so that it points to the start
   // of the header.
-  if (offset > 0) {
-uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
-if (offset < HeaderSize)
-  return llvm::createStringError(errc::invalid_argument,
- "did not detect a valid"
- " list table with base = 0x%" PRIx64 "\n",
- offset);
-offset -= HeaderSize;
+  if (offset == 0) {
+// This means DW_AT_rnglists_base is missing and therefore DW_FORM_rnglistx
+// cannot be handled. Returning a default-constructed ListTableType allows
+// DW_FORM_sec_offset to be supported.
+return ListTableType();
   }
+
+  uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
+  if (offset < HeaderSize)
+return llvm::createStringError(errc::invalid_argument,
+   "did not detect a valid"
+   " list table with base = 0x%" PRIx64 "\n",
+   offset);
+  offset -= HeaderSize;
   ListTableType Table;
   if (llvm::Error E = Table.extractHeaderAndOffsets(data, ))
 return std::move(E);
@@ -996,8 +1001,12 @@
 return 

[Lldb-commits] [lldb] e21a21a - [lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[Jan Kratochvil via lldb-commits]

Author: Jan Kratochvil
Date: 2021-08-17T22:19:16+02:00
New Revision: e21a21a977b492cc7172779d080db146e3c39e06

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

LOG: [lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using 
DW_AT_rnglists_base (used by GCC)

Fix D98289 so that it works even for 2nd..nth compilation unit
(.debug_rnglists).

Reviewed By: dblaikie, ikudrin

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
index a498117c264b..9a56e1e6aa2c 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -437,15 +437,20 @@ ParseListTableHeader(const llvm::DWARFDataExtractor 
, uint64_t offset,
   // We are expected to be called with Offset 0 or pointing just past the table
   // header. Correct Offset in the latter case so that it points to the start
   // of the header.
-  if (offset > 0) {
-uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
-if (offset < HeaderSize)
-  return llvm::createStringError(errc::invalid_argument,
- "did not detect a valid"
- " list table with base = 0x%" PRIx64 "\n",
- offset);
-offset -= HeaderSize;
+  if (offset == 0) {
+// This means DW_AT_rnglists_base is missing and therefore DW_FORM_rnglistx
+// cannot be handled. Returning a default-constructed ListTableType allows
+// DW_FORM_sec_offset to be supported.
+return ListTableType();
   }
+
+  uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
+  if (offset < HeaderSize)
+return llvm::createStringError(errc::invalid_argument,
+   "did not detect a valid"
+   " list table with base = 0x%" PRIx64 "\n",
+   offset);
+  offset -= HeaderSize;
   ListTableType Table;
   if (llvm::Error E = Table.extractHeaderAndOffsets(data, ))
 return std::move(E);
@@ -996,8 +1001,12 @@ DWARFUnit::FindRnglistFromOffset(dw_offset_t offset) {
 return llvm::createStringError(errc::invalid_argument,
"missing or invalid range list table");
 
-  auto range_list_or_error = GetRnglistTable()->findList(
-  m_dwarf.GetDWARFContext().getOrLoadRngListsData().GetAsLLVM(), offset);
+  llvm::DWARFDataExtractor data =
+  m_dwarf.GetDWARFContext().getOrLoadRngListsData().GetAsLLVM();
+
+  // As DW_AT_rnglists_base may be missing we need to call setAddressSize.
+  data.setAddressSize(m_header.GetAddressByteSize());
+  auto range_list_or_error = GetRnglistTable()->findList(data, offset);
   if (!range_list_or_error)
 return range_list_or_error.takeError();
 

diff  --git a/lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s 
b/lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
index da1fff61dc02..5c1f6c46f739 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ b/lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -29,7 +29,7 @@
 # RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 12), please file a bug and attach the file at the start of this error message
+# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
@@ -97,7 +97,7 @@ lookup_rnglists:
 .long   .Lrnglists_end-rnglists # DW_AT_high_pc
 .long   .Laddr_table_base0  # DW_AT_addr_base
 .ifdef RNGLISTBASE
-.long   .Ldebug_ranges0 # DW_AT_rnglists_base
+.long   .Ldebug_ranges1 # DW_AT_rnglists_base
 .endif
 .byte   2   # Abbrev [2] 0x2b:0x37 
DW_TAG_subprogram
 .quad   rnglists# DW_AT_low_pc
@@ -105,7 +105,7 @@ lookup_rnglists:
 .asciz  "rnglists"  # DW_AT_name
 .byte   5   # Abbrev [5] 0x52:0xf 
DW_TAG_lexical_block
 

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[David Blaikie via Phabricator via lldb-commits]

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

In D106466#2950268 , @jankratochvil 
wrote:

> In D106466#2947710 , @dblaikie 
> wrote:
>
>> I assume there's already test coverage for rnglistx in debug_info.dwo/split 
>> unit? (because in that case there's no rnglists_base, but rnglistx is usable)
>
> I admit I did not check it, thanks for catching it. But it is already tested 
> by lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwo.s 
> 
>  implemented by Pavel Labath in 2019 
> .
>  It is using DWO `DW_TAG_lexical_block->DW_AT_ranges->DW_FORM_rnglistx`.
>
>> - could you explain how this code avoids treating the split unit 
>> rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't 
>> usable"?
>
> `m_ranges_base` is not zero in such case as it has been set from the skeleton 
> .

ah, OK - was going to say that setting it from the skeleton would be incorrect 
(for DWARF 5, at least) - but I see that's handled a few lines down: 
https://github.com/llvm/llvm-project/blob/f58a642da19c64cb6ee1badb0f176a872c1d7a0a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp#L110

Looks all good to me, then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

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

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D106466#2947710 , @dblaikie wrote:

> I assume there's already test coverage for rnglistx in debug_info.dwo/split 
> unit? (because in that case there's no rnglists_base, but rnglistx is usable)

I admit I did not check it, thanks for catching it. But it is already tested by 
lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwo.s 

 implemented by Pavel Labath in 2019 
.
 It is using DWO `DW_TAG_lexical_block->DW_AT_ranges->DW_FORM_rnglistx`.

> - could you explain how this code avoids treating the split unit 
> rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't 
> usable"?

`m_ranges_base` is not zero in such case as it has been set from the skeleton 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

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

[Lldb-commits] [PATCH] D108233: WIP: Add minidump save-core functionality to ELF object files

[Andrej Korman via Phabricator via lldb-commits]

Aj0SK created this revision.
Aj0SK added a reviewer: clayborg.
Herald added subscribers: pengfei, mgorny, emaste.
Aj0SK requested review of this revision.
Herald added subscribers: lldb-commits, MaskRay.
Herald added a project: LLDB.

This change adds save-core functionality into the ObjectFileELF that enables 
saving minidump of a stopped process. This change is mainly targeting Linux
running on x86_64 machines. Minidump should contain basic information needed
to examine state of threads, local variables and stack traces. Full support
for other platforms is not so far implemented. API tests are using LLDB's
MinidumpParser.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108233

Files:
  lldb/source/Plugins/ObjectFile/ELF/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/ELF/MinidumpFileBuilder.cpp
  lldb/source/Plugins/ObjectFile/ELF/MinidumpFileBuilder.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  lldb/test/API/functionalities/process_save_core_minidump/Makefile
  
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
  lldb/test/API/functionalities/process_save_core_minidump/main.cpp

Index: lldb/test/API/functionalities/process_save_core_minidump/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/process_save_core_minidump/main.cpp
@@ -0,0 +1,30 @@
+#include 
+#include 
+#include 
+
+using namespace std;
+
+void g() { assert(false); }
+
+void f() { g(); }
+
+size_t h() {
+  size_t sum = 0;
+  for (size_t i = 0; i < 100; ++i)
+for (size_t j = 0; j < 100; ++j)
+  if ((i * j) % 2 == 0) {
+sum += 1;
+  }
+  return sum;
+}
+
+int main() {
+  thread t1(f);
+
+  size_t x = h();
+
+  t1.join();
+
+  cout << "X is " << x << "\n";
+  return 0;
+}
\ No newline at end of file
Index: lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
===
--- /dev/null
+++ lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -0,0 +1,78 @@
+"""
+Test saving a mini dump.
+"""
+
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ProcessSaveCoreMinidumpTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipUnlessPlatform(['linux'])
+def test_save_linux_mini_dump(self):
+"""Test that we can save a Linux mini dump."""
+self.build()
+exe = self.getBuildArtifact("a.out")
+core = self.getBuildArtifact("core.dmp")
+try:
+target = self.dbg.CreateTarget(exe)
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertEqual(process.GetState(), lldb.eStateStopped)
+
+# get neccessary data for the verification phase
+process_info = process.GetProcessInfo()
+expected_pid = process_info.GetProcessID() if process_info.IsValid() else -1
+expected_number_of_modules = target.GetNumModules()
+expected_modules = target.modules
+expected_number_of_threads = process.GetNumThreads()
+expected_threads = []
+
+for thread_idx in range(process.GetNumThreads()):
+thread = process.GetThreadAtIndex(thread_idx)
+thread_id = thread.GetThreadID()
+expected_threads.append(thread_id)
+
+# save core and, kill process and verify corefile existence
+self.assertTrue(process.SaveCore(core))
+self.assertTrue(os.path.isfile(core))
+self.assertTrue(process.Kill().Success())
+
+# To verify, we'll launch with the mini dump
+target = self.dbg.CreateTarget(None)
+process = target.LoadCore(core)
+
+# check if the core is in desired state
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertTrue(process.GetProcessInfo().IsValid())
+self.assertEqual(process.GetProcessInfo().GetProcessID(), expected_pid)
+self.assertTrue(target.GetTriple().find("linux") != -1)
+self.assertTrue(target.GetNumModules(), expected_number_of_modules)
+self.assertEqual(process.GetNumThreads(), expected_number_of_threads)
+
+for module, expected in zip(target.modules, expected_modules):
+self.assertTrue(module.IsValid())
+module_file_name = module.GetFileSpec().GetFilename()
+expected_file_name = expected.GetFileSpec().GetFilename()
+# skip kernel virtual dynamic shared objects
+if "vdso" in expected_file_name:
+continue
+self.assertEqual(module_file_name, 

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: clayborg, jingham, teemperor.
Herald added a subscriber: mgorny.
bulbazord requested review of this revision.
Herald added a project: LLDB.

Module::LookupInfo's constructor currently goes over supported languages
trying to figure out the best way to search for a symbol name. This
seems like a great candidate for refactoring. Specifically, this is work
that can be delegated to language plugins.

Once again, the goal here is to further decouple plugins from
non-plugins. The idea is to have each language plugin take a name and
give you back some information about the name from the perspective of
the language. Specifically, each language now implements a
`GetFunctionNameInfo` method which returns an object of type
`Language::FunctionNameInfo`. Right now, it consists of a basename,
a context, and a FunctionNameType. Module::LookupInfo's constructor will
call `GetFunctionNameInfo` with the appropriate language plugin(s) and
then decide what to do with that information. I have attempted to maintain
existing behavior as best as possible.

A nice side effect of this change is that lldbCore no longer links
against the ObjC Language plugin.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108229

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Target/Language.h
  lldb/source/Core/CMakeLists.txt
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
@@ -104,6 +104,9 @@
   std::vector
   GetMethodNameVariants(ConstString method_name) const override;
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   lldb::TypeCategoryImplSP GetFormatters() override;
Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -275,6 +275,22 @@
   return variant_names;
 }
 
+Language::FunctionNameInfo
+ObjCLanguage::GetFunctionNameInfo(ConstString name) const {
+  Language::FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (ObjCLanguage::IsPossibleObjCMethodName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  if (ObjCLanguage::IsPossibleObjCSelector(name.GetCString())) {
+info.func_name_type |= lldb::eFunctionNameTypeSelector;
+  }
+
+  return info;
+}
+
 bool ObjCLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   ConstString demangled_name = mangled.GetDemangledName();
   if (!demangled_name)
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -104,6 +104,9 @@
 
   static lldb_private::ConstString GetPluginNameStatic();
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   ConstString
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -59,6 +59,39 @@
   return g_name;
 }
 
+Language::FunctionNameInfo
+CPlusPlusLanguage::GetFunctionNameInfo(ConstString name) const {
+  FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (IsCPPMangledName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  CPlusPlusLanguage::MethodName method(name);
+  llvm::StringRef basename = method.GetBasename();
+  if (basename.empty()) {
+if (CPlusPlusLanguage::ExtractContextAndIdentifier(
+name.GetCString(), info.context, info.basename)) {
+  info.func_name_type |=
+  (lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+} else {
+  info.func_name_type |= lldb::eFunctionNameTypeFull;
+}
+  } else {
+info.func_name_type |=
+(lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+  }
+
+  if (!method.GetQualifiers().empty()) {
+// There is a 'const' or other qualifier following the end of the function
+// parens, this can't be a eFunctionNameTypeBase.
+info.func_name_type &= ~(lldb::eFunctionNameTypeBase);
+  }
+

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added a reviewer: JDevlieghere.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The formatter tries to get the data field of the std::string, and to check 
whether that fails it just checks that the ValueObjectSP returned is not empty. 
 But we never return empty ValueObjectSP's to indicate failure, since doing so 
would lose the Error object that tells you why fetching the ValueObject failed.

This patch adds a check for ValueObject::GetError().Success().

I also added a test case for this failure, and reworked the test case a bit (to 
use run_to_source_breakpoint).  I also renamed a couple of single letter locals 
which don't follow the lldb coding conventions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108228

Files:
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
@@ -57,6 +57,11 @@
   uint64_t data = 0xfffeULL;
 } garbage_string_long_mode4;
 
+size_t touch_string(std::string _str)
+{
+  return in_str.size(); // Break here to look at bad string
+}
+
 int main()
 {
 std::wstring wempty(L"");
@@ -93,5 +98,7 @@
 #endif
 
 S.assign(L"!"); // Set break point at this line.
+std::string *not_a_string = (std::string *) 0x0;
+touch_string(*not_a_string);
 return 0;
 }
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
@@ -19,7 +19,7 @@
 # Call super's setUp().
 TestBase.setUp(self)
 # Find the line number to break at.
-self.line = line_number('main.cpp', '// Set break point at this line.')
+self.main_spec = lldb.SBFileSpec("main.cpp")
 self.namespace = 'std'
 
 @add_test_categories(["libc++"])
@@ -30,17 +30,11 @@
 def test_with_run_command(self):
 """Test that that file and class static variables display correctly."""
 self.build()
-self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
 
-lldbutil.run_break_set_by_file_and_line(
-self, "main.cpp", self.line, num_expected_locations=-1)
-
-self.runCmd("run", RUN_SUCCEEDED)
-
-# The stop reason of the thread should be breakpoint.
-self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
-substrs=['stopped',
- 'stop reason = breakpoint'])
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+"Set break point at this line.",
+self.main_spec)
+frame = thread.frames[0]
 
 # This is the function to remove the custom formats in order to have a
 # clean slate for the next test case.
@@ -83,9 +77,9 @@
 '(%s::string *) null_str = nullptr'%ns,
 ])
 
-self.runCmd("n")
+thread.StepOver()
 
-TheVeryLongOne = self.frame().FindVariable("TheVeryLongOne")
+TheVeryLongOne = frame.FindVariable("TheVeryLongOne")
 summaryOptions = lldb.SBTypeSummaryOptions()
 summaryOptions.SetCapping(lldb.eTypeSummaryUncapped)
 uncappedSummaryStream = lldb.SBStream()
@@ -129,3 +123,15 @@
 self.expect("frame variable garbage3", substrs=[r'garbage3 = "\xf0\xf0"'])
 self.expect("frame variable garbage4", substrs=['garbage4 = Summary Unavailable'])
 self.expect("frame variable garbage5", substrs=['garbage5 = Summary Unavailable'])
+
+# Finally, make sure that if the string is not readable, we give an error:
+bkpt_2 = target.BreakpointCreateBySourceRegex("Break here to look at bad string", self.main_spec)
+self.assertEqual(bkpt_2.GetNumLocations(), 1, "Got one location")
+threads = lldbutil.continue_to_breakpoint(process, bkpt_2)
+self.assertEqual(len(threads), 1, "Stopped at second breakpoint")
+frame = threads[0].frames[0]
+var = frame.FindVariable("in_str")
+

[Lldb-commits] [PATCH] D104281: [lldb][docs] Add reference docs for Lua scripting

[Pedro Tammela via Phabricator via lldb-commits]

tammela added a comment.

I think we are getting there, just some more details




Comment at: lldb/docs/use/scripting-example.rst:822
+The complete code for the Dictionary program (with case-conversion bug), the
+DFS function and other Python script examples (tree_utils.py) used for this
+example are available below.





Comment at: lldb/docs/use/scripting-reference.rst:1
-Python Reference
-
+Scripting Reference
+===

I'm seeing some references to a "python script" when it should be really just 
"the following script" or something more neutral.

A quick way to solve this is to search for [Pp]ython and try to rewrite the 
phrase into something generic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104281

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

[Lldb-commits] [PATCH] D108145: [lldb] Make TestAArch64AdrpAdd depend on the AArch64 target

[Raphael Isemann via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfc5495c351a1: [lldb] Make TestAArch64AdrpAdd depend on the 
AArch64 target (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108145

Files:
  
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py


Index: 
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
===
--- 
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
+++ 
lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
@@ -11,6 +11,7 @@
 mydir = TestBase.compute_mydir(__file__)
 
 @no_debug_info_test
+@skipIfLLVMTargetMissing("AArch64")
 def test_arm64(self):
 src_dir = self.getSourceDir()
 yaml_path = os.path.join(src_dir, "a.out-arm64.yaml")
@@ -27,6 +28,7 @@
 self.disassemble_check_for_hi_and_foo(target, f, binaryname)
 
 @no_debug_info_test
+@skipIfLLVMTargetMissing("AArch64")
 def test_arm64_32(self):
 src_dir = self.getSourceDir()
 yaml_path = os.path.join(src_dir, "a.out-arm64_32.yaml")


Index: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
===
--- lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
+++ lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
@@ -11,6 +11,7 @@
 mydir = TestBase.compute_mydir(__file__)
 
 @no_debug_info_test
+@skipIfLLVMTargetMissing("AArch64")
 def test_arm64(self):
 src_dir = self.getSourceDir()
 yaml_path = os.path.join(src_dir, "a.out-arm64.yaml")
@@ -27,6 +28,7 @@
 self.disassemble_check_for_hi_and_foo(target, f, binaryname)
 
 @no_debug_info_test
+@skipIfLLVMTargetMissing("AArch64")
 def test_arm64_32(self):
 src_dir = self.getSourceDir()
 yaml_path = os.path.join(src_dir, "a.out-arm64_32.yaml")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] fc5495c - [lldb] Make TestAArch64AdrpAdd depend on the AArch64 target

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-08-17T13:47:22+02:00
New Revision: fc5495c351a1f7ce28c7166a70113ce45906ff7b

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

LOG: [lldb] Make TestAArch64AdrpAdd depend on the AArch64 target

LLDB is using LLVM's target-specific disassembler which is only available when
the respective LLVM target has been enabled in the build config.

This patch just skips the test if there is no arm64 target (and its
disassembler) available in the current build config.

Reviewed By: jasonmolenda

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

Added: 


Modified: 

lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py

Removed: 




diff  --git 
a/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
 
b/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
index 325607028c033..c5b9921d7e62d 100644
--- 
a/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
+++ 
b/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
@@ -11,6 +11,7 @@ class TestAArch64AdrpAdd(TestBase):
 mydir = TestBase.compute_mydir(__file__)
 
 @no_debug_info_test
+@skipIfLLVMTargetMissing("AArch64")
 def test_arm64(self):
 src_dir = self.getSourceDir()
 yaml_path = os.path.join(src_dir, "a.out-arm64.yaml")
@@ -27,6 +28,7 @@ def test_arm64(self):
 self.disassemble_check_for_hi_and_foo(target, f, binaryname)
 
 @no_debug_info_test
+@skipIfLLVMTargetMissing("AArch64")
 def test_arm64_32(self):
 src_dir = self.getSourceDir()
 yaml_path = os.path.join(src_dir, "a.out-arm64_32.yaml")



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

[Lldb-commits] [PATCH] D101406: Rename human-readable name for DW_LANG_Mips_Assembler

[Dimitry Andric via Phabricator via lldb-commits]

dim added a comment.

In D101406#2948152 , @clayborg wrote:

> Everyone uses this for any assembly in their code. Should we get rid of this 
> warning? I find it is useless and would like to see it go.

Well, for other languages it might be a helpful warning, but not for the 
special case of "mips" assembler. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101406

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