[Lldb-commits] [PATCH] D123128: don't extra notify ModulesDidLoad() from LoadModuleAtAddress()

[Luboš Luňák via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG195a8b977efe: dont extra notify ModulesDidLoad() from 
LoadModuleAtAddress() (authored by llunak).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123128

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py


Index: 
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
===
--- 
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
+++ 
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
@@ -16,10 +16,10 @@
 mydir = TestBase.compute_mydir(__file__)
 NO_DEBUG_INFO_TESTCASE = True
 
-# DynamicLoaderDarwin should batch up notifications about
-# newly added/removed libraries.  Other DynamicLoaders may
+# At least DynamicLoaderDarwin and DynamicLoaderPOSIXDYLD should batch up
+# notifications about newly added/removed libraries.  Other DynamicLoaders 
may
 # not be written this way.
-@skipUnlessDarwin
+@skipUnlessPlatform(["linux"]+lldbplatformutil.getDarwinOSTriples())
 
 def setUp(self):
 # Call super's setUp().
@@ -72,20 +72,24 @@
 total_solibs_removed = 0
 total_modules_added_events = 0
 total_modules_removed_events = 0
+already_loaded_modules = []
 while listener.GetNextEvent(event):
 if lldb.SBTarget.EventIsTargetEvent(event):
 if event.GetType() == lldb.SBTarget.eBroadcastBitModulesLoaded:
 solib_count = lldb.SBTarget.GetNumModulesFromEvent(event)
 total_modules_added_events += 1
 total_solibs_added += solib_count
+added_files = []
+i = 0
+while i < solib_count:
+module = lldb.SBTarget.GetModuleAtIndexFromEvent(i, 
event)
+self.assertTrue(module not in already_loaded_modules)
+already_loaded_modules.append(module)
+if self.TraceOn():
+
added_files.append(module.GetFileSpec().GetFilename())
+i = i + 1
 if self.TraceOn():
 # print all of the binaries that have been added
-added_files = []
-i = 0
-while i < solib_count:
-module = 
lldb.SBTarget.GetModuleAtIndexFromEvent(i, event)
-
added_files.append(module.GetFileSpec().GetFilename())
-i = i + 1
 print("Loaded files: %s" % (', '.join(added_files)))
 
 if event.GetType() == 
lldb.SBTarget.eBroadcastBitModulesUnloaded:
@@ -107,6 +111,7 @@
 # binaries in batches.  Check that we got back more than 1 solib per 
event.  
 # In practice on Darwin today, we get back two events for a do-nothing 
c 
 # program: a.out and dyld, and then all the rest of the system 
libraries.
+# On Linux we get events for ld.so, [vdso], the binary and then all 
libraries.
 
-avg_solibs_added_per_event = int(float(total_solibs_added) / 
float(total_modules_added_events))
+avg_solibs_added_per_event = round(float(total_solibs_added) / 
float(total_modules_added_events))
 self.assertGreater(avg_solibs_added_per_event, 1)
Index: lldb/source/Core/DynamicLoader.cpp
===
--- lldb/source/Core/DynamicLoader.cpp
+++ lldb/source/Core/DynamicLoader.cpp
@@ -152,8 +152,7 @@
   if (ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec))
 return module_sp;
 
-  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec,
-/*notify=*/true))
+  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec, false))
 return module_sp;
 
   return nullptr;
Index: lldb/include/lldb/Target/DynamicLoader.h
===
--- lldb/include/lldb/Target/DynamicLoader.h
+++ lldb/include/lldb/Target/DynamicLoader.h
@@ -203,6 +203,8 @@
 
   /// Locates or creates a module given by \p file and updates/loads the
   /// resulting module at the virtual base address \p base_addr.
+  /// Note that this calls Target::GetOrCreateModule with notify being false,
+  /// so it is necessary to call Target::ModulesDidLoad afterwards.
   virtual lldb::ModuleSP LoadModuleAtAddress(const lldb_private::FileSpec 
,
  lldb::addr_t link_map_addr,

[Lldb-commits] [PATCH] D123128: don't extra notify ModulesDidLoad() from LoadModuleAtAddress()

[Pavel Labath via Phabricator via lldb-commits]

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

Cool. Thanks.


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

https://reviews.llvm.org/D123128

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

[Lldb-commits] [PATCH] D123128: don't extra notify ModulesDidLoad() from LoadModuleAtAddress()

[Luboš Luňák via Phabricator via lldb-commits]

llunak updated this revision to Diff 421782.
llunak edited the summary of this revision.
llunak added a comment.

Changed to always disable notify and added a comment about that to 
LoadModuleAtAddress().


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

https://reviews.llvm.org/D123128

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py


Index: 
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
===
--- 
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
+++ 
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
@@ -16,10 +16,10 @@
 mydir = TestBase.compute_mydir(__file__)
 NO_DEBUG_INFO_TESTCASE = True
 
-# DynamicLoaderDarwin should batch up notifications about
-# newly added/removed libraries.  Other DynamicLoaders may
+# At least DynamicLoaderDarwin and DynamicLoaderPOSIXDYLD should batch up
+# notifications about newly added/removed libraries.  Other DynamicLoaders 
may
 # not be written this way.
-@skipUnlessDarwin
+@skipUnlessPlatform(["linux"]+lldbplatformutil.getDarwinOSTriples())
 
 def setUp(self):
 # Call super's setUp().
@@ -72,20 +72,24 @@
 total_solibs_removed = 0
 total_modules_added_events = 0
 total_modules_removed_events = 0
+already_loaded_modules = []
 while listener.GetNextEvent(event):
 if lldb.SBTarget.EventIsTargetEvent(event):
 if event.GetType() == lldb.SBTarget.eBroadcastBitModulesLoaded:
 solib_count = lldb.SBTarget.GetNumModulesFromEvent(event)
 total_modules_added_events += 1
 total_solibs_added += solib_count
+added_files = []
+i = 0
+while i < solib_count:
+module = lldb.SBTarget.GetModuleAtIndexFromEvent(i, 
event)
+self.assertTrue(module not in already_loaded_modules)
+already_loaded_modules.append(module)
+if self.TraceOn():
+
added_files.append(module.GetFileSpec().GetFilename())
+i = i + 1
 if self.TraceOn():
 # print all of the binaries that have been added
-added_files = []
-i = 0
-while i < solib_count:
-module = 
lldb.SBTarget.GetModuleAtIndexFromEvent(i, event)
-
added_files.append(module.GetFileSpec().GetFilename())
-i = i + 1
 print("Loaded files: %s" % (', '.join(added_files)))
 
 if event.GetType() == 
lldb.SBTarget.eBroadcastBitModulesUnloaded:
@@ -107,6 +111,7 @@
 # binaries in batches.  Check that we got back more than 1 solib per 
event.  
 # In practice on Darwin today, we get back two events for a do-nothing 
c 
 # program: a.out and dyld, and then all the rest of the system 
libraries.
+# On Linux we get events for ld.so, [vdso], the binary and then all 
libraries.
 
-avg_solibs_added_per_event = int(float(total_solibs_added) / 
float(total_modules_added_events))
+avg_solibs_added_per_event = round(float(total_solibs_added) / 
float(total_modules_added_events))
 self.assertGreater(avg_solibs_added_per_event, 1)
Index: lldb/source/Core/DynamicLoader.cpp
===
--- lldb/source/Core/DynamicLoader.cpp
+++ lldb/source/Core/DynamicLoader.cpp
@@ -152,8 +152,7 @@
   if (ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec))
 return module_sp;
 
-  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec,
-/*notify=*/true))
+  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec, false))
 return module_sp;
 
   return nullptr;
Index: lldb/include/lldb/Target/DynamicLoader.h
===
--- lldb/include/lldb/Target/DynamicLoader.h
+++ lldb/include/lldb/Target/DynamicLoader.h
@@ -203,6 +203,8 @@
 
   /// Locates or creates a module given by \p file and updates/loads the
   /// resulting module at the virtual base address \p base_addr.
+  /// Note that this calls Target::GetOrCreateModule with notify being false,
+  /// so it is necessary to call Target::ModulesDidLoad afterwards.
   virtual lldb::ModuleSP LoadModuleAtAddress(const lldb_private::FileSpec 
,
  lldb::addr_t link_map_addr,
  

[Lldb-commits] [PATCH] D123128: don't extra notify ModulesDidLoad() from LoadModuleAtAddress()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> Note that this patch could have been shorter if I simply changed 
> LoadModuleAtAddress() to always pass false for notify (all callers currently 
> call ModulesDidLoad() afterwards) and added a note to LoadModuleAtAddress() 
> docs about it, but I don't know if that's a good design choice.

That would definitely be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123128

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

[Lldb-commits] [PATCH] D123128: don't extra notify ModulesDidLoad() from LoadModuleAtAddress()

[Luboš Luňák via Phabricator via lldb-commits]

llunak created this revision.
llunak added a reviewer: jasonmolenda.
llunak added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
llunak requested review of this revision.
Herald added a subscriber: lldb-commits.

Places calling LoadModuleAddress() already call ModulesDidLoad() after a loop 
calling LoadModuleAtAddress(), so it's not necessary to call it from there, and 
the batched ModulesDidLoad() may be more efficient than this place calling it 
one after one.

This also makes the ModuleLoadedNotifys test pass on Linux now that the 
duplicates no longer bring down the average of modules notified per call.

This change is a prerequisite for parallelizing PreloadSymbols() in D122975 
.

Note that this patch could have been shorter if I simply changed 
LoadModuleAtAddress() to always pass false for `notify` (all callers currently 
call ModulesDidLoad() afterwards) and added a note to LoadModuleAtAddress() 
docs about it, but I don't know if that's a good design choice. I can change 
the patch to be that way if you want.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123128

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  
lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py

Index: lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
===
--- lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
+++ lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
@@ -16,10 +16,10 @@
 mydir = TestBase.compute_mydir(__file__)
 NO_DEBUG_INFO_TESTCASE = True
 
-# DynamicLoaderDarwin should batch up notifications about
-# newly added/removed libraries.  Other DynamicLoaders may
+# At least DynamicLoaderDarwin and DynamicLoaderPOSIXDYLD should batch up
+# notifications about newly added/removed libraries.  Other DynamicLoaders may
 # not be written this way.
-@skipUnlessDarwin
+@skipUnlessPlatform(["linux"]+lldbplatformutil.getDarwinOSTriples())
 
 def setUp(self):
 # Call super's setUp().
@@ -107,6 +107,7 @@
 # binaries in batches.  Check that we got back more than 1 solib per event.  
 # In practice on Darwin today, we get back two events for a do-nothing c 
 # program: a.out and dyld, and then all the rest of the system libraries.
+# On Linux we get events for ld.so, [vdso], the binary and then all libraries.
 
-avg_solibs_added_per_event = int(float(total_solibs_added) / float(total_modules_added_events))
+avg_solibs_added_per_event = round(float(total_solibs_added) / float(total_modules_added_events))
 self.assertGreater(avg_solibs_added_per_event, 1)
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -396,7 +396,7 @@
   lldb::ModuleSP LoadModuleAtAddress(const FileSpec ,
  lldb::addr_t link_map,
  lldb::addr_t base_addr,
- bool value_is_offset);
+ bool value_is_offset, bool notify);
 
   Status UpdateAutomaticSignalFiltering() override;
 
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
@@ -4548,13 +4548,14 @@
 lldb::ModuleSP ProcessGDBRemote::LoadModuleAtAddress(const FileSpec ,
  lldb::addr_t link_map,
  lldb::addr_t base_addr,
- bool value_is_offset) {
+ bool value_is_offset,
+ bool notify) {
   DynamicLoader *loader = GetDynamicLoader();
   if (!loader)
 return nullptr;
 
-  return loader->LoadModuleAtAddress(file, link_map, base_addr,
- value_is_offset);
+  return loader->LoadModuleAtAddress(file, link_map, base_addr, value_is_offset,
+ notify);
 }
 
 llvm::Error ProcessGDBRemote::LoadModules() {
@@ -4586,8 +4587,8 @@