[Lldb-commits] [PATCH] D133129: [lldb] Add boilerplate for debugger interrupts

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

I don't know how this is intended to be used, but I find it a rather 
heavyweight approach towards implementing, well... anything. The idea that 
something as innocuous as SBAddress::IsValid() will end up iterating through 
_all_ SBDebugger objects seems wrong, regardless of whether it has a measurable 
performance impact (and I'd be surprised if it doesn't). I'd hope that the 
interrupter can at least know which debugger it is trying to interrupt.

It's also not clear how something like this can be used to reliably interrupt 
an activity, as it's prone to all sorts of race conditions. (If the interuptee 
has not started the blocking call yet, then the interrupt request can be 
"eaten" by an innocuous IsValid call, whereas if it has already finished, then 
the interrupt request can remain pending and interrupt a totally unrelated 
call).

Overall, I think it would be good to have a RFC on this.


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

https://reviews.llvm.org/D133129

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

[Lldb-commits] [PATCH] D133181: [test] Remove problematic thread from MainLoopTest to fix flakiness

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

In D133181#3772830 , @rupprecht wrote:

> In D133181#3771747 , @labath wrote:
>
>> (I've reverted the pthread_kill part, as the mac build did not like it.)
>
> Thanks! I didn't get any buildbot notification; do LLDB build bots not send 
> email?

The regular buildbot bots do, but I'm not sure about the GreenDragon 
(@JDevlieghere ?). Although no emails would help in this case, as the bot was 
already red at the time this landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133181

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

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

I've only seen one flake 
 on linux so far, but 
the windows bot seems more susceptible 
 to this 
 problem. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133410

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

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath created this revision.
labath added reviewers: mgorny, JDevlieghere.
Herald added a project: All.
labath requested review of this revision.
Herald added a project: LLDB.

The Read function could end up blocking if data (or EOF) arrived just as
it was about to start waiting for the events. This was only discovered
now, because we did not have unit tests for this functionality before.
We need to check for data *after* we start listening for incoming
events. There were no changes to the read thread code needed, as we
already use this pattern in SynchronizeWithReadThread, so I just updated
the comments to make it clear that it is used for reading as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133410

Files:
  lldb/source/Core/ThreadedCommunication.cpp

Index: lldb/source/Core/ThreadedCommunication.cpp
===
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -104,34 +104,50 @@
   return 0;
 }
 
+// No data yet, we have to start listening.
 ListenerSP listener_sp(
 Listener::MakeListener("ThreadedCommunication::Read"));
 listener_sp->StartListeningForEvents(
 this, eBroadcastBitReadThreadGotBytes | eBroadcastBitReadThreadDidExit);
-EventSP event_sp;
-while (listener_sp->GetEvent(event_sp, timeout)) {
-  const uint32_t event_type = event_sp->GetType();
-  if (event_type & eBroadcastBitReadThreadGotBytes) {
-return GetCachedBytes(dst, dst_len);
-  }
 
-  if (event_type & eBroadcastBitReadThreadDidExit) {
-// If the thread exited of its own accord, it either means it
-// hit an end-of-file condition or an error.
-status = m_pass_status;
-if (error_ptr)
-  *error_ptr = std::move(m_pass_error);
+// Re-check for data, as it might have arrived while we were setting up our
+// listener.
+cached_bytes = GetCachedBytes(dst, dst_len);
+if (cached_bytes > 0) {
+  status = eConnectionStatusSuccess;
+  return cached_bytes;
+}
 
-if (GetCloseOnEOF())
-  Disconnect(nullptr);
+EventSP event_sp;
+// Explicitly check for the thread exit, for the same reason.
+if (m_read_thread_did_exit) {
+  // We've missed the event, lets just conjure one up.
+  event_sp = std::make_shared(eBroadcastBitReadThreadDidExit);
+} else {
+  if (!listener_sp->GetEvent(event_sp, timeout)) {
+if (error_ptr)
+  error_ptr->SetErrorString("Timed out.");
+status = eConnectionStatusTimedOut;
 return 0;
   }
 }
+const uint32_t event_type = event_sp->GetType();
+if (event_type & eBroadcastBitReadThreadGotBytes) {
+  return GetCachedBytes(dst, dst_len);
+}
 
-if (error_ptr)
-  error_ptr->SetErrorString("Timed out.");
-status = eConnectionStatusTimedOut;
-return 0;
+if (event_type & eBroadcastBitReadThreadDidExit) {
+  // If the thread exited of its own accord, it either means it
+  // hit an end-of-file condition or an error.
+  status = m_pass_status;
+  if (error_ptr)
+*error_ptr = std::move(m_pass_error);
+
+  if (GetCloseOnEOF())
+Disconnect(nullptr);
+  return 0;
+}
+llvm_unreachable("Got unexpected event type!");
   }
 
   // We aren't using a read thread, just read the data synchronously in this
@@ -299,22 +315,25 @@
   m_pass_error = std::move(error);
   LLDB_LOG(log, "Communication({0}) thread exiting...", this);
 
-  // Handle threads wishing to synchronize with us.
-  {
-// Prevent new ones from showing up.
-m_read_thread_did_exit = true;
+  // Start shutting down. We need to do this in a very specific order to ensure
+  // we don't race with threads wanting to read/synchronize with us.
 
-// Unblock any existing thread waiting for the synchronization signal.
-BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  // First, we signal our intent to exit. This ensures no new thread start
+  // waiting on events from us.
+  m_read_thread_did_exit = true;
 
-// Wait for the thread to finish...
+  // Unblock any existing thread waiting for the synchronization signal.
+  BroadcastEvent(eBroadcastBitNoMorePendingInput);
+
+  {
+// Wait for the synchronization thread to finish...
 std::lock_guard guard(m_synchronize_mutex);
 // ... and disconnect.
 if (disconnect)
   Disconnect();
   }
 
-  // Let clients know that this thread is exiting
+  // Finally, unblock any readers waiting for us to exit.
   BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133129: [lldb] Add boilerplate for debugger interrupts

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

I've added you to D133410 , as I think it 
demonstrates very well the difficulties in getting two threads to talk to one 
another. This code has been here since forever (and may have contributed to 
your desire to introduce interrupts), but we've only found the problem now, 
when Michał added some unit tests for this class.


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

https://reviews.llvm.org/D133129

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

[Lldb-commits] [PATCH] D130062: [lldb][DWARF5] Enable macro evaluation

2022-09-07 Thread Pavel Kosov via Phabricator via lldb-commits 
kpdev42 added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130062

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath updated this revision to Diff 458405.
labath added a comment.

dynamically resize error buffer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -1205,128 +1205,6 @@
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
 
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_A(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-# NB: strictly speaking we should use %x here but this packet
-# is deprecated, so no point in changing lldb-server's expectations
-self.test_sequence.add_log_lines(
-["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
- tuple(itertools.chain.from_iterable(
- [(len(x), x) for x in hex_args])),
- "send packet: $OK#00",
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_vRun(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@add_test_categories(["llgs"])
-def test_launch_via_vRun_no_args(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-hex_path = binascii.b2a_hex(exe_path.encode()).decode()
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (hex_path,),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-self.expect_gdbremote_sequence()
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironment(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "test", "BAR": "a=z"}
-args = [exe_path, "print-env:FOO", "print-env:BAR"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-
-for key, value in env.items():
-self.test_sequence.add_log_lines(
-["read packet: $QEnvironment:%s=%s#00" % (key, value),
- "send packet: $OK#00"],
-True)
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (";".join(hex_args),),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"], b"test\r\na=z\r\n")
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironmentHexEncoded(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "test", "BAR": "a=z", "BAZ": "a*}#z"}

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:293
+r = llvm::sys::RetryAfterSignal(-1, read, pipe.GetReadFileDescriptor(), 
pos,
+buf.end() - pos);
+  } while (r > 0);

rupprecht wrote:
> IIUC, this will overrun the buffer if there are >1000 bytes to read; whereas 
> previously we just wouldn't have read everything.
> 
> Should each loop iteration grow the buffer by a certain amount? Otherwise I 
> think we need to remove the loop.
It won't overrun because I am passing the remaining part of the buffer as the 
size argument. However, I am not totally sure what would happen if we actually 
filled the buffer (and size becomes zero).  That won't happen right now because 
the error string is coming from `ExitWithError` (line 50) and its length is 
limited by the longest errno message. That said, the dynamical resize is quite 
simple in this case, so I might as well do it.



Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [seven.hexlify(x) for x in args]
+

mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > Since we no longer support Python 2, I'd rather prefer to work towards 
> > > removing `seven` rather than making more use of it.
> > Is the problem with the name/location of the function or the functionality 
> > (string/byte conversion) itself?
> > Because, if it's the first, then that could easily be solved by renaming 
> > the module (now or later), but in order to avoid elaborate casts we'd have 
> > to make all code be byte/string correct. This is not a problem here 
> > (because of the fixed strings), but it becomes one once you start working 
> > with things that aren't necessarily valid utf8 strings.
> Ok, I suppose this makes sense given how LLDB's Python API is screwed up :-(.
Just to clarify, I would like if we used the byte/string separation more (and I 
started that when I refactored the gdb server test harness), but it's fairly 
tricky because we're often working with things (e.g. memory contents, or 
stdout) that can represent arbitrary data in general, but which usually (for 
our own convenience and sanity) contain simple ASCII strings. So we have a 
problem where the API (and here I just mean the gdbserver test API, which we 
can change) really wants us to use bytes, but the the most natural way to write 
the test is with strings. I haven't come up with a solution to that yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath updated this revision to Diff 458408.
labath added a comment.

Use `pos` instead of the previous size in the resize expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -1205,128 +1205,6 @@
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
 
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_A(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-# NB: strictly speaking we should use %x here but this packet
-# is deprecated, so no point in changing lldb-server's expectations
-self.test_sequence.add_log_lines(
-["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
- tuple(itertools.chain.from_iterable(
- [(len(x), x) for x in hex_args])),
- "send packet: $OK#00",
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_vRun(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@add_test_categories(["llgs"])
-def test_launch_via_vRun_no_args(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-hex_path = binascii.b2a_hex(exe_path.encode()).decode()
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (hex_path,),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-self.expect_gdbremote_sequence()
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironment(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "test", "BAR": "a=z"}
-args = [exe_path, "print-env:FOO", "print-env:BAR"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-
-for key, value in env.items():
-self.test_sequence.add_log_lines(
-["read packet: $QEnvironment:%s=%s#00" % (key, value),
- "send packet: $OK#00"],
-True)
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (";".join(hex_args),),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"], b"test\r\na=z\r\n")
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironmentHexEncoded(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "tes

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

2022-09-07 Thread Michał Górny via Phabricator via lldb-commits 
mgorny added inline comments.



Comment at: lldb/source/Core/ThreadedCommunication.cpp:113
 
-  if (event_type & eBroadcastBitReadThreadDidExit) {
-// If the thread exited of its own accord, it either means it
-// hit an end-of-file condition or an error.
-status = m_pass_status;
-if (error_ptr)
-  *error_ptr = std::move(m_pass_error);
+// Re-check for data, as it might have arrived while we were setting up our
+// listener.

Can you think of any reason not to move listener setup before the first 
`GetCachedBytes()` call instead of duplicating it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133410

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

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/source/Core/ThreadedCommunication.cpp:113
 
-  if (event_type & eBroadcastBitReadThreadDidExit) {
-// If the thread exited of its own accord, it either means it
-// hit an end-of-file condition or an error.
-status = m_pass_status;
-if (error_ptr)
-  *error_ptr = std::move(m_pass_error);
+// Re-check for data, as it might have arrived while we were setting up our
+// listener.

mgorny wrote:
> Can you think of any reason not to move listener setup before the first 
> `GetCachedBytes()` call instead of duplicating it?
The only possible reason is "efficiency" (avoiding the creation of the listener 
and all that goes with it). But I'm definitely not convinced that this actually 
matters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133410

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

2022-09-07 Thread Michał Górny via Phabricator via lldb-commits 
mgorny added inline comments.



Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [seven.hexlify(x) for x in args]
+

labath wrote:
> mgorny wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > Since we no longer support Python 2, I'd rather prefer to work towards 
> > > > removing `seven` rather than making more use of it.
> > > Is the problem with the name/location of the function or the 
> > > functionality (string/byte conversion) itself?
> > > Because, if it's the first, then that could easily be solved by renaming 
> > > the module (now or later), but in order to avoid elaborate casts we'd 
> > > have to make all code be byte/string correct. This is not a problem here 
> > > (because of the fixed strings), but it becomes one once you start working 
> > > with things that aren't necessarily valid utf8 strings.
> > Ok, I suppose this makes sense given how LLDB's Python API is screwed up 
> > :-(.
> Just to clarify, I would like if we used the byte/string separation more (and 
> I started that when I refactored the gdb server test harness), but it's 
> fairly tricky because we're often working with things (e.g. memory contents, 
> or stdout) that can represent arbitrary data in general, but which usually 
> (for our own convenience and sanity) contain simple ASCII strings. So we have 
> a problem where the API (and here I just mean the gdbserver test API, which 
> we can change) really wants us to use bytes, but the the most natural way to 
> write the test is with strings. I haven't come up with a solution to that yet.
Yeah, I know. On the plus side, as long as we're dealing with plain ASCII, 
using `bytes` is relatively easy and most of the string-like functionality just 
works. You just have to remember that you want to use byte-strings ;-).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

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

[Lldb-commits] [PATCH] D133164: Add the ability to show when variables fails to be available when debug info is valid.

2022-09-07 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/include/lldb/Target/StackFrame.h:264
   /// A pointer to a list of variables.
-  VariableList *GetVariableList(bool get_file_globals);
+  VariableList *GetVariableList(bool get_file_globals, Status *error_ptr);
 

clayborg wrote:
> There are many places that call this function that also don't need to check 
> the error and if we use a Expected, we need to add a bunch of 
> consumeError(...) code. See all of the call sites where I added a "nullptr" 
> for the "error_ptr" to see why I chose to do it this way to keep the code 
> cleaner. Many places are getting the variable list to look for things or use 
> them for autocomplete.
Ok, I am convinced by the optionalness of the error in this case.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4161
+  if (command) {
+if (command->contains(" -gline-tables-only"))
+  return Status("-gline-tables-only enabled, no variable information is "

clayborg wrote:
> labath wrote:
> > This isn't a particularly reliable way of detecting whether variable 
> > information was emitted. For example a command line `clang 
> > -gline-tables-only -g2` will in fact produce full debug info and `clang 
> > -g1` will not. Could we make that determination based on the presence of 
> > actual variable DIEs in the debug info? Perhaps query the index whether it 
> > knows of any (global) variable or any type defined within the compile unit?
> This function only gets called when there are no variables in a stack frame 
> at all and checks for reasons why. So if "-gline-tables-only -g2" is used, 
> this function won't get called if there were variables.
> 
> I planned to make a follow up patch that detected no variables in a compile 
> uint by checking the compile unit's abbreviation set to see if it had any 
> DW_TAG_variable or DW_TAG_formal_parameter definitions, and if there weren't 
> any respond with "-gline-tables-only might be enabled". 
> 
> If we see this option for sure and have the side affect of there being no 
> variables, I would like to user the users know what they can do to fix things 
> if at all possible. 
I get that, but this check is not completely correct in either direction. For 
example, `clang -g1` will not produce variable information, but this code will 
not detect it. Same goes for `clang -gmlt`. And I probably missed some other 
ways one can prevent variable info from being generated. Keeping up with all 
the changes in clang flags will just be a game of whack-a-mole. If we checked 
the actual debug info, then we would catch all of these cases, including the 
(default) case when the compiler did not embed command line information into 
the debug info.

And I don't think we need to know the precise command line to give meaningful 
advice to the users. In all of these cases, sticking an extra `-g` at the end 
of the command line will cause the variables to reappear. If we wanted to, we 
could also put a link to the lldb web page where we can (at length) describe 
the various reasons why variables may not be available and how to fix them.



Comment at: lldb/test/API/commands/frame/var/TestFrameVar.py:101
+for s in error_strings:
+self.assertTrue(s in command_error, 'Make sure "%s" exists in the 
command error "%s"' % (s, command_error))
+for s in error_strings:

just change to `assertIn(s, command_error)`, and then you get the error message 
for free.



Comment at: lldb/test/API/commands/frame/var/TestFrameVar.py:174-175
+'''
+self.build(dictionary={'CFLAGS_EXTRAS': '-gline-tables-only'},
+   env={"RC_DEBUG_OPTIONS": "1"})
+exe = self.getBuildArtifact("a.out")

clayborg wrote:
> labath wrote:
> > Why not just pass `-grecord-command-line` in CFLAGS_EXTRAS? I think then 
> > you should be able to remove @skipUnlessDarwin from this test...
> I thought I tried that, but I was able to get this to work as suggested. I 
> will change this.
Cool. Can we also remove @skipUnlessDarwin then?



Comment at: lldb/test/API/functionalities/archives/Makefile:12
$(AR) $(ARFLAGS) $@ $^
-   $(RM) $^
+   # $(RM) $^
 

I don't know how important this is, but I believe the build was deleting the .o 
files to ensure that we access the copies within the archive. If you think 
that's fine, then just delete this line.



Comment at: lldb/test/API/functionalities/archives/Makefile:19
$(LLVM_AR) $(LLVM_ARFLAGS) $@ $^
# Note for thin archive case, we cannot remove c.o
 

And probably this comment as well, because it doesn't make sense if we don't 
delete the other files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133164

__

[Lldb-commits] [PATCH] D132940: [lldb] Use just-built libcxx for tests when available

2022-09-07 Thread Felipe de Azevedo Piovezan via Phabricator via lldb-commits 
fdeazeve updated this revision to Diff 458439.
fdeazeve added a comment.

Addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132940

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/test/API/commands/expression/fixits/TestFixIts.py
  
lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
  lldb/test/API/lang/objc/exceptions/Makefile
  lldb/test/API/macosx/macCatalyst/Makefile
  lldb/test/API/python_api/sbmodule/TestSBModule.py
  lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py

Index: lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
===
--- lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -70,6 +70,7 @@
 'SDKROOT': sdkroot.strip(),
 'ARCH': arch,
 'ARCH_CFLAGS': '-target {} {}'.format(triple, version_min),
+'USE_SYSTEM_STDLIB': 1,
 })
 exe_path = os.path.realpath(self.getBuildArtifact(exe_name))
 cmd = [
Index: lldb/test/API/python_api/sbmodule/TestSBModule.py
===
--- lldb/test/API/python_api/sbmodule/TestSBModule.py
+++ lldb/test/API/python_api/sbmodule/TestSBModule.py
@@ -47,8 +47,8 @@
 process = target.AttachToProcessWithID(self.dbg.GetListener(),
self.background_pid, error)
 self.assertTrue(error.Success() and process,  PROCESS_IS_VALID)
-main_module = target.GetModuleAtIndex(0)
-self.assertEqual(main_module.GetFileSpec().GetFilename(), "a.out")
+main_module = target.FindModule(lldb.SBFileSpec("a.out"))
+self.assertTrue(main_module is not None)
 self.assertFalse(main_module.IsFileBacked(),
  "The module should not be backed by a file on disk.")
 
Index: lldb/test/API/macosx/macCatalyst/Makefile
===
--- lldb/test/API/macosx/macCatalyst/Makefile
+++ lldb/test/API/macosx/macCatalyst/Makefile
@@ -3,6 +3,8 @@
 override TRIPLE := $(ARCH)-apple-ios13.1-macabi
 CFLAGS_EXTRAS := -target $(TRIPLE)
 
+USE_SYSTEM_STDLIB := 1
+
 # FIXME: rdar://problem/54986190
 # There is a Clang driver change missing on llvm.org.
 override CC=xcrun clang
Index: lldb/test/API/lang/objc/exceptions/Makefile
===
--- lldb/test/API/lang/objc/exceptions/Makefile
+++ lldb/test/API/lang/objc/exceptions/Makefile
@@ -2,7 +2,7 @@
 
 CFLAGS_EXTRAS := -w
 
-
+USE_SYSTEM_STDLIB := 1
 
 LD_EXTRAS := -framework Foundation
 include Makefile.rules
Index: lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
@@ -29,8 +29,3 @@
 self.expect_expr("std::distance(a.begin(), a.end())", result_value="3")
 self.expect_expr("a.front().a", result_type="int", result_value="3")
 self.expect_expr("a.begin()->a", result_type="int", result_value="3")
-
-# FIXME: The value here isn't actually empty.
-self.expect_expr("a.front()",
- result_type=value_type,
- result_children=[ValueCheck()])
Index: lldb/test/API/commands/expression/fixits/TestFixIts.py
===
--- lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ lldb/test/API/commands/expression/fixits/TestFixIts.py
@@ -45,7 +45,7 @@
 
 # Try with one error in a top-level expression.
 # The Fix-It changes "ptr.m" to "ptr->m".
-expr = "struct X { int m; }; X x; X *ptr = &x; int m = ptr.m;"
+expr = "struct MyTy { int m; }; MyTy x; MyTy *ptr = &x; int m = ptr.m;"
 value = frame.EvaluateExpression(expr, top_level_options)
 # A successfully parsed top-level expression will yield an error
 # that there is 'no value'. If a parsing error would have happened we
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -387,6 +387,16 @@
 #--
 # C++ standard library options
 #---

[Lldb-commits] [PATCH] D132940: [lldb] Use just-built libcxx for tests when available

2022-09-07 Thread Felipe de Azevedo Piovezan via Phabricator via lldb-commits 
fdeazeve added inline comments.



Comment at: lldb/test/API/commands/expression/fixits/TestFixIts.py:54
+# FIXME: LLDB struggles with this when stdlib has debug info.
+if not lldbutil.has_debug_info_in_libcxx(target):
+self.assertEquals(value.GetError().GetCString(), "error: No value")

labath wrote:
> What kind of error are you getting here? Are you sure this is not some form 
> of https://github.com/llvm/llvm-project/issues/34391, which could be worked 
> around by renaming some variables in the expression?
The error is indeed about a name clash with `X`, where the expression evaluator 
also sees a definition of a type `X` inside a function in 
`libcxxabi/src/private_typeinfo.cpp`.

It seems like the link you described has a variant of this problem, so I'll 
make a comment there and rename the `X` in this test to something else. Thanks 
for the link!



Comment at: 
lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py:35
 # FIXME: The value here isn't actually empty.
-self.expect_expr("a.front()",
- result_type=value_type,
- result_children=[ValueCheck()])
+# FIXME: LLDB struggles with this when stdlib has debug info.
+if not lldbutil.has_debug_info_in_libcxx(target):

labath wrote:
> And here it might be better to just remove the check, as it's checking for 
> buggy behavior anyway.
Done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132940

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

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

2022-09-07 Thread Michał Górny via Phabricator via lldb-commits 
mgorny accepted this revision.
mgorny added a comment.
This revision is now accepted and ready to land.

Thanks for noticing and fixing this.




Comment at: lldb/source/Core/ThreadedCommunication.cpp:113
 
-  if (event_type & eBroadcastBitReadThreadDidExit) {
-// If the thread exited of its own accord, it either means it
-// hit an end-of-file condition or an error.
-status = m_pass_status;
-if (error_ptr)
-  *error_ptr = std::move(m_pass_error);
+// Re-check for data, as it might have arrived while we were setting up our
+// listener.

labath wrote:
> mgorny wrote:
> > Can you think of any reason not to move listener setup before the first 
> > `GetCachedBytes()` call instead of duplicating it?
> The only possible reason is "efficiency" (avoiding the creation of the 
> listener and all that goes with it). But I'm definitely not convinced that 
> this actually matters.
Ok, I'm not bent on it either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133410

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

[Lldb-commits] [PATCH] D133427: [gdb-remote] Move broadcasting logic down to GDBRemoteClientBase

2022-09-07 Thread Michał Górny via Phabricator via lldb-commits 
mgorny created this revision.
mgorny added reviewers: labath, krytarowski, emaste, jingham.
Herald added a subscriber: arichardson.
Herald added a project: All.
mgorny requested review of this revision.

Move the broadcasting support from GDBRemoteCommunication
to GDBRemoteClientBase since this is where it is actually used.  Remove
GDBRemoteCommunication and subclass constructor arguments left over
after Communication cleanup.

Sponsored by: The FreeBSD Foundation


https://reviews.llvm.org/D133427

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h

Index: lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
+++ lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
@@ -53,8 +53,7 @@
 
 class MockServer : public GDBRemoteCommunicationServer {
 public:
-  MockServer()
-  : GDBRemoteCommunicationServer("mock-server", "mock-server.listener") {
+  MockServer() : GDBRemoteCommunicationServer() {
 m_send_acks = false;
 m_send_error_strings = true;
   }
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
@@ -17,8 +17,7 @@
 
 class TestClient : public GDBRemoteCommunication {
 public:
-  TestClient()
-  : GDBRemoteCommunication("test.client", "test.client.listener") {}
+  TestClient() : GDBRemoteCommunication() {}
 
   PacketResult ReadPacket(StringExtractorGDBRemote &response) {
 return GDBRemoteCommunication::ReadPacket(response, std::chrono::seconds(1),
Index: lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
@@ -39,7 +39,7 @@
 };
 
 struct TestClient : public GDBRemoteClientBase {
-  TestClient() : GDBRemoteClientBase("test.client", "test.client.listener") {
+  TestClient() : GDBRemoteClientBase("test.client") {
 m_send_acks = false;
   }
 };
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1168,7 +1168,7 @@
   ListenerSP listener_sp(
   Listener::MakeListener("gdb-remote.resume-packet-sent"));
   if (listener_sp->StartListeningForEvents(
-  &m_gdb_comm, GDBRemoteCommunication::eBroadcastBitRunPacketSent)) {
+  &m_gdb_comm, GDBRemoteClientBase::eBroadcastBitRunPacketSent)) {
 listener_sp->StartListeningForEvents(
 &m_async_broadcaster,
 ProcessGDBRemote::eBroadcastBitAsyncThreadDidExit);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
@@ -109,8 +109,7 @@
 // GDBRemoteCommunicationServerPlatform constructor
 GDBRemoteCommunicationServerPlatform::GDBRemoteCommunicationServerPlatform(
 const Socket::SocketProtocol socket_protocol, const char *socket_scheme)
-: GDBRemoteCommunicationServerCommon("gdb-remote.server",
- "gdb-remote.server.rx_packet"),
+: GDBRemoteCommunicationServerCommon(),
   m_socket_protocol(socket_protocol), m_socket_scheme(socket_scheme),
   m_spawned_pids_mutex(), m_port_map(), m_port_offset(0) {
   m_pending_gdb_server.pid = LLDB_INVALID_PROCESS_ID;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===

[Lldb-commits] [PATCH] D133446: [LLDB][NativePDB] Global ctor and dtor should be global decls.

2022-09-07 Thread Zequan Wu via Phabricator via lldb-commits 
zequanwu created this revision.
zequanwu added reviewers: labath, rnk.
Herald added a project: All.
zequanwu requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This fixes a crash that mistaken global ctor/dtor as funciton methods.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133446

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp


Index: lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
@@ -0,0 +1,30 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Global ctor and dtor should be globals decls.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -fno-addrsig -c 
/Fo%t.obj -- %s
+// RUN: lld-link -opt:icf -debug:full -nodefaultlib -entry:main %t.obj 
-out:%t.exe -pdb:%t.pdb -force
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | 
FileCheck %s
+
+struct A {
+  ~A() {};
+};
+struct B {
+  static A glob;
+};
+
+A B::glob = A();
+int main() {
+  return 0;
+}
+
+// CHECK:  struct B {
+// CHECK-NEXT: static A glob;
+// CHECK-NEXT: };
+// CHECK-NEXT: static void `dynamic initializer for 'glob'();
+// CHECK-NEXT: static void `dynamic atexit destructor for 'glob'();
+// CHECK-NEXT: int main();
+// CHECK-NEXT: static void _GLOBAL__sub_I_global_ctor_dtor.cpp();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT: ~A();
+// CHECK-NEXT: };
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -1226,7 +1226,10 @@
   llvm::StringRef proc_name = proc.Name;
   proc_name.consume_front(context_name);
   proc_name.consume_front("::");
-
+  // Global ctor and dtor are global decls.
+  if (proc_name.contains("dynamic initializer for") ||
+  proc_name.contains("dynamic atexit destructor for"))
+parent = FromCompilerDeclContext(GetTranslationUnitDecl());
   clang::FunctionDecl *function_decl =
   CreateFunctionDecl(func_id, proc_name, proc.FunctionType, func_ct,
  func_type->getNumParams(), storage, false, parent);


Index: lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
@@ -0,0 +1,30 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Global ctor and dtor should be globals decls.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -fno-addrsig -c /Fo%t.obj -- %s
+// RUN: lld-link -opt:icf -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb -force
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | FileCheck %s
+
+struct A {
+  ~A() {};
+};
+struct B {
+  static A glob;
+};
+
+A B::glob = A();
+int main() {
+  return 0;
+}
+
+// CHECK:  struct B {
+// CHECK-NEXT: static A glob;
+// CHECK-NEXT: };
+// CHECK-NEXT: static void `dynamic initializer for 'glob'();
+// CHECK-NEXT: static void `dynamic atexit destructor for 'glob'();
+// CHECK-NEXT: int main();
+// CHECK-NEXT: static void _GLOBAL__sub_I_global_ctor_dtor.cpp();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT: ~A();
+// CHECK-NEXT: };
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -1226,7 +1226,10 @@
   llvm::StringRef proc_name = proc.Name;
   proc_name.consume_front(context_name);
   proc_name.consume_front("::");
-
+  // Global ctor and dtor are global decls.
+  if (proc_name.contains("dynamic initializer for") ||
+  proc_name.contains("dynamic atexit destructor for"))
+parent = FromCompilerDeclContext(GetTranslationUnitDecl());
   clang::FunctionDecl *function_decl =
   CreateFunctionDecl(func_id, proc_name, proc.FunctionType, func_ct,
  func_type->getNumParams(), storage, false, parent);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133446: [LLDB][NativePDB] Global ctor and dtor should be global decls.

2022-09-07 Thread Reid Kleckner via Phabricator via lldb-commits 
rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1198
 
   clang::DeclContext *parent = GetParentDeclContext(PdbSymUid(func_id));
   if (!parent)

Should the check be done before GetParentDeclContext or be done inside 
GetParentDeclContext so that we don't do wasted work computing a parent that 
won't be used for dynamic initializers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133446

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

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

2022-09-07 Thread Jordan Rupprecht via Phabricator via lldb-commits 
rupprecht accepted this revision.
rupprecht added a comment.

Thanks again! Still looks good.




Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:70
 
+static void consumeNamespace(llvm::StringRef &name) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::

nit: this consumes just the inline namespace, so I think 
`consumeInlineNamespace` might be a better name. I don't feel that strongly 
though so I'll leave it up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

2022-09-07 Thread Jordan Rupprecht via Phabricator via lldb-commits 
rupprecht accepted this revision.
rupprecht added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:293
+r = llvm::sys::RetryAfterSignal(-1, read, pipe.GetReadFileDescriptor(), 
pos,
+buf.end() - pos);
+  } while (r > 0);

labath wrote:
> rupprecht wrote:
> > IIUC, this will overrun the buffer if there are >1000 bytes to read; 
> > whereas previously we just wouldn't have read everything.
> > 
> > Should each loop iteration grow the buffer by a certain amount? Otherwise I 
> > think we need to remove the loop.
> It won't overrun because I am passing the remaining part of the buffer as the 
> size argument. However, I am not totally sure what would happen if we 
> actually filled the buffer (and size becomes zero).  That won't happen right 
> now because the error string is coming from `ExitWithError` (line 50) and its 
> length is limited by the longest errno message. That said, the dynamical 
> resize is quite simple in this case, so I might as well do it.
Thanks! I still think it's good to be dynamic in case this assumption changes 
one day, and the error string ends up being longer than 1000 bytes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

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

[Lldb-commits] [PATCH] D133446: [LLDB][NativePDB] Global ctor and dtor should be global decls.

2022-09-07 Thread Zequan Wu via Phabricator via lldb-commits 
zequanwu updated this revision to Diff 458583.
zequanwu marked an inline comment as done.
zequanwu added a comment.

Address comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133446

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp


Index: lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
@@ -0,0 +1,30 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Global ctor and dtor should be globals decls.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -fno-addrsig -c 
/Fo%t.obj -- %s
+// RUN: lld-link -opt:icf -debug:full -nodefaultlib -entry:main %t.obj 
-out:%t.exe -pdb:%t.pdb -force
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | 
FileCheck %s
+
+struct A {
+  ~A() {};
+};
+struct B {
+  static A glob;
+};
+
+A B::glob = A();
+int main() {
+  return 0;
+}
+
+// CHECK:  struct B {
+// CHECK-NEXT: static A glob;
+// CHECK-NEXT: };
+// CHECK-NEXT: static void `dynamic initializer for 'glob'();
+// CHECK-NEXT: static void `dynamic atexit destructor for 'glob'();
+// CHECK-NEXT: int main();
+// CHECK-NEXT: static void _GLOBAL__sub_I_global_ctor_dtor.cpp();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT: ~A();
+// CHECK-NEXT: };
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -534,11 +534,15 @@
 
 std::pair
 PdbAstBuilder::CreateDeclInfoForUndecoratedName(llvm::StringRef name) {
+  clang::DeclContext *context =
+  FromCompilerDeclContext(GetTranslationUnitDecl());
+  // Global ctor and dtor are global decls.
+  if (name.contains("dynamic initializer for") ||
+  name.contains("dynamic atexit destructor for"))
+return {context, std::string(name)};
   MSVCUndecoratedNameParser parser(name);
   llvm::ArrayRef specs = parser.GetSpecifiers();
 
-  auto context = FromCompilerDeclContext(GetTranslationUnitDecl());
-
   llvm::StringRef uname = specs.back().GetBaseName();
   specs = specs.drop_back();
   if (specs.empty())
@@ -1226,7 +1230,6 @@
   llvm::StringRef proc_name = proc.Name;
   proc_name.consume_front(context_name);
   proc_name.consume_front("::");
-
   clang::FunctionDecl *function_decl =
   CreateFunctionDecl(func_id, proc_name, proc.FunctionType, func_ct,
  func_type->getNumParams(), storage, false, parent);


Index: lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp
@@ -0,0 +1,30 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Global ctor and dtor should be globals decls.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -fno-addrsig -c /Fo%t.obj -- %s
+// RUN: lld-link -opt:icf -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb -force
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | FileCheck %s
+
+struct A {
+  ~A() {};
+};
+struct B {
+  static A glob;
+};
+
+A B::glob = A();
+int main() {
+  return 0;
+}
+
+// CHECK:  struct B {
+// CHECK-NEXT: static A glob;
+// CHECK-NEXT: };
+// CHECK-NEXT: static void `dynamic initializer for 'glob'();
+// CHECK-NEXT: static void `dynamic atexit destructor for 'glob'();
+// CHECK-NEXT: int main();
+// CHECK-NEXT: static void _GLOBAL__sub_I_global_ctor_dtor.cpp();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT: ~A();
+// CHECK-NEXT: };
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -534,11 +534,15 @@
 
 std::pair
 PdbAstBuilder::CreateDeclInfoForUndecoratedName(llvm::StringRef name) {
+  clang::DeclContext *context =
+  FromCompilerDeclContext(GetTranslationUnitDecl());
+  // Global ctor and dtor are global decls.
+  if (name.contains("dynamic initializer for") ||
+  name.contains("dynamic atexit destructor for"))
+return {context, std::string(name)};
   MSVCUndecoratedNameParser parser(name);
   llvm::ArrayRef specs = parser.GetSpecifiers();
 
-  auto context = FromCompilerDeclContext(GetTranslationUnitDecl());
-
   llvm::StringRef uname = specs.back().GetBaseName();
   specs = specs.drop_back();
   if (specs.empty())
@@ -1226,7 +1230,6 @@
   llvm::StringRef proc_name = proc.Name;
   proc_name.consume_front(context_name);
   proc_name.consume_front("::");
-
   clang::FunctionDecl *function_decl =
   C

[Lldb-commits] [PATCH] D133461: [LLDB][NativePDB] Set block address range.

2022-09-07 Thread Zequan Wu via Phabricator via lldb-commits 
zequanwu created this revision.
zequanwu added reviewers: rnk, labath.
Herald added a project: All.
zequanwu requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The block address range was missing before.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133461

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/test/Shell/SymbolFile/NativePDB/blocks.cpp


Index: lldb/test/Shell/SymbolFile/NativePDB/blocks.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/blocks.cpp
@@ -0,0 +1,20 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test block range is set.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -c /Fo%t.obj -- %s
+// RUN: lld-link -debug:full -nodefaultlib -entry:main -base:0x14000 
%t.obj -out:%t.exe -pdb:%t.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb %t.exe -o "image lookup -a 
0x140001014 -v" | FileCheck %s
+
+int main() {
+  int count = 0;
+  for (int i = 0; i < 3; ++i) {
+++count;
+  }
+  return count;
+}
+
+// CHECK:  Function: id = {{.*}}, name = "main", range = 
[0x000140001000-0x00014000104b)
+// CHECK-NEXT: FuncType: id = {{.*}}, byte-size = 0, compiler_type = "int 
(void)"
+// CHECK-NEXT:   Blocks: id = {{.*}}, range = [0x140001000-0x14000104b)
+// CHECK-NEXT:   id = {{.*}}, range = [0x140001014-0x140001042)
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -363,6 +363,24 @@
 lldbassert(block.Parent != 0);
 PdbCompilandSymId parent_id(block_id.modi, block.Parent);
 Block &parent_block = GetOrCreateBlock(parent_id);
+Function *func = parent_block.CalculateSymbolContextFunction();
+lldbassert(func);
+lldb::addr_t block_base =
+m_index->MakeVirtualAddress(block.Segment, block.CodeOffset);
+lldb::addr_t func_base =
+func->GetAddressRange().GetBaseAddress().GetFileAddress();
+Block::Range range = Block::Range(block_base - func_base, block.CodeSize);
+if (block_base >= func_base)
+  child_block->AddRange(range);
+else {
+  GetObjectFile()->GetModule()->ReportError(
+  "S_BLOCK32 at modi: %d offset: %d: adding range [0x%" PRIx64
+  "-0x%" PRIx64 ") which has a base that is less than the function's "
+  "low PC 0x%" PRIx64 ". Please file a bug and attach the file at the "
+  "start of this error message",
+  block_id.modi, block_id.offset, block_base,
+  block_base + block.CodeSize, func_base);
+}
 parent_block.AddChild(child_block);
 m_ast->GetOrCreateBlockDecl(block_id);
 m_blocks.insert({opaque_block_uid, child_block});


Index: lldb/test/Shell/SymbolFile/NativePDB/blocks.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/blocks.cpp
@@ -0,0 +1,20 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test block range is set.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -c /Fo%t.obj -- %s
+// RUN: lld-link -debug:full -nodefaultlib -entry:main -base:0x14000 %t.obj -out:%t.exe -pdb:%t.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb %t.exe -o "image lookup -a 0x140001014 -v" | FileCheck %s
+
+int main() {
+  int count = 0;
+  for (int i = 0; i < 3; ++i) {
+++count;
+  }
+  return count;
+}
+
+// CHECK:  Function: id = {{.*}}, name = "main", range = [0x000140001000-0x00014000104b)
+// CHECK-NEXT: FuncType: id = {{.*}}, byte-size = 0, compiler_type = "int (void)"
+// CHECK-NEXT:   Blocks: id = {{.*}}, range = [0x140001000-0x14000104b)
+// CHECK-NEXT:   id = {{.*}}, range = [0x140001014-0x140001042)
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -363,6 +363,24 @@
 lldbassert(block.Parent != 0);
 PdbCompilandSymId parent_id(block_id.modi, block.Parent);
 Block &parent_block = GetOrCreateBlock(parent_id);
+Function *func = parent_block.CalculateSymbolContextFunction();
+lldbassert(func);
+lldb::addr_t block_base =
+m_index->MakeVirtualAddress(block.Segment, block.CodeOffset);
+lldb::addr_t func_base =
+func->GetAddressRange().GetBaseAddress().GetFileAddress();
+Block::Range range = Block::Range(block_base - func_base, block.CodeSize);
+if (block_base >= func_base)
+  child_block->AddRange(range);
+else {
+  GetObjectFile()->GetModule()->ReportError(
+  "S_BLOCK32 at modi: %d offset: %d: adding range [0x%

[Lldb-commits] [PATCH] D111509: [clang] use getCommonSugar in an assortment of places

2022-09-07 Thread Matheus Izvekov via Phabricator via lldb-commits 
mizvekov updated this revision to Diff 458608.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111509

Files:
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/AST/ast-dump-fpfeatures.cpp
  clang/test/CodeGen/compound-assign-overflow.c
  clang/test/Sema/matrix-type-operators.c
  clang/test/Sema/nullability.c
  clang/test/Sema/sugar-common-types.c
  clang/test/SemaCXX/matrix-type-operators.cpp
  clang/test/SemaCXX/sugar-common-types.cpp
  clang/test/SemaCXX/sugared-auto.cpp
  clang/test/SemaObjC/format-strings-objc.m
  compiler-rt/test/ubsan/TestCases/Integer/add-overflow.cpp
  compiler-rt/test/ubsan/TestCases/Integer/no-recover.cpp
  compiler-rt/test/ubsan/TestCases/Integer/sub-overflow.cpp
  compiler-rt/test/ubsan/TestCases/Integer/uadd-overflow.cpp
  compiler-rt/test/ubsan/TestCases/Integer/umul-overflow.cpp
  compiler-rt/test/ubsan/TestCases/Integer/usub-overflow.cpp
  libcxx/DELETE.ME
  lldb/test/API/commands/expression/rdar42038760/main.c
  lldb/test/API/commands/expression/rdar44436068/main.c

Index: lldb/test/API/commands/expression/rdar44436068/main.c
===
--- lldb/test/API/commands/expression/rdar44436068/main.c
+++ lldb/test/API/commands/expression/rdar44436068/main.c
@@ -3,6 +3,6 @@
 __int128_t n = 1;
 n = n + n;
 return n; //%self.expect("p n", substrs=['(__int128_t) $0 = 2'])
-  //%self.expect("p n + 6", substrs=['(__int128) $1 = 8'])
-  //%self.expect("p n + n", substrs=['(__int128) $2 = 4'])
+  //%self.expect("p n + 6", substrs=['(__int128_t) $1 = 8'])
+  //%self.expect("p n + n", substrs=['(__int128_t) $2 = 4'])
 }
Index: lldb/test/API/commands/expression/rdar42038760/main.c
===
--- lldb/test/API/commands/expression/rdar42038760/main.c
+++ lldb/test/API/commands/expression/rdar42038760/main.c
@@ -10,7 +10,7 @@
   struct S0 l_19;
   l_19.f2 = 419;
   uint32_t l_4037 = 4294967295UL;
-  l_19.f2 = g_463; //%self.expect("expr ((l_4037 % (-(g_463))) | l_19.f2)", substrs=['(unsigned int) $0 = 358717883'])
+  l_19.f2 = g_463; //%self.expect("expr ((l_4037 % (-(g_463))) | l_19.f2)", substrs=['(uint32_t) $0 = 358717883'])
 }
 int main()
 {
Index: libcxx/DELETE.ME
===
--- libcxx/DELETE.ME
+++ libcxx/DELETE.ME
@@ -1,2 +1,3 @@
 D133262
 D111283
+D111509
Index: compiler-rt/test/ubsan/TestCases/Integer/usub-overflow.cpp
===
--- compiler-rt/test/ubsan/TestCases/Integer/usub-overflow.cpp
+++ compiler-rt/test/ubsan/TestCases/Integer/usub-overflow.cpp
@@ -12,12 +12,12 @@
 
 #ifdef SUB_I32
   (void)(uint32_t(1) - uint32_t(2));
-  // CHECK-SUB_I32: usub-overflow.cpp:[[@LINE-1]]:22: runtime error: unsigned integer overflow: 1 - 2 cannot be represented in type 'unsigned int'
+  // CHECK-SUB_I32: usub-overflow.cpp:[[@LINE-1]]:22: runtime error: unsigned integer overflow: 1 - 2 cannot be represented in type '{{uint32_t|unsigned int}}'
 #endif
 
 #ifdef SUB_I64
   (void)(uint64_t(800ll) - uint64_t(900ll));
-  // CHECK-SUB_I64: 800 - 900 cannot be represented in type 'unsigned {{long( long)?}}'
+  // CHECK-SUB_I64: 800 - 900 cannot be represented in type '{{uint64_t|unsigned long( long)?}}'
 #endif
 
 #ifdef SUB_I128
@@ -26,6 +26,6 @@
 # else
   puts("__int128 not supported\n");
 # endif
-  // CHECK-SUB_I128: {{0x4000 - 0x8000 cannot be represented in type 'unsigned __int128'|__int128 not supported}}
+  // CHECK-SUB_I128: {{0x4000 - 0x8000 cannot be represented in type '__uint128_t'|__int128 not supported}}
 #endif
 }
Index: compiler-rt/test/ubsan/TestCases/Integer/umul-overflow.cpp
===
--- compiler-rt/test/ubsan/TestCases/Integer/umul-overflow.cpp
+++ compiler-rt/test/ubsan/TestCases/Integer/umul-overflow.cpp
@@ -13,7 +13,7 @@
   (void)(uint16_t(0x) * uint16_t(0x8001));
 
   (void)(uint32_t(0x) * uint32_t(0x2));
-  // CHECK: umul-overflow.cpp:15:31: runtime error: unsigned integer overflow: 4294967295 * 2 cannot be represented in type 'unsigned int'
+  // CHECK: umul-overflow.cpp:15:31: runtime error: unsigned integer overflow: 4294967295 * 2 cannot be represented in type '{{uint32_t|unsigned int}}'
 
   return 0;
 }
Index: compiler-rt/test/ubsan/TestCases/Integer/uadd-overflow.cpp
==

[Lldb-commits] [PATCH] D133393: [test] Use localhost in place of 127.0.0.1 to run in ipv6-only environments.

2022-09-07 Thread Jordan Rupprecht via Phabricator via lldb-commits 
rupprecht updated this revision to Diff 458609.
rupprecht added a comment.

- Add a GetLocalhostIp helper to socket host utilities


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133393

Files:
  lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
  lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
  lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
  lldb/unittests/tools/lldb-server/tests/TestClient.cpp


Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "TestClient.h"
+#include "TestingSupport/Host/SocketTestUtilities.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
@@ -77,14 +78,20 @@
   args.AppendArgument("--log-flags=0x80");
   }
 
+  auto LocalhostIpOrErr = GetLocalhostIp();
+  if (!LocalhostIpOrErr)
+return LocalhostIpOrErr.takeError();
+  const std::string &LocalhostIp = *LocalhostIpOrErr;
+
   Status status;
   TCPSocket listen_socket(true, false);
-  status = listen_socket.Listen("127.0.0.1:0", 5);
+  status = listen_socket.Listen(LocalhostIp + ":0", 5);
   if (status.Fail())
 return status.ToError();
 
   args.AppendArgument(
-  ("127.0.0.1:" + Twine(listen_socket.GetLocalPortNumber())).str());
+  formatv("{0}:{1}", LocalhostIp, listen_socket.GetLocalPortNumber())
+  .str());
 
   for (StringRef arg : ServerArgs)
 args.AppendArgument(arg);
Index: lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
===
--- lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
+++ lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
@@ -7,6 +7,7 @@
   LINK_LIBS
 lldbHost
 lldbCore
+lldbHostHelpers
 lldbInterpreter
 lldbTarget
 lldbPluginPlatformLinux
Index: lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
===
--- lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
+++ lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
@@ -42,6 +42,13 @@
 
 bool HostSupportsIPv6();
 bool HostSupportsIPv4();
+
+/// Return an IP for localhost based on host support.
+///
+/// This will return either "127.0.0.1" if IPv4 is detected, or "[::1]" if IPv6
+/// is detected. If neither are detected, return an error.
+llvm::Expected GetLocalhostIp();
+
 } // namespace lldb_private
 
 #endif
Index: lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
===
--- lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
+++ lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
@@ -126,3 +126,13 @@
 bool lldb_private::HostSupportsIPv6() {
   return CheckIPSupport("IPv6", "[::1]:0");
 }
+
+llvm::Expected lldb_private::GetLocalhostIp() {
+  if (HostSupportsIPv4())
+return "127.0.0.1";
+  if (HostSupportsIPv6())
+return "[::1]";
+  return llvm::make_error(
+  "Neither IPv4 nor IPv6 appear to be supported",
+  llvm::inconvertibleErrorCode());
+}


Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "TestClient.h"
+#include "TestingSupport/Host/SocketTestUtilities.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
@@ -77,14 +78,20 @@
   args.AppendArgument("--log-flags=0x80");
   }
 
+  auto LocalhostIpOrErr = GetLocalhostIp();
+  if (!LocalhostIpOrErr)
+return LocalhostIpOrErr.takeError();
+  const std::string &LocalhostIp = *LocalhostIpOrErr;
+
   Status status;
   TCPSocket listen_socket(true, false);
-  status = listen_socket.Listen("127.0.0.1:0", 5);
+  status = listen_socket.Listen(LocalhostIp + ":0", 5);
   if (status.Fail())
 return status.ToError();
 
   args.AppendArgument(
-  ("127.0.0.1:" + Twine(listen_socket.GetLocalPortNumber())).str());
+  formatv("{0}:{1}", LocalhostIp, listen_socket.GetLocalPortNumber())
+  .str());
 
   for (StringRef arg : ServerArgs)
 args.AppendArgument(arg);
Index: lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
===
--- lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
+++ lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
@@ -7,6 +7,7 @

[Lldb-commits] [PATCH] D133393: [test] Use localhost in place of 127.0.0.1 to run in ipv6-only environments.

2022-09-07 Thread Jordan Rupprecht via Phabricator via lldb-commits 
rupprecht added a comment.

In D133393#3773793 , @labath wrote:

> I believe the reasons are still relevant. Basically the problem is that 
> listening on `localhost:x` creates two sockets (one for 127.0.0.1, one for 
> ::1), and there's no way to guarantee that we'll be able to grab the same 
> port for both (one could be taken by some other application). Our listening 
> code will succeed if it opens at least one socket, but then if we again try 
> to connect using the `localhost` name, we may end up connecting to the wrong 
> thing. I think the correct fix is to take the address (ip+port) that we've 
> *actually* started listening on, and then pass *that* as the argument to the 
> connect command, but I'm not sure if our current Socket APIs allow you to get 
> that information.

There's `listen_socket.GetLocalIPAddress()`, but that returns an empty string 
here.

Anyway, I just changed to use the `HostSupportsIPv4()/HostSupportsIPv6()` 
helper methods and pick an appropriate scheme. PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133393

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

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

2022-09-07 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:70
 
+static void consumeNamespace(llvm::StringRef &name) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::

rupprecht wrote:
> nit: this consumes just the inline namespace, so I think 
> `consumeInlineNamespace` might be a better name. I don't feel that strongly 
> though so I'll leave it up to you.
Rather than modifying the passed-in string, would it make sense to return a 
`llvm::StringRef`? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

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

[Lldb-commits] [PATCH] D133393: [test] Use either `127.0.0.1` or `[::1]` to run in ipv6-only environments.

2022-09-07 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added inline comments.



Comment at: lldb/unittests/TestingSupport/Host/SocketTestUtilities.h:50
+/// is detected. If neither are detected, return an error.
+llvm::Expected GetLocalhostIp();
+

nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133393

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

[Lldb-commits] [PATCH] D133393: [test] Use either `127.0.0.1` or `[::1]` to run in ipv6-only environments.

2022-09-07 Thread Jordan Rupprecht via Phabricator via lldb-commits 
rupprecht updated this revision to Diff 458611.
rupprecht added a comment.

- Ip -> IP casing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133393

Files:
  lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
  lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
  lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
  lldb/unittests/tools/lldb-server/tests/TestClient.cpp


Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "TestClient.h"
+#include "TestingSupport/Host/SocketTestUtilities.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
@@ -77,14 +78,20 @@
   args.AppendArgument("--log-flags=0x80");
   }
 
+  auto LocalhostIPOrErr = GetLocalhostIP();
+  if (!LocalhostIPOrErr)
+return LocalhostIPOrErr.takeError();
+  const std::string &LocalhostIP = *LocalhostIPOrErr;
+
   Status status;
   TCPSocket listen_socket(true, false);
-  status = listen_socket.Listen("127.0.0.1:0", 5);
+  status = listen_socket.Listen(LocalhostIP + ":0", 5);
   if (status.Fail())
 return status.ToError();
 
   args.AppendArgument(
-  ("127.0.0.1:" + Twine(listen_socket.GetLocalPortNumber())).str());
+  formatv("{0}:{1}", LocalhostIP, listen_socket.GetLocalPortNumber())
+  .str());
 
   for (StringRef arg : ServerArgs)
 args.AppendArgument(arg);
Index: lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
===
--- lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
+++ lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
@@ -7,6 +7,7 @@
   LINK_LIBS
 lldbHost
 lldbCore
+lldbHostHelpers
 lldbInterpreter
 lldbTarget
 lldbPluginPlatformLinux
Index: lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
===
--- lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
+++ lldb/unittests/TestingSupport/Host/SocketTestUtilities.h
@@ -42,6 +42,13 @@
 
 bool HostSupportsIPv6();
 bool HostSupportsIPv4();
+
+/// Return an IP for localhost based on host support.
+///
+/// This will return either "127.0.0.1" if IPv4 is detected, or "[::1]" if IPv6
+/// is detected. If neither are detected, return an error.
+llvm::Expected GetLocalhostIP();
+
 } // namespace lldb_private
 
 #endif
Index: lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
===
--- lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
+++ lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
@@ -126,3 +126,13 @@
 bool lldb_private::HostSupportsIPv6() {
   return CheckIPSupport("IPv6", "[::1]:0");
 }
+
+llvm::Expected lldb_private::GetLocalhostIP() {
+  if (HostSupportsIPv4())
+return "127.0.0.1";
+  if (HostSupportsIPv6())
+return "[::1]";
+  return llvm::make_error(
+  "Neither IPv4 nor IPv6 appear to be supported",
+  llvm::inconvertibleErrorCode());
+}


Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "TestClient.h"
+#include "TestingSupport/Host/SocketTestUtilities.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
@@ -77,14 +78,20 @@
   args.AppendArgument("--log-flags=0x80");
   }
 
+  auto LocalhostIPOrErr = GetLocalhostIP();
+  if (!LocalhostIPOrErr)
+return LocalhostIPOrErr.takeError();
+  const std::string &LocalhostIP = *LocalhostIPOrErr;
+
   Status status;
   TCPSocket listen_socket(true, false);
-  status = listen_socket.Listen("127.0.0.1:0", 5);
+  status = listen_socket.Listen(LocalhostIP + ":0", 5);
   if (status.Fail())
 return status.ToError();
 
   args.AppendArgument(
-  ("127.0.0.1:" + Twine(listen_socket.GetLocalPortNumber())).str());
+  formatv("{0}:{1}", LocalhostIP, listen_socket.GetLocalPortNumber())
+  .str());
 
   for (StringRef arg : ServerArgs)
 args.AppendArgument(arg);
Index: lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
===
--- lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
+++ lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
@@ -7,6 +7,7 @@
   LINK_LIBS
 lldbHost
 lld

[Lldb-commits] [PATCH] D132940: [lldb] Use just-built libcxx for tests when available

2022-09-07 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM




Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:396
+   ifneq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP)),)
+   $(error Cannot use system's library and a custom library 
together)
+   endif

s/library/standard library/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132940

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