[Lldb-commits] [PATCH] D133181: [test] Ensure MainLoop has time to start listening for signals.

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

> AFAICT kill is entirely asynchronous

This is not exactly true. POSIX describes this situation quite well (emphasis 
mine):

> If the value of pid causes sig to be generated for the sending process, and 
> if sig is not blocked for the calling thread and if no other thread has sig 
> unblocked or is waiting in a sigwait() function for sig, **either sig or at 
> least one pending unblocked signal shall be delivered to the sending thread 
> before kill() returns**.

Our problem is that this sentence does not apply here, not for one, but two 
reasons:

- "sig is not blocked for the calling thread" -- the calling thread in fact has 
the signal blocked. That is expected, as it will be unblocked (and delivered) 
in the `ppoll` call inside `MainLoop::Run`. That is a pretty good way to catch 
signals race-free, but it also relies on the another part of the sentence above.
- "no other thread has sig unblocked" -- it only works if there are no other 
threads willing to accept that signal, and I believe that is what is failing us 
here. This test does in fact create an extra thread in its SetUp function on 
line 35. By the time we leave the SetUp function, that thread has finished with 
its useful work (producing the future object), but I suspect what is happening 
is that, occasionally, the OS-level thread fails to exit on time and eats our 
signal.

In this case, I believe that the simplest way to fix this is to get rid of that 
thread. I believe it was necessary at some point in the past (when we were 
doing the Listen+Accept calls as a single action), but now it is not necessary 
as the self-connection can be completed without having two threads actively 
connecting to each other -- it's enough that one socket declares its intent to 
accept (listen to) a connection. That will make the test simpler. and I believe 
it will also fix the flakyness you observed.




Comment at: lldb/unittests/Host/MainLoopTest.cpp:35
 Socket *accept_socket;
 std::future accept_error = std::async(std::launch::async, [&] {
   return listen_socket_up->Accept(accept_socket);

Thread created here.



Comment at: lldb/unittests/Host/MainLoopTest.cpp:45
 ASSERT_TRUE(error.Success());
 ASSERT_TRUE(accept_error.get().Success());
 

Delete the async call above, and put something like 
`ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success())` here.


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] D133181: [test] Ensure MainLoop has time to start listening for signals.

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

- Remove async call to avoid deadlock instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133181

Files:
  lldb/unittests/Host/MainLoopTest.cpp


Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -32,17 +32,13 @@
 ASSERT_TRUE(error.Success());
 
 Socket *accept_socket;
-std::future accept_error = std::async(std::launch::async, [&] {
-  return listen_socket_up->Accept(accept_socket);
-});
-
 std::unique_ptr connect_socket_up(
 new TCPSocket(true, child_processes_inherit));
 error = connect_socket_up->Connect(
 llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
 .str());
 ASSERT_TRUE(error.Success());
-ASSERT_TRUE(accept_error.get().Success());
+ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
 
 callback_count = 0;
 socketpair[0] = std::move(connect_socket_up);


Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -32,17 +32,13 @@
 ASSERT_TRUE(error.Success());
 
 Socket *accept_socket;
-std::future accept_error = std::async(std::launch::async, [&] {
-  return listen_socket_up->Accept(accept_socket);
-});
-
 std::unique_ptr connect_socket_up(
 new TCPSocket(true, child_processes_inherit));
 error = connect_socket_up->Connect(
 llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
 .str());
 ASSERT_TRUE(error.Success());
-ASSERT_TRUE(accept_error.get().Success());
+ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
 
 callback_count = 0;
 socketpair[0] = std::move(connect_socket_up);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133181: [test] Ensure MainLoop has time to start listening for signals.

2022-09-02 Thread Jordan Rupprecht via Phabricator via lldb-commits 
rupprecht marked 2 inline comments as done.
rupprecht added a comment.

In D133181#3766319 , @labath wrote:

>> AFAICT kill is entirely asynchronous
>
> This is not exactly true. POSIX describes this situation quite well (emphasis 
> mine):
>
>> If the value of pid causes sig to be generated for the sending process, and 
>> if sig is not blocked for the calling thread and if no other thread has sig 
>> unblocked or is waiting in a sigwait() function for sig, **either sig or at 
>> least one pending unblocked signal shall be delivered to the sending thread 
>> before kill() returns**.
>
> Our problem is that this sentence does not apply here, not for one, but two 
> reasons:
>
> - "sig is not blocked for the calling thread" -- the calling thread in fact 
> has the signal blocked. That is expected, as it will be unblocked (and 
> delivered) in the `ppoll` call inside `MainLoop::Run`. That is a pretty good 
> way to catch signals race-free, but it also relies on the another part of the 
> sentence above.
> - "no other thread has sig unblocked" -- it only works if there are no other 
> threads willing to accept that signal, and I believe that is what is failing 
> us here. This test does in fact create an extra thread in its SetUp function 
> on line 35. By the time we leave the SetUp function, that thread has finished 
> with its useful work (producing the future object), but I suspect what is 
> happening is that, occasionally, the OS-level thread fails to exit on time 
> and eats our signal.
>
> In this case, I believe that the simplest way to fix this is to get rid of 
> that thread. I believe it was necessary at some point in the past (when we 
> were doing the Listen+Accept calls as a single action), but now it is not 
> necessary as the self-connection can be completed without having two threads 
> actively connecting to each other -- it's enough that one socket declares its 
> intent to accept (listen to) a connection. That will make the test simpler. 
> and I believe it will also fix the flakyness you observed.

Yes, this works too. Updated the diff with that suggestion.

It definitely is simpler in terms of the delta from this diff, although I do 
worry it kicks the can down the road -- AFAIK it's generally a hard problem 
within a block of code to verify a thread hasn't been started somewhere else, 
especially in this case where it was done via a `std::future/std::async` with 
no hint that the thread wasn't cleaned up yet. So if we ever have another test 
in /Host/ that gets linked into the same test binary, and that test runs first 
and starts a thread, this test could start being flaky again. Is there anything 
we can do to make sure that kind of scenario doesn't happen?


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] D133181: [test] Ensure MainLoop has time to start listening for signals.

2022-09-02 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.

It might be a good idea to also change the `kill(getpid(), sig);` statements 
into `raise(sig)` (a.k.a. `pthread_kill(pthread_self(), sig)`), so that they're 
sent to a specific thread, instead of the whole process.

It would also be possible to implement the MainLoop class in such a way that it 
responds to signals received by other threads as well, although one can ask 
himself which behavior is more natural. For the use case we're currently using 
this (catching SIGCHLDs) it wouldn't make a difference though.


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] D132954: lldb: Add support for R_386_32 relocations to ObjectFileELF

2022-09-02 Thread David M. Lary via Phabricator via lldb-commits 
dmlary added a comment.

@labath it doesn't look like the relocations are applied during `lldb-test 
object-file --contents`:

from obj2yaml for rel.c described in summary (with added spacing for 
readabilty):

  [...]
- Name:.debug_info
  Type:SHT_PROGBITS
  AddressAlign:0x1
  Content:
7400 0400 0401 0900 0189 0062  00023400
2D00 032D 001F 00040407  04010684 0005 7C00
01024C00 0503  061D 00023400 6100 032D 0004
00077661 72000103 7200 0503 0651 
  [...]
- Name:.rel.debug_info
  Type:SHT_REL
  Flags:   [ SHF_INFO_LINK ]
  Link:.symtab
  AddressAlign:0x4
  Info:.debug_info
  Relocations:
- Offset:  0x6
  Symbol:  .debug_abbrev
  Type:R_386_32
- Offset:  0xC
  Symbol:  .debug_str
  Type:R_386_32
- Offset:  0x11
  [...]

output for `.debug_info` from `lldb-test object-file --contents rel.o`:

  txt
Index: 3
ID: 0x4
Name: .debug_info
Type: dwarf-info
Permissions: ---
Thread specific: no
VM address: 0x0
VM size: 0
File size: 120
Data:  (
: 7400 0400 0401 0900 0189 0062  
00023400  |tb4.|
0020: 2D00 032D 001F 00040407  04010684 0005 
7C00  |..--|...|
0040: 01024C00 0503  061D 00023400 6100 032D 
0004  |..L...4...a-|
0060: 00077661 72000103 7200 0503 0651  
   |..var...r..Q|
)

I would expect at least one byte in the output to be changed if relocations 
were being applied to `.debug_info`.  To ensure it's not just relocating using 
a base address of 0, I modified one relocation to use `var


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132954

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

[Lldb-commits] [lldb] a2ec18e - [lldb] From unordered_map synthetic provider, return std::pair children

2022-09-02 Thread Dave Lee via lldb-commits 

Author: Dave Lee
Date: 2022-09-02T08:53:46-07:00
New Revision: a2ec18ee043f7c8caa0e9e329721182b9f1a5dcd

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

LOG: [lldb] From unordered_map synthetic provider, return std::pair children

Change the behavior of the libc++ `unordered_map` synthetic provider to present
children as `std::pair` values, just like `std::map` does.

The synthetic provider for libc++ `std::unordered_map` has returned children
that expose a level of internal structure (over top of the key/value pair). For
example, given an unordered map initialized with `{{1,2}, {3, 4}}`, the output
is:

```
(std::unordered_map, std::equal_to, 
std::allocator > >) map = size=2 {
  [0] = {
__cc = (first = 3, second = 4)
  }
  [1] = {
__cc = (first = 1, second = 2)
  }
}
```

It's not ideal/necessary to have the numbered children embdedded in the `__cc`
field.

Note: the numbered children have type
`std::__hash_node, void *>::__node_value_type`,
and the `__cc` fields have type `std::__hash_value_type::value_type`.

Compare this output to `std::map`:

```
(std::map, std::allocator > 
>) map = size=2 {
  [0] = (first = 1, second = 2)
  [1] = (first = 3, second = 4)
```

Where the numbered children have type `std::pair`.

This changes the behavior of the synthetic provider for `unordered_map` to also
present children as `pairs`, just like `std::map`.

It appears the synthetic provider implementation for `unordered_map` was meant
to provide this behavior, but was maybe incomplete (see
d22a94377f7554a7e9df050f6dfc3ee42384e3fe). It has both an `m_node_type` and an
`m_element_type`, but uses only the former. The latter is exactly the type
needed for the children pairs. With this existing code, it's not much of a
change to make this work.

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

Added: 


Modified: 
lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py

Removed: 




diff  --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp 
b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
index 85c04ea728f56..90e6e124090e9 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -13,10 +13,12 @@
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
+#include "llvm/ADT/StringRef.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -65,6 +67,19 @@ size_t 
lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
   return m_num_elements;
 }
 
+static bool isStdTemplate(ConstString type_name, llvm::StringRef type) {
+  llvm::StringRef name = type_name.GetStringRef();
+  // The type name may or may not be prefixed with `std::` or `std::__1::`.
+  if (name.consume_front("std::"))
+name.consume_front("__1::");
+  return name.consume_front(type) && name.startswith("<");
+}
+
+static bool isUnorderedMap(ConstString type_name) {
+  return isStdTemplate(type_name, "unordered_map") ||
+ isStdTemplate(type_name, "unordered_multimap");
+}
+
 lldb::ValueObjectSP lldb_private::formatters::
 LibcxxStdUnorderedMapSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx >= CalculateNumChildren())
@@ -118,10 +133,20 @@ lldb::ValueObjectSP lldb_private::formatters::
 m_element_type = m_element_type.GetPointeeType();
 m_node_type = m_element_type;
 m_element_type = m_element_type.GetTypeTemplateArgument(0);
-std::string name;
-m_element_type =
-m_element_type.GetFieldAtIndex(0, name, nullptr, nullptr, nullptr);
-m_element_type = m_element_type.GetTypedefedType();
+// This synthetic provider is used for both unordered_(multi)map and
+// unordered_(multi)set. For unordered_map, the element type has an
+// additional type layer, an internal struct (`__hash_value_type`)
+// that wraps a std::pair. Peel away the internal wrapper type - whose
+// structure is of no value to users, to expose the std::pair. This
+// matches the structure returned by the std::map synthetic provider.
+if (isUnorderedMap(m_backend.GetTypeName())) {
+  std::string name;
+  CompilerType field_type = m_element_type.GetFieldAtIndex(
+  0, name, nullptr, nullptr, nullptr);
+  CompilerType actual_type = field_type.GetTypedefedType();
+

[Lldb-commits] [PATCH] D117383: [lldb] Expose std::pair children for unordered_map

2022-09-02 Thread Dave Lee via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa2ec18ee043f: [lldb] From unordered_map synthetic provider, 
return std::pair children (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117383

Files:
  lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py


Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py
@@ -47,12 +47,17 @@
 "corrupt_map", ['%s::unordered_map' %
 ns, 'size=0 {}'])
 
+must_not_contain__cc = r'(?s)^(?!.*\b__cc = )'
+
 self.look_for_content_and_continue(
-"map", ['%s::unordered_map' %
-ns, 'size=5 {', 'hello', 'world', 'this', 'is', 'me'])
+"map", ['%s::unordered_map' % ns,
+must_not_contain__cc,
+'size=5 {', 'hello', 'world', 'this', 'is', 'me'])
 
 self.look_for_content_and_continue(
-"mmap", ['%s::unordered_multimap' % ns, 'size=6 {', 'first = 3', 
'second = "this"',
+"mmap", ['%s::unordered_multimap' % ns,
+ must_not_contain__cc,
+ 'size=6 {', 'first = 3', 'second = "this"',
  'first = 2', 'second = "hello"'])
 
 self.look_for_content_and_continue(
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -13,10 +13,12 @@
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
+#include "llvm/ADT/StringRef.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -65,6 +67,19 @@
   return m_num_elements;
 }
 
+static bool isStdTemplate(ConstString type_name, llvm::StringRef type) {
+  llvm::StringRef name = type_name.GetStringRef();
+  // The type name may or may not be prefixed with `std::` or `std::__1::`.
+  if (name.consume_front("std::"))
+name.consume_front("__1::");
+  return name.consume_front(type) && name.startswith("<");
+}
+
+static bool isUnorderedMap(ConstString type_name) {
+  return isStdTemplate(type_name, "unordered_map") ||
+ isStdTemplate(type_name, "unordered_multimap");
+}
+
 lldb::ValueObjectSP lldb_private::formatters::
 LibcxxStdUnorderedMapSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx >= CalculateNumChildren())
@@ -118,10 +133,20 @@
 m_element_type = m_element_type.GetPointeeType();
 m_node_type = m_element_type;
 m_element_type = m_element_type.GetTypeTemplateArgument(0);
-std::string name;
-m_element_type =
-m_element_type.GetFieldAtIndex(0, name, nullptr, nullptr, nullptr);
-m_element_type = m_element_type.GetTypedefedType();
+// This synthetic provider is used for both unordered_(multi)map and
+// unordered_(multi)set. For unordered_map, the element type has an
+// additional type layer, an internal struct (`__hash_value_type`)
+// that wraps a std::pair. Peel away the internal wrapper type - whose
+// structure is of no value to users, to expose the std::pair. This
+// matches the structure returned by the std::map synthetic provider.
+if (isUnorderedMap(m_backend.GetTypeName())) {
+  std::string name;
+  CompilerType field_type = m_element_type.GetFieldAtIndex(
+  0, name, nullptr, nullptr, nullptr);
+  CompilerType actual_type = field_type.GetTypedefedType();
+  if (isStdTemplate(actual_type.GetTypeName(), "pair"))
+m_element_type = actual_type;
+}
   }
   if (!m_node_type)
 return nullptr;
@@ -153,7 +178,7 @@
   ExecutionContext exe_ctx = val_hash.first->GetExecutionContextRef().Lock(
   thread_and_frame_only_if_stopped);
   return CreateValueObjectFromData(stream.GetString(), data, exe_ctx,
-   val_hash.first->GetCompilerType());
+   m_element_type);
 }
 
 bool lldb_private::formatters::LibcxxStdUnordere

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

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

- Use pthread_kill to only kill the current thread


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133181

Files:
  lldb/unittests/Host/MainLoopTest.cpp


Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -32,17 +32,13 @@
 ASSERT_TRUE(error.Success());
 
 Socket *accept_socket;
-std::future accept_error = std::async(std::launch::async, [&] {
-  return listen_socket_up->Accept(accept_socket);
-});
-
 std::unique_ptr connect_socket_up(
 new TCPSocket(true, child_processes_inherit));
 error = connect_socket_up->Connect(
 llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
 .str());
 ASSERT_TRUE(error.Success());
-ASSERT_TRUE(accept_error.get().Success());
+ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
 
 callback_count = 0;
 socketpair[0] = std::move(connect_socket_up);
@@ -174,7 +170,7 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(1u, callback_count);
 }
@@ -192,14 +188,9 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  std::thread killer([]() {
-sleep(1);
-kill(getpid(), SIGUSR2);
-sleep(1);
-kill(getpid(), SIGUSR1);
-  });
+  pthread_kill(pthread_self(), SIGUSR2);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
-  killer.join();
   ASSERT_EQ(1u, callback_count);
 }
 
@@ -220,7 +211,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback2_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(1u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -233,7 +224,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback3_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(2u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -241,7 +232,7 @@
   }
 
   // Both extra callbacks should be unregistered now.
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(3u, callback_count);
   ASSERT_EQ(1u, callback2_count);


Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -32,17 +32,13 @@
 ASSERT_TRUE(error.Success());
 
 Socket *accept_socket;
-std::future accept_error = std::async(std::launch::async, [&] {
-  return listen_socket_up->Accept(accept_socket);
-});
-
 std::unique_ptr connect_socket_up(
 new TCPSocket(true, child_processes_inherit));
 error = connect_socket_up->Connect(
 llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
 .str());
 ASSERT_TRUE(error.Success());
-ASSERT_TRUE(accept_error.get().Success());
+ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
 
 callback_count = 0;
 socketpair[0] = std::move(connect_socket_up);
@@ -174,7 +170,7 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(1u, callback_count);
 }
@@ -192,14 +188,9 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  std::thread killer([]() {
-sleep(1);
-kill(getpid(), SIGUSR2);
-sleep(1);
-kill(getpid(), SIGUSR1);
-  });
+  pthread_kill(pthread_self(), SIGUSR2);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
-  killer.join();
   ASSERT_EQ(1u, callback_count);
 }
 
@@ -220,7 +211,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback2_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(1u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -233,7 +224,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback3_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(2u, callback_count);
 ASSERT_EQ

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

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

In D133181#3767072 , @labath wrote:

> It might be a good idea to also change the `kill(getpid(), sig);` statements 
> into `raise(sig)` (a.k.a. `pthread_kill(pthread_self(), sig)`), so that 
> they're sent to a specific thread, instead of the whole process.

Nice, that also works. It fixes the problem on its own, but I'll leave both 
changes in (removing the async too).


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] [lldb] 945bdb1 - [test] Remove problematic thread from MainLoopTest to fix flakiness

2022-09-02 Thread Jordan Rupprecht via lldb-commits 

Author: Jordan Rupprecht
Date: 2022-09-02T10:30:56-07:00
New Revision: 945bdb167ff5d73559780acc2391dabb816ce9e1

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

LOG: [test] Remove problematic thread from MainLoopTest to fix flakiness

This test, specifically `TwoSignalCallbacks`, can be a little bit flaky, 
failing in around 5/2000 runs.

POSIX says:

> If the value of pid causes sig to be generated for the sending process, and 
> if sig is not blocked for the calling thread and if no other thread has sig 
> unblocked or is waiting in a sigwait() function for sig, either sig or at 
> least one pending unblocked signal shall be delivered to the sending thread 
> before kill() returns.

The problem is that in test setup, we create a new thread with `std::async` and 
that is occasionally not cleaned up. This leaves that thread available to eat 
the signal we're polling for.

The need for this to be async does not apply anymore, so we can just make it 
synchronous.

This makes the test passes in 1 runs.

Reviewed By: labath

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

Added: 


Modified: 
lldb/unittests/Host/MainLoopTest.cpp

Removed: 




diff  --git a/lldb/unittests/Host/MainLoopTest.cpp 
b/lldb/unittests/Host/MainLoopTest.cpp
index fc79c0e38a39..7c12c5223123 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -32,17 +32,13 @@ class MainLoopTest : public testing::Test {
 ASSERT_TRUE(error.Success());
 
 Socket *accept_socket;
-std::future accept_error = std::async(std::launch::async, [&] {
-  return listen_socket_up->Accept(accept_socket);
-});
-
 std::unique_ptr connect_socket_up(
 new TCPSocket(true, child_processes_inherit));
 error = connect_socket_up->Connect(
 llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
 .str());
 ASSERT_TRUE(error.Success());
-ASSERT_TRUE(accept_error.get().Success());
+ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
 
 callback_count = 0;
 socketpair[0] = std::move(connect_socket_up);
@@ -174,7 +170,7 @@ TEST_F(MainLoopTest, Signal) {
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(1u, callback_count);
 }
@@ -192,14 +188,9 @@ TEST_F(MainLoopTest, UnmonitoredSignal) {
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  std::thread killer([]() {
-sleep(1);
-kill(getpid(), SIGUSR2);
-sleep(1);
-kill(getpid(), SIGUSR1);
-  });
+  pthread_kill(pthread_self(), SIGUSR2);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
-  killer.join();
   ASSERT_EQ(1u, callback_count);
 }
 
@@ -220,7 +211,7 @@ TEST_F(MainLoopTest, TwoSignalCallbacks) {
 SIGUSR1, [&](MainLoopBase &loop) { ++callback2_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(1u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -233,7 +224,7 @@ TEST_F(MainLoopTest, TwoSignalCallbacks) {
 SIGUSR1, [&](MainLoopBase &loop) { ++callback3_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(2u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -241,7 +232,7 @@ TEST_F(MainLoopTest, TwoSignalCallbacks) {
   }
 
   // Both extra callbacks should be unregistered now.
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(3u, callback_count);
   ASSERT_EQ(1u, callback2_count);



___
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-02 Thread Jordan Rupprecht via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG945bdb167ff5: [test] Remove problematic thread from 
MainLoopTest to fix flakiness (authored by rupprecht).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133181

Files:
  lldb/unittests/Host/MainLoopTest.cpp


Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -32,17 +32,13 @@
 ASSERT_TRUE(error.Success());
 
 Socket *accept_socket;
-std::future accept_error = std::async(std::launch::async, [&] {
-  return listen_socket_up->Accept(accept_socket);
-});
-
 std::unique_ptr connect_socket_up(
 new TCPSocket(true, child_processes_inherit));
 error = connect_socket_up->Connect(
 llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
 .str());
 ASSERT_TRUE(error.Success());
-ASSERT_TRUE(accept_error.get().Success());
+ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
 
 callback_count = 0;
 socketpair[0] = std::move(connect_socket_up);
@@ -174,7 +170,7 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(1u, callback_count);
 }
@@ -192,14 +188,9 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  std::thread killer([]() {
-sleep(1);
-kill(getpid(), SIGUSR2);
-sleep(1);
-kill(getpid(), SIGUSR1);
-  });
+  pthread_kill(pthread_self(), SIGUSR2);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
-  killer.join();
   ASSERT_EQ(1u, callback_count);
 }
 
@@ -220,7 +211,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback2_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(1u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -233,7 +224,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback3_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(2u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -241,7 +232,7 @@
   }
 
   // Both extra callbacks should be unregistered now.
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(3u, callback_count);
   ASSERT_EQ(1u, callback2_count);


Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -32,17 +32,13 @@
 ASSERT_TRUE(error.Success());
 
 Socket *accept_socket;
-std::future accept_error = std::async(std::launch::async, [&] {
-  return listen_socket_up->Accept(accept_socket);
-});
-
 std::unique_ptr connect_socket_up(
 new TCPSocket(true, child_processes_inherit));
 error = connect_socket_up->Connect(
 llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
 .str());
 ASSERT_TRUE(error.Success());
-ASSERT_TRUE(accept_error.get().Success());
+ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
 
 callback_count = 0;
 socketpair[0] = std::move(connect_socket_up);
@@ -174,7 +170,7 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  kill(getpid(), SIGUSR1);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(1u, callback_count);
 }
@@ -192,14 +188,9 @@
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
-  std::thread killer([]() {
-sleep(1);
-kill(getpid(), SIGUSR2);
-sleep(1);
-kill(getpid(), SIGUSR1);
-  });
+  pthread_kill(pthread_self(), SIGUSR2);
+  pthread_kill(pthread_self(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
-  killer.join();
   ASSERT_EQ(1u, callback_count);
 }
 
@@ -220,7 +211,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback2_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(loop.Run().Success());
 ASSERT_EQ(1u, callback_count);
 ASSERT_EQ(1u, callback2_count);
@@ -233,7 +224,7 @@
 SIGUSR1, [&](MainLoopBase &loop) { ++callback3_count; }, error);
 ASSERT_TRUE(error.Success());
 
-kill(getpid(), SIGUSR1);
+pthread_kill(pthread_self(), SIGUSR1);
 ASSERT_TRUE(

[Lldb-commits] [PATCH] D133230: [NFCI] Remove duplicate code in SBTypeCategory

2022-09-02 Thread Jorge Gorbe Moya via Phabricator via lldb-commits 
jgorbe created this revision.
jgorbe added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jgorbe requested review of this revision.

TypeCategoryImpl has its own implementation of these, so it makes no
sense to have the same logic inlined in SBTypeCategory.

There are other methods in SBTypeCategory that are directly implemented
there, instead of delegating to TypeCategoryImpl (which IMO kinda
defeats the point of having an "opaque" member pointer in the SB type),
but they don't have equivalent implementations in TypeCategoryImpl, so
this patch only picks the low-hanging fruit for now.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133230

Files:
  lldb/source/API/SBTypeCategory.cpp


Index: lldb/source/API/SBTypeCategory.cpp
===
--- lldb/source/API/SBTypeCategory.cpp
+++ lldb/source/API/SBTypeCategory.cpp
@@ -185,14 +185,8 @@
   if (!spec.IsValid())
 return SBTypeFilter();
 
-  lldb::TypeFilterImplSP children_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeFiltersContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
-  else
-m_opaque_sp->GetTypeFiltersContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
+  lldb::TypeFilterImplSP children_sp =
+  m_opaque_sp->GetFilterForType(spec.GetSP());
 
   if (!children_sp)
 return lldb::SBTypeFilter();
@@ -211,14 +205,8 @@
   if (!spec.IsValid())
 return SBTypeFormat();
 
-  lldb::TypeFormatImplSP format_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeFormatsContainer()->GetExact(
-ConstString(spec.GetName()), format_sp);
-  else
-m_opaque_sp->GetTypeFormatsContainer()->GetExact(
-ConstString(spec.GetName()), format_sp);
+  lldb::TypeFormatImplSP format_sp =
+  m_opaque_sp->GetFormatForType(spec.GetSP());
 
   if (!format_sp)
 return lldb::SBTypeFormat();
@@ -235,14 +223,8 @@
   if (!spec.IsValid())
 return SBTypeSummary();
 
-  lldb::TypeSummaryImplSP summary_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeSummariesContainer()->GetExact(
-ConstString(spec.GetName()), summary_sp);
-  else
-m_opaque_sp->GetTypeSummariesContainer()->GetExact(
-ConstString(spec.GetName()), summary_sp);
+  lldb::TypeSummaryImplSP summary_sp =
+  m_opaque_sp->GetSummaryForType(spec.GetSP());
 
   if (!summary_sp)
 return lldb::SBTypeSummary();
@@ -259,14 +241,8 @@
   if (!spec.IsValid())
 return SBTypeSynthetic();
 
-  lldb::SyntheticChildrenSP children_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeSyntheticsContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
-  else
-m_opaque_sp->GetTypeSyntheticsContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
+  lldb::SyntheticChildrenSP children_sp =
+  m_opaque_sp->GetSyntheticForType(spec.GetSP());
 
   if (!children_sp)
 return lldb::SBTypeSynthetic();


Index: lldb/source/API/SBTypeCategory.cpp
===
--- lldb/source/API/SBTypeCategory.cpp
+++ lldb/source/API/SBTypeCategory.cpp
@@ -185,14 +185,8 @@
   if (!spec.IsValid())
 return SBTypeFilter();
 
-  lldb::TypeFilterImplSP children_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeFiltersContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
-  else
-m_opaque_sp->GetTypeFiltersContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
+  lldb::TypeFilterImplSP children_sp =
+  m_opaque_sp->GetFilterForType(spec.GetSP());
 
   if (!children_sp)
 return lldb::SBTypeFilter();
@@ -211,14 +205,8 @@
   if (!spec.IsValid())
 return SBTypeFormat();
 
-  lldb::TypeFormatImplSP format_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeFormatsContainer()->GetExact(
-ConstString(spec.GetName()), format_sp);
-  else
-m_opaque_sp->GetTypeFormatsContainer()->GetExact(
-ConstString(spec.GetName()), format_sp);
+  lldb::TypeFormatImplSP format_sp =
+  m_opaque_sp->GetFormatForType(spec.GetSP());
 
   if (!format_sp)
 return lldb::SBTypeFormat();
@@ -235,14 +223,8 @@
   if (!spec.IsValid())
 return SBTypeSummary();
 
-  lldb::TypeSummaryImplSP summary_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeSummariesContainer()->GetExact(
-ConstString(spec.GetName()), summary_sp);
-  else
-m_opaque_sp->GetTypeSummariesContainer()->GetExact(
-ConstString(spec.GetName()), summary_sp);
+  lldb::TypeSummaryImplSP summary_sp =
+  m_opaque_sp->GetSummaryForType(spec.GetSP());
 
   if (!summary_sp)
 return lldb::SBTypeSummary();
@@ -259,14 +241,8 @@
   if (!spec.IsValid())
 return SBTypeSynthetic();
 
-  lldb::SyntheticChildrenSP children_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeSyntheticsContainer()->GetExact(
-ConstString(spec.GetName()), childre

[Lldb-commits] [PATCH] D117383: [lldb] Expose std::pair children for unordered_map

2022-09-02 Thread Richard Smith - zygoloid via Phabricator via lldb-commits 
rsmith added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:74
+  if (name.consume_front("std::"))
+name.consume_front("__1::");
+  return name.consume_front(type) && name.startswith("<");

This is not right -- libc++ allows customizing its inline namespace name for 
versioning purpose; it's not `__1` across all deployments. For example, with 
the libc++ unstable ABI, it will likely be something else. To handle this 
properly I think you'll need to look for either `std::name<` or something like 
`std::[a-zA-Z0-9_]*::name<`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117383

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

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

2022-09-02 Thread Richard Smith - zygoloid via Phabricator via lldb-commits 
rsmith added a comment.

Hi, the newly-added test, `TestDynamicLoaderDarwin.py`, fails under asan: 
https://gist.github.com/zygoloid/bf7b21f03d7db966e456b6c365c4635d

Please can you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133069

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

[Lldb-commits] [lldb] f021cb5 - [NFC] Remove duplicate code in SBTypeCategory

2022-09-02 Thread Jorge Gorbe Moya via lldb-commits 

Author: Jorge Gorbe Moya
Date: 2022-09-02T16:04:04-07:00
New Revision: f021cb57ce786f7d5a6c921a19bbcf56709b6ce0

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

LOG: [NFC] Remove duplicate code in SBTypeCategory

TypeCategoryImpl has its own implementation of these, so it makes no
sense to have the same logic inlined in SBTypeCategory.

There are other methods in SBTypeCategory that are directly implemented
there, instead of delegating to TypeCategoryImpl (which IMO kinda
defeats the point of having an "opaque" member pointer in the SB type),
but they don't have equivalent implementations in TypeCategoryImpl, so
this patch only picks the low-hanging fruit for now.

Added: 


Modified: 
lldb/source/API/SBTypeCategory.cpp

Removed: 




diff  --git a/lldb/source/API/SBTypeCategory.cpp 
b/lldb/source/API/SBTypeCategory.cpp
index 7d929fe497954..c6b13498bb1ee 100644
--- a/lldb/source/API/SBTypeCategory.cpp
+++ b/lldb/source/API/SBTypeCategory.cpp
@@ -185,14 +185,8 @@ SBTypeFilter 
SBTypeCategory::GetFilterForType(SBTypeNameSpecifier spec) {
   if (!spec.IsValid())
 return SBTypeFilter();
 
-  lldb::TypeFilterImplSP children_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeFiltersContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
-  else
-m_opaque_sp->GetTypeFiltersContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
+  lldb::TypeFilterImplSP children_sp =
+  m_opaque_sp->GetFilterForType(spec.GetSP());
 
   if (!children_sp)
 return lldb::SBTypeFilter();
@@ -211,14 +205,8 @@ SBTypeFormat 
SBTypeCategory::GetFormatForType(SBTypeNameSpecifier spec) {
   if (!spec.IsValid())
 return SBTypeFormat();
 
-  lldb::TypeFormatImplSP format_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeFormatsContainer()->GetExact(
-ConstString(spec.GetName()), format_sp);
-  else
-m_opaque_sp->GetTypeFormatsContainer()->GetExact(
-ConstString(spec.GetName()), format_sp);
+  lldb::TypeFormatImplSP format_sp =
+  m_opaque_sp->GetFormatForType(spec.GetSP());
 
   if (!format_sp)
 return lldb::SBTypeFormat();
@@ -235,14 +223,8 @@ SBTypeSummary 
SBTypeCategory::GetSummaryForType(SBTypeNameSpecifier spec) {
   if (!spec.IsValid())
 return SBTypeSummary();
 
-  lldb::TypeSummaryImplSP summary_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeSummariesContainer()->GetExact(
-ConstString(spec.GetName()), summary_sp);
-  else
-m_opaque_sp->GetTypeSummariesContainer()->GetExact(
-ConstString(spec.GetName()), summary_sp);
+  lldb::TypeSummaryImplSP summary_sp =
+  m_opaque_sp->GetSummaryForType(spec.GetSP());
 
   if (!summary_sp)
 return lldb::SBTypeSummary();
@@ -259,14 +241,8 @@ SBTypeSynthetic 
SBTypeCategory::GetSyntheticForType(SBTypeNameSpecifier spec) {
   if (!spec.IsValid())
 return SBTypeSynthetic();
 
-  lldb::SyntheticChildrenSP children_sp;
-
-  if (spec.IsRegex())
-m_opaque_sp->GetRegexTypeSyntheticsContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
-  else
-m_opaque_sp->GetTypeSyntheticsContainer()->GetExact(
-ConstString(spec.GetName()), children_sp);
+  lldb::SyntheticChildrenSP children_sp =
+  m_opaque_sp->GetSyntheticForType(spec.GetSP());
 
   if (!children_sp)
 return lldb::SBTypeSynthetic();



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

[Lldb-commits] [PATCH] D133240: [Formatters][NFCI] Replace 'is_regex' arguments with an enum.

2022-09-02 Thread Jorge Gorbe Moya via Phabricator via lldb-commits 
jgorbe created this revision.
jgorbe added reviewers: jingham, labath.
jgorbe added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jgorbe requested review of this revision.

Modify `SBTypeNameSpecifier` and `lldb_private::TypeMatcher` so they
have an enum value for the type of matching to perform instead of an
`m_is_regex` boolean value.

This change paves the way for introducing formatter matching based on
the result of a python callback in addition to the existing name-based
matching. See the RFC thread at
https://discourse.llvm.org/t/rfc-python-callback-for-data-formatters-type-matching/64204
for more details.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133240

Files:
  lldb/bindings/interface/SBTypeNameSpecifier.i
  lldb/include/lldb/API/SBTypeNameSpecifier.h
  lldb/include/lldb/DataFormatters/FormatClasses.h
  lldb/include/lldb/DataFormatters/FormattersContainer.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/SBTypeNameSpecifier.cpp
  lldb/source/Core/FormatEntity.cpp

Index: lldb/source/Core/FormatEntity.cpp
===
--- lldb/source/Core/FormatEntity.cpp
+++ lldb/source/Core/FormatEntity.cpp
@@ -825,7 +825,7 @@
 bitfield_name.Printf("%s:%d", target->GetTypeName().AsCString(),
  target->GetBitfieldBitSize());
 auto type_sp = std::make_shared(
-bitfield_name.GetString(), false);
+bitfield_name.GetString(), lldb::eFormatterMatchExact);
 if (val_obj_display ==
 ValueObject::eValueObjectRepresentationStyleSummary &&
 !DataVisualization::GetSummaryForType(type_sp))
Index: lldb/source/API/SBTypeNameSpecifier.cpp
===
--- lldb/source/API/SBTypeNameSpecifier.cpp
+++ lldb/source/API/SBTypeNameSpecifier.cpp
@@ -20,8 +20,15 @@
 SBTypeNameSpecifier::SBTypeNameSpecifier() { LLDB_INSTRUMENT_VA(this); }
 
 SBTypeNameSpecifier::SBTypeNameSpecifier(const char *name, bool is_regex)
-: m_opaque_sp(new TypeNameSpecifierImpl(name, is_regex)) {
+: SBTypeNameSpecifier(name, is_regex ? eFormatterMatchRegex
+ : eFormatterMatchExact) {
   LLDB_INSTRUMENT_VA(this, name, is_regex);
+}
+
+SBTypeNameSpecifier::SBTypeNameSpecifier(const char *name,
+ FormatterMatchType match_type)
+: m_opaque_sp(new TypeNameSpecifierImpl(name, match_type)) {
+  LLDB_INSTRUMENT_VA(this, name, match_type);
 
   if (name == nullptr || (*name) == 0)
 m_opaque_sp.reset();
@@ -72,13 +79,20 @@
   return SBType();
 }
 
+FormatterMatchType SBTypeNameSpecifier::GetMatchType() {
+  LLDB_INSTRUMENT_VA(this);
+  if (!IsValid())
+return eFormatterMatchExact;
+  return m_opaque_sp->GetMatchType();
+}
+
 bool SBTypeNameSpecifier::IsRegex() {
   LLDB_INSTRUMENT_VA(this);
 
   if (!IsValid())
 return false;
 
-  return m_opaque_sp->IsRegex();
+  return m_opaque_sp->GetMatchType() == eFormatterMatchRegex;
 }
 
 bool SBTypeNameSpecifier::GetDescription(
@@ -116,7 +130,7 @@
   if (!IsValid())
 return !rhs.IsValid();
 
-  if (IsRegex() != rhs.IsRegex())
+  if (GetMatchType() != rhs.GetMatchType())
 return false;
   if (GetName() == nullptr || rhs.GetName() == nullptr)
 return false;
Index: lldb/include/lldb/lldb-enumerations.h
===
--- lldb/include/lldb/lldb-enumerations.h
+++ lldb/include/lldb/lldb-enumerations.h
@@ -832,6 +832,15 @@
   eTemplateArgumentKindNullPtr,
 };
 
+/// Type of match to be performed when looking for a formatter for a data type.
+/// Used by classes like SBTypeNameSpecifier or lldb_private::TypeMatcher.
+enum FormatterMatchType {
+  eFormatterMatchExact,
+  eFormatterMatchRegex,
+
+  kNumFormatterMatchTypes,
+};
+
 /// Options that can be set for a formatter to alter its behavior. Not
 /// all of these are applicable to all formatter types.
 FLAGS_ENUM(TypeOptions){eTypeOptionNone = (0u),
Index: lldb/include/lldb/DataFormatters/FormattersContainer.h
===
--- lldb/include/lldb/DataFormatters/FormattersContainer.h
+++ lldb/include/lldb/DataFormatters/FormattersContainer.h
@@ -43,7 +43,7 @@
   ConstString m_type_name;
   /// False if m_type_name_regex should be used for matching. False if this is
   /// just matching by comparing with m_type_name string.
-  bool m_is_regex;
+  lldb::FormatterMatchType m_match_type;
 
   // if the user tries to add formatters for, say, "struct Foo" those will not
   // match any type because of the way we strip qualifiers from typenames this
@@ -71,22 +71,25 @@
   TypeMatcher() = delete;
   /// Creates a matcher that accepts any type with exactly the given type name.
   TypeMatcher(ConstString type_name)
-  : m_type_name(type_name), m_is_regex(false) {}
+  : m_type_name(type_name), m_match_type(lldb::eFormat

[Lldb-commits] [lldb] 603d5a8 - Fix out-of-bounds memory access in test

2022-09-02 Thread Adrian Prantl via lldb-commits 

Author: Adrian Prantl
Date: 2022-09-02T18:14:23-07:00
New Revision: 603d5a8d632164038a869c200818c4cc238bb85a

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

LOG: Fix out-of-bounds memory access in test

Added: 


Modified: 
lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py

Removed: 




diff  --git 
a/lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py 
b/lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py
index 72f9c47890652..1580746c67378 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py
@@ -88,7 +88,7 @@
 }
 """
 
-arm64_binary = 
"cffaedfe0c0102001000e80285002000190048005f5f504147455a45524f01001900e8005f5f54455854014000400500050002005f5f746578745f5f54455854b03f01000800b03f020400805f5f756e77696e645f696e666f005f5f54455854b83f01004800b83f0200190048005f5f4c494e4b45444954004001400040b801010001003480104038003380100038403200180070400100804018000b00510001000e002c002f7573722f6c69622f64796c64001b001800a9981092eb3632f4afd9957e769160d9320021000c050c000100030633032a00100028801800b03f0c0038001800020001781f0501002f7573722f6c69622f6c696253797374656d2e422e64796c696226001000684008002900100070401d001000a0401801"
 + '0'*16384
+arm64_binary = 
"cffaedfe0c0102001000e80285002000190048005f5f504147455a45524f01001900e8005f5f54455854014000400500050002005f5f746578745f5f54455854b03f01000800b03f020400805f5f756e77696e645f696e666f005f5f54455854b83f01004800b83f0200190048005f5f4c494e4b45444954004001400040b801010001003480104038003380100038403200180070400100804018000b00510001000e002c002f7573722f6c69622f64796c64001b001800a9981092eb3632f4afd9957e769160d9320021000c050c000100030633032a00100028801800b03f0c0038001800020001781f0501002f7573722f6c69622f6c696253797374656d2e422e64796c696226001000684008002900100070401d001000a0401801"
 
 class TestDynamicLoaderDarwin(GDBRemoteTestBase):
 
@@ -119,7 +119,15 @@ def vCont(self):
 return "vCont;"
 
 def readMemory(self, addr, length):
-return arm64_binary[addr-4369842176:length]
+vm_addr = 4369842176
+file_offset = addr - vm_addr
+if file_offset < 0:
+return None
+# arm64_binary is just a hex-encoded (hence the 2*) Mach-O
+# header, pad out the rest with NUL characters, it doesn't
+# matter for this test.
+memory = arm64_binary + '00'*(length-len(arm64_binary)<<1)
+return memory[2*file_offset:]
 
 def setBreakpoint(self, packet):
 return ""



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

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

2022-09-02 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added a comment.

In D133069#3768088 , @rsmith wrote:

> Hi, the newly-added test, `TestDynamicLoaderDarwin.py`, fails under asan: 
> https://gist.github.com/zygoloid/bf7b21f03d7db966e456b6c365c4635d

Thanks! Should be fixed in

  commit 603d5a8d632164038a869c200818c4cc238bb85a (HEAD -> main)
  Author: Adrian Prantl 
  Date:   Fri Sep 2 18:14:12 2022 -0700
  
  Fix out-of-bounds memory access in test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133069

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

[Lldb-commits] [PATCH] D133240: [Formatters][NFCI] Replace 'is_regex' arguments with an enum.

2022-09-02 Thread Jorge Gorbe Moya via Phabricator via lldb-commits 
jgorbe added a comment.

This is the first step towards something like this big diff: 
https://reviews.llvm.org/differential/diff/457740/. It's a pretty simple patch, 
but I would like some feedback about whether you think it's going in the right 
direction or I should do something else instead.

Here's some more context for that diff linked above. There is, as you could 
expect, some amount of plumbing so that at callback matching time we have 
around pointers to the script interpreter, the type of the value, etc, and we 
can call the callback. But the biggest problem I've found is that there's some 
logic that is duplicated for each one of {exact, regex} AND for each kind of 
formatter (summaries, synthetic child providers...). An example of this is 
`TypeCategoryImpl::AnyMatches`. Adding a new matching strategy ({exact, regex, 
callback}) makes the combinatorial problem even worse. So a big chunk of that 
patch is moving stuff around to try to minimize this problem.

The main refactoring is replacing the existing `FormatterContainerPair` (that 
has two members that contain exact match formatters and regex formatters, 
respectively) with a `TieredFormatterContainer` which has an array of 
containers in priority order. The idea is to sink as much of duplicated logic 
as possible around `TypeCategoryImpl` (for example, `GetFormatAtIndex`, 
`GetSummaryAtIndex`, `GetFilterAtIndex`...) into this 
`TieredFormatterContainer` to reuse the implementation.

I hope this helps understand the general idea a bit better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133240

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

[Lldb-commits] [PATCH] D133243: [LLDB][NativePDB] Fix PdbAstBuilder::GetParentDeclContextForSymbol when ICF happens.

2022-09-02 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.

`PdbAstBuilder::GetParentDeclContextForSymbol` finds the public symbol that has
same address as the given symbol, then creates parent DeclContext from demangled
name. When ICF happens, there are multiple public symbols share the same 
address.
It may creates the wrong parent DeclContext.

This fixes it by filtering all public symbols that has the same addrss with base
name.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133243

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/test/Shell/SymbolFile/NativePDB/icf.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/icf.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/icf.cpp
@@ -0,0 +1,55 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test lldb is not treating function as class method or other way around when icf applies to a
+// function and a class method.
+// 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
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | FileCheck %s
+
+struct A {
+  int f1(int x) {
+return x * 2;
+  }
+};
+struct B {
+  int f2(int x) {
+return x * 2;
+  }
+};
+namespace N1 {
+int f3(void*, int x) {
+  return  x * 2;
+}
+} // namespace N1
+
+namespace N2 {
+namespace N3 {
+  int f4(void*, int x) {
+return  x * 2;
+  }
+} // namespace N3  
+} // namespace N2
+
+int main() {
+  A a;
+  B b;
+  return a.f1(1) + b.f2(1) + N1::f3(nullptr, 1) + N2::N3::f4(nullptr, 1);
+}
+
+
+// CHECK:  namespace N1 {
+// CHECK-NEXT: int f3(void *, int x);
+// CHECK-NEXT: }
+// CHECK-NEXT: namespace N2 {
+// CHECK-NEXT: namespace N3 {
+// CHECK-NEXT: int f4(void *, int x);
+// CHECK-NEXT: }
+// CHECK-NEXT: }
+// CHECK-NEXT: int main();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT: int f1(int);
+// CHECK-NEXT: };
+// CHECK-NEXT: struct B {
+// CHECK-NEXT: int f2(int);
+// CHECK-NEXT: };
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -470,9 +470,9 @@
   return result;
 }
 
-static llvm::Optional FindPublicSym(const SegmentOffset &addr,
- SymbolStream &syms,
- PublicsStream &publics) {
+static llvm::SmallVector FindPublicSyms(const SegmentOffset &addr,
+ SymbolStream &syms,
+ PublicsStream &publics) {
   llvm::FixedStreamArray addr_map = publics.getAddressMap();
   auto iter = std::lower_bound(
   addr_map.begin(), addr_map.end(), addr,
@@ -485,15 +485,17 @@
   return true;
 return p1.Offset < y.offset;
   });
-  if (iter == addr_map.end())
-return llvm::None;
-  CVSymbol sym = syms.readRecord(*iter);
-  lldbassert(sym.kind() == S_PUB32);
-  PublicSym32 p;
-  llvm::cantFail(SymbolDeserializer::deserializeAs(sym, p));
-  if (p.Segment == addr.segment && p.Offset == addr.offset)
-return p;
-  return llvm::None;
+  llvm::SmallVector public_syms;
+  while (iter != addr_map.end()) {
+CVSymbol sym = syms.readRecord(*iter);
+lldbassert(sym.kind() == S_PUB32);
+PublicSym32 p;
+llvm::cantFail(SymbolDeserializer::deserializeAs(sym, p));
+if (p.Segment == addr.segment && p.Offset == addr.offset)
+  public_syms.push_back(p);
+++iter;
+  }
+  return public_syms;
 }
 
 clang::Decl *PdbAstBuilder::GetOrCreateSymbolForId(PdbCompilandSymId id) {
@@ -608,48 +610,56 @@
 
 clang::DeclContext *
 PdbAstBuilder::GetParentDeclContextForSymbol(const CVSymbol &sym) {
-  if (!SymbolHasAddress(sym))
-return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
+  llvm::StringRef full_name = getSymbolName(sym);
+  llvm::StringRef base_name = full_name.rsplit("::").second;
+  if (!SymbolHasAddress(sym) || base_name.empty())
+return CreateDeclInfoForUndecoratedName(full_name).first;
   SegmentOffset addr = GetSegmentAndOffset(sym);
-  llvm::Optional pub =
-  FindPublicSym(addr, m_index.symrecords(), m_index.publics());
-  if (!pub)
-return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
+  llvm::SmallVector public_syms =
+  FindPublicSyms(addr, m_index.symrecords(), m_index.publics());
+
+  for (const PublicSym32 &pub : public_syms) {
+llvm::ms_demangle::Demangler demangler;
+StringView name{pub.Name.begin(), pub.Name.size()};
+llvm::ms_d

[Lldb-commits] [PATCH] D133243: [LLDB][NativePDB] Fix PdbAstBuilder::GetParentDeclContextForSymbol when ICF happens.

2022-09-02 Thread Zequan Wu via Phabricator via lldb-commits 
zequanwu updated this revision to Diff 457752.
zequanwu added a comment.
Herald added a subscriber: JDevlieghere.

Fix comment in test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133243

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/test/Shell/SymbolFile/NativePDB/icf.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/icf.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/icf.cpp
@@ -0,0 +1,54 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test lldb finds the correct parent context decl for functions and class methods when icf happens.
+// 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
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | FileCheck %s
+
+struct A {
+  int f1(int x) {
+return x * 2;
+  }
+};
+struct B {
+  int f2(int x) {
+return x * 2;
+  }
+};
+namespace N1 {
+int f3(void*, int x) {
+  return  x * 2;
+}
+} // namespace N1
+
+namespace N2 {
+namespace N3 {
+  int f4(void*, int x) {
+return  x * 2;
+  }
+} // namespace N3  
+} // namespace N2
+
+int main() {
+  A a;
+  B b;
+  return a.f1(1) + b.f2(1) + N1::f3(nullptr, 1) + N2::N3::f4(nullptr, 1);
+}
+
+
+// CHECK:  namespace N1 {
+// CHECK-NEXT: int f3(void *, int x);
+// CHECK-NEXT: }
+// CHECK-NEXT: namespace N2 {
+// CHECK-NEXT: namespace N3 {
+// CHECK-NEXT: int f4(void *, int x);
+// CHECK-NEXT: }
+// CHECK-NEXT: }
+// CHECK-NEXT: int main();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT: int f1(int);
+// CHECK-NEXT: };
+// CHECK-NEXT: struct B {
+// CHECK-NEXT: int f2(int);
+// CHECK-NEXT: };
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -470,9 +470,9 @@
   return result;
 }
 
-static llvm::Optional FindPublicSym(const SegmentOffset &addr,
- SymbolStream &syms,
- PublicsStream &publics) {
+static llvm::SmallVector FindPublicSyms(const SegmentOffset &addr,
+ SymbolStream &syms,
+ PublicsStream &publics) {
   llvm::FixedStreamArray addr_map = publics.getAddressMap();
   auto iter = std::lower_bound(
   addr_map.begin(), addr_map.end(), addr,
@@ -485,15 +485,17 @@
   return true;
 return p1.Offset < y.offset;
   });
-  if (iter == addr_map.end())
-return llvm::None;
-  CVSymbol sym = syms.readRecord(*iter);
-  lldbassert(sym.kind() == S_PUB32);
-  PublicSym32 p;
-  llvm::cantFail(SymbolDeserializer::deserializeAs(sym, p));
-  if (p.Segment == addr.segment && p.Offset == addr.offset)
-return p;
-  return llvm::None;
+  llvm::SmallVector public_syms;
+  while (iter != addr_map.end()) {
+CVSymbol sym = syms.readRecord(*iter);
+lldbassert(sym.kind() == S_PUB32);
+PublicSym32 p;
+llvm::cantFail(SymbolDeserializer::deserializeAs(sym, p));
+if (p.Segment == addr.segment && p.Offset == addr.offset)
+  public_syms.push_back(p);
+++iter;
+  }
+  return public_syms;
 }
 
 clang::Decl *PdbAstBuilder::GetOrCreateSymbolForId(PdbCompilandSymId id) {
@@ -608,48 +610,56 @@
 
 clang::DeclContext *
 PdbAstBuilder::GetParentDeclContextForSymbol(const CVSymbol &sym) {
-  if (!SymbolHasAddress(sym))
-return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
+  llvm::StringRef full_name = getSymbolName(sym);
+  llvm::StringRef base_name = full_name.rsplit("::").second;
+  if (!SymbolHasAddress(sym) || base_name.empty())
+return CreateDeclInfoForUndecoratedName(full_name).first;
   SegmentOffset addr = GetSegmentAndOffset(sym);
-  llvm::Optional pub =
-  FindPublicSym(addr, m_index.symrecords(), m_index.publics());
-  if (!pub)
-return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
+  llvm::SmallVector public_syms =
+  FindPublicSyms(addr, m_index.symrecords(), m_index.publics());
+
+  for (const PublicSym32 &pub : public_syms) {
+llvm::ms_demangle::Demangler demangler;
+StringView name{pub.Name.begin(), pub.Name.size()};
+llvm::ms_demangle::SymbolNode *node = demangler.parse(name);
+if (!node)
+  continue;
+llvm::ArrayRef name_components{
+node->Name->Components->Nodes, node->Name->Components->Count};
+// Because ICF, we need to filter out the public symbols by base name.
+if (name_components.empty() ||
+base_name != name_components.back()->toString())
+  continue;
 
-  llvm::ms_demangle::Demangler