[Lldb-commits] [PATCH] D62213: [ABI] Implement Windows ABI for x86_64

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:231
+  case llvm::Triple::OSType::Solaris:
+  case llvm::Triple::OSType::UnknownOS:
+return ABISP(new ABISysV_x86_64(process_sp));

There's NetBSD missing here. This broke our buildbots. I'll fix it but please 
be more careful in the future.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62213



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

[Lldb-commits] [lldb] r364503 - [lldb] [Plugins/SysV-x86_64] NetBSD is also using SysV ABI

[Michal Gorny via lldb-commits]

Author: mgorny
Date: Thu Jun 27 00:09:51 2019
New Revision: 364503

URL: http://llvm.org/viewvc/llvm-project?rev=364503&view=rev
Log:
[lldb] [Plugins/SysV-x86_64] NetBSD is also using SysV ABI

Reenable SysV x86_64 ABI usage on NetBSD that was accidentally removed
in r364216.  This fixes numerous test failures with messages similar
to the following:

  error: Can't run the expression locally: Interpreter doesn't handle
  one of the expression's opcodes

Modified:
lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

Modified: lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp?rev=364503&r1=364502&r2=364503&view=diff
==
--- lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp (original)
+++ lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp Thu Jun 27 
00:09:51 2019
@@ -227,6 +227,7 @@ ABISysV_x86_64::CreateInstance(lldb::Pro
   case llvm::Triple::OSType::MacOSX:
   case llvm::Triple::OSType::Linux:
   case llvm::Triple::OSType::FreeBSD:
+  case llvm::Triple::OSType::NetBSD:
   case llvm::Triple::OSType::Solaris:
   case llvm::Triple::OSType::UnknownOS:
 return ABISP(new ABISysV_x86_64(process_sp));


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

[Lldb-commits] [PATCH] D63868: Unify+fix remote XML libraries handling with the legacy one

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked an inline comment as done.
jankratochvil added inline comments.



Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:185
 
-  // If we can't get the SO info from the remote, return failure.
-  if (fromRemote && m_process->LoadModules(module_list) == 0)
+  if (m_current.map_addr == 0)
 return false;

This change is not really required but I do not see why the `map_addr` check 
should differ when the remote XML protocol is in use.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63868



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

[Lldb-commits] [PATCH] D63868: Unify+fix remote XML libraries handling with the legacy one

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: aadsm, labath, xiaobai, clayborg.
jankratochvil added a project: LLDB.
Herald added a subscriber: abidh.
jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
jankratochvil retitled this revision from "Unify+fix remote XML libraries 
handling with legacy one" to "Unify+fix remote XML libraries handling with the 
legacy one".



Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:185
 
-  // If we can't get the SO info from the remote, return failure.
-  if (fromRemote && m_process->LoadModules(module_list) == 0)
+  if (m_current.map_addr == 0)
 return false;

This change is not really required but I do not see why the `map_addr` check 
should differ when the remote XML protocol is in use.


D62503  broke Fedora 30 x86_64 
. That is because it fixed failing 
`ReadMemory` there and thus enabling to really use the XML libraries protocol 
which broke LLDB because then `DynamicLoaderPOSIXDYLD::RendezvousBreakpointHit` 
happens only once and any further library updates are ignored.
So D62503  should be reapplied after this fix 
as it is safe then.
In this patch I do not understand why there should be any special add/remove 
handling in `ProcessGDBRemote::LoadModules`, it is just a new retrieved list of 
libraries and loading their modules is handled by later code the same way as it 
always worked with legacy memory reading from the list at `_r_debug`.
There are some more opportunities how to unify the XML protocol vs. legacy 
libraries handling but I wanted to first fix this pending regression.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D63868

Files:
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

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
@@ -4853,77 +4853,7 @@
   if (GetLoadedModuleList(module_list).Fail())
 return 0;
 
-  // get a list of all the modules
-  ModuleList new_modules;
-
-  for (LoadedModuleInfoList::LoadedModuleInfo &modInfo : module_list.m_list) {
-std::string mod_name;
-lldb::addr_t mod_base;
-lldb::addr_t link_map;
-bool mod_base_is_offset;
-
-bool valid = true;
-valid &= modInfo.get_name(mod_name);
-valid &= modInfo.get_base(mod_base);
-valid &= modInfo.get_base_is_offset(mod_base_is_offset);
-if (!valid)
-  continue;
-
-if (!modInfo.get_link_map(link_map))
-  link_map = LLDB_INVALID_ADDRESS;
-
-FileSpec file(mod_name);
-FileSystem::Instance().Resolve(file);
-lldb::ModuleSP module_sp =
-LoadModuleAtAddress(file, link_map, mod_base, mod_base_is_offset);
-
-if (module_sp.get())
-  new_modules.Append(module_sp);
-  }
-
-  if (new_modules.GetSize() > 0) {
-ModuleList removed_modules;
-Target &target = GetTarget();
-ModuleList &loaded_modules = m_process->GetTarget().GetImages();
-
-for (size_t i = 0; i < loaded_modules.GetSize(); ++i) {
-  const lldb::ModuleSP loaded_module = loaded_modules.GetModuleAtIndex(i);
-
-  bool found = false;
-  for (size_t j = 0; j < new_modules.GetSize(); ++j) {
-if (new_modules.GetModuleAtIndex(j).get() == loaded_module.get())
-  found = true;
-  }
-
-  // The main executable will never be included in libraries-svr4, don't
-  // remove it
-  if (!found &&
-  loaded_module.get() != target.GetExecutableModulePointer()) {
-removed_modules.Append(loaded_module);
-  }
-}
-
-loaded_modules.Remove(removed_modules);
-m_process->GetTarget().ModulesDidUnload(removed_modules, false);
-
-new_modules.ForEach([&target](const lldb::ModuleSP module_sp) -> bool {
-  lldb_private::ObjectFile *obj = module_sp->GetObjectFile();
-  if (!obj)
-return true;
-
-  if (obj->GetType() != ObjectFile::Type::eTypeExecutable)
-return true;
-
-  lldb::ModuleSP module_copy_sp = module_sp;
-  target.SetExecutableModule(module_copy_sp, eLoadDependentsNo);
-  return false;
-});
-
-loaded_modules.AppendIfNeeded(new_modules);
-m_process->GetTarget().ModulesDidLoad(new_modules);
-  }
-
-  return new_modules.GetSize();
+  return module_list.m_list.size();
 }
 
 size_t ProcessGDBRemote::LoadModules() {
Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
===
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
@@ -182,11 +182,11 @@
   SOEntry entry;
   Lo

[Lldb-commits] [PATCH] D63868: Unify+fix remote XML libraries handling with the legacy one

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

I'm not sure why `DynamicLoaderPOSIXDYLD::RendezvousBreakpointHit` happens only 
once, I thought I had fixed that here: D62168 .
I don't think we can stop loading/unloading the libraries in 
`ProcessGDBRemote::LoadModules`. The windows dynamic loader relies on this and 
the `library` entry on the stop packet does as well: 
https://github.com/llvm-mirror/lldb/blob/9689521b477c75b52257fba9655cabefc3db676c/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp#L2374-L2375

I actually proposed a different way of loading the libraries here (and it's 
faster because it reduces the generation of `libraries-svr4` in half): D62504 
. What do you think?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63868



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

[Lldb-commits] [PATCH] D62504: Avoid calling LoadModules twice when modules change

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

So it does fix the massive regression of D62503 
 similarly as my D63868 
 but it still has 2 regressions there (=Fedora 
30 x86_64, after reapplication of D62503 ), I 
haven't yet investigated why:

  lldb-Suite :: functionalities/load_unload/TestLoadUnload.py
  lldb-Suite :: functionalities/load_using_paths/TestLoadUsingPaths.py


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62504



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

[Lldb-commits] [PATCH] D62504: Avoid calling LoadModules twice when modules change

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

@jankratochvil I imagine the first test to be failing because this patch 
doesn't handle correctly the unload (or rather as soon as it unloads one shared 
object it stops working as expected).
I'm going to look at this today (right now it's 9am for me).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62504



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

[Lldb-commits] [PATCH] D63868: Unify+fix remote XML libraries handling with the legacy one

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D63868#1560872 , @aadsm wrote:

> I don't think we can stop loading/unloading the libraries in 
> `ProcessGDBRemote::LoadModules`. The windows dynamic loader relies on this


Then it could be Windows-specific.

> and the `library` entry on the stop packet does as well: 
> https://github.com/llvm-mirror/lldb/blob/9689521b477c75b52257fba9655cabefc3db676c/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp#L2374-L2375

When is this `library` entry being used?

For implementing fast shared libraries introspection one should implement what 
glibc+gdb have - STAP (SystemTap) probes 
:
 Those are few breakpoints each directly reporting the one added/removed 
library as one can see from GDB:

  (gdb) maintenance info breakpoints
  (gdb) info probes
  Num Type  Disp Enb AddressWhat
  Type Provider   Name   Where  Semaphore Object   
  -2  shlib events  keep y   0x77fd67a5  inf 1
  stap rtld   init_complete  0x77fd67a5   
/lib64/ld-linux-x86-64.so.2
  -5  shlib events  keep y   0x77fe61a0 
 inf 1
  stap rtld   reloc_complete 0x77fe61a0   
/lib64/ld-linux-x86-64.so.2
  -7  shlib events  keep y   0x77fe715e 
<_dl_close_worker+3070> inf 1
  stap rtld   unmap_complete 0x77fe715e   
/lib64/ld-linux-x86-64.so.2


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63868



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

[Lldb-commits] [PATCH] D63868: Unify+fix remote XML libraries handling with the legacy one

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

The `library` is part of the GDB protocol itself: 
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets
 and it's to be used with conjunction of `qXfer:libraries:read` (or 
libraries-svr4 like we do here).

Interesting, I'm not familiar with STAP but will take a look. I do think 
however that changing how we load the libraries is outside the scope of this 
diff. My main goal is to add the `libraries-svr4` packet, but while doing so I 
found a few bugs in lldb, which I'm addressing here.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63868



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

[Lldb-commits] [lldb] r364562 - Add a sanity check to the domain socket tests.

[Adrian Prantl via lldb-commits]

Author: adrian
Date: Thu Jun 27 09:45:23 2019
New Revision: 364562

URL: http://llvm.org/viewvc/llvm-project?rev=364562&view=rev
Log:
Add a sanity check to the domain socket tests.

rdar://problem/52062631

Modified:
lldb/trunk/unittests/Host/SocketTest.cpp

Modified: lldb/trunk/unittests/Host/SocketTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Host/SocketTest.cpp?rev=364562&r1=364561&r2=364562&view=diff
==
--- lldb/trunk/unittests/Host/SocketTest.cpp (original)
+++ lldb/trunk/unittests/Host/SocketTest.cpp Thu Jun 27 09:45:23 2019
@@ -93,6 +93,8 @@ TEST_F(SocketTest, DomainListenConnectAc
   std::error_code EC = 
llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
   ASSERT_FALSE(EC);
   llvm::sys::path::append(Path, "test");
+  // If this fails, $TMPDIR is too long to hold a domain socket.
+  EXPECT_LE(Path.size(), 107u);
 
   std::unique_ptr socket_a_up;
   std::unique_ptr socket_b_up;
@@ -194,6 +196,8 @@ TEST_F(SocketTest, DomainGetConnectURI)
   llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", 
domain_path);
   ASSERT_FALSE(EC);
   llvm::sys::path::append(domain_path, "test");
+  // If this fails, $TMPDIR is too long to hold a domain socket.
+  EXPECT_LE(domain_path.size(), 107u);
 
   std::unique_ptr socket_a_up;
   std::unique_ptr socket_b_up;
@@ -208,4 +212,4 @@ TEST_F(SocketTest, DomainGetConnectURI)
   EXPECT_EQ(scheme, "unix-connect");
   EXPECT_EQ(path, domain_path);
 }
-#endif
\ No newline at end of file
+#endif


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

[Lldb-commits] [PATCH] D63545: [lldb] [Process/NetBSD] Support reading YMM registers via PT_*XSTATE

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

This is ready to be reviewed now.


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

https://reviews.llvm.org/D63545



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

[Lldb-commits] [PATCH] D63540: Fix lookup of symbols at the same address with no size vs. size

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 4 inline comments as done.
jankratochvil added a comment.

In D63540#1549977 , @labath wrote:

> Why was it necessary to change the iteration order here?


No, it was an accidental leftover.

Thanks for the review.




Comment at: lldb/lit/SymbolFile/sizeless-symbol.test:1
+# RUN: %clang %S/Inputs/sizeless-symbol.s -c -o %t.o
+# RUN: %lldb %t.o -s %s -o quit | FileCheck %s

labath wrote:
> Are you sure this will work whatever the host triple happens to be ? (you can 
> try it out by manually forcing a couple of random targets with the `-target` 
> argument). I have a feeling the `.type`, `.size`, etc. directives are a 
> feature of elf-targetting assemblers. If that doesn't work you can always 
> force a specific triple with the `-target` command.
Yes, you are right.



Comment at: lldb/lit/SymbolFile/sizeless-symbol.test:4
+
+image lookup --address 1
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful

labath wrote:
> As you're fixing this bug by changing the way symbol size is computed, it 
> would be nice to also put (and check it's output) a "image dump symtab" 
> command here. That would also make it clear what are the assumptions the 
> following "lookup" commands are operating under.
I only hope the `image dump symtab` output ordering is stable.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63540



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

[Lldb-commits] [PATCH] D63540: Fix lookup of symbols at the same address with no size vs. size

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 206905.
jankratochvil marked 2 inline comments as done.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63540

Files:
  lldb/lit/SymbolFile/Inputs/sizeless-symbol.s
  lldb/lit/SymbolFile/sizeless-symbol.test
  lldb/source/Symbol/Symtab.cpp


Index: lldb/source/Symbol/Symtab.cpp
===
--- lldb/source/Symbol/Symtab.cpp
+++ lldb/source/Symbol/Symtab.cpp
@@ -896,8 +896,12 @@
   for (size_t i = 0; i < num_entries; i++) {
 FileRangeToIndexMap::Entry *entry =
 m_file_addr_to_index.GetMutableEntryAtIndex(i);
-if (entry->GetByteSize() == 0) {
-  addr_t curr_base_addr = entry->GetRangeBase();
+if (entry->GetByteSize() > 0)
+  continue;
+addr_t curr_base_addr = entry->GetRangeBase();
+// Symbols with non-zero size will show after zero-sized symbols on the
+// same address. So do not set size of a non-last zero-sized symbol.
+if (i == num_entries - 1 || 
m_file_addr_to_index.GetMutableEntryAtIndex(i + 1)->GetRangeBase() != 
curr_base_addr) {
   const RangeVector::Entry *containing_section =
   section_ranges.FindEntryThatContains(curr_base_addr);
 
Index: lldb/lit/SymbolFile/sizeless-symbol.test
===
--- /dev/null
+++ lldb/lit/SymbolFile/sizeless-symbol.test
@@ -0,0 +1,14 @@
+# Some targets do not have the .size directive.
+# RUN: %clang -target x86_64-unknown-unknown-elf %S/Inputs/sizeless-symbol.s 
-c -o %t.o
+# RUN: %lldb %t.o -s %s -o quit | FileCheck %s
+
+image lookup --address 1
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful
+image lookup --address 2
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful + 1
+image dump symtab
+# CHECK:Index   UserID DSX TypeFile Address/Value Load Address 
  Size   Flags  Name
+# CHECK-NEXT:--- -- --- --- -- 
-- -- -- 
--
+# CHECK-NEXT:[0]  1 Code0x0003 
   0x 0x sizeend
+# CHECK-NEXT:[1]  2 Code0x0001 
   0x0002 0x sizeful
+# CHECK-NEXT:[2]  3 Code0x0001 
   0x 0x sizeless
Index: lldb/lit/SymbolFile/Inputs/sizeless-symbol.s
===
--- /dev/null
+++ lldb/lit/SymbolFile/Inputs/sizeless-symbol.s
@@ -0,0 +1,8 @@
+.text
+.byte   0
+sizeless:
+sizeful:
+.byte   0
+.byte   0
+sizeend:
+.size   sizeful, sizeend - sizeful


Index: lldb/source/Symbol/Symtab.cpp
===
--- lldb/source/Symbol/Symtab.cpp
+++ lldb/source/Symbol/Symtab.cpp
@@ -896,8 +896,12 @@
   for (size_t i = 0; i < num_entries; i++) {
 FileRangeToIndexMap::Entry *entry =
 m_file_addr_to_index.GetMutableEntryAtIndex(i);
-if (entry->GetByteSize() == 0) {
-  addr_t curr_base_addr = entry->GetRangeBase();
+if (entry->GetByteSize() > 0)
+  continue;
+addr_t curr_base_addr = entry->GetRangeBase();
+// Symbols with non-zero size will show after zero-sized symbols on the
+// same address. So do not set size of a non-last zero-sized symbol.
+if (i == num_entries - 1 || m_file_addr_to_index.GetMutableEntryAtIndex(i + 1)->GetRangeBase() != curr_base_addr) {
   const RangeVector::Entry *containing_section =
   section_ranges.FindEntryThatContains(curr_base_addr);
 
Index: lldb/lit/SymbolFile/sizeless-symbol.test
===
--- /dev/null
+++ lldb/lit/SymbolFile/sizeless-symbol.test
@@ -0,0 +1,14 @@
+# Some targets do not have the .size directive.
+# RUN: %clang -target x86_64-unknown-unknown-elf %S/Inputs/sizeless-symbol.s -c -o %t.o
+# RUN: %lldb %t.o -s %s -o quit | FileCheck %s
+
+image lookup --address 1
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful
+image lookup --address 2
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful + 1
+image dump symtab
+# CHECK:Index   UserID DSX TypeFile Address/Value Load Address   Size   Flags  Name
+# CHECK-NEXT:--- -- --- --- -- -- -- -- --
+# CHECK-NEXT:[0]  1 Code0x00030x 0x sizeend
+# CHECK-NEXT:[1]  2 Code0x00010x0002 0x sizeful
+# CHECK-NEXT:[2]  3 Code

[Lldb-commits] [PATCH] D63540: Fix lookup of symbols at the same address with no size vs. size

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D63540#1558347 , @clayborg wrote:

> I am saying to leave symbols with zero size as is _if_ there is another 
> symbol that does have a valid size with the same start address.


This is what this patch does.

> Exactly. The best solution in my mind is:
> 
> - leave all symbols with sizes as is

This is what this patch does.

> - only set a symbol size if there is no other symbol at that address that 
> didn't originally have a size

This is what this patch does.

> - maybe leave zero sizes symbols out of the lookup map if they have no sizes 
> after doing the two steps above

I did not change this, IIUC it is already solved in existing code because 
`InitAddressIndexes` will expand any such zero-sized symbols first and it 
already uses the sized symbol in preference to the sizeless one during lookup.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63540



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

[Lldb-commits] [PATCH] D63667: Support __kernel_rt_sigreturn in frame initialization

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

Shouldn't you add also symbol `__restore_rt` (Fedora 30 x86_64)?

  (gdb) bt
  #0  handler (sig=6) at sigtest2.c:7
  #1  
  #2  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
  #3  0x77dea895 in __GI_abort () at abort.c:79
  #4  0x0040118e in abort_caller () at sigtest2.c:12
  #5  0x004011c2 in main () at sigtest2.c:23
  (gdb) frame 1
  #1  
  (gdb) disas
  Dump of assembler code for function __restore_rt:
  => 0x77dfff00 <+0>:   mov$0xf,%rax
 0x77dfff07 <+7>:   syscall 
 0x77dfff09 <+9>:   nopl   0x0(%rax)
  End of assembler dump.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63667



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

[Lldb-commits] [PATCH] D63667: Support __kernel_rt_sigreturn in frame initialization

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet updated this revision to Diff 206931.
JosephTremoulet added a comment.

- Include __restore_rt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63667

Files:
  lldb/include/lldb/Symbol/UnwindPlan.h
  
lldb/packages/Python/lldbsuite/test/functionalities/signal/handle-abrt/Makefile
  
lldb/packages/Python/lldbsuite/test/functionalities/signal/handle-abrt/TestHandleAbort.py
  lldb/packages/Python/lldbsuite/test/functionalities/signal/handle-abrt/main.c
  lldb/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
  lldb/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
  lldb/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
  lldb/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
  lldb/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
  lldb/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
  lldb/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
  lldb/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
  lldb/source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
  lldb/source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
  lldb/source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp
  lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
  lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
  lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
  lldb/source/Symbol/ArmUnwindInfo.cpp
  lldb/source/Symbol/CompactUnwindInfo.cpp
  lldb/source/Symbol/DWARFCallFrameInfo.cpp

Index: lldb/source/Symbol/DWARFCallFrameInfo.cpp
===
--- lldb/source/Symbol/DWARFCallFrameInfo.cpp
+++ lldb/source/Symbol/DWARFCallFrameInfo.cpp
@@ -19,6 +19,7 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timer.h"
 #include 
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -601,6 +602,9 @@
 }
 offset += aug_data_len;
   }
+  unwind_plan.SetUnwindPlanForSignalTrap(
+strchr(cie->augmentation, 'S') ? eLazyBoolYes : eLazyBoolNo);
+
   Address lsda_data;
   Address personality_function_ptr;
 
Index: lldb/source/Symbol/CompactUnwindInfo.cpp
===
--- lldb/source/Symbol/CompactUnwindInfo.cpp
+++ lldb/source/Symbol/CompactUnwindInfo.cpp
@@ -737,6 +737,7 @@
   unwind_plan.SetSourceName("compact unwind info");
   unwind_plan.SetSourcedFromCompiler(eLazyBoolYes);
   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan.SetUnwindPlanForSignalTrap(eLazyBoolNo);
   unwind_plan.SetRegisterKind(eRegisterKindEHFrame);
 
   unwind_plan.SetLSDAAddress(function_info.lsda_address);
@@ -1008,6 +1009,7 @@
   unwind_plan.SetSourceName("compact unwind info");
   unwind_plan.SetSourcedFromCompiler(eLazyBoolYes);
   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan.SetUnwindPlanForSignalTrap(eLazyBoolNo);
   unwind_plan.SetRegisterKind(eRegisterKindEHFrame);
 
   unwind_plan.SetLSDAAddress(function_info.lsda_address);
@@ -1304,6 +1306,7 @@
   unwind_plan.SetSourceName("compact unwind info");
   unwind_plan.SetSourcedFromCompiler(eLazyBoolYes);
   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan.SetUnwindPlanForSignalTrap(eLazyBoolNo);
   unwind_plan.SetRegisterKind(eRegisterKindEHFrame);
 
   unwind_plan.SetLSDAAddress(function_info.lsda_address);
@@ -1437,6 +1440,7 @@
   unwind_plan.SetSourceName("compact unwind info");
   unwind_plan.SetSourcedFromCompiler(eLazyBoolYes);
   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan.SetUnwindPlanForSignalTrap(eLazyBoolNo);
   unwind_plan.SetRegisterKind(eRegisterKindEHFrame);
 
   unwind_plan.SetLSDAAddress(function_info.lsda_address);
Index: lldb/source/Symbol/ArmUnwindInfo.cpp
===
--- lldb/source/Symbol/ArmUnwindInfo.cpp
+++ lldb/source/Symbol/ArmUnwindInfo.cpp
@@ -344,6 +344,7 @@
   unwind_plan.SetSourceName("ARM.exidx unwind info");
   unwind_plan.SetSourcedFromCompiler(eLazyBoolYes);
   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan.SetUnwindPlanForSignalTrap(eLazyBoolNo);
   unwind_plan.SetRegisterKind(eRegisterKindDWARF);
 
   return true;
Index: lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
===
--- lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
+++ lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngi

[Lldb-commits] [PATCH] D63667: Support __kernel_rt_sigreturn in frame initialization

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet added a comment.

In D63667#1561365 , @jankratochvil 
wrote:

> Shouldn't you add also symbol `__restore_rt` (Fedora 30 x86_64)?


Thanks, yes, looks like I should.  Updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63667



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

[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

@jingham: Okay, so I tried to do what you suggested and that solution doesn't 
work because of ObjC++. After looking into it, it looks like Variable and 
Function just ask the CompileUnit for the language type instead of determining 
it themselves meaning that we determining the Variable's language boils down to 
asking the CompileUnit for its language. That can probably be improved, but I 
think that just relying on the SymbolContextScope alone isn't yet sufficient. I 
think it would be worth it to ask the SymbolContextScope before trying all the 
loaded language runtimes though. Would you be okay with that solution?


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

https://reviews.llvm.org/D63240



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

[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D63240#1561488 , @xiaobai wrote:

> @jingham: Okay, so I tried to do what you suggested and that solution doesn't 
> work because of ObjC++. After looking into it, it looks like Variable and 
> Function just ask the CompileUnit for the language type instead of 
> determining it themselves meaning that we determining the Variable's language 
> boils down to asking the CompileUnit for its language. That can probably be 
> improved, but I think that just relying on the SymbolContextScope alone isn't 
> yet sufficient. I think it would be worth it to ask the SymbolContextScope 
> before trying all the loaded language runtimes though. Would you be okay with 
> that solution?


That's unfortunate.  For ObjC++ CompUnits we should get the language from the 
function name: it's either a C++ mangled name (language => , or an ObjC name...

One way to solve this would be an ObjC++ language runtime which dispatches to 
the ObjC and C++ ones as is appropriate.  I'm not sure whether that would 
always be correct, but it would provide a more explicit way to get this right.  
OTOH, that's a lot more work...

Is your suggestion to say that if the value IS whitelisted for the 
SymbolContextScope language, then we're done, otherwise we ask all the runtimes?

Asking C, C++ & ObjC in an ObjC++ frame is not really right - it would fail my 
contrived example above - but seems like it has a low probability of getting us 
into trouble.  But if you are in a ObjC++ file, you certainly don't want to ask 
the Swift LanguageRuntime whether it whitelists the symbol.  Those are entirely 
incompatible languages and you shouldn't be asking Swift questions about any 
C-family language frame.  Perhaps a more precise version of your suggestion 
would be that if your SymbolContextScope language is a C-family language, we 
ask all the other C Family language runtimes, but not the orthogonal languages.

We don't have this problem of intermixable "language's" with Swift (and 
probably not with Rust...) or really anything else sensible.  There's just one 
language and you can't intermix it with code from another language...  But if 
we did we could add the notion of language families more generally.


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

https://reviews.llvm.org/D63240



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

[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D63240#1561531 , @jingham wrote:

> In D63240#1561488 , @xiaobai wrote:
>
> > @jingham: Okay, so I tried to do what you suggested and that solution 
> > doesn't work because of ObjC++. After looking into it, it looks like 
> > Variable and Function just ask the CompileUnit for the language type 
> > instead of determining it themselves meaning that we determining the 
> > Variable's language boils down to asking the CompileUnit for its language. 
> > That can probably be improved, but I think that just relying on the 
> > SymbolContextScope alone isn't yet sufficient. I think it would be worth it 
> > to ask the SymbolContextScope before trying all the loaded language 
> > runtimes though. Would you be okay with that solution?
>
>
> That's unfortunate.  For ObjC++ CompUnits we should get the language from the 
> function name: it's either a C++ mangled name (language => , or an ObjC 
> name...


I'm going to try to modify `Function::GetLanguage` to see if it can figure out 
the language based on the mangled name before it asks the compilation unit. I 
think that could potentially solve the issue with ObjC++

> One way to solve this would be an ObjC++ language runtime which dispatches to 
> the ObjC and C++ ones as is appropriate.  I'm not sure whether that would 
> always be correct, but it would provide a more explicit way to get this 
> right.  OTOH, that's a lot more work...

This could possibly work, but it feels like the wrong abstraction. Creating a 
LanguageRuntime for a language with no runtime library to work around the fact 
that it uses two runtimes for different languages is just masking the issue.

> Is your suggestion to say that if the value IS whitelisted for the 
> SymbolContextScope language, then we're done, otherwise we ask all the 
> runtimes?

Yup! I personally don't think that asking every language runtime is that big of 
a deal, but I recognize that I could be wrong about that.

> Asking C, C++ & ObjC in an ObjC++ frame is not really right - it would fail 
> my contrived example above - but seems like it has a low probability of 
> getting us into trouble.  But if you are in a ObjC++ file, you certainly 
> don't want to ask the Swift LanguageRuntime whether it whitelists the symbol. 
>  Those are entirely incompatible languages and you shouldn't be asking Swift 
> questions about any C-family language frame.  Perhaps a more precise version 
> of your suggestion would be that if your SymbolContextScope language is a 
> C-family language, we ask all the other C Family language runtimes, but not 
> the orthogonal languages.

This idea isn't super terrible imo, but you're right here: Asking runtimes of 
other languages in the same family isn't right even if it has a lower 
probability of getting us into trouble. It will work until it doesn't.


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

https://reviews.llvm.org/D63240



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

Re: [Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

[Jim Ingham via lldb-commits]

> On Jun 27, 2019, at 3:28 PM, Alex Langford via Phabricator 
>  wrote:
> 
> xiaobai added a comment.
> 
> In D63240#1561531 , @jingham wrote:
> 
>> In D63240#1561488 , @xiaobai wrote:
>> 
>>> @jingham: Okay, so I tried to do what you suggested and that solution 
>>> doesn't work because of ObjC++. After looking into it, it looks like 
>>> Variable and Function just ask the CompileUnit for the language type 
>>> instead of determining it themselves meaning that we determining the 
>>> Variable's language boils down to asking the CompileUnit for its language. 
>>> That can probably be improved, but I think that just relying on the 
>>> SymbolContextScope alone isn't yet sufficient. I think it would be worth it 
>>> to ask the SymbolContextScope before trying all the loaded language 
>>> runtimes though. Would you be okay with that solution?
>> 
>> 
>> That's unfortunate.  For ObjC++ CompUnits we should get the language from 
>> the function name: it's either a C++ mangled name (language => , or an ObjC 
>> name...
> 
> 
> I'm going to try to modify `Function::GetLanguage` to see if it can figure 
> out the language based on the mangled name before it asks the compilation 
> unit. I think that could potentially solve the issue with ObjC++

Yes, that seems like the right thing to do.  Even though you can mix ObjC and 
C++ in an ObjC++ file, you can't mix the object models so you'll never have a 
method that is both ObjC & C++.  It either dispatches like ObjC or it 
dispatches like C++...  So the runtime gotten from the method you're stopped 
in's name should be right.

Jim

> 
>> One way to solve this would be an ObjC++ language runtime which dispatches 
>> to the ObjC and C++ ones as is appropriate.  I'm not sure whether that would 
>> always be correct, but it would provide a more explicit way to get this 
>> right.  OTOH, that's a lot more work...
> 
> This could possibly work, but it feels like the wrong abstraction. Creating a 
> LanguageRuntime for a language with no runtime library to work around the 
> fact that it uses two runtimes for different languages is just masking the 
> issue.
> 
>> Is your suggestion to say that if the value IS whitelisted for the 
>> SymbolContextScope language, then we're done, otherwise we ask all the 
>> runtimes?
> 
> Yup! I personally don't think that asking every language runtime is that big 
> of a deal, but I recognize that I could be wrong about that.
> 
>> Asking C, C++ & ObjC in an ObjC++ frame is not really right - it would fail 
>> my contrived example above - but seems like it has a low probability of 
>> getting us into trouble.  But if you are in a ObjC++ file, you certainly 
>> don't want to ask the Swift LanguageRuntime whether it whitelists the 
>> symbol.  Those are entirely incompatible languages and you shouldn't be 
>> asking Swift questions about any C-family language frame.  Perhaps a more 
>> precise version of your suggestion would be that if your SymbolContextScope 
>> language is a C-family language, we ask all the other C Family language 
>> runtimes, but not the orthogonal languages.
> 
> This idea isn't super terrible imo, but you're right here: Asking runtimes of 
> other languages in the same family isn't right even if it has a lower 
> probability of getting us into trouble. It will work until it doesn't.
> 
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D63240/new/
> 
> https://reviews.llvm.org/D63240
> 
> 
> 

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

[Lldb-commits] [PATCH] D62887: Update the thread list before setting stop reasons with an OS plugin

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 206980.
jingham added a comment.

Addresses Greg's question about what happens when we load a new OS plugin.  
Indeed we should limit the work we do only to the case where we didn't have an 
OS plugin, then tried to load one and succeeded.  Only then do we need to clear 
and update the thread list.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62887

Files:
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestRecognizeBreakpoint.py
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system_2.py
  source/Host/common/Editline.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -3019,8 +3019,16 @@
 }
   }
 
-  if (!m_os_up)
+  if (!m_os_up) {
 LoadOperatingSystemPlugin(false);
+if (m_os_up) {
+  // Somebody might have gotten threads before now, but we need to force the
+  // update after we've loaded the OperatingSystem plugin or it won't get a
+  // chance to process the threads.
+  m_thread_list.Clear();
+  UpdateThreadListIfNeeded();
+}
+  }
   // Figure out which one is the executable, and set that in our target:
   const ModuleList &target_modules = GetTarget().GetImages();
   std::lock_guard guard(target_modules.GetMutex());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2423,6 +2423,15 @@
 
   // Scope for the lock
   {
+// Check to see if SetThreadStopInfo() filled in m_thread_ids?
+if (m_thread_ids.empty()) {
+// No, we need to fetch the thread list manually
+UpdateThreadIDList();
+}
+// We might set some stop info's so make sure the thread list is up to
+// date before we do that or we might overwrite what was computed here.
+UpdateThreadListIfNeeded();
+
 // Lock the thread stack while we access it
 std::lock_guard guard(m_last_stop_packet_mutex);
 // Get the number of stop packets on the stack
@@ -2437,13 +2446,7 @@
 // Clear the thread stop stack
 m_stop_packet_stack.clear();
   }
-
-  // Check to see if SetThreadStopInfo() filled in m_thread_ids?
-  if (m_thread_ids.empty()) {
-// No, we need to fetch the thread list manually
-UpdateThreadIDList();
-  }
-
+  
   // If we have queried for a default thread id
   if (m_initial_tid != LLDB_INVALID_THREAD_ID) {
 m_thread_list.SetSelectedThreadByID(m_initial_tid);
Index: source/Host/common/Editline.cpp
===
--- source/Host/common/Editline.cpp
+++ source/Host/common/Editline.cpp
@@ -1380,6 +1380,7 @@
   stream->Write(s, len);
   stream->Flush();
   if (m_editor_status == EditorStatus::Editing) {
+SaveEditedLine();
 DisplayInput();
 MoveCursor(CursorLocation::BlockEnd, CursorLocation::EditingCursor);
   }
Index: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system_2.py
===
--- packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system_2.py
+++ packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system_2.py
@@ -0,0 +1,62 @@
+import lldb
+import struct
+
+
+class OperatingSystemPlugIn(object):
+"""Class that provides data for an instance of a LLDB 'OperatingSystemPython' plug-in class
+   This version stops once with threads 0x111 and 0x222, then stops a second time with threads
+   0x111 and 0x333."""
+
+def __init__(self, process):
+'''Initialization needs a valid.SBProcess object.
+
+This plug-in will get created after a live process is valid and has stopped for the first time.
+'''
+self.process = None
+self.registers = None
+self.threads = None
+self.times_called = 0
+if isinstance(process, lldb.SBProcess) and process.IsValid():
+self.process = process
+self.threads = None  # Will be an dictionary containing info for each thread
+
+def get_target(self):
+return self.process.target
+
+def get_thread_info(self):
+self.times_called += 1
+
+if self.times_called == 1:
+self.threads = [{
+'tid': 0x111,
+'name': 'one',
+'queue': 'queue1',
+'state': 'stopped',
+'stop_reason': 'none',
+'core': 1
+}, {
+'tid': 0x222,
+'name'

[Lldb-commits] [PATCH] D63914: Make the expression parser work for missing weak symbols

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, clayborg.
Herald added subscribers: lldb-commits, teemperor, abidh.
Herald added a project: LLDB.

lldb wasn't handling weak symbols, so if you were debugging a binary that used 
a weak symbol, and the symbol was missing, any references to the symbol would 
result in a symbol not found error.  But it is actually legitimate to refer to 
missing weak symbols, you just have to test them against NULL first.

This patch does three things:

1. Adds a bit to indicate weakness to Symbol and IsWeak & SetIsWeak accessors
2. Teaches the ObjectFileMachO to detect weak symbols and mark them 
appropriately
3. Gets the symbol -> address lookup in IRExecutionUnit.cpp to mark missing 
weak symbols as such.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D63914

Files:
  include/lldb/Expression/IRExecutionUnit.h
  include/lldb/Symbol/Symbol.h
  packages/Python/lldbsuite/test/expression_command/weak_symbols/Makefile
  
packages/Python/lldbsuite/test/expression_command/weak_symbols/TestWeakSymbols.py
  packages/Python/lldbsuite/test/expression_command/weak_symbols/dylib.c
  packages/Python/lldbsuite/test/expression_command/weak_symbols/dylib.h
  packages/Python/lldbsuite/test/expression_command/weak_symbols/main.c
  
packages/Python/lldbsuite/test/expression_command/weak_symbols/module.modulemap
  source/Expression/IRExecutionUnit.cpp
  source/Expression/IRInterpreter.cpp
  source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Symbol/Symbol.cpp

Index: source/Symbol/Symbol.cpp
===
--- source/Symbol/Symbol.cpp
+++ source/Symbol/Symbol.cpp
@@ -28,7 +28,8 @@
   m_is_external(false), m_size_is_sibling(false),
   m_size_is_synthesized(false), m_size_is_valid(false),
   m_demangled_is_synthesized(false), m_contains_linker_annotations(false),
-  m_type(eSymbolTypeInvalid), m_mangled(), m_addr_range(), m_flags() {}
+  m_is_weak(false), m_type(eSymbolTypeInvalid), m_mangled(), m_addr_range(),
+  m_flags() {}
 
 Symbol::Symbol(uint32_t symID, const char *name, bool name_is_mangled,
SymbolType type, bool external, bool is_debug,
@@ -41,7 +42,8 @@
   m_is_debug(is_debug), m_is_external(external), m_size_is_sibling(false),
   m_size_is_synthesized(false), m_size_is_valid(size_is_valid || size > 0),
   m_demangled_is_synthesized(false),
-  m_contains_linker_annotations(contains_linker_annotations), m_type(type),
+  m_contains_linker_annotations(contains_linker_annotations), 
+  m_is_weak(false), m_type(type),
   m_mangled(ConstString(name), name_is_mangled),
   m_addr_range(section_sp, offset, size), m_flags(flags) {}
 
@@ -56,8 +58,9 @@
   m_size_is_synthesized(false),
   m_size_is_valid(size_is_valid || range.GetByteSize() > 0),
   m_demangled_is_synthesized(false),
-  m_contains_linker_annotations(contains_linker_annotations), m_type(type),
-  m_mangled(mangled), m_addr_range(range), m_flags(flags) {}
+  m_contains_linker_annotations(contains_linker_annotations), 
+  m_is_weak(false), m_type(type), m_mangled(mangled), m_addr_range(range), 
+  m_flags(flags) {}
 
 Symbol::Symbol(const Symbol &rhs)
 : SymbolContextScope(rhs), m_uid(rhs.m_uid), m_type_data(rhs.m_type_data),
@@ -68,7 +71,7 @@
   m_size_is_valid(rhs.m_size_is_valid),
   m_demangled_is_synthesized(rhs.m_demangled_is_synthesized),
   m_contains_linker_annotations(rhs.m_contains_linker_annotations),
-  m_type(rhs.m_type), m_mangled(rhs.m_mangled),
+  m_is_weak(rhs.m_is_weak), m_type(rhs.m_type), m_mangled(rhs.m_mangled),
   m_addr_range(rhs.m_addr_range), m_flags(rhs.m_flags) {}
 
 const Symbol &Symbol::operator=(const Symbol &rhs) {
@@ -85,6 +88,7 @@
 m_size_is_valid = rhs.m_size_is_valid;
 m_demangled_is_synthesized = rhs.m_demangled_is_synthesized;
 m_contains_linker_annotations = rhs.m_contains_linker_annotations;
+m_is_weak = rhs.m_is_weak;
 m_type = rhs.m_type;
 m_mangled = rhs.m_mangled;
 m_addr_range = rhs.m_addr_range;
@@ -106,6 +110,7 @@
   m_size_is_valid = false;
   m_demangled_is_synthesized = false;
   m_contains_linker_annotations = false;
+  m_is_weak = false;
   m_type = eSymbolTypeInvalid;
   m_flags = 0;
   m_addr_range.Clear();
Index: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -4575,6 +4575,8 @@
 sym[sym_idx].GetAddressRef().SetOffset(symbol_value);
   }
   sym[sym_idx].SetFlags(nlist.n_type << 16 | nlist.n_desc);
+  if (nlist.n_desc & N_WEAK_REF)
+sym[sym_idx].SetIsWeak(true);
 
   if (symbol_byte_size > 0)
 sym[sym_idx].SetByteSize(symbol_byt

[Lldb-commits] [PATCH] D62887: Update the thread list before setting stop reasons with an OS plugin

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 2 inline comments as done.
jingham added inline comments.



Comment at: source/Target/Process.cpp:3037-3041
+  // Somebody might have gotten threads before now, but we need to force the
+  // update after we've loaded the OperatingSystem plugin or it won't get a
+  // chance to process the threads.
+  m_thread_list.Clear();
+  UpdateThreadListIfNeeded();

clayborg wrote:
> Should this be in the above if statement?
> 
> ```
> if (!m_os_up) {
> LoadOperatingSystemPlugin(false);
> // Somebody might have gotten threads before now, but we need to force the
> // update after we've loaded the OperatingSystem plugin or it won't get a
> // chance to process the threads.
> m_thread_list.Clear();
> UpdateThreadListIfNeeded();
> }
> ```
> 
> And if we do this in the if statement, do we need to clear the m_thread_list?
You're right.  Actually, we should try to get the OS plugin and only if we 
succeed do we need to do any work.  I changed the code to do that.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62887



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

[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

[Alex Langford via Phabricator via lldb-commits]

xiaobai updated this revision to Diff 206994.
xiaobai added a comment.
Herald added a subscriber: jdoerfert.

- Implement @jingham's suggestion
- Change Function::GetLanguage to first guess the language from the name of the 
function you're in.
- Fix a bug in DWARFASTParserClang::ParseFunctionFromDWARF that qualifies ObjC 
methods as if they were C++ methods when parsing a function from an ObjC++ CU


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

https://reviews.llvm.org/D63240

Files:
  include/lldb/Target/CPPLanguageRuntime.h
  include/lldb/Target/LanguageRuntime.h
  include/lldb/Target/ObjCLanguageRuntime.h
  source/Core/ValueObject.cpp
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  source/Symbol/Function.cpp
  source/Target/CPPLanguageRuntime.cpp
  source/Target/ObjCLanguageRuntime.cpp

Index: source/Target/ObjCLanguageRuntime.cpp
===
--- source/Target/ObjCLanguageRuntime.cpp
+++ source/Target/ObjCLanguageRuntime.cpp
@@ -46,19 +46,6 @@
   return name == g_self || name == g_cmd;
 }
 
-bool ObjCLanguageRuntime::IsRuntimeSupportValue(ValueObject &valobj) {
-  // All runtime support values have to be marked as artificial by the
-  // compiler. But not all artificial variables should be hidden from
-  // the user.
-  if (!valobj.GetVariable())
-return false;
-  if (!valobj.GetVariable()->IsArtificial())
-return false;
-
-  // Whitelist "self" and "_cmd".
-  return !IsWhitelistedRuntimeValue(valobj.GetName());
-}
-
 bool ObjCLanguageRuntime::AddClass(ObjCISA isa,
const ClassDescriptorSP &descriptor_sp,
const char *class_name) {
Index: source/Target/CPPLanguageRuntime.cpp
===
--- source/Target/CPPLanguageRuntime.cpp
+++ source/Target/CPPLanguageRuntime.cpp
@@ -43,20 +43,8 @@
 CPPLanguageRuntime::CPPLanguageRuntime(Process *process)
 : LanguageRuntime(process) {}
 
-bool CPPLanguageRuntime::IsRuntimeSupportValue(ValueObject &valobj) {
-  // All runtime support values have to be marked as artificial by the
-  // compiler. But not all artificial variables should be hidden from
-  // the user.
-  if (!valobj.GetVariable())
-return false;
-  if (!valobj.GetVariable()->IsArtificial())
-return false;
-
-  // Whitelist "this" and since there is no ObjC++ runtime, any ObjC names.
-  ConstString name = valobj.GetName();
-  if (name == g_this)
-return false;
-  return !ObjCLanguageRuntime::IsWhitelistedRuntimeValue(name);
+bool CPPLanguageRuntime::IsWhitelistedRuntimeValue(ConstString name) {
+  return name == g_this;
 }
 
 bool CPPLanguageRuntime::GetObjectDescription(Stream &str,
Index: source/Symbol/Function.cpp
===
--- source/Symbol/Function.cpp
+++ source/Symbol/Function.cpp
@@ -589,10 +589,14 @@
 }
 
 lldb::LanguageType Function::GetLanguage() const {
+  lldb::LanguageType lang = m_mangled.GuessLanguage();
+  if (lang != lldb::eLanguageTypeUnknown)
+return lang;
+
   if (m_comp_unit)
 return m_comp_unit->GetLanguage();
-  else
-return lldb::eLanguageTypeUnknown;
+
+  return lldb::eLanguageTypeUnknown;
 }
 
 ConstString Function::GetName() const {
Index: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2371,7 +2371,8 @@
 func_name.SetValue(ConstString(mangled), true);
   else if ((die.GetParent().Tag() == DW_TAG_compile_unit ||
 die.GetParent().Tag() == DW_TAG_partial_unit) &&
-   Language::LanguageIsCPlusPlus(die.GetLanguage()) && name &&
+   Language::LanguageIsCPlusPlus(die.GetLanguage()) &&
+   !Language::LanguageIsObjC(die.GetLanguage()) && name &&
strcmp(name, "main") != 0) {
 // If the mangled name is not present in the DWARF, generate the
 // demangled name using the decl context. We skip if the function is
Index: source/Core/ValueObject.cpp
===
--- source/Core/ValueObject.cpp
+++ source/Core/ValueObject.cpp
@@ -1695,18 +1695,28 @@
 
 bool ValueObject::IsRuntimeSupportValue() {
   Process *process(GetProcessSP().get());
-  if (process) {
-LanguageRuntime *runtime =
-process->GetLanguageRuntime(GetObjectRuntimeLanguage());
-if (!runtime)
-  runtime = ObjCLanguageRuntime::Get(*process);
-if (runtime)
-  return runtime->IsRuntimeSupportValue(*this);
-// If there is no language runtime, trust the compiler to mark all
-// runtime support variables as artificial.
-return GetVariable() && GetVariable()->IsArtificial();
+  if (!process)
+return false;
+
+  // We trust the the compiler did the 

[Lldb-commits] [PATCH] D62504: Avoid calling LoadModules twice when modules change

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

I looked into this a bit today and at one point in time (when the list of 
modules is being processed) it seems that some previously loaded modules are 
somehow removed (at least I stop seeing them in the `image list`) including the 
one where the rendezvous breakpoint is so that breakpoint becomes "unresolved" 
and that's why it doesn't stop anymore. I'll need to check why this is the case 
tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62504



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