[Lldb-commits] [PATCH] D110981: [lldb] Improve help for platform put-file

2021-10-04 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1079
+
+Omitting the destination places the file in the debuggers current working 
directory.)");
   }

It would be more correct to say "the platform working directory" (as selected 
by platform settings -w).

It might also be nice to mention what happens with relative paths. I guess they 
are going to be interpreted relative to the remove WD (?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110981

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

[Lldb-commits] [PATCH] D110962: [lldb] Add unit tests for Terminal API

2021-10-04 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

Have you considered using the PseudoTerminal class to create the ptys ?




Comment at: lldb/unittests/Host/CMakeLists.txt:27
+if (LLDB_ENABLE_TERMIOS AND
+CMAKE_SYSTEM_NAME MATCHES "Darwin|FreeBSD|Linux|NetBSD|OpenBSD")
+  list(APPEND FILES

mgorny wrote:
> FTR, I've tested it on FreeBSD and Linux so far. NetBSD in progress. Darwin 
> and OpenBSD support is based on what I've found in gnulib but explicit 
> testing would be helpful.
At this point, I'd just go with `NOT windows`


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

https://reviews.llvm.org/D110962

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

[Lldb-commits] [PATCH] D110996: [lldb] [test] Terminate "process connect" connections via kill

2021-10-04 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Yeah, I'm not sure how this came to be. Disconnecting the platform makes no 
sense here, as it was never connected.


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

https://reviews.llvm.org/D110996

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

[Lldb-commits] [PATCH] D110996: [lldb] [test] Terminate "process connect" connections via kill

2021-10-04 Thread Michał Górny via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG45f9795085ee: [lldb] [test] Terminate "process 
connect" connections via kill (authored by mgorny).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110996

Files:
  lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
  lldb/test/API/functionalities/gdb_remote_client/TestPty.py


Index: lldb/test/API/functionalities/gdb_remote_client/TestPty.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestPty.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestPty.py
@@ -18,7 +18,7 @@
 self.expect("process connect " + self.server.get_connect_url(),
 substrs=['Process', 'stopped'])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
 
 def test_process_connect_async(self):
 """Test the process connect command in asynchronous mode"""
@@ -32,4 +32,6 @@
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateStopped])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
+lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+  self.process(), [lldb.eStateExited])
Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -18,7 +18,7 @@
 self.expect("gdb-remote " + self.server.get_connect_address(),
 substrs=['Process', 'stopped'])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
 
 def test_gdb_remote_async(self):
 """Test the gdb-remote command in asynchronous mode"""
@@ -30,7 +30,9 @@
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateStopped])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
+lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+  self.process(), [lldb.eStateExited])
 
 @skipIfWindows
 def test_process_connect_sync(self):
@@ -42,7 +44,7 @@
 self.expect("process connect " + self.server.get_connect_url(),
 substrs=['Process', 'stopped'])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
 
 @skipIfWindows
 def test_process_connect_async(self):
@@ -57,4 +59,6 @@
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateStopped])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
+lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+  self.process(), [lldb.eStateExited])


Index: lldb/test/API/functionalities/gdb_remote_client/TestPty.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestPty.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestPty.py
@@ -18,7 +18,7 @@
 self.expect("process connect " + self.server.get_connect_url(),
 substrs=['Process', 'stopped'])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
 
 def test_process_connect_async(self):
 """Test the process connect command in asynchronous mode"""
@@ -32,4 +32,6 @@
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateStopped])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
+lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+  self.process(), [lldb.eStateExited])
Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/T

[Lldb-commits] [lldb] 45f9795 - [lldb] [test] Terminate "process connect" connections via kill

2021-10-04 Thread Michał Górny via lldb-commits 

Author: Michał Górny
Date: 2021-10-04T12:29:06+02:00
New Revision: 45f9795085ee0fe6fd23129d80aad5a2ea5026a0

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

LOG: [lldb] [test] Terminate "process connect" connections via kill

Fix the termination of "process connect" (and "gdb-remote") to kill
the process rather than attempting to disconnect the platform.
The latter only results in an error since we did not use "platform
connect", and apparently process-level connections (at least via
gdb-remote) do not really support disconnecting.

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

Added: 


Modified: 
lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
lldb/test/API/functionalities/gdb_remote_client/TestPty.py

Removed: 




diff  --git 
a/lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py 
b/lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
index 6f99c939562b6..a62d8aece790b 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -18,7 +18,7 @@ def test_gdb_remote_sync(self):
 self.expect("gdb-remote " + self.server.get_connect_address(),
 substrs=['Process', 'stopped'])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
 
 def test_gdb_remote_async(self):
 """Test the gdb-remote command in asynchronous mode"""
@@ -30,7 +30,9 @@ def test_gdb_remote_async(self):
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateStopped])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
+lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+  self.process(), [lldb.eStateExited])
 
 @skipIfWindows
 def test_process_connect_sync(self):
@@ -42,7 +44,7 @@ def test_process_connect_sync(self):
 self.expect("process connect " + self.server.get_connect_url(),
 substrs=['Process', 'stopped'])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
 
 @skipIfWindows
 def test_process_connect_async(self):
@@ -57,4 +59,6 @@ def test_process_connect_async(self):
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateStopped])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
+lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+  self.process(), [lldb.eStateExited])

diff  --git a/lldb/test/API/functionalities/gdb_remote_client/TestPty.py 
b/lldb/test/API/functionalities/gdb_remote_client/TestPty.py
index 46078b7a0adb3..55935d901c665 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestPty.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestPty.py
@@ -18,7 +18,7 @@ def test_process_connect_sync(self):
 self.expect("process connect " + self.server.get_connect_url(),
 substrs=['Process', 'stopped'])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
 
 def test_process_connect_async(self):
 """Test the process connect command in asynchronous mode"""
@@ -32,4 +32,6 @@ def test_process_connect_async(self):
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateStopped])
 finally:
-self.dbg.GetSelectedPlatform().DisconnectRemote()
+self.dbg.GetSelectedTarget().GetProcess().Kill()
+lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+  self.process(), [lldb.eStateExited])



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

[Lldb-commits] [PATCH] D110997: [lldb] Restore tty attributes on disconnect

2021-10-04 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

Why is it important to restore the tty settings? (just asking, I have no clue 
about how this is supposed to work)


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

https://reviews.llvm.org/D110997

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

[Lldb-commits] [PATCH] D110962: [lldb] Add unit tests for Terminal API

2021-10-04 Thread Michał Górny via Phabricator via lldb-commits 
mgorny marked an inline comment as done.
mgorny added a comment.

In D110962#3039428 , @labath wrote:

> Have you considered using the PseudoTerminal class to create the ptys ?

That's really stupid of me but for some reason I've assumed that if LLDB had 
any pty-related logic, it'd live in `Terminal.h` ;-).




Comment at: lldb/unittests/Host/CMakeLists.txt:27
+if (LLDB_ENABLE_TERMIOS AND
+CMAKE_SYSTEM_NAME MATCHES "Darwin|FreeBSD|Linux|NetBSD|OpenBSD")
+  list(APPEND FILES

labath wrote:
> mgorny wrote:
> > FTR, I've tested it on FreeBSD and Linux so far. NetBSD in progress. Darwin 
> > and OpenBSD support is based on what I've found in gnulib but explicit 
> > testing would be helpful.
> At this point, I'd just go with `NOT windows`
If I'm using `PseudoTerminal`, then I guess I don't need the platform check at 
all (and 'not Windows' is implied by `LLDB_ENABLE_TERMIOS`).


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

https://reviews.llvm.org/D110962

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

[Lldb-commits] [PATCH] D110997: [lldb] Restore tty attributes on disconnect

2021-10-04 Thread Michał Górny via Phabricator via lldb-commits 
mgorny added a comment.

In D110997#3039540 , @labath wrote:

> Why is it important to restore the tty settings? (just asking, I have no clue 
> about how this is supposed to work)

Well, maybe my logic is wrong but the idea is that since LLDB overrides serial 
port parameters, it should restore them to the original state when it's done 
with the serial port. Roughly the principle that the program cleans up after 
itself.


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

https://reviews.llvm.org/D110997

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

[Lldb-commits] [PATCH] D110962: [lldb] Add unit tests for Terminal API

2021-10-04 Thread Michał Górny via Phabricator via lldb-commits 
mgorny updated this revision to Diff 376861.
mgorny marked an inline comment as done.
mgorny added a comment.

Use `PseudoTerminal` class.


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

https://reviews.llvm.org/D110962

Files:
  lldb/unittests/Host/CMakeLists.txt
  lldb/unittests/Host/TerminalTest.cpp

Index: lldb/unittests/Host/TerminalTest.cpp
===
--- /dev/null
+++ lldb/unittests/Host/TerminalTest.cpp
@@ -0,0 +1,126 @@
+//===-- TerminalTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Host/PseudoTerminal.h"
+#include "lldb/Host/Terminal.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include 
+#include 
+
+using namespace lldb_private;
+
+class TerminalTest : public ::testing::Test {
+protected:
+  PseudoTerminal m_pty;
+
+  void SetUp() override {
+EXPECT_THAT_ERROR(m_pty.OpenFirstAvailablePrimary(O_RDWR | O_NOCTTY),
+  llvm::Succeeded());
+  }
+};
+
+TEST_F(TerminalTest, PtyIsATerminal) {
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+  EXPECT_EQ(term.IsATerminal(), true);
+}
+
+TEST_F(TerminalTest, PipeIsNotATerminal) {
+  int pipefd[2];
+  ASSERT_EQ(pipe(pipefd), 0);
+  Terminal pipeterm{pipefd[0]};
+  EXPECT_EQ(pipeterm.IsATerminal(), false);
+  close(pipefd[0]);
+  close(pipefd[1]);
+}
+
+TEST_F(TerminalTest, SetEcho) {
+  struct termios terminfo;
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  ASSERT_EQ(term.SetEcho(true), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ECHO, 0U);
+
+  ASSERT_EQ(term.SetEcho(false), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ECHO, 0U);
+}
+
+TEST_F(TerminalTest, SetCanonical) {
+  struct termios terminfo;
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  ASSERT_EQ(term.SetCanonical(true), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ICANON, 0U);
+
+  ASSERT_EQ(term.SetCanonical(false), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ICANON, 0U);
+}
+
+TEST_F(TerminalTest, SaveRestoreRAII) {
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &orig_terminfo), 0);
+
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  {
+TerminalState term_state{term};
+terminfo = orig_terminfo;
+
+// make some arbitrary changes
+terminfo.c_iflag ^= IGNPAR | INLCR;
+terminfo.c_oflag ^= OPOST | OCRNL;
+terminfo.c_cflag ^= PARENB | PARODD;
+terminfo.c_lflag ^= ICANON | ECHO;
+terminfo.c_cc[VEOF] ^= 8;
+terminfo.c_cc[VEOL] ^= 4;
+cfsetispeed(&terminfo, B9600);
+cfsetospeed(&terminfo, B9600);
+
+ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo),
+  0);
+  }
+
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  ASSERT_EQ(memcmp(&terminfo, &orig_terminfo, sizeof(terminfo)), 0);
+}
+
+TEST_F(TerminalTest, SaveRestore) {
+  TerminalState term_state;
+
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &orig_terminfo), 0);
+
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+  term_state.Save(term, false);
+  terminfo = orig_terminfo;
+
+  // make some arbitrary changes
+  terminfo.c_iflag ^= IGNPAR | INLCR;
+  terminfo.c_oflag ^= OPOST | OCRNL;
+  terminfo.c_cflag ^= PARENB | PARODD;
+  terminfo.c_lflag ^= ICANON | ECHO;
+  terminfo.c_cc[VEOF] ^= 8;
+  terminfo.c_cc[VEOL] ^= 4;
+  cfsetispeed(&terminfo, B9600);
+  cfsetospeed(&terminfo, B9600);
+
+  ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo), 0);
+
+  term_state.Restore();
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  ASSERT_EQ(memcmp(&terminfo, &orig_terminfo, sizeof(terminfo)), 0);
+}
Index: lldb/unittests/Host/CMakeLists.txt
===
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -22,6 +22,12 @@
   )
 endif()
 
+if (LLDB_ENABLE_TERMIOS)
+  list(APPEND FILES
+TerminalTest.cpp
+  )
+endif()
+
 add_lldb_unittest(HostTests
   ${FILES}
   LINK_LIBS
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

2021-10-04 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.

Looks good, apart from some stylistic comments inline. Thank you for taking the 
time to do this right.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3545
+
+DIEVector MergeBlockAbstractParameters(const DWARFDIE &block_die,
+   DIEVector &&variable_dies) {

In llvm, we prefer `static` functions over anonymous namespaces. Theoretically, 
you could keep the anonymous namespace around the using declaration (per 
, as those 
can't use `static`), though I would actually probably prefer  DIEArray type 
defined in DIERef.h over a custom type.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3588
+  concrete_it->GetReferencedDIE(DW_AT_abstract_origin);
+  if (origin_of_concrete && origin_of_concrete == abstract_child) {
+// The current abstract paramater is the origin of the current

I would assume this is redundant, as an invalid DIE will never match 
`abstract_child`.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:402
+  const lldb_private::SymbolContext &sc,
+  const llvm::SmallVectorImpl &variable_dies,
+  const lldb::addr_t func_low_pc);

When you're not mutating the vector, the usual argument type is `ArrayRef`



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:403
+  const llvm::SmallVectorImpl &variable_dies,
+  const lldb::addr_t func_low_pc);
 

We generally do not put a const qualifier on by-value arguments (it's pretty 
useless).

(I see it's present on other functions too, but I don't know if they were 
introduced by you or you're just propagating them.)



Comment at: 
lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py:17
+
+# For the unused parameters, only check the types.
+self.assertEqual("void *", 
self.frame().FindVariable("unused1").GetType().GetName())

jarin wrote:
> labath wrote:
> > Maybe we could check something else as well... Do `GetDescription` or 
> > `str(value)` return something reasonable here?
> Actually, the most important thing is the type here, so this was quite 
> deliberate.
> 
> GetDescription returns `(void *) unused1 =  optimized out>\n\n`, but I am not sure if we can safely require that all 
> future versions/platforms optimize the parameter out.
I don't feel strongly about it, but I would say that this function is so simple 
than any optimizer worthy of that name should be able to optimize those 
arguments away. I might replace printf with a `noinline`/`optnone` function 
though, to avoid any libc shenanigans.


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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [PATCH] D110962: [lldb] Add unit tests for Terminal API

2021-10-04 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/unittests/Host/CMakeLists.txt:27
+if (LLDB_ENABLE_TERMIOS AND
+CMAKE_SYSTEM_NAME MATCHES "Darwin|FreeBSD|Linux|NetBSD|OpenBSD")
+  list(APPEND FILES

mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > FTR, I've tested it on FreeBSD and Linux so far. NetBSD in progress. 
> > > Darwin and OpenBSD support is based on what I've found in gnulib but 
> > > explicit testing would be helpful.
> > At this point, I'd just go with `NOT windows`
> If I'm using `PseudoTerminal`, then I guess I don't need the platform check 
> at all (and 'not Windows' is implied by `LLDB_ENABLE_TERMIOS`).
Yep.

Although, I have a feeling that this will actually not work (as in, it will 
trigger the llvm unrecognized-source-file cmake alarm) when termios is 
disabled. IIRC, the llvm solution is to pass this file in some "optionally 
compiled sources" argument. The lldb solution would be to put the file in a 
subdirectory (posix, I guess).
I'd probably go with the second one, since we already have a linux subfolder.


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

https://reviews.llvm.org/D110962

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

[Lldb-commits] [PATCH] D110962: [lldb] Add unit tests for Terminal API

2021-10-04 Thread Michał Górny via Phabricator via lldb-commits 
mgorny marked an inline comment as done.
mgorny added inline comments.



Comment at: lldb/unittests/Host/CMakeLists.txt:27
+if (LLDB_ENABLE_TERMIOS AND
+CMAKE_SYSTEM_NAME MATCHES "Darwin|FreeBSD|Linux|NetBSD|OpenBSD")
+  list(APPEND FILES

labath wrote:
> mgorny wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > FTR, I've tested it on FreeBSD and Linux so far. NetBSD in progress. 
> > > > Darwin and OpenBSD support is based on what I've found in gnulib but 
> > > > explicit testing would be helpful.
> > > At this point, I'd just go with `NOT windows`
> > If I'm using `PseudoTerminal`, then I guess I don't need the platform check 
> > at all (and 'not Windows' is implied by `LLDB_ENABLE_TERMIOS`).
> Yep.
> 
> Although, I have a feeling that this will actually not work (as in, it will 
> trigger the llvm unrecognized-source-file cmake alarm) when termios is 
> disabled. IIRC, the llvm solution is to pass this file in some "optionally 
> compiled sources" argument. The lldb solution would be to put the file in a 
> subdirectory (posix, I guess).
> I'd probably go with the second one, since we already have a linux subfolder.
Sure, done that.


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

https://reviews.llvm.org/D110962

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

[Lldb-commits] [lldb] e77959c - [lldb] Add unit tests for Terminal API

2021-10-04 Thread Michał Górny via lldb-commits 

Author: Michał Górny
Date: 2021-10-04T14:19:34+02:00
New Revision: e77959cba777baa6c5fba528a6f97406b43f8b64

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

LOG: [lldb] Add unit tests for Terminal API

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

Added: 
lldb/unittests/Host/posix/TerminalTest.cpp

Modified: 
lldb/unittests/Host/CMakeLists.txt

Removed: 




diff  --git a/lldb/unittests/Host/CMakeLists.txt 
b/lldb/unittests/Host/CMakeLists.txt
index 14c4fe4d0b8a6..ae6afd592e547 100644
--- a/lldb/unittests/Host/CMakeLists.txt
+++ b/lldb/unittests/Host/CMakeLists.txt
@@ -22,6 +22,12 @@ if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   )
 endif()
 
+if (LLDB_ENABLE_TERMIOS)
+  list(APPEND FILES
+posix/TerminalTest.cpp
+  )
+endif()
+
 add_lldb_unittest(HostTests
   ${FILES}
   LINK_LIBS

diff  --git a/lldb/unittests/Host/posix/TerminalTest.cpp 
b/lldb/unittests/Host/posix/TerminalTest.cpp
new file mode 100644
index 0..ecdb548021643
--- /dev/null
+++ b/lldb/unittests/Host/posix/TerminalTest.cpp
@@ -0,0 +1,126 @@
+//===-- TerminalTest.cpp 
--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Host/PseudoTerminal.h"
+#include "lldb/Host/Terminal.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include 
+#include 
+
+using namespace lldb_private;
+
+class TerminalTest : public ::testing::Test {
+protected:
+  PseudoTerminal m_pty;
+
+  void SetUp() override {
+EXPECT_THAT_ERROR(m_pty.OpenFirstAvailablePrimary(O_RDWR | O_NOCTTY),
+  llvm::Succeeded());
+  }
+};
+
+TEST_F(TerminalTest, PtyIsATerminal) {
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+  EXPECT_EQ(term.IsATerminal(), true);
+}
+
+TEST_F(TerminalTest, PipeIsNotATerminal) {
+  int pipefd[2];
+  ASSERT_EQ(pipe(pipefd), 0);
+  Terminal pipeterm{pipefd[0]};
+  EXPECT_EQ(pipeterm.IsATerminal(), false);
+  close(pipefd[0]);
+  close(pipefd[1]);
+}
+
+TEST_F(TerminalTest, SetEcho) {
+  struct termios terminfo;
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  ASSERT_EQ(term.SetEcho(true), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ECHO, 0U);
+
+  ASSERT_EQ(term.SetEcho(false), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ECHO, 0U);
+}
+
+TEST_F(TerminalTest, SetCanonical) {
+  struct termios terminfo;
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  ASSERT_EQ(term.SetCanonical(true), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ICANON, 0U);
+
+  ASSERT_EQ(term.SetCanonical(false), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ICANON, 0U);
+}
+
+TEST_F(TerminalTest, SaveRestoreRAII) {
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &orig_terminfo), 0);
+
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  {
+TerminalState term_state{term};
+terminfo = orig_terminfo;
+
+// make some arbitrary changes
+terminfo.c_iflag ^= IGNPAR | INLCR;
+terminfo.c_oflag ^= OPOST | OCRNL;
+terminfo.c_cflag ^= PARENB | PARODD;
+terminfo.c_lflag ^= ICANON | ECHO;
+terminfo.c_cc[VEOF] ^= 8;
+terminfo.c_cc[VEOL] ^= 4;
+cfsetispeed(&terminfo, B9600);
+cfsetospeed(&terminfo, B9600);
+
+ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo),
+  0);
+  }
+
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  ASSERT_EQ(memcmp(&terminfo, &orig_terminfo, sizeof(terminfo)), 0);
+}
+
+TEST_F(TerminalTest, SaveRestore) {
+  TerminalState term_state;
+
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &orig_terminfo), 0);
+
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+  term_state.Save(term, false);
+  terminfo = orig_terminfo;
+
+  // make some arbitrary changes
+  terminfo.c_iflag ^= IGNPAR | INLCR;
+  terminfo.c_oflag ^= OPOST | OCRNL;
+  terminfo.c_cflag ^= PARENB | PARODD;
+  terminfo.c_lflag ^= ICANON | ECHO;
+  terminfo.c_cc[VEOF] ^= 8;
+  terminfo.c_cc[VEOL] ^= 4;
+  cfsetispeed(&terminfo, B9600);
+  cfsetospeed(&terminfo, B9600);
+
+  ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo), 
0

[Lldb-commits] [PATCH] D110962: [lldb] Add unit tests for Terminal API

2021-10-04 Thread Michał Górny via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
Closed by commit rGe77959cba777: [lldb] Add unit tests for Terminal API 
(authored by mgorny).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D110962?vs=376861&id=376872#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110962

Files:
  lldb/unittests/Host/CMakeLists.txt
  lldb/unittests/Host/posix/TerminalTest.cpp

Index: lldb/unittests/Host/posix/TerminalTest.cpp
===
--- /dev/null
+++ lldb/unittests/Host/posix/TerminalTest.cpp
@@ -0,0 +1,126 @@
+//===-- TerminalTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Host/PseudoTerminal.h"
+#include "lldb/Host/Terminal.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include 
+#include 
+
+using namespace lldb_private;
+
+class TerminalTest : public ::testing::Test {
+protected:
+  PseudoTerminal m_pty;
+
+  void SetUp() override {
+EXPECT_THAT_ERROR(m_pty.OpenFirstAvailablePrimary(O_RDWR | O_NOCTTY),
+  llvm::Succeeded());
+  }
+};
+
+TEST_F(TerminalTest, PtyIsATerminal) {
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+  EXPECT_EQ(term.IsATerminal(), true);
+}
+
+TEST_F(TerminalTest, PipeIsNotATerminal) {
+  int pipefd[2];
+  ASSERT_EQ(pipe(pipefd), 0);
+  Terminal pipeterm{pipefd[0]};
+  EXPECT_EQ(pipeterm.IsATerminal(), false);
+  close(pipefd[0]);
+  close(pipefd[1]);
+}
+
+TEST_F(TerminalTest, SetEcho) {
+  struct termios terminfo;
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  ASSERT_EQ(term.SetEcho(true), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ECHO, 0U);
+
+  ASSERT_EQ(term.SetEcho(false), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ECHO, 0U);
+}
+
+TEST_F(TerminalTest, SetCanonical) {
+  struct termios terminfo;
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  ASSERT_EQ(term.SetCanonical(true), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_NE(terminfo.c_lflag & ICANON, 0U);
+
+  ASSERT_EQ(term.SetCanonical(false), true);
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  EXPECT_EQ(terminfo.c_lflag & ICANON, 0U);
+}
+
+TEST_F(TerminalTest, SaveRestoreRAII) {
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &orig_terminfo), 0);
+
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+
+  {
+TerminalState term_state{term};
+terminfo = orig_terminfo;
+
+// make some arbitrary changes
+terminfo.c_iflag ^= IGNPAR | INLCR;
+terminfo.c_oflag ^= OPOST | OCRNL;
+terminfo.c_cflag ^= PARENB | PARODD;
+terminfo.c_lflag ^= ICANON | ECHO;
+terminfo.c_cc[VEOF] ^= 8;
+terminfo.c_cc[VEOL] ^= 4;
+cfsetispeed(&terminfo, B9600);
+cfsetospeed(&terminfo, B9600);
+
+ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo),
+  0);
+  }
+
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  ASSERT_EQ(memcmp(&terminfo, &orig_terminfo, sizeof(terminfo)), 0);
+}
+
+TEST_F(TerminalTest, SaveRestore) {
+  TerminalState term_state;
+
+  struct termios orig_terminfo;
+  struct termios terminfo;
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &orig_terminfo), 0);
+
+  Terminal term{m_pty.GetPrimaryFileDescriptor()};
+  term_state.Save(term, false);
+  terminfo = orig_terminfo;
+
+  // make some arbitrary changes
+  terminfo.c_iflag ^= IGNPAR | INLCR;
+  terminfo.c_oflag ^= OPOST | OCRNL;
+  terminfo.c_cflag ^= PARENB | PARODD;
+  terminfo.c_lflag ^= ICANON | ECHO;
+  terminfo.c_cc[VEOF] ^= 8;
+  terminfo.c_cc[VEOL] ^= 4;
+  cfsetispeed(&terminfo, B9600);
+  cfsetospeed(&terminfo, B9600);
+
+  ASSERT_EQ(tcsetattr(m_pty.GetPrimaryFileDescriptor(), TCSANOW, &terminfo), 0);
+
+  term_state.Restore();
+  ASSERT_EQ(tcgetattr(m_pty.GetPrimaryFileDescriptor(), &terminfo), 0);
+  ASSERT_EQ(memcmp(&terminfo, &orig_terminfo, sizeof(terminfo)), 0);
+}
Index: lldb/unittests/Host/CMakeLists.txt
===
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -22,6 +22,12 @@
   )
 endif()
 
+if (LLDB_ENABLE_TERMIOS)
+  list(APPEND FILES
+posix/TerminalTest.cpp
+  )
+endif()
+
 add_lldb_unittest(HostTests
   ${FILES}
   L

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

2021-10-04 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

In D108228#2961132 , @jingham wrote:

> Raphael's analysis of what the test needs is right.  We always check pointers 
> for validity before we do operations on them, so we wouldn't have tried to 
> get the summary, and the expression evaluation will just crash if we do * of 
> a null ptr, so there wouldn't be anything to format.
>
> The reason I passed the string reference to a function and check it there is 
> because from many years of experience, I don't trust compilers even at -O0 
> not to elide references to silly unused variables.  But at -O0 a function 
> argument is never going away.
>
> But if folks think this is over paranoid on my part, I can simplify the test.

I don't have a strong objection against the extra step and so on, so LGTM.


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] [lldb] fd9bc13 - [lldb] Fix a stray array access in Editline

2021-10-04 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2021-10-04T14:26:02+02:00
New Revision: fd9bc13803ee24bfa674311584126b83e059d756

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

LOG: [lldb] Fix a stray array access in Editline

This manifested itself as an asan failure in TestMultilineNavigation.py.

Added: 


Modified: 
lldb/source/Host/common/Editline.cpp

Removed: 




diff  --git a/lldb/source/Host/common/Editline.cpp 
b/lldb/source/Host/common/Editline.cpp
index b55061301f4d0..6898f8452161d 100644
--- a/lldb/source/Host/common/Editline.cpp
+++ b/lldb/source/Host/common/Editline.cpp
@@ -1560,7 +1560,7 @@ bool Editline::GetLines(int first_line_number, StringList 
&lines,
   if (!interrupted) {
 // Save the completed entry in history before returning. Don't save empty
 // input as that just clutters the command history.
-if (m_input_lines.size() > 1 || !m_input_lines.front().empty())
+if (!m_input_lines.empty())
   m_history_sp->Enter(CombineLines(m_input_lines).c_str());
 
 lines = GetInputAsStringList();



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

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

2021-10-04 Thread Michał Górny via Phabricator via lldb-commits 
mgorny created this revision.
mgorny added reviewers: teemperor, labath, krytarowski, emaste.
mgorny requested review of this revision.

Correct the st_dev values used by vFile:fstat packet to conform to
the GDB protocol.  Thanks to Raphael Isemann for noticing.


https://reviews.llvm.org/D111052

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py


Index: lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
@@ -266,7 +266,7 @@
 .encode("iso-8859-1")))
 sys_stat = os.fstat(temp_file.fileno())
 
-self.assertEqual(gdb_stat.st_dev, uint32_or_zero(sys_stat.st_dev))
+self.assertEqual(gdb_stat.st_dev, 0)
 self.assertEqual(gdb_stat.st_ino, uint32_or_zero(sys_stat.st_ino))
 self.assertEqual(gdb_stat.st_mode, uint32_trunc(sys_stat.st_mode))
 self.assertEqual(gdb_stat.st_nlink, 
uint32_or_max(sys_stat.st_nlink))
Index: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===
--- 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -790,7 +790,9 @@
   }
 
   GDBRemoteFStatData data;
-  fill_clamp(data.gdb_st_dev, file_stats.st_dev, 0);
+  // st_dev is used specially here: 0 means a file, 1 a console
+  // (i.e. a fd used as program's stdin/stdout/stderr)
+  data.gdb_st_dev = 0;
   fill_clamp(data.gdb_st_ino, file_stats.st_ino, 0);
   data.gdb_st_mode = file_stats.st_mode;
   fill_clamp(data.gdb_st_nlink, file_stats.st_nlink, UINT32_MAX);


Index: lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
@@ -266,7 +266,7 @@
 .encode("iso-8859-1")))
 sys_stat = os.fstat(temp_file.fileno())
 
-self.assertEqual(gdb_stat.st_dev, uint32_or_zero(sys_stat.st_dev))
+self.assertEqual(gdb_stat.st_dev, 0)
 self.assertEqual(gdb_stat.st_ino, uint32_or_zero(sys_stat.st_ino))
 self.assertEqual(gdb_stat.st_mode, uint32_trunc(sys_stat.st_mode))
 self.assertEqual(gdb_stat.st_nlink, uint32_or_max(sys_stat.st_nlink))
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -790,7 +790,9 @@
   }
 
   GDBRemoteFStatData data;
-  fill_clamp(data.gdb_st_dev, file_stats.st_dev, 0);
+  // st_dev is used specially here: 0 means a file, 1 a console
+  // (i.e. a fd used as program's stdin/stdout/stderr)
+  data.gdb_st_dev = 0;
   fill_clamp(data.gdb_st_ino, file_stats.st_ino, 0);
   data.gdb_st_mode = file_stats.st_mode;
   fill_clamp(data.gdb_st_nlink, file_stats.st_nlink, UINT32_MAX);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

2021-10-04 Thread 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. Could you maybe add a `FIXME:` at the end of that comment to point out 
that the whole 'console' thing isn't implemented/supported.

Probably wait and see if Pavel has any objections against this, but I think 
this is more correct than the old implementation so I think this can land.


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

https://reviews.llvm.org/D111052

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

[Lldb-commits] [lldb] 3fe771b - [LLDB] Fix objc_clsopt_v16_t struct

2021-10-04 Thread Jonas Devlieghere via lldb-commits 

Author: Alfsonso Gregory
Date: 2021-10-04T08:55:09-07:00
New Revision: 3fe771bf02d0318bfa12befd0be235383744439f

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

LOG: [LLDB] Fix objc_clsopt_v16_t struct

The objc_clsopt_v16_t struct does not match up with the macOS/iOS15
dyld_shared_cache ObjC runtime structures. A struct field was seemingly
omitted.

Differential revision: https://reviews.llvm.org/D110477

Added: 


Modified: 

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
 
b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
index 8d71f5b830b31..57de1faa1ca20 100644
--- 
a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ 
b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -292,6 +292,7 @@ struct objc_clsopt_v16_t {
uint32_t occupied;
uint32_t shift;
uint32_t mask;
+   uint32_t zero;
uint64_t salt;
uint32_t scramble[256];
uint8_t  tab[0]; // tab[mask+1]



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

[Lldb-commits] [PATCH] D110477: [LLDB] Fix objc_clsopt_v16_t struct

2021-10-04 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3fe771bf02d0: [LLDB] Fix objc_clsopt_v16_t struct (authored 
by gAlfonso-bit, committed by JDevlieghere).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110477

Files:
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp


Index: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -292,6 +292,7 @@
uint32_t occupied;
uint32_t shift;
uint32_t mask;
+   uint32_t zero;
uint64_t salt;
uint32_t scramble[256];
uint8_t  tab[0]; // tab[mask+1]


Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -292,6 +292,7 @@
uint32_t occupied;
uint32_t shift;
uint32_t mask;
+   uint32_t zero;
uint64_t salt;
uint32_t scramble[256];
uint8_t  tab[0]; // tab[mask+1]
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D110477: [LLDB] Fix objc_clsopt_v16_t struct

2021-10-04 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D110477#3038298 , @gAlfonso-bit 
wrote:

> In D110477#3034438 , @JDevlieghere 
> wrote:
>
>> In D110477#3034393 , @gAlfonso-bit 
>> wrote:
>>
>>> Any updates?
>>
>> This patch is accepted and ready to land. Let me know if you don't have 
>> commit access and need someone to land this for you.
>
> Unfortunately I do not have commit access

No worries, I've landed this patch for you. Next time don't hesitate to ask.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110477

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

[Lldb-commits] [lldb] 6fcb857 - [lldb][import-std-module] Prefer the non-module diagnostics when in fallback mode

2021-10-04 Thread Raphael Isemann via lldb-commits 

Author: Raphael Isemann
Date: 2021-10-04T19:16:03+02:00
New Revision: 6fcb857746c19b5ed46afdf732b839082326f9d4

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

LOG: [lldb][import-std-module] Prefer the non-module diagnostics when in 
fallback mode

The `fallback` setting for import-std-module is supposed to allow running
expression that require an imported C++ module without causing any regressions
for users (neither in terms of functionality nor performance). This is done by
first trying to normally parse/evaluate an expression and when an error occurred
during this first attempt, we retry with the loaded 'std' module.

When we run into a system with a 'std' module that for some reason doesn't build
or otherwise causes parse errors, then this currently means that the second
parse attempt will overwrite the error diagnostics of the first parse attempt.
Given that the module build errors are outside of the scope of what the user can
influence, it makes more sense to show the errors from the first parse attempt
that are only concerned with the actual user input.

Reviewed By: aprantl

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

Added: 

lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile

lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py

lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp

lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm

lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap

lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/vector

lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h

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

lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py

Removed: 




diff  --git 
a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index 41e86a1f897e9..38166391006a0 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -685,15 +685,22 @@ bool ClangUserExpression::Parse(DiagnosticManager 
&diagnostic_manager,
 SetupCppModuleImports(exe_ctx);
 // If we did load any modules, then retry parsing.
 if (!m_imported_cpp_modules.empty()) {
+  // Create a dedicated diagnostic manager for the second parse attempt.
+  // These diagnostics are only returned to the caller if using the 
fallback
+  // actually succeeded in getting the expression to parse. This prevents
+  // that module-specific issues regress diagnostic quality with the
+  // fallback mode.
+  DiagnosticManager retry_manager;
   // The module imports are injected into the source code wrapper,
   // so recreate those.
-  CreateSourceCode(diagnostic_manager, exe_ctx, m_imported_cpp_modules,
+  CreateSourceCode(retry_manager, exe_ctx, m_imported_cpp_modules,
/*for_completion*/ false);
-  // Clear the error diagnostics from the previous parse attempt.
-  diagnostic_manager.Clear();
-  parse_success = TryParse(diagnostic_manager, exe_scope, exe_ctx,
+  parse_success = TryParse(retry_manager, exe_scope, exe_ctx,
execution_policy, keep_result_in_memory,
generate_debug_info);
+  // Return the parse diagnostics if we were successful.
+  if (parse_success)
+diagnostic_manager = std::move(retry_manager);
 }
   }
   if (!parse_success)

diff  --git 
a/lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
 
b/lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
new file mode 100644
index 0..617045d760cc4
--- /dev/null
+++ 
b/lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
@@ -0,0 +1,9 @@
+# We don't have any standard include directories, so we can't
+# parse the test_common.h header we usually inject as it includes
+# system headers.
+NO_TEST_COMMON_H := 1
+
+CXXFLAGS_EXTRAS = -I $(SRCDIR)/root/usr/include/c++/v1/ -I 
$(SRCDIR)/root/usr/include/ -nostdinc -nostdinc++ -DBUILDING_OUTSIDE_LLDB=1
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff  --git 
a/lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
 
b/lldb/test/API/commands/expression/import-std-modu

[Lldb-commits] [PATCH] D110696: [lldb][import-std-module] Prefer the non-module diagnostics when in fallback mode

2021-10-04 Thread Raphael Isemann via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6fcb857746c1: [lldb][import-std-module] Prefer the 
non-module diagnostics when in fallback… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110696

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/vector
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
  
lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py

Index: lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
@@ -48,16 +48,6 @@
 self.expect("expr --top-level -- int i = std::max(1, 2);", error=True,
 substrs=["no member named 'max' in namespace 'std'"])
 
-# Check that diagnostics from the first parse attempt don't show up
-# in the C++ module parse attempt. In the expression below, we first
-# fail to parse 'std::max'. Then we retry with a loaded C++ module
-# and succeed to parse the 'std::max' part. However, the
-# trailing 'unknown_identifier' will fail to parse even with the
-# loaded module. The 'std::max' diagnostic from the first attempt
-# however should not be shown to the user.
-self.expect("expr std::max(1, 2); unknown_identifier", error=True,
-matching=False,
-substrs=["no member named 'max'"])
 # The proper diagnostic however should be shown on the retry.
 self.expect("expr std::max(1, 2); unknown_identifier", error=True,
 substrs=["use of undeclared identifier 'unknown_identifier'"])
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
@@ -0,0 +1 @@
+struct libc_struct {};
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
@@ -0,0 +1,3 @@
+module std {
+  module "algorithm" { header "algorithm" export * }
+}
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
@@ -0,0 +1,18 @@
+// This is only defined when building, but LLDB is missing this flag when loading the standard
+// library module so the actual contents of the module are missing.
+#ifdef BUILDING_OUTSIDE_LLDB
+
+#include "stdio.h"
+
+namespace std {
+  inline namespace __1 {
+// Pretend to be a std::vector template we need to instantiate
+// in LLDB.
+template
+struct vector { T i; int size() { return 2; } };
+  }
+}
+#else
+// Break the build when parsing from within LLDB.
+random_token_to_fail_the_build
+#endif // BUILDING_OUTSIDE_LLDB
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
@@ -0,0 +1,8 @@
+#include 
+
+int main(int argc, char **argv) {
+  // Makes sure we have the mock libc headers in the debug information.
+  libc_struct s;
+  std::vector v;
+  return 0; // Set break point at this line.
+}
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
===
--- /dev/null
++

[Lldb-commits] [PATCH] D107585: [lldb/Plugins] Add support for ScriptedThread in ScriptedProcess

2021-10-04 Thread 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 with a few nits that don't require re-review if they're fixed or turn out 
to be not relevant.




Comment at: lldb/include/lldb/Interpreter/ScriptedInterface.h:43-51
+  bool CheckStructuredDataObject(llvm::StringRef caller, T obj, Status &error) 
{
+if (!obj || !obj->IsValid() || error.Fail()) {
+  return ErrorWithMessage(caller,
+llvm::Twine("Null or invalid object (" +
+llvm::Twine(error.AsCString()) 
+
+llvm::Twine(")."))
+.str(),

I think I left a similar comment in the past, but since you know whether the 
object is null or invalid, why drop that information and be less precise in 
your error message? 



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:186
+void ScriptedThread::RefreshStateAfterStop() {
+  // TODO: Implement
+  if (m_reg_context_sp)

Still relevant?



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:205-206
+*reg_info, m_scripted_process.GetTarget().GetArchitecture());
+assert(m_register_info_sp->GetNumRegisters() > 0);
+assert(m_register_info_sp->GetNumRegisterSets() > 0);
+  }

Does this assertion depend on "user-input"? In other words, can this be 
triggered by not returning any registers from the interface? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107585

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

[Lldb-commits] [PATCH] D108953: [lldb/Plugins] Add memory region support in ScriptedProcess

2021-10-04 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

I'm slightly worried about the switch from a unique pointer to a shared 
pointer. It seems like a memory region is a simple enough object that it's 
warranted to make a copy to avoid this change. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108953

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

[Lldb-commits] [PATCH] D110981: [lldb] Improve help for platform put-file

2021-10-04 Thread Keith Smiley via Phabricator via lldb-commits 
keith updated this revision to Diff 377010.
keith marked an inline comment as done.
keith added a comment.

Update text about resolving paths


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110981

Files:
  lldb/source/Commands/CommandObjectPlatform.cpp


Index: lldb/source/Commands/CommandObjectPlatform.cpp
===
--- lldb/source/Commands/CommandObjectPlatform.cpp
+++ lldb/source/Commands/CommandObjectPlatform.cpp
@@ -1067,7 +1067,18 @@
   CommandObjectPlatformPutFile(CommandInterpreter &interpreter)
   : CommandObjectParsed(
 interpreter, "platform put-file",
-"Transfer a file from this system to the remote end.", nullptr, 0) 
{
+"Transfer a file from this system to the remote end.",
+"platform put-file  []", 0) {
+SetHelpLong(
+R"(Examples:
+
+(lldb) platform put-file /source/foo.txt /destination/bar.txt
+
+(lldb) platform put-file /source/foo.txt
+
+Relative source file paths are resolved against lldb's local working 
directory.
+
+Omitting the destination places the file in the platform working 
directory.)");
   }
 
   ~CommandObjectPlatformPutFile() override = default;


Index: lldb/source/Commands/CommandObjectPlatform.cpp
===
--- lldb/source/Commands/CommandObjectPlatform.cpp
+++ lldb/source/Commands/CommandObjectPlatform.cpp
@@ -1067,7 +1067,18 @@
   CommandObjectPlatformPutFile(CommandInterpreter &interpreter)
   : CommandObjectParsed(
 interpreter, "platform put-file",
-"Transfer a file from this system to the remote end.", nullptr, 0) {
+"Transfer a file from this system to the remote end.",
+"platform put-file  []", 0) {
+SetHelpLong(
+R"(Examples:
+
+(lldb) platform put-file /source/foo.txt /destination/bar.txt
+
+(lldb) platform put-file /source/foo.txt
+
+Relative source file paths are resolved against lldb's local working directory.
+
+Omitting the destination places the file in the platform working directory.)");
   }
 
   ~CommandObjectPlatformPutFile() override = default;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

2021-10-04 Thread Jaroslav Sevcik via Phabricator via lldb-commits 
jarin updated this revision to Diff 377020.
jarin marked 4 inline comments as done.
jarin added a comment.

Addressed Pavel's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/test/API/functionalities/unused-inlined-parameters/Makefile
  
lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py
  lldb/test/API/functionalities/unused-inlined-parameters/main.c
  lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
  lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

Index: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
@@ -0,0 +1,48 @@
+# RUN: llvm-mc -filetype=obj %S/Inputs/unused-inlined-params.s \
+# RUN: -triple x86_64-pc-linux -o %t.o
+# RUN: %lldb %t.o -s %s -o exit | FileCheck %s
+
+
+# In this test we verify that inlined functions still mention
+# all their parameters in `frame variable`, even when those
+# parameters were completely optimized away from the concrete
+# instance of the inlined function in the debug info.
+# The debugger should look up the parameters in the abstract
+# origin of the concrete instance.
+
+# Let us check that unused parameters of an inlined function are listed
+# at the inlined function entry.
+image lookup -v -s break_at_inlined_f_in_main
+# CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main
+# CHECK: name = "unused1", type = "void *", location = 
+# CHECK: name = "used", type = "int", location = DW_OP_consts +42
+# CHECK: name = "unused2", type = "int", location = 
+# CHECK: name = "partial", type = "int", location = DW_OP_reg4 RSI
+# CHECK: name = "unused3", type = "int", location = 
+
+# Show variables outsid of the live range of the 'partial' parameter
+# and verify that the output is as expected.
+image lookup -v -s break_at_inlined_f_in_main_between_printfs
+# CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main_between_printfs
+# CHECK: name = "unused1", type = "void *", location = 
+# CHECK: name = "used", type = "int", location = DW_OP_reg3 RBX
+# CHECK: name = "unused2", type = "int", location = 
+# Note: image lookup does not show variables outside of their
+#   location, so |partial| is missing here.
+# CHECK-NOT: partial
+# CHECK: name = "unused3", type = "int", location = 
+
+# Check that we show parameters even if all of them are compiled away.
+image lookup -v -s  break_at_inlined_g_in_main
+# CHECK-LABEL: image lookup -v -s  break_at_inlined_g_in_main
+# CHECK: name = "unused", type = "int", location = 
+
+# Check that even the other inlined instance of f displays the correct
+# parameters.
+image lookup -v -s  break_at_inlined_f_in_other
+# CHECK-LABEL: image lookup -v -s  break_at_inlined_f_in_other
+# CHECK: name = "unused1", type = "void *", location = 
+# CHECK: name = "used", type = "int", location = DW_OP_consts +1
+# CHECK: name = "unused2", type = "int", location = 
+# CHECK: name = "partial", type = "int", location =  DW_OP_consts +2
+# CHECK: name = "unused3", type = "int", location = 
Index: lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
@@ -0,0 +1,458 @@
+# The below program is roughly derived from the following C program.
+# To see the annotated debug info, look for the section 
+# '.section.debug_info' below.
+#
+# __attribute__((always_inline))
+# void f(void* unused1, int used, int unused2, int partial, int unused3) {
+#   used += partial;
+#   printf("f %i", partial);
+#   printf("f %i", used);   // |partial| is not live at this line.
+# }
+#
+# void g(int unused) {
+#   printf("Hello");
+# }
+#
+# __attribute__((noinline))
+# void other() {
+#   f(nullptr, 1, 0, 2, 0);
+# }
+#
+# int main(int argc, char** argv) {
+#   f(argv, 42, 1, argc, 2);
+#   g(1);
+#   other();
+#   return 0;
+# }
+
+.text
+.file"unused-inlined-params.c"
+
+.Lcu_begin:
+
+.globlother
+other:
+nop
+.Linlined_f_in_other:
+break_at_inlined_f_in_other:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_in_other_between_printfs:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_in_other_end:
+retq
+.Lother_end:
+.sizeother, .Lother_end-other
+
+.globlmain
+main:
+.file1 "/example" "unused-inlined-params.c"
+movl$1, %esi
+.Linlined_f:
+break_at_inlined_f_in_main:
+leal42(%rsi), %ebx
+.Linlined_f_before_printf:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_between_printfs:
+b

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

2021-10-04 Thread Jaroslav Sevcik via Phabricator via lldb-commits 
jarin added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3545
+
+DIEVector MergeBlockAbstractParameters(const DWARFDIE &block_die,
+   DIEVector &&variable_dies) {

labath wrote:
> In llvm, we prefer `static` functions over anonymous namespaces. 
> Theoretically, you could keep the anonymous namespace around the using 
> declaration (per 
> , as those 
> can't use `static`), though I would actually probably prefer  DIEArray type 
> defined in DIERef.h over a custom type.
Changed to static function, DIEArray (interestingly, this file actually starts 
with anonymous namespace, see line 121).



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:403
+  const llvm::SmallVectorImpl &variable_dies,
+  const lldb::addr_t func_low_pc);
 

labath wrote:
> We generally do not put a const qualifier on by-value arguments (it's pretty 
> useless).
> 
> (I see it's present on other functions too, but I don't know if they were 
> introduced by you or you're just propagating them.)
Copy pasta, unfortunately.



Comment at: 
lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py:17
+
+# For the unused parameters, only check the types.
+self.assertEqual("void *", 
self.frame().FindVariable("unused1").GetType().GetName())

labath wrote:
> jarin wrote:
> > labath wrote:
> > > Maybe we could check something else as well... Do `GetDescription` or 
> > > `str(value)` return something reasonable here?
> > Actually, the most important thing is the type here, so this was quite 
> > deliberate.
> > 
> > GetDescription returns `(void *) unused1 =  > been optimized out>\n\n`, but I am not sure if we can safely require that 
> > all future versions/platforms optimize the parameter out.
> I don't feel strongly about it, but I would say that this function is so 
> simple than any optimizer worthy of that name should be able to optimize 
> those arguments away. I might replace printf with a `noinline`/`optnone` 
> function though, to avoid any libc shenanigans.
I do not feel too strongly about this eiher.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [lldb] 93c1b3c - [lldb] Remove some anonymous namespaces

2021-10-04 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2021-10-05T08:35:18+02:00
New Revision: 93c1b3caf052f6575abffd29ae53441db2849534

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

LOG: [lldb] Remove some anonymous namespaces

.. and reduce the scope of others. They don't follow llvm coding
standards (which say they should be used only when the same effect
cannot be achieved with the static keyword), and they set a bad example.

Added: 


Modified: 
lldb/source/API/SBTarget.cpp
lldb/source/Breakpoint/Breakpoint.cpp
lldb/source/Core/PluginManager.cpp
lldb/source/Host/common/LockFileBase.cpp
lldb/source/Host/common/Socket.cpp
lldb/source/Host/common/TCPSocket.cpp
lldb/source/Host/common/UDPSocket.cpp
lldb/source/Host/common/XML.cpp
lldb/source/Host/linux/HostInfoLinux.cpp
lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
lldb/source/Host/posix/DomainSocket.cpp
lldb/source/Host/posix/HostProcessPosix.cpp
lldb/source/Host/posix/LockFilePosix.cpp
lldb/source/Host/posix/PipePosix.cpp
lldb/source/Host/windows/Host.cpp
lldb/source/Host/windows/HostThreadWindows.cpp
lldb/source/Host/windows/LockFileWindows.cpp
lldb/source/Host/windows/PipeWindows.cpp
lldb/source/Host/windows/ProcessLauncherWindows.cpp
lldb/source/Interpreter/OptionValuePathMappings.cpp
lldb/source/Plugins/ABI/ARC/ABISysV_arc.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
lldb/source/Plugins/Language/ObjC/NSDictionary.cpp

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptExpressionOpts.cpp

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/source/Plugins/Platform/Android/AdbClient.cpp
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp

Removed: 




diff  --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index 8829a55d57a4..af62d4e02eb1 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -73,9 +73,7 @@ using namespace lldb_private;
 
 #define DEFAULT_DISASM_BYTE_SIZE 32
 
-namespace {
-
-Status AttachToProcess(ProcessAttachInfo &attach_info, Target &target) {
+static Status AttachToProcess(ProcessAttachInfo &attach_info, Target &target) {
   std::lock_guard guard(target.GetAPIMutex());
 
   auto process_sp = target.GetProcessSP();
@@ -94,8 +92,6 @@ Status AttachToProcess(ProcessAttachInfo &attach_info, Target 
&target) {
   return target.Attach(attach_info, nullptr);
 }
 
-} // namespace
-
 // SBTarget constructor
 SBTarget::SBTarget() : m_opaque_sp() {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBTarget);

diff  --git a/lldb/source/Breakpoint/Breakpoint.cpp 
b/lldb/source/Breakpoint/Breakpoint.cpp
index 8d5d5a31337c..c644463719d4 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -602,7 +602,6 @@ void Breakpoint::ModulesChanged(ModuleList &module_list, 
bool load,
   }
 }
 
-namespace {
 static bool SymbolContextsMightBeEquivalent(SymbolContext &old_sc,
 SymbolContext &new_sc) {
   bool equivalent_scs = false;
@@ -640,7 +639,6 @@ static bool SymbolContextsMightBeEquivalent(SymbolContext 
&old_sc,
   }
   return equivalent_scs;
 }
-} // anonymous namespace
 
 void Breakpoint::ModuleReplaced(ModuleSP old_module_sp,
 ModuleSP new_module_sp) {

diff  --git a/lldb/source/Core/PluginManager.cpp 
b/lldb/source/Core/PluginManager.cpp
index 012143576e52..c0669927b636 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -1435,8 +1435,9 @@ namespace {
 typedef lldb::OptionValuePropertiesSP
 GetDebuggerPropertyForPluginsPtr(Debugger &, ConstString, ConstString,
  bool can_create);
+}
 
-lldb::OptionValuePropertiesSP
+static lldb::OptionValuePropertiesSP
 GetSettingForPlugin(Debugger &debugger, ConstString setting_name,
 ConstString plugin_type_name,
 GetDebuggerPropertyForPluginsPtr get_debugger_property =
@@ -1452,13 +1453,13 @@ GetSettingForPlugin(Debugger &debugger, ConstString 
setting_name,
   return properties_sp;
 }
 
-bool CreateSettingForPlugin(
-Debugger &debugger, ConstString plugin_type_name,
-ConstString plugin_type_desc,
-const lldb::OptionValuePropertiesSP &properties_sp, ConstString 
description,
-bool is_global_property,
-GetDebuggerPropertyForPluginsPtr get_debugger_property =
-GetDebuggerPropertyForPlugins) {
+static bool
+CreateSe

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

2021-10-04 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.

Let's ship this.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3545
+
+DIEVector MergeBlockAbstractParameters(const DWARFDIE &block_die,
+   DIEVector &&variable_dies) {

jarin wrote:
> labath wrote:
> > In llvm, we prefer `static` functions over anonymous namespaces. 
> > Theoretically, you could keep the anonymous namespace around the using 
> > declaration (per 
> > , as those 
> > can't use `static`), though I would actually probably prefer  DIEArray type 
> > defined in DIERef.h over a custom type.
> Changed to static function, DIEArray (interestingly, this file actually 
> starts with anonymous namespace, see line 121).
We're not always very good at following llvm policies, although I would say 
that this particular namespace is mostly ok-ish -- it mostly contains type 
declarations (classes, enums), where `static` does not work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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