[Lldb-commits] [PATCH] D105779: RFC: [lldb] Fix editline unicode on Linux

[Raphael Isemann via Phabricator via lldb-commits]

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

LGTM, thanks!




Comment at: lldb/tools/driver/Driver.cpp:872
+  ::setlocale(LC_ALL, "");
+  ::setlocale(LC_CTYPE, "");
+

jankratochvil wrote:
> teemperor wrote:
> > I don't think we need this if we set `LC_ALL`?
> Originally I have just copy-pasted it. But when testing it more now I think 
> it makes some sense:
> ```
> #include 
> #include 
> #include 
> int main(void) {
>   printf("setlocale(LC_ALL  )=%s\n",setlocale(LC_ALL  ,""));
>   printf("iswprint(0x17e)=%d\n",iswprint(0x17e));
>   printf("setlocale(LC_CTYPE):%s\n",setlocale(LC_CTYPE,""));
>   printf("iswprint(0x17e)=%d\n",iswprint(0x17e));
>   return 0;
> }
> $ LANG=C LC_NAME=foobar LC_CTYPE=en_US.UTF-8 ./setlocale
> setlocale(LC_ALL  )=(null)
> iswprint(0x17e)=0
> setlocale(LC_CTYPE):en_US.UTF-8
> iswprint(0x17e)=1
> ```
> Because: [[ 
> https://pubs.opengroup.org/onlinepubs/009695399/functions/setlocale.html | 
> Setting all of the categories of the locale of the process is similar to 
> successively setting each individual category of the locale of the process, 
> except that all error checking is done before any actions are performed. ]]
Good point, didn't think of that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105779

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

[Lldb-commits] [lldb] 7274848 - [lldb] Fix editline unicode on Linux

[Jan Kratochvil via lldb-commits]

Author: Jan Kratochvil
Date: 2021-07-13T12:37:53+02:00
New Revision: 72748488addd651beb7b60da462c721f3e175357

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

LOG: [lldb] Fix editline unicode on Linux

Based on:
  [lldb-dev] proposed change to remove conditional WCHAR support in libedit 
wrapper
  https://lists.llvm.org/pipermail/lldb-dev/2021-July/016961.html

There is already setlocale in lldb/source/Core/IOHandlerCursesGUI.cpp
but that does not apply for Editline GUI editing.

Unaware how to make automated test for this, it requires pty.

Reviewed By: teemperor

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

Added: 


Modified: 
lldb/source/Core/IOHandlerCursesGUI.cpp
lldb/tools/driver/Driver.cpp

Removed: 




diff  --git a/lldb/source/Core/IOHandlerCursesGUI.cpp 
b/lldb/source/Core/IOHandlerCursesGUI.cpp
index d37271bd1e53a..fb18e7d90eaa8 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -67,7 +67,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -2627,8 +2626,6 @@ class Application {
   }
 
   void Initialize() {
-::setlocale(LC_ALL, "");
-::setlocale(LC_CTYPE, "");
 m_screen = ::newterm(nullptr, m_out, m_in);
 ::start_color();
 ::curs_set(0);

diff  --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp
index b8c5aec5fd80b..a6a4a2a1b80b8 100644
--- a/lldb/tools/driver/Driver.cpp
+++ b/lldb/tools/driver/Driver.cpp
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -867,6 +868,10 @@ static llvm::Optional 
InitializeReproducer(llvm::StringRef argv0,
 }
 
 int main(int argc, char const *argv[]) {
+  // Editline uses for example iswprint which is dependent on LC_CTYPE.
+  std::setlocale(LC_ALL, "");
+  std::setlocale(LC_CTYPE, "");
+
   // Setup LLVM signal handlers and make sure we call llvm_shutdown() on
   // destruction.
   llvm::InitLLVM IL(argc, argv, /*InstallPipeSignalExitHandler=*/false);



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

[Lldb-commits] [PATCH] D105779: RFC: [lldb] Fix editline unicode on Linux

[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.
jankratochvil marked an inline comment as done.
Closed by commit rG72748488addd: [lldb] Fix editline unicode on Linux (authored 
by jankratochvil).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105779

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/tools/driver/Driver.cpp


Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -867,6 +868,10 @@
 }
 
 int main(int argc, char const *argv[]) {
+  // Editline uses for example iswprint which is dependent on LC_CTYPE.
+  std::setlocale(LC_ALL, "");
+  std::setlocale(LC_CTYPE, "");
+
   // Setup LLVM signal handlers and make sure we call llvm_shutdown() on
   // destruction.
   llvm::InitLLVM IL(argc, argv, /*InstallPipeSignalExitHandler=*/false);
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -67,7 +67,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -2627,8 +2626,6 @@
   }
 
   void Initialize() {
-::setlocale(LC_ALL, "");
-::setlocale(LC_CTYPE, "");
 m_screen = ::newterm(nullptr, m_out, m_in);
 ::start_color();
 ::curs_set(0);


Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -867,6 +868,10 @@
 }
 
 int main(int argc, char const *argv[]) {
+  // Editline uses for example iswprint which is dependent on LC_CTYPE.
+  std::setlocale(LC_ALL, "");
+  std::setlocale(LC_CTYPE, "");
+
   // Setup LLVM signal handlers and make sure we call llvm_shutdown() on
   // destruction.
   llvm::InitLLVM IL(argc, argv, /*InstallPipeSignalExitHandler=*/false);
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -67,7 +67,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -2627,8 +2626,6 @@
   }
 
   void Initialize() {
-::setlocale(LC_ALL, "");
-::setlocale(LC_CTYPE, "");
 m_screen = ::newterm(nullptr, m_out, m_in);
 ::start_color();
 ::curs_set(0);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 358228.
OmarEmaraDev marked 10 inline comments as done.
OmarEmaraDev added a comment.

- Use unique pointers for field delegates.
- Fix typos in Select methods.
- Set the process name to the main executable name by default.
- Turn plugin name into a choice field.
- Update debugger target if a new one was created.
- Remove superfluous continue operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -40,6 +40,7 @@
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
@@ -608,9 +609,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,9 +1061,18 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
-typedef std::shared_ptr FieldDelegateSP;
+typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
@@ -1227,7 +1247,6 @@
 
   void SetError(const char *error) { m_error = error; }
 
-  // Returns the text content of the field.
   const std::string &GetText() { return m_content; }
 
 protected:
@@ -1634,7 +1653,7 @@
   m_selection_type = SelectionType::NewButton;
   }
 
-  HandleCharResult SelecteNext(int key) {
+  HandleCharResult SelectNext(int key) {
 if (m_selection_type == SelectionType::NewButton)
   return eKeyNotHandled;
 
@@ -1707,7 +1726,7 @@
   }
   break;
 case '\t':
-  SelecteNext(key);
+  SelectNext(key);
   return eKeyHandled;
 case KEY_SHIFT_TAB:
   SelectPrevious(key);
@@ -1812,7 +1831,11 @@
 
   virtual ~FormDelegate() = default;
 
-  FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
 
@@ -1832,8 +1855,7 @@
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
 TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1841,8 +1863,7 @@
   bool need_to_exist = true) {
 FileFieldDelegate *delegate =
 new FileFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1851,22 +1872,19 @@
 bool need_to_exist = true) {
 DirectoryFieldDelegate *delegate =
 new DirectoryFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
 IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   BooleanFieldDelegate *AddBooleanField(const char *label, bool content) {
 BooleanFieldDelegate *delegate = new BooleanFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp)

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev planned changes to this revision.
OmarEmaraDev added a comment.

Currently working on the detach/kill form.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2346
+  return;
+
+if (process->GetShouldDetach()) {

clayborg wrote:
> We might want to pop up a dialog here asking the user if they want to kill or 
> detach since we have a GUI. In the "process attach" command we confirm with 
> the user if the terminal is interactive. 
> A few ways we can do this:
> - popup a form asking the user to kill/detach/cancel when the user hits 
> submit on this form
> - pop up the dialog before even showing the this form and asking the user to 
> kill or detach, then show this form after that has been handled
> - not allow the Process->Attach to happen by disabling the menu item when a 
> process is running.
> 
> 
I think the first option is the best, because it delays this decision at the 
very end giving the user as much time as possible to cancel attaching without 
altering the debugger state.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2420-2421
+
+if (attach_info.GetContinueOnceAttached())
+  process_sp->Resume();
+

clayborg wrote:
> You shouldn't need to do this since you already called 
> ProcessAttachInfo::SetContinueOnceAttached(true) on the attach_info. LLDB 
> should take care of this for you already I think.
In the command code, there is this snippet of code at the end, so I though it 
might be necessary:

```lang=cpp
// This supports the use-case scenario of immediately continuing the
// process once attached.
if (m_options.attach_info.GetContinueOnceAttached())
  m_interpreter.HandleCommand("process continue", eLazyBoolNo, result);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

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

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 358268.
OmarEmaraDev added a comment.

- Add kill/detach form.
- Fix forms with no fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -40,6 +40,7 @@
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
@@ -608,9 +609,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,9 +1061,18 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
-typedef std::shared_ptr FieldDelegateSP;
+typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
@@ -1227,7 +1247,6 @@
 
   void SetError(const char *error) { m_error = error; }
 
-  // Returns the text content of the field.
   const std::string &GetText() { return m_content; }
 
 protected:
@@ -1634,7 +1653,7 @@
   m_selection_type = SelectionType::NewButton;
   }
 
-  HandleCharResult SelecteNext(int key) {
+  HandleCharResult SelectNext(int key) {
 if (m_selection_type == SelectionType::NewButton)
   return eKeyNotHandled;
 
@@ -1707,7 +1726,7 @@
   }
   break;
 case '\t':
-  SelecteNext(key);
+  SelectNext(key);
   return eKeyHandled;
 case KEY_SHIFT_TAB:
   SelectPrevious(key);
@@ -1812,7 +1831,11 @@
 
   virtual ~FormDelegate() = default;
 
-  FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
 
@@ -1832,8 +1855,7 @@
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
 TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1841,8 +1863,7 @@
   bool need_to_exist = true) {
 FileFieldDelegate *delegate =
 new FileFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1851,22 +1872,19 @@
 bool need_to_exist = true) {
 DirectoryFieldDelegate *delegate =
 new DirectoryFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
 IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   BooleanFieldDelegate *AddBooleanField(const char *label, bool content) {
 BooleanFieldDelegate *delegate = new BooleanFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1874,8 +1892,7 @@
 std::vector choices) {
 ChoicesFieldDelegate *delegate =
 new ChoicesFieldDelegate(label, height, choices);

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Updated UI:

F17912164: 20210713-170719.png <https://reviews.llvm.org/F17912164>

Kill/Detach form.

F17912163: 20210713-170734.png <https://reviews.llvm.org/F17912163>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

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

[Lldb-commits] [PATCH] D105178: [lldb][AArch64] Annotate synchronous tag faults

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 358276.
DavidSpickett added a comment.

- Removed use of tagged address ABI
- Set pointer tags with a function instead of generator intrinsic
- Add a memory access to confirm mapping was done properly
- Add a few more comments about mmap and others


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105178

Files:
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
  lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
  
lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
  lldb/test/API/linux/aarch64/mte_tag_faults/main.c

Index: lldb/test/API/linux/aarch64/mte_tag_faults/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/main.c
@@ -0,0 +1,56 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+// Set bits 59-56 to tag, removing any existing tag
+static char *set_tag(char *ptr, size_t tag) {
+  return (char *)(((size_t)ptr & ~((size_t)0xf << 56)) | (tag << 56));
+}
+
+int main(int argc, char const *argv[]) {
+  // We assume that the test runner has checked we're on an MTE system
+
+  // Only expect to get the fault type
+  if (argc != 2)
+return 1;
+
+  unsigned long prctl_arg2 = 0;
+  if (!strcmp(argv[1], "sync"))
+prctl_arg2 = PR_MTE_TCF_SYNC;
+  else if (!strcmp(argv[1], "async"))
+prctl_arg2 = PR_MTE_TCF_ASYNC;
+  else
+return 1;
+
+  // Set fault type
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL, prctl_arg2, 0, 0, 0))
+return 1;
+
+  // Allocate some memory with tagging enabled that we
+  // can read/write if we use correct tags.
+  char *buf = mmap(0, sysconf(_SC_PAGESIZE), PROT_MTE | PROT_READ | PROT_WRITE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+return 1;
+
+  // Our pointer will have tag 9
+  char *tagged_buf = set_tag(buf, 9);
+  // Set allocation tags for the first 2 granules
+  __arm_mte_set_tag(set_tag(tagged_buf, 9));
+  __arm_mte_set_tag(set_tag(tagged_buf+16, 10));
+
+  // Confirm that we can write when tags match
+  *tagged_buf = ' ';
+
+  // Breakpoint here
+  // Faults because tag 9 in the ptr != allocation tag of 10
+  *(tagged_buf+16) = '?';
+
+  return 0;
+}
Index: lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
@@ -0,0 +1,59 @@
+"""
+Test reporting of MTE tag access faults.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxMTEMemoryTagFaultsTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def setup_mte_test(self, fault_type):
+if not self.isAArch64MTE():
+self.skipTest('Target must support MTE.')
+
+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', '// Breakpoint here'),
+num_expected_locations=1)
+
+self.runCmd("run {}".format(fault_type), 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'])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_sync(self):
+self.setup_mte_test("sync")
+# The logical tag should be included in the fault address
+# and we know what the bottom byte should be.
+self.expect("continue",
+patterns=[
+"\* thread #1, name = 'a.out', stop reason = signal SIGSEGV: "
+"sync tag check fault \(fault address: 0x9[0-9A-Fa-f]+10\ "
+"logical tag: 0x9 allocation tag: 0xa\)"])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_async(self):
+self.setup_mte_test("async")
+self.expect("continue",
+substrs=[
+"* thread #1, name = 'a.out', stop reason = "
+"signal SIGSEGV: async tag check fault"])
Index: lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -ma

[Lldb-commits] [PATCH] D105178: [lldb][AArch64] Annotate synchronous tag faults

[David Spickett via Phabricator via lldb-commits]

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

> This is actually independent of the tagged address ABI.

Thanks for the info, I keep thinking I need to enable it but never do. Updated 
the test case and commit msg.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105178

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

[Lldb-commits] [PATCH] D105178: [lldb][AArch64] Annotate synchronous tag faults

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 358277.
DavidSpickett added a comment.

clang-format the test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105178

Files:
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
  lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
  
lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
  lldb/test/API/linux/aarch64/mte_tag_faults/main.c

Index: lldb/test/API/linux/aarch64/mte_tag_faults/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/main.c
@@ -0,0 +1,56 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+// Set bits 59-56 to tag, removing any existing tag
+static char *set_tag(char *ptr, size_t tag) {
+  return (char *)(((size_t)ptr & ~((size_t)0xf << 56)) | (tag << 56));
+}
+
+int main(int argc, char const *argv[]) {
+  // We assume that the test runner has checked we're on an MTE system
+
+  // Only expect to get the fault type
+  if (argc != 2)
+return 1;
+
+  unsigned long prctl_arg2 = 0;
+  if (!strcmp(argv[1], "sync"))
+prctl_arg2 = PR_MTE_TCF_SYNC;
+  else if (!strcmp(argv[1], "async"))
+prctl_arg2 = PR_MTE_TCF_ASYNC;
+  else
+return 1;
+
+  // Set fault type
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL, prctl_arg2, 0, 0, 0))
+return 1;
+
+  // Allocate some memory with tagging enabled that we
+  // can read/write if we use correct tags.
+  char *buf = mmap(0, sysconf(_SC_PAGESIZE), PROT_MTE | PROT_READ | PROT_WRITE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+return 1;
+
+  // Our pointer will have tag 9
+  char *tagged_buf = set_tag(buf, 9);
+  // Set allocation tags for the first 2 granules
+  __arm_mte_set_tag(set_tag(tagged_buf, 9));
+  __arm_mte_set_tag(set_tag(tagged_buf + 16, 10));
+
+  // Confirm that we can write when tags match
+  *tagged_buf = ' ';
+
+  // Breakpoint here
+  // Faults because tag 9 in the ptr != allocation tag of 10
+  *(tagged_buf + 16) = '?';
+
+  return 0;
+}
Index: lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
@@ -0,0 +1,59 @@
+"""
+Test reporting of MTE tag access faults.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxMTEMemoryTagFaultsTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def setup_mte_test(self, fault_type):
+if not self.isAArch64MTE():
+self.skipTest('Target must support MTE.')
+
+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', '// Breakpoint here'),
+num_expected_locations=1)
+
+self.runCmd("run {}".format(fault_type), 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'])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_sync(self):
+self.setup_mte_test("sync")
+# The logical tag should be included in the fault address
+# and we know what the bottom byte should be.
+self.expect("continue",
+patterns=[
+"\* thread #1, name = 'a.out', stop reason = signal SIGSEGV: "
+"sync tag check fault \(fault address: 0x9[0-9A-Fa-f]+10\ "
+"logical tag: 0x9 allocation tag: 0xa\)"])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_async(self):
+self.setup_mte_test("async")
+self.expect("continue",
+substrs=[
+"* thread #1, name = 'a.out', stop reason = "
+"signal SIGSEGV: async tag check fault"])
Index: lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -march=armv8.5-a+memtag
+
+include Makefile.rules
Index: lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
===
-

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:118
+  }
+  s.Printf("\nraw trace size %zu\n", *raw_size);
+  return;

the presentation of this line could be better. Something like this would look 
nicer

  thread 1: tid = 123123

- Tracing technology: Intel PT
- Raw trace size: 1231232 bytes 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105717

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

[Lldb-commits] [PATCH] D105914: [lldb] Make TargetList iterable

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: jingham.
JDevlieghere requested review of this revision.

Make it possible to iterate over the TargetList.


https://reviews.llvm.org/D105914

Files:
  lldb/include/lldb/Target/TargetList.h


Index: lldb/include/lldb/Target/TargetList.h
===
--- lldb/include/lldb/Target/TargetList.h
+++ lldb/include/lldb/Target/TargetList.h
@@ -14,6 +14,7 @@
 
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Broadcaster.h"
+#include "lldb/Utility/Iterable.h"
 
 namespace lldb_private {
 
@@ -42,6 +43,11 @@
 return GetStaticBroadcasterClass();
   }
 
+  typedef std::vector collection;
+  typedef LockingAdaptedIterable
+  TargetIterable;
+
   /// Create a new Target.
   ///
   /// Clients must use this function to create a Target. This allows
@@ -179,14 +185,15 @@
 
   lldb::TargetSP GetSelectedTarget();
 
-protected:
-  typedef std::vector collection;
-  // Member variables.
+  TargetIterable Targets() {
+return TargetIterable(m_target_list, m_target_list_mutex);
+  }
+
+private:
   collection m_target_list;
   mutable std::recursive_mutex m_target_list_mutex;
   uint32_t m_selected_target_idx;
 
-private:
   static Status CreateTargetInternal(
   Debugger &debugger, llvm::StringRef user_exe_path,
   llvm::StringRef triple_str, LoadDependentFiles load_dependent_files,


Index: lldb/include/lldb/Target/TargetList.h
===
--- lldb/include/lldb/Target/TargetList.h
+++ lldb/include/lldb/Target/TargetList.h
@@ -14,6 +14,7 @@
 
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Broadcaster.h"
+#include "lldb/Utility/Iterable.h"
 
 namespace lldb_private {
 
@@ -42,6 +43,11 @@
 return GetStaticBroadcasterClass();
   }
 
+  typedef std::vector collection;
+  typedef LockingAdaptedIterable
+  TargetIterable;
+
   /// Create a new Target.
   ///
   /// Clients must use this function to create a Target. This allows
@@ -179,14 +185,15 @@
 
   lldb::TargetSP GetSelectedTarget();
 
-protected:
-  typedef std::vector collection;
-  // Member variables.
+  TargetIterable Targets() {
+return TargetIterable(m_target_list, m_target_list_mutex);
+  }
+
+private:
   collection m_target_list;
   mutable std::recursive_mutex m_target_list_mutex;
   uint32_t m_selected_target_idx;
 
-private:
   static Status CreateTargetInternal(
   Debugger &debugger, llvm::StringRef user_exe_path,
   llvm::StringRef triple_str, LoadDependentFiles load_dependent_files,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 358373.
shafik added a comment.

- Modified `FindTypeForAutoReturnForDIE` to take into account if we have 
multiple symbols with the same name.
- Modified `ParseSubroutine` to take into account that case we get the 
definition first, this can happen when we set a breakpoint inside the 
definition.
- Added new tests suggested by Raphael.


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

https://reviews.llvm.org/D105564

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/test/API/lang/cpp/auto_return/Makefile
  lldb/test/API/lang/cpp/auto_return/TestCppAutoReturn.py
  lldb/test/API/lang/cpp/auto_return/main.cpp
  lldb/test/API/lang/cpp/auto_return/other.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/auto_return_symtab.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/auto_return_symtab.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/auto_return_symtab.s
@@ -0,0 +1,257 @@
+# This tests that lldb when using symbol table we are able to find the definition
+# for an auto return function.
+
+# RUN: llvm-mc -triple x86_64-apple-macosx10.15.0 %s -filetype=obj > %t.o
+# RUN: lldb-test symbols --dump-clang-ast %t.o | FileCheck %s
+
+# CHECK: CXXMethodDecl {{.*}} <>  f 'int ()'
+
+# This was compiled from the following code:
+#
+# struct A {
+# auto f();
+# };
+#
+# auto A::f() {
+# return 0;
+# }
+#
+# Compiled using:
+#
+#  -target x86_64-apple-macosx10.15.0
+#
+# and edited to remove some uneeded sections.
+
+	.section	__TEXT,__text,regular,pure_instructions
+	.globl	__ZN1A1fEv  ## -- Begin function _ZN1A1fEv
+	.p2align	4, 0x90
+__ZN1A1fEv: ## @_ZN1A1fEv
+Lfunc_begin0:
+	.cfi_startproc
+## %bb.0:   ## %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	movq	%rdi, -8(%rbp)
+Ltmp0:
+	xorl	%eax, %eax
+	popq	%rbp
+	retq
+Ltmp1:
+Lfunc_end0:
+	.cfi_endproc
+## -- End function
+	.section	__DWARF,__debug_abbrev,regular,debug
+Lsection_abbrev:
+	.byte	1   ## Abbreviation Code
+	.byte	17  ## DW_TAG_compile_unit
+	.byte	1   ## DW_CHILDREN_yes
+	.byte	37  ## DW_AT_producer
+	.byte	14  ## DW_FORM_strp
+	.byte	19  ## DW_AT_language
+	.byte	5   ## DW_FORM_data2
+	.byte	3   ## DW_AT_name
+	.byte	14  ## DW_FORM_strp
+	.ascii	"\202|" ## DW_AT_LLVM_sysroot
+	.byte	14  ## DW_FORM_strp
+	.ascii	"\357\177"  ## DW_AT_APPLE_sdk
+	.byte	14  ## DW_FORM_strp
+	.byte	16  ## DW_AT_stmt_list
+	.byte	23  ## DW_FORM_sec_offset
+	.byte	27  ## DW_AT_comp_dir
+	.byte	14  ## DW_FORM_strp
+	.byte	17  ## DW_AT_low_pc
+	.byte	1   ## DW_FORM_addr
+	.byte	18  ## DW_AT_high_pc
+	.byte	6   ## DW_FORM_data4
+	.byte	0   ## EOM(1)
+	.byte	0   ## EOM(2)
+	.byte	2   ## Abbreviation Code
+	.byte	19  ## DW_TAG_structure_type
+	.byte	1   ## DW_CHILDREN_yes
+	.byte	54  ## DW_AT_calling_convention
+	.byte	11  ## DW_FORM_data1
+	.byte	3   ## DW_AT_name
+	.byte	14  ## DW_FORM_strp
+	.byte	11  ## DW_AT_byte_size
+	.byte	11  ## DW_FORM_data1
+	.byte	58  ## DW_AT_decl_file
+	.byte	11  ## DW_FORM_data1
+	.byte	59  ## DW_AT_decl_line
+	.byte	11  ## DW_FORM_data1
+	.byte	0   ## EOM(1)
+	.byte	0   ## EOM(2)
+	.byte	3   ## Abbreviation Code
+	.byte	46  ## DW_TAG_subprogram
+	.byte	1   ## DW_CHILDREN_yes
+	.byte	110 ## DW_AT_linkage_name
+	.byte	14  ## DW_FORM_strp
+	.byte	3   ## DW_AT_name
+	.byte	14  ## DW_FORM_strp
+	.byte	58   

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 

You don't want to return a unique_ptr here, if you do something like:
```
FieldDelegateUP field_up = form->GetField(1);
```
You have now given away the one reference to this field to the local variable 
and it will destruct the item when the local goes out of scope.

Easy solution: just return a pointer, not a unique_ptr. The FormDelegate owns 
all of its fields, so it is ok to hand out a pointer as others should 
temporarily use the FieldDelegate. Also we should bounds check "field_index".



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1985
 
-FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
 ScrollContext context = field->FieldDelegateGetScrollContext();





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2152-2153
+  if (m_selection_type == SelectionType::Field) {
+FieldDelegateUP &next_field =
+m_delegate_sp->GetField(m_selection_index);
+next_field->FieldDelegateSelectFirstElement();





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2159
 
-FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
 if (!field->FieldDelegateOnLastOrOnlyElement()) {





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2176
+if (m_selection_type == SelectionType::Field) {
+  FieldDelegateUP &next_field = m_delegate_sp->GetField(m_selection_index);
+  next_field->FieldDelegateSelectFirstElement();





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2208-2209
+  if (m_selection_type == SelectionType::Field) {
+FieldDelegateUP &previous_field =
+m_delegate_sp->GetField(m_selection_index);
+previous_field->FieldDelegateSelectLastElement();





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2215
 
-FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
 if (!field->FieldDelegateOnFirstOrOnlyElement()) {

No need to use a std::unique_ptr here, and GetField should return a pointer as 
mentioned above.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2232-2233
+if (m_selection_type == SelectionType::Field) {
+  FieldDelegateUP &previous_field =
+  m_delegate_sp->GetField(m_selection_index);
+  previous_field->FieldDelegateSelectLastElement();





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2274
 if (m_selection_type == SelectionType::Field) {
-  FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+  FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
   return field->FieldDelegateHandleChar(key);





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2298
+  DetachOrKillProcessFormDelegate(Process *process) : m_process(process) {
+if (process->GetShouldDetach()) {
+  SetError("There is a running process, detach from it?");

When we have interaction, we should allow the user to select kill or detach. 
The GetShouldDetach is only for when the user doesn't interact. Can we use a 
ChoicesField for this with the two options? We can even go a step further and 
add three options: "Detach", "Detach (stopped)", and "Kill". This is easy since 
lldb_private::Process has this option on detach:

```
  Status Process::Detach(bool keep_stopped);
```

When you detach with "keep_stopped" set to true the process will not resume. 
This allows another debugger to be attached, or someone to run another tool on 
the process in its suspended state.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2324
+}
+window.GetParent()->RemoveSubWindow(&window);
+  }

Does anyone remove the sub window if the user cancels this dialog?



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2342-2343
+AddTextField("Process Name", GetDefaultProcessName().c_str());
+m_plugin_field =
+AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_continue_field = AddBooleanField("Continue once attached.", false);

most people when attaching won't ever specify the plug-in names. I wonder if we 
should include an extra boolean field that enables this? Maybe:
```
m_show_plugin_field = AddBooleanField("Sho

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/Target/Trace.h:155
+  /// If \b false, print compact stats
+  virtual void DumpTraceStats(Thread &thread, Stream &s, bool verbose) = 0;
+

Are any statistics being dumped here? Maybe DumpTraceSummary(...) or 
DumpTraceInfo(...) would be better?



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2195-2197
+LoadSubCommand(
+"stats", CommandObjectSP(new 
CommandObjectTraceDumpStats(interpreter)));
   }

Since we are iterating on this new command, I am wondering if we should have 
just a "thread trace dump" command with options?
```
(lldb) thread trace dump --stats
(lldb) thread trace dump --instructions
```
This way the user could dump more than one thing at a time with a single 
command?:
```
(lldb) thread trace dump --stats --instructions
```
Just thinking out loud here, so no worries if you feel this is already correct 
and should stay this way




Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:118
+  }
+  s.Printf("\nraw trace size %zu\n", *raw_size);
+  return;

wallace wrote:
> the presentation of this line could be better. Something like this would look 
> nicer
> 
>   thread 1: tid = 123123
> 
> - Tracing technology: Intel PT
> - Raw trace size: 1231232 bytes 
The "Tracing technology: Intel PT" should probably come before any of the 
thread infos if it is added:
```
Tracing technology: Intel PT
thread 1: tid = 111, size = 0x1000
thread 2: tid = 222, size = 0x1000
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105717

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

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:406
 
+  virtual lldb::TypeSP
+  FindTypeForAutoReturnForDIE(const DWARFDIE &die,

teemperor wrote:
> If this is virtual then I guess `SymbolFileDWARFDwo` should overload it?
I am actually not sure if this has to be virtual or not, let me dig into this 
and see if there would be any difference for `SymbolFileDWARFDwo` or not.


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

https://reviews.llvm.org/D105564

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

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 358399.
OmarEmaraDev marked 10 inline comments as done.
OmarEmaraDev added a comment.

- Return raw pointers instead of unique ones.
- Add Show Advance Settings option.
- Allow detaching and killing at the same time.
- Allow detaching while keeping process stopped.
- Handle review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -40,6 +40,7 @@
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
@@ -608,9 +609,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,9 +1061,18 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
-typedef std::shared_ptr FieldDelegateSP;
+typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
@@ -1227,7 +1247,6 @@
 
   void SetError(const char *error) { m_error = error; }
 
-  // Returns the text content of the field.
   const std::string &GetText() { return m_content; }
 
 protected:
@@ -1634,7 +1653,7 @@
   m_selection_type = SelectionType::NewButton;
   }
 
-  HandleCharResult SelecteNext(int key) {
+  HandleCharResult SelectNext(int key) {
 if (m_selection_type == SelectionType::NewButton)
   return eKeyNotHandled;
 
@@ -1707,7 +1726,7 @@
   }
   break;
 case '\t':
-  SelecteNext(key);
+  SelectNext(key);
   return eKeyHandled;
 case KEY_SHIFT_TAB:
   SelectPrevious(key);
@@ -1812,7 +1831,15 @@
 
   virtual ~FormDelegate() = default;
 
-  FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
+  FieldDelegate *GetField(uint32_t field_index) {
+if (field_index < m_fields.size())
+  return m_fields[field_index].get();
+return nullptr;
+  }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
 
@@ -1832,8 +1859,7 @@
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
 TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1841,8 +1867,7 @@
   bool need_to_exist = true) {
 FileFieldDelegate *delegate =
 new FileFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1851,22 +1876,19 @@
 bool need_to_exist = true) {
 DirectoryFieldDelegate *delegate =
 new DirectoryFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
 IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   BooleanFieldDelegate *AddBooleanField(const char *label, bool content) {
 BooleanFieldDelegate *delegate = new BooleanFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_b

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Two actions with an option:

F17916215: 20210713-214929.png <https://reviews.llvm.org/F17916215>




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 

clayborg wrote:
> You don't want to return a unique_ptr here, if you do something like:
> ```
> FieldDelegateUP field_up = form->GetField(1);
> ```
> You have now given away the one reference to this field to the local variable 
> and it will destruct the item when the local goes out of scope.
> 
> Easy solution: just return a pointer, not a unique_ptr. The FormDelegate owns 
> all of its fields, so it is ok to hand out a pointer as others should 
> temporarily use the FieldDelegate. Also we should bounds check "field_index".
I see, I though returning it as a reference wouldn't case such issues.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2298
+  DetachOrKillProcessFormDelegate(Process *process) : m_process(process) {
+if (process->GetShouldDetach()) {
+  SetError("There is a running process, detach from it?");

clayborg wrote:
> When we have interaction, we should allow the user to select kill or detach. 
> The GetShouldDetach is only for when the user doesn't interact. Can we use a 
> ChoicesField for this with the two options? We can even go a step further and 
> add three options: "Detach", "Detach (stopped)", and "Kill". This is easy 
> since lldb_private::Process has this option on detach:
> 
> ```
>   Status Process::Detach(bool keep_stopped);
> ```
> 
> When you detach with "keep_stopped" set to true the process will not resume. 
> This allows another debugger to be attached, or someone to run another tool 
> on the process in its suspended state.
> 
I think this might be a good place to add two actions with possible parameters, 
let me know if you think this would be a good idea.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2324
+}
+window.GetParent()->RemoveSubWindow(&window);
+  }

clayborg wrote:
> Does anyone remove the sub window if the user cancels this dialog?
Yes. This is done in `WindowDelegateHandleChar` of the `FormWindowDelegate`.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2342-2343
+AddTextField("Process Name", GetDefaultProcessName().c_str());
+m_plugin_field =
+AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_continue_field = AddBooleanField("Continue once attached.", false);

clayborg wrote:
> most people when attaching won't ever specify the plug-in names. I wonder if 
> we should include an extra boolean field that enables this? Maybe:
> ```
> m_show_plugin_field = AddBooleanField("Show advanced settings.", false);
> ```
> And then show/hide any settings that aren't commonly used like the "Plugin 
> Name" field?
This what I do for the launch and target creation forms. In this case, I didn't 
think it was worth it because it was just one field. But now that it is a 
choice field that takes a bit more space, maybe it would be a good idea to do 
this here as well.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2418
+form_window_sp->SetDelegate(window_delegate_sp);
+
+return true;

clayborg wrote:
> Is the form for the kill/detach done by the time it reaches this code? If so, 
> what if the user cancels the Kill/Detach dialog? Can we return false here if 
> the user cancels?
No, the return boolean here essentially denotes that a form for 
killing/detaching was spawned and that the attach form shouldn't continue 
executing. Once the user executes an action or cancels, the user will end up in 
the same attach from, where the user can execute attach again where it will 
succeed without the kill/detach dialog.

I can't think of a way to make the form block execution of another form until 
it is done without resorting to complicated trickery.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

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

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2195-2197
+LoadSubCommand(
+"stats", CommandObjectSP(new 
CommandObjectTraceDumpStats(interpreter)));
   }

clayborg wrote:
> Since we are iterating on this new command, I am wondering if we should have 
> just a "thread trace dump" command with options?
> ```
> (lldb) thread trace dump --stats
> (lldb) thread trace dump --instructions
> ```
> This way the user could dump more than one thing at a time with a single 
> command?:
> ```
> (lldb) thread trace dump --stats --instructions
> ```
> Just thinking out loud here, so no worries if you feel this is already 
> correct and should stay this way
> 
I think that's a bit too much for this patch, but I'll keep it in mind if we 
end up having more dumpers.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:118
+  }
+  s.Printf("\nraw trace size %zu\n", *raw_size);
+  return;

clayborg wrote:
> wallace wrote:
> > the presentation of this line could be better. Something like this would 
> > look nicer
> > 
> >   thread 1: tid = 123123
> > 
> > - Tracing technology: Intel PT
> > - Raw trace size: 1231232 bytes 
> The "Tracing technology: Intel PT" should probably come before any of the 
> thread infos if it is added:
> ```
> Tracing technology: Intel PT
> thread 1: tid = 111, size = 0x1000
> thread 2: tid = 222, size = 0x1000
> ```
That's a pretty good idea.

@hanbingwang , you can invoke trace_sp->GetPluginName() for getting the name of 
the tracing technology being used


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105717

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

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace dump ctf -f ` command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Target/TraceHTR.h:31
+public:
+  HTRBlockMetadata(Thread &thread, TraceInstruction curr_instruction,
+   llvm::Optional next_instruction);

  const TraceInstruction &curr_instruction
to avoid making an unnecessary copy



Comment at: lldb/include/lldb/Target/TraceHTR.h:33
+   llvm::Optional next_instruction);
+  HTRBlockMetadata(size_t num_instructions,
+   std::unordered_map func_calls)

pass the map by reference



Comment at: lldb/include/lldb/Target/TraceHTR.h:42-68
+  friend llvm::json::Value toJSON(const HTRBlockMetadata &metadata);
+  template  friend llvm::json::Value toJSON(const TraceHTR &layer);
+};
+
+/// \class HTRBlock TraceHTR.h "lldb/Target/TraceHTR.h"
+/// Trace agnostic block structure
+/// Sequences of blocks are merged to create a new, single block

the consistent style is that toJSON methods are not class members but static 
functions



Comment at: lldb/include/lldb/Target/TraceHTR.h:73
+/// Maps the unique Block IDs to their respective Blocks
+template  class HTRBlockDefs {
+public:

clayborg wrote:
> Does this need to be templatized? Are we using this same class with multiple 
> different TMetadata classes or are we always using it with HTRBlockMetadata?
+1
I don't think you should go with templates unless you had more TMetadata objects


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105741

___
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 358429.

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
@@ -16,6 +16,7 @@
 #include "llvm/Support/Errno.h"
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -143,10 +144,30 @@
 // 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::filesystem::path fp(proc_fd_path);
+if (std::filesystem::is_directory(fp)) {
+  std::vector files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (auto &entry : std::filesystem::directory_iterator(proc_fd_path)) {
+int fd =
+std::stoi(entry.path().string().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 &file_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)
   ExitWithError(error_fd, "ptrace");


Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
===
--- lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -16,6 +16,7 @@
 #include "llvm/Support/Errno.h"
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -143,10 +144,30 @@
 // 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::filesystem::path fp(proc_fd_path);
+if (std::filesystem::is_directory(fp)) {
+  std::vector files_to_close;
+  // Directory iterator doesn't ensure any sequence.
+  for (auto &entry : std::filesystem::directory_iterator(proc_fd_path)) {
+int fd =
+std::stoi(entry.path().string().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 &file_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)
   ExitWithError(error_fd, "ptrace");
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Raphael Isemann via Phabricator via lldb-commits]

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

I guess Phabricator just sent this back to review automatically when you 
updated the diff? Just mark this as 'needs review' again when this is ready for 
review. I'll send this back in the meantime to clear my review queue.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:899
+  clang::QualType qt = ClangUtil::GetQualType(function_type);
+  const auto *ft = qt->getAs();
+  TypeSystemClang *ts =

unused variable



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:919
   // Parse the function children for the parameters
-
   DWARFDIE decl_ctx_die;

unrelated change?


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

https://reviews.llvm.org/D105564

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

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach 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.

I like the new detach/kill dialog as it incorporates many of your features that 
you added to the forms! Very close, just a few minor issues and this will be 
good to go.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+  FieldDelegate *GetField(uint32_t field_index) {
+if (field_index < m_fields.size())

You are correct unless the caller of the function doesn't use a reference. All 
of the call sites you had in here were using references, so there wasn't an 
issue. But if someone else comes along and actually wrote code like the snipped 
I posted above, it would transfer ownership and that would be bad.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2051-2054
+  if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+continue;
   bool is_field_selected = a_field_is_selected && m_selection_index == i;
+  FieldDelegate *field = m_delegate_sp->GetField(i);





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2421
+WindowDelegateSP window_delegate_sp =
+WindowDelegateSP(new FormWindowDelegate(form_delegate_sp));
+form_window_sp->SetDelegate(window_delegate_sp);

Ok, thanks for the explanation. It makes sense.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2425
+return true;
+  }
+

Should be easy to verify if it is needed or not. If it is, we will need to add 
it back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

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

[Lldb-commits] [PATCH] D105134: [jenkins] Update script to use cross-project lit test suite

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 355494.
mib added a comment.

Address @jhenderson comment.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D105134

Files:
  zorg/jenkins/jobs/jobs/lldb-cmake
  zorg/jenkins/jobs/jobs/llvm-coverage

Index: zorg/jenkins/jobs/jobs/llvm-coverage
===
--- zorg/jenkins/jobs/jobs/llvm-coverage
+++ zorg/jenkins/jobs/jobs/llvm-coverage
@@ -1,7 +1,7 @@
 #!/usr/bin/env groovy
 pipeline {
 agent { label 'green-dragon-23' }
-
+
 parameters {
 string(name: 'GIT_REVISION', defaultValue: '*/main', description: 'Git revision to build')
 string(name: 'ARTIFACT', defaultValue: 'clang-stage1-RA/latest', description: 'Compiler artifact to use for building the project')
@@ -35,21 +35,21 @@
 sh '''
 set -u
 rm -rf build.properties
-
+
 cd llvm-project
 git tag -a -m "First Commit" first_commit 97724f18c79c7cc81ced24239eb5e883bf1398ef || true
-
+
 git_desc=$(git describe --match "first_commit")
-
+
 export GIT_DISTANCE=$(echo ${git_desc} | cut -f 2 -d "-")
-
+
 sha=$(echo ${git_desc} | cut -f 3 -d "-")
 export GIT_SHA=${sha:1}
-
+
 cd -
 
 export PATH=$PATH:/usr/bin:/usr/local/bin
-
+
 python llvm-zorg/zorg/jenkins/monorepo_build.py cmake build \
   --cmake-build-target FileCheck \
   --cmake-build-target count \
@@ -59,10 +59,10 @@
   --projects "" \
   --noinstall \
   --noupload
-
+
 python llvm-zorg/zorg/jenkins/monorepo_build.py lldb-cmake build \
   --assertions \
-  --projects="clang;libcxx;libcxxabi;lldb;debuginfo-tests"  \
+  --projects="clang;libcxx;libcxxabi;lldb;cross-project-tests"  \
   --cmake-flag="-DPYTHON_LIBRARY=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/libpython3.7.dylib" \
   --cmake-flag="-DPYTHON_INCLUDE_DIR=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/include/python3.7m" \
   --cmake-flag="-DPYTHON_LIBRARY_DEBUG=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/libpython3.7.dylib" \
@@ -85,7 +85,7 @@
 export PATH=$PATH:/usr/bin:/usr/local/bin
 
 rm -rf test/results.xml
-
+
 BUILD_DIR=$(pwd)/lldb-build
 FAST_BUILD_DIR=$(pwd)/clang-build
 REPORT_DIR=$(pwd)/coverage-reports
@@ -93,31 +93,31 @@
 LLVM_PROFDATA=$HOST/llvm-profdata
 LLVM_COV=$HOST/llvm-cov
 ARTIFACT_PREP_SCRIPT=$WORKSPACE/llvm-project/llvm/utils/prepare-code-coverage-artifact.py
-
+
 FAST_TOOLS=(FileCheck count not)
 for TOOL in ${FAST_TOOLS[@]}; do
   cp $FAST_BUILD_DIR/bin/$TOOL $BUILD_DIR/bin/$TOOL
 done
-
+
 # Clear out any stale profiles.
 rm -rf $BUILD_DIR/profiles
 
 # Run the tests.
 IGNORE_ERRORS_OVERRIDE=1 python llvm-zorg/zorg/jenkins/monorepo_build.py lldb-cmake testlong || echo "Some tests may have failed."
-
+
 cd $BUILD_DIR
 ninja -k 0 check-llvm check-clang || echo "Some tests may have failed."
 cd -
-
+
 COV_BINARIES=$(find $BUILD_DIR/bin $BUILD_DIR/lib -depth 1 -type f -exec file {} \\; | grep Mach-O | cut -d':' -f1 | grep -vE '/(FileCheck|count|not)$' | xargs)
-
+
 rm -rf $REPORT_DIR
 mkdir -p $REPORT_DIR
 python $ARTIFACT_PREP_SCRIPT $LLVM_PROFDATA $LLVM_COV $BUILD_DIR/profiles $REPORT_DIR $COV_BINARIES --unified-report --restrict $WORKSPACE/llvm-project
-
+
 scp -r $REPORT_DIR buildslave@labmaster2.local:/Library/WebServer/Documents/coverage
 ssh buildslave@labmaster2.local "chmod -R 777 /Library/WebServer/Documents/coverage"
-
+
 rm -rf lldb-build/profiles
 '''
 }
Index: zorg/jenkins/jobs/jobs/lldb-cmake
===
--- zorg/jenkins/jobs/jobs/lldb-cmake
+++ zorg/jenkins/jobs/jobs/lldb-cmake
@@ -51,11 +51,11 @@
 

[Lldb-commits] [PATCH] D105134: [jenkins] Update script to use cross-project lit test suite

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rZORGe48da4b7b382: [jenkins] Update script to use cross 
project lit test suite (authored by mib).

Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D105134

Files:
  zorg/jenkins/jobs/jobs/lldb-cmake
  zorg/jenkins/jobs/jobs/llvm-coverage

Index: zorg/jenkins/jobs/jobs/llvm-coverage
===
--- zorg/jenkins/jobs/jobs/llvm-coverage
+++ zorg/jenkins/jobs/jobs/llvm-coverage
@@ -1,7 +1,7 @@
 #!/usr/bin/env groovy
 pipeline {
 agent { label 'green-dragon-23' }
-
+
 parameters {
 string(name: 'GIT_REVISION', defaultValue: '*/main', description: 'Git revision to build')
 string(name: 'ARTIFACT', defaultValue: 'clang-stage1-RA/latest', description: 'Compiler artifact to use for building the project')
@@ -35,21 +35,21 @@
 sh '''
 set -u
 rm -rf build.properties
-
+
 cd llvm-project
 git tag -a -m "First Commit" first_commit 97724f18c79c7cc81ced24239eb5e883bf1398ef || true
-
+
 git_desc=$(git describe --match "first_commit")
-
+
 export GIT_DISTANCE=$(echo ${git_desc} | cut -f 2 -d "-")
-
+
 sha=$(echo ${git_desc} | cut -f 3 -d "-")
 export GIT_SHA=${sha:1}
-
+
 cd -
 
 export PATH=$PATH:/usr/bin:/usr/local/bin
-
+
 python llvm-zorg/zorg/jenkins/monorepo_build.py cmake build \
   --cmake-build-target FileCheck \
   --cmake-build-target count \
@@ -59,10 +59,10 @@
   --projects "" \
   --noinstall \
   --noupload
-
+
 python llvm-zorg/zorg/jenkins/monorepo_build.py lldb-cmake build \
   --assertions \
-  --projects="clang;libcxx;libcxxabi;lldb;debuginfo-tests"  \
+  --projects="clang;libcxx;libcxxabi;lldb;cross-project-tests"  \
   --cmake-flag="-DPYTHON_LIBRARY=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/libpython3.7.dylib" \
   --cmake-flag="-DPYTHON_INCLUDE_DIR=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/include/python3.7m" \
   --cmake-flag="-DPYTHON_LIBRARY_DEBUG=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/libpython3.7.dylib" \
@@ -85,7 +85,7 @@
 export PATH=$PATH:/usr/bin:/usr/local/bin
 
 rm -rf test/results.xml
-
+
 BUILD_DIR=$(pwd)/lldb-build
 FAST_BUILD_DIR=$(pwd)/clang-build
 REPORT_DIR=$(pwd)/coverage-reports
@@ -93,31 +93,31 @@
 LLVM_PROFDATA=$HOST/llvm-profdata
 LLVM_COV=$HOST/llvm-cov
 ARTIFACT_PREP_SCRIPT=$WORKSPACE/llvm-project/llvm/utils/prepare-code-coverage-artifact.py
-
+
 FAST_TOOLS=(FileCheck count not)
 for TOOL in ${FAST_TOOLS[@]}; do
   cp $FAST_BUILD_DIR/bin/$TOOL $BUILD_DIR/bin/$TOOL
 done
-
+
 # Clear out any stale profiles.
 rm -rf $BUILD_DIR/profiles
 
 # Run the tests.
 IGNORE_ERRORS_OVERRIDE=1 python llvm-zorg/zorg/jenkins/monorepo_build.py lldb-cmake testlong || echo "Some tests may have failed."
-
+
 cd $BUILD_DIR
 ninja -k 0 check-llvm check-clang || echo "Some tests may have failed."
 cd -
-
+
 COV_BINARIES=$(find $BUILD_DIR/bin $BUILD_DIR/lib -depth 1 -type f -exec file {} \\; | grep Mach-O | cut -d':' -f1 | grep -vE '/(FileCheck|count|not)$' | xargs)
-
+
 rm -rf $REPORT_DIR
 mkdir -p $REPORT_DIR
 python $ARTIFACT_PREP_SCRIPT $LLVM_PROFDATA $LLVM_COV $BUILD_DIR/profiles $REPORT_DIR $COV_BINARIES --unified-report --restrict $WORKSPACE/llvm-project
-
+
 scp -r $REPORT_DIR buildslave@labmaster2.local:/Library/WebServer/Documents/coverage
 ssh buildslave@labmaster2.local "chmod -R 777 /Library/WebServer/Documents/coverage"
-
+
 rm -rf lldb-build/profiles
 '''
 }
Index: zorg/jenkins/jobs/jobs/lldb-cmake
===
--- zorg/jenkins/

[Lldb-commits] [PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 357642.
Ericson2314 added a comment.

Rebased, fixed unintersting conflicts, organized docs better


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

Files:
  clang/tools/scan-build/CMakeLists.txt
  libclc/CMakeLists.txt
  lldb/cmake/modules/FindLibEdit.cmake
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddSphinxTarget.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMInstallSymlink.cmake
  llvm/docs/CMake.rst
  llvm/examples/Bye/CMakeLists.txt
  llvm/include/llvm/CMakeLists.txt
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/lto/CMakeLists.txt
  llvm/tools/opt-viewer/CMakeLists.txt
  llvm/tools/remarks-shlib/CMakeLists.txt
  openmp/runtime/src/CMakeLists.txt

Index: openmp/runtime/src/CMakeLists.txt
===
--- openmp/runtime/src/CMakeLists.txt
+++ openmp/runtime/src/CMakeLists.txt
@@ -324,7 +324,7 @@
 install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_LIB_FILE}\"
   \"${alias}${LIBOMP_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \${CMAKE_INSTALL_PREFIX}/bin)")
 install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_IMP_LIB_FILE}\"
-  \"${alias}${CMAKE_STATIC_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR})")
+  \"${alias}${CMAKE_STATIC_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \"\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR}\")")
   endforeach()
 else()
 
@@ -336,7 +336,7 @@
 foreach(alias IN LISTS LIBOMP_ALIASES)
   install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E create_symlink \"${LIBOMP_LIB_FILE}\"
 \"${alias}${LIBOMP_LIBRARY_SUFFIX}\" WORKING_DIRECTORY
-\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR})")
+\"\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR}\")")
 endforeach()
   endif()
 endif()
Index: llvm/tools/remarks-shlib/CMakeLists.txt
===
--- llvm/tools/remarks-shlib/CMakeLists.txt
+++ llvm/tools/remarks-shlib/CMakeLists.txt
@@ -19,7 +19,7 @@
   endif()
   
   install(FILES ${LLVM_MAIN_INCLUDE_DIR}/llvm-c/Remarks.h
-DESTINATION include/llvm-c
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-c"
 COMPONENT Remarks)
 
   if (APPLE)
Index: llvm/tools/opt-viewer/CMakeLists.txt
===
--- llvm/tools/opt-viewer/CMakeLists.txt
+++ llvm/tools/opt-viewer/CMakeLists.txt
@@ -8,7 +8,7 @@
 
 foreach (file ${files})
   install(PROGRAMS ${file}
-DESTINATION share/opt-viewer
+DESTINATION "${CMAKE_INSTALL_DATADIR}/opt-viewer"
 COMPONENT opt-viewer)
 endforeach (file)
 
Index: llvm/tools/lto/CMakeLists.txt
===
--- llvm/tools/lto/CMakeLists.txt
+++ llvm/tools/lto/CMakeLists.txt
@@ -33,7 +33,7 @@
 ${SOURCES} DEPENDS intrinsics_gen)
 
 install(FILES ${LLVM_MAIN_INCLUDE_DIR}/llvm-c/lto.h
-  DESTINATION include/llvm-c
+  DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-c"
   COMPONENT LTO)
 
 if (APPLE)
Index: llvm/tools/llvm-config/llvm-config.cpp
===
--- llvm/tools/llvm-config/llvm-config.cpp
+++ llvm/tools/llvm-config/llvm-config.cpp
@@ -357,10 +357,16 @@
 ("-I" + ActiveIncludeDir + " " + "-I" + ActiveObjRoot + "/include");
   } else {
 ActivePrefix = CurrentExecPrefix;
-ActiveIncludeDir = ActivePrefix + "/include";
-SmallString<256> path(StringRef(LLVM_TOOLS_INSTALL_DIR));
-sys::fs::make_absolute(ActivePrefix, path);
-ActiveBinDir = std::string(path.str());
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_INCLUDEDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);
+  ActiveIncludeDir = std::string(Path.str());
+}
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_BINDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);
+  ActiveBinDir = std::string(Path.str());
+}
 ActiveLibDir = ActivePrefix + "/lib" + LLVM_LIBDIR_SUFFIX;
 ActiveCMakeDir = ActiveLibDir + "/cmake/llvm";
 ActiveIncludeOption = "-I" + ActiveIncludeDir;
Index: llvm/tools/llvm-config/BuildVariables.inc.in
===
--- llvm/tools/llvm-config/BuildVariables.inc.in
+++ llvm/tools/llvm-config/BuildVariables.inc.in
@@ -23,6 +23,8 @@
 #define LLVM_CXXFLAGS "@LLVM_CXXFLAGS@"
 #define LLVM_BUILDMODE "@LLVM_BUILDMODE@"
 #define LLVM_LIBDIR_SUFFIX "@LLVM_LIBDIR_SUFFIX@"
+#define LLVM_INSTALL_BINDIR "@CMAKE_INSTALL_BINDIR@"
+#define LLVM_INSTALL_INCLUDEDIR "@CMAKE_INSTALL_INCLUDEDIR@"
 #define LLVM_TARGETS_BUILT "@LLVM_TARGETS_BUILT@"
 #define LLVM_SYSTEM_LIBS "@LLVM_SYSTEM_LIBS@"
 #defi

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 357643.
Ericson2314 added a comment.
Herald added a subscriber: whisperity.

rebased, fixed some new DESTINATION


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

Files:
  clang-tools-extra/clang-doc/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/find-all-symbols/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/tool/CMakeLists.txt
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/modularize/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/modules/AddClang.cmake
  clang/tools/c-index-test/CMakeLists.txt
  clang/tools/clang-format/CMakeLists.txt
  clang/tools/clang-rename/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build-py/CMakeLists.txt
  clang/tools/scan-build/CMakeLists.txt
  clang/tools/scan-view/CMakeLists.txt
  clang/utils/hmaptool/CMakeLists.txt
  compiler-rt/cmake/base-config-ix.cmake
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/tools/f18/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  libc/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/include/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/src/CMakeLists.txt
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/tools/lld/CMakeLists.txt
  lldb/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  openmp/CMakeLists.txt
  openmp/runtime/src/CMakeLists.txt
  openmp/tools/multiplex/CMakeLists.txt
  polly/CMakeLists.txt
  polly/cmake/CMakeLists.txt
  polly/lib/External/CMakeLists.txt
  pstl/CMakeLists.txt

Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -7,6 +7,8 @@
 #===--===##
 cmake_minimum_required(VERSION 3.13.4)
 
+include(GNUInstallDirs)
+
 set(PARALLELSTL_VERSION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/include/pstl/internal/pstl_config.h")
 file(STRINGS "${PARALLELSTL_VERSION_FILE}" PARALLELSTL_VERSION_SOURCE REGEX "#define _PSTL_VERSION .*$")
 string(REGEX REPLACE "#define _PSTL_VERSION (.*)$" "\\1" PARALLELSTL_VERSION_SOURCE "${PARALLELSTL_VERSION_SOURCE}")
@@ -86,10 +88,10 @@
   "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfigVersion.cmake"
 DESTINATION lib/cmake/ParallelSTL)
 install(DIRECTORY include/
-DESTINATION include
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
 PATTERN "*.in" EXCLUDE)
 install(FILES "${PSTL_CONFIG_SITE_PATH}"
-DESTINATION include)
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
 
 add_custom_target(install-pstl
   COMMAND "${CMAKE_COMMAND}" -P "${PROJECT_BINARY_DIR}/cmake_install.cmake" -DCOMPONENT=ParallelSTL)
Index: polly/lib/External/CMakeLists.txt
===
--- polly/lib/External/CMakeLists.txt
+++ polly/lib/External/CMakeLists.txt
@@ -290,7 +290,7 @@
 install(DIRECTORY
   ${ISL_SOURCE_DIR}/include/
   ${ISL_BINARY_DIR}/include/
-  DESTINATION include/polly
+  DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/polly"
   FILES_MATCHING
   PATTERN "*.h"
   PATTERN "CMakeFiles" EXCLUDE
Index: polly/cmake/CMakeLists.txt
===
--- polly/cmake/CMakeLists.txt
+++ polly/cmake/CMakeLists.txt
@@ -83,14 +83,15 @@
 set(POLLY_CONFIG_LLVM_CMAKE_DIR "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+get_filename_component(base_includedir "${CMAKE_INSTALL_INCLUDEDIR}" ABSOLUTE BASE_DIR "${POLLY_INSTALL_PREFIX}")
 if (POLLY_BUNDLED_ISL)
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
-"${POLLY_INSTALL_PREFIX}/include/polly"
+"${base_includedir}"
+"${base_includedir}/polly"
 )
 else()
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
+"${base_includedir}"
 ${ISL_INCLUDE_DIRS}
 )
 endif()
@@ -100,12 +101,12 @@
 foreach(tgt IN LISTS POLLY_CONFIG_EXPORTED_TARGETS)
   get_target_property(tgt_type ${tgt} TYPE)
   if (tgt_type STREQUAL "EXECUTABLE")
-set(tgt_prefix "bin/")
+set(tgt_prefix "${CMAKE_INSTALL_FULL_BINDIR}/")
   else()
-set(tgt_prefix "lib/")
+set(tgt_prefix "${CMAKE_INSTALL_FULL_LIBDIR}/")
   endif()
 
-  set(tgt_path "${CMAKE_INSTALL_PREFIX}/${tgt_prefix}$")
+  set(tgt_path "${tgt_prefix}$")
   file(RELATIVE_PATH tgt_path ${POLLY_CONFIG_CMAKE_DIR} ${tgt_path})
 
   if (NOT tgt_type STREQUAL "INTERFACE_LIBRARY")
Index: polly/

[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

[Luís Marques via Phabricator via lldb-commits]

luismarques added a comment.

In D62732#2875107 , @kasper81 wrote:

> Hi, I have my fingers crossed since this request was opened in 2019. It seems 
> like it compiles and usable to certain degree. Can this patch be merged and 
> included in llvm 13 as initial riscv64 support? We can then improve it 
> subsequently if bugs show up. Otherwise it will be one more year of waiting 
> for the consumers. Thank you for your effort!

I think the main blocker for merging was testing. If it helps, I now have the 
RISC-V server in my hands and I should be able to set up a buildbot soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732

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

[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

[kasper via Phabricator via lldb-commits]

kasper81 added a comment.

Hi, I have my fingers crossed since this request was opened in 2019. It seems 
like it compiles and usable to certain degree. Can this patch be merged and 
included in llvm 13 as initial riscv64 support? We can then improve it 
subsequently if bugs show up. Otherwise it will be one more year of waiting for 
the consumers. Thank you for your effort!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732

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

[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

[Jessica Clarke via Phabricator via lldb-commits]

jrtc27 added inline comments.



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:118
+
+  // Previous frames pc is in ra
+  row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);





Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:141
+
+  // The previous frames pc is stored in ra.
+  row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);





Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:142
+  // The previous frames pc is stored in ra.
+  row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);
+

And SP is just.. the same? Surely the default unwind plan is to load SP and RA 
via FP? You won't get very far with this.



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:167-168
+  .Cases("x22", "x23", "x24", "x25", "x26", "x27", true)
+  .Cases("f8", "f9", "f18", "f19", "f20", "f21", IsHardFloatProcess())
+  .Cases("f22", "f23", "f24", "f25", "f26", "f27", 
IsHardFloatProcess())
+  .Default(false);

What about if I have the D extension but only use LP64F as the ABI? Does it 
matter that this says f9 is callee-saved but in reality only the low 32 bits 
are?



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:16
+class ABISysV_riscv : public lldb_private::MCBasedABI {
+  bool isRV64;
+

IsRV64?



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:70-71
+// Ensure addresses are smaller than XLEN bits wide. Calls can use the 
least
+// significant bit to store auxiliary information, so no strict check is
+// done for alignment.
+if (!isRV64)

2 and 3 mod 4 are nevertheless invalid if you don't have the C extension and 
will take an alignment fault



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:72-73
+// done for alignment.
+if (!isRV64)
+  return (pc <= UINT32_MAX);
+return (pc <= UINT64_MAX);

Are addresses reliably zero-extended or can you ever get cases where they're 
sign-extended from a valid 32-bit address to a 64-bit address?



Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1152
+  triple.getArch() == llvm::Triple::riscv64)
+features_str += "+a,+c,+d,+f,+m";
+

This will override whatever the ELF says in its attributes section. This might 
make sense as a default for if you don't have a binary and are just poking at 
memory, but it makes things worse when you do, the common case that need to 
work as best as we can manage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732

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

[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

[kasper via Phabricator via lldb-commits]

kasper81 added a comment.

> I think the main blocker for merging was testing. If it helps, I now have the 
> RISC-V server in my hands and I should be able to set up a buildbot soon.

It certainly will help. Thank you! :)

On July 27, 13.x will be branched out. If we can squeeze it in that would be a 
huge thing! I can help anyway you want with testing (I don't have the device 
atm, but can run the builds in Debian and Alpine qmeu-based chroot 
environments). Otherwise, we can request the maintainers later if they would 
cherry-pick this patch once merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732

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

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:402-403
 case DW_AT_type:
-  type = form_value;
+  if (!type.IsValid())
+type = form_value;
   break;

What's the purpose of this? Do we expect to see the type attribute more than 
once? 



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:900-901
+  const auto *ft = qt->getAs();
+  TypeSystemClang *ts =
+  llvm::dyn_cast_or_null(function_type.GetTypeSystem());
+  ast.adjustDeducedFunctionResultType(

You're doing `dyn_cast_or_null` but then below you're dereferencing `ts` 
unconditionally? 



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1337-1339
+TypeSP ret_type = dwarf->FindTypeForAutoReturnForDIE(
+die, ConstString(attrs.mangled_name));
+if (ret_type) {

LLVM likes to put these variables in the if-clause, which I personally really 
like because it conveys the scope without hurting readability. 



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1341-1344
+  auto *function_decl = llvm::dyn_cast_or_null(
+  GetCachedClangDeclContextForDIE(die));
+
+  if (function_decl)





Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2672-2675
+  TypeSP type_sp;
+
+  if (!name)
+return type_sp;

I know this pattern is common in LLDB, but I really dislike it because you have 
to backtrack all the way to the beginning of the function to know if `type_sp` 
has been modified in any way. When I write code like, this I tend to use 
`return {};` to make it clear I'm returning a default constructed instance of 
the return type. That also makes it clear where we're actually returning a 
non-default instance by just looking at the `return`s. 



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2718
+
+  type_sp = func_type->shared_from_this();
+}




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

https://reviews.llvm.org/D105564

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