[Lldb-commits] [PATCH] D122944: [lldb] Prevent object file plugins from messing with the data buffer if they don't match

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It would be better if we could automatically "reset" these variables by having 
the plugins accept them as values instead of references. Does anyone I actually 
need the new values of the variables after these functions are done? I know of 
at least one place 
,
 which actively tries to avoid that...


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

https://reviews.llvm.org/D122944

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

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

llunak created this revision.
llunak added reviewers: clayborg, chandlerc.
Herald added subscribers: dexonsmith, hiraditya.
Herald added a project: All.
llunak requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.

Pool::GetConstCStringWithStringRef() may call djbHash() up to 3 times for the 
same string: First from Pool::hash() when deciding which StringMap in the pool 
to use, then from read-locked StringMap::find() and then possibly from 
StringMap::insert() if insertion is needed.

If LLDB's index cache is enabled and everything is cached, calls to djbHash() 
are ~30% of the total CPU time here when I profile lldb start. Modifying 
StringMap to allow explicitly passing in a precomputed hash and calling 
djbHash() just once reduces startup CPU time by ~19%.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122974

Files:
  lldb/source/Utility/ConstString.cpp
  llvm/include/llvm/ADT/StringMap.h
  llvm/lib/Support/StringMap.cpp

Index: llvm/lib/Support/StringMap.cpp
===
--- llvm/lib/Support/StringMap.cpp
+++ llvm/lib/Support/StringMap.cpp
@@ -11,7 +11,6 @@
 //===--===//
 
 #include "llvm/ADT/StringMap.h"
-#include "llvm/Support/DJB.h"
 #include "llvm/Support/MathExtras.h"
 
 using namespace llvm;
@@ -80,11 +79,11 @@
 /// specified bucket will be non-null.  Otherwise, it will be null.  In either
 /// case, the FullHashValue field of the bucket will be set to the hash value
 /// of the string.
-unsigned StringMapImpl::LookupBucketFor(StringRef Name) {
+unsigned StringMapImpl::LookupBucketFor(StringRef Name,
+unsigned FullHashValue) {
   // Hash table unallocated so far?
   if (NumBuckets == 0)
 init(16);
-  unsigned FullHashValue = djbHash(Name, 0);
   unsigned BucketNo = FullHashValue & (NumBuckets - 1);
   unsigned *HashTable = getHashTable(TheTable, NumBuckets);
 
@@ -136,10 +135,9 @@
 /// FindKey - Look up the bucket that contains the specified key. If it exists
 /// in the map, return the bucket number of the key.  Otherwise return -1.
 /// This does not modify the map.
-int StringMapImpl::FindKey(StringRef Key) const {
+int StringMapImpl::FindKey(StringRef Key, unsigned FullHashValue) const {
   if (NumBuckets == 0)
 return -1; // Really empty table?
-  unsigned FullHashValue = djbHash(Key, 0);
   unsigned BucketNo = FullHashValue & (NumBuckets - 1);
   unsigned *HashTable = getHashTable(TheTable, NumBuckets);
 
Index: llvm/include/llvm/ADT/StringMap.h
===
--- llvm/include/llvm/ADT/StringMap.h
+++ llvm/include/llvm/ADT/StringMap.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/StringMapEntry.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/Support/AllocatorBase.h"
+#include "llvm/Support/DJB.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 #include 
 #include 
@@ -60,12 +61,20 @@
   /// specified bucket will be non-null.  Otherwise, it will be null.  In either
   /// case, the FullHashValue field of the bucket will be set to the hash value
   /// of the string.
-  unsigned LookupBucketFor(StringRef Key);
+  unsigned LookupBucketFor(StringRef Key) {
+return LookupBucketFor(Key, hash(Key));
+  }
+
+  /// Overload that explicitly takes precomputed hash(Key).
+  unsigned LookupBucketFor(StringRef Key, unsigned FullHashValue);
 
   /// FindKey - Look up the bucket that contains the specified key. If it exists
   /// in the map, return the bucket number of the key.  Otherwise return -1.
   /// This does not modify the map.
-  int FindKey(StringRef Key) const;
+  int FindKey(StringRef Key) const { return FindKey(Key, hash(Key)); }
+
+  /// Overload that explicitly takes precomputed hash(Key).
+  int FindKey(StringRef Key, unsigned FullHashValue) const;
 
   /// RemoveKey - Remove the specified StringMapEntry from the table, but do not
   /// delete it.  This aborts if the value isn't in the table.
@@ -94,6 +103,8 @@
   bool empty() const { return NumItems == 0; }
   unsigned size() const { return NumItems; }
 
+  static unsigned hash(StringRef Key) { return llvm::djbHash(Key, 0); }
+
   void swap(StringMapImpl &Other) {
 std::swap(TheTable, Other.TheTable);
 std::swap(NumBuckets, Other.NumBuckets);
@@ -215,15 +226,19 @@
   StringMapKeyIterator(end()));
   }
 
-  iterator find(StringRef Key) {
-int Bucket = FindKey(Key);
+  iterator find(StringRef Key) { return find(Key, hash(Key)); }
+
+  iterator find(StringRef Key, unsigned FullHashValue) {
+int Bucket = FindKey(Key, FullHashValue);
 if (Bucket == -1)
   return end();
 return iterator(TheTable + Bucket, true);
   }
 
-  const_iterator find(StringRef Key) const {
-int Bucket = FindKey(Key);
+  const_iterator find(StringRef Key) const { return find(Key, hash(Key)); }
+
+  const_ite

[Lldb-commits] [PATCH] D122975: parallelize module loading in DynamicLoaderPOSIXDYLD()

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

llunak created this revision.
llunak added reviewers: clayborg, labath.
llunak added a project: LLDB.
Herald added subscribers: usaxena95, JDevlieghere, kadircet.
Herald added a project: All.
llunak requested review of this revision.
Herald added subscribers: lldb-commits, ilya-biryukov.

If LLDB index cache is enabled and everything is cached, then loading of debug 
info is essentially single-threaded. This patch parallelizes module loading in 
DynamicLoaderPOSIXDYLD::LoadAllCurrentModules(), which may greatly reduce the 
load time if the debugged program uses a large number of binaries (as opposed 
to monolithic programs where this presumably doesn't make a difference). In my 
specific case of LibreOffice Calc this reduces startup time from 6s to 2s.

One problem with this change is that it creates a threadpool-within-threadpool 
situation in ManualDWARFIndex::Index(), and so the number of threads running at 
the same time may not be properly limited. I think the threadpool in 
ManualDWARFIndex::Index() is still useful even with this change because of the 
case of a monolithic program that's not cached.

If not limiting thread count properly is considered a problem, I think a 
solution to that is using a global semaphore to limit tasks from both places. 
There's a simple Semaphore class in clangd (with clangd-specific trace code, so 
I guess it'd need to be copy&pasted into lldb) and the semaphore object would 
need to be stored somewhere (and I don't know where).

I have not checked if all the relevant code is thread-safe, but since all of it 
is already running in another thread I assume that to be the case.

Presumably a similar change could be done for other platforms (I have no plans 
to do so).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122975

Files:
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp


Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -24,6 +24,7 @@
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/ProcessInfo.h"
+#include "llvm/Support/ThreadPool.h"
 
 #include 
 
@@ -602,9 +603,24 @@
   m_process->PrefetchModuleSpecs(
   module_names, m_process->GetTarget().GetArchitecture().GetTriple());
 
-  for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I) {
-ModuleSP module_sp =
+  std::vector infos;
+  for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I)
+infos.push_back(I);
+  std::vector loaded_modules;
+  loaded_modules.resize(infos.size());
+  llvm::ThreadPool pool(llvm::optimal_concurrency(infos.size()));
+  auto load_module_fn = [&](size_t idx) {
+DYLDRendezvous::iterator I = infos[idx];
+loaded_modules[idx] =
 LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
+  };
+  for (size_t i = 0; i < infos.size(); ++i)
+pool.async(load_module_fn, i);
+  pool.wait();
+
+  for (size_t i = 0; i < infos.size(); ++i) {
+DYLDRendezvous::iterator I = infos[i];
+ModuleSP module_sp = loaded_modules[i];
 if (module_sp.get()) {
   LLDB_LOG(log, "LoadAllCurrentModules loading module: {0}",
I->file_spec.GetFilename());


Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -24,6 +24,7 @@
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/ProcessInfo.h"
+#include "llvm/Support/ThreadPool.h"
 
 #include 
 
@@ -602,9 +603,24 @@
   m_process->PrefetchModuleSpecs(
   module_names, m_process->GetTarget().GetArchitecture().GetTriple());
 
-  for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I) {
-ModuleSP module_sp =
+  std::vector infos;
+  for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I)
+infos.push_back(I);
+  std::vector loaded_modules;
+  loaded_modules.resize(infos.size());
+  llvm::ThreadPool pool(llvm::optimal_concurrency(infos.size()));
+  auto load_module_fn = [&](size_t idx) {
+DYLDRendezvous::iterator I = infos[idx];
+loaded_modules[idx] =
 LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
+  };
+  for (size_t i = 0; i < infos.size(); ++i)
+pool.async(load_module_fn, i);
+  pool.wait();
+
+  for (size_t i = 0; i < infos.size(); ++i) {
+DYLDRendezvous::iterator I = infos[i];
+ModuleSP module_sp = loaded_modules[i];
 if (module_sp.get()) {
   LLDB_LOG(log, "LoadAllCurrentModules loading module: {0}",
I->file_spec.GetFilename());
___

[Lldb-commits] [PATCH] D122980: make ConstStringTable use std::unordered_map rather than std::map

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

llunak created this revision.
llunak added a reviewer: clayborg.
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.

The ordering is not needed, and std::unordered_map is faster (and std::hash for 
ConstString is trivial). I can measure time spent in the SaveToCache() calls 
reduced to ~50% during LLDB startup (and a ~25% reduction of the total startup 
cost).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122980

Files:
  lldb/include/lldb/Core/DataFileCache.h
  lldb/include/lldb/Utility/ConstString.h


Index: lldb/include/lldb/Utility/ConstString.h
===
--- lldb/include/lldb/Utility/ConstString.h
+++ lldb/include/lldb/Utility/ConstString.h
@@ -464,6 +464,16 @@
 }
 } // namespace llvm
 
+namespace std {
+
+template <> struct hash {
+  std::size_t operator()(const lldb_private::ConstString &str) const {
+return reinterpret_cast(str.GetCString());
+  }
+};
+
+} // namespace std
+
 LLVM_YAML_IS_SEQUENCE_VECTOR(lldb_private::ConstString)
 
 #endif // LLDB_UTILITY_CONSTSTRING_H
Index: lldb/include/lldb/Core/DataFileCache.h
===
--- lldb/include/lldb/Core/DataFileCache.h
+++ lldb/include/lldb/Core/DataFileCache.h
@@ -15,6 +15,7 @@
 #include "lldb/lldb-forward.h"
 #include "llvm/Support/Caching.h"
 #include 
+#include 
 
 namespace lldb_private {
 
@@ -190,7 +191,7 @@
 
 private:
   std::vector m_strings;
-  std::map m_string_to_offset;
+  std::unordered_map m_string_to_offset;
   /// Skip one byte to start the string table off with an empty string.
   uint32_t m_next_offset = 1;
 };


Index: lldb/include/lldb/Utility/ConstString.h
===
--- lldb/include/lldb/Utility/ConstString.h
+++ lldb/include/lldb/Utility/ConstString.h
@@ -464,6 +464,16 @@
 }
 } // namespace llvm
 
+namespace std {
+
+template <> struct hash {
+  std::size_t operator()(const lldb_private::ConstString &str) const {
+return reinterpret_cast(str.GetCString());
+  }
+};
+
+} // namespace std
+
 LLVM_YAML_IS_SEQUENCE_VECTOR(lldb_private::ConstString)
 
 #endif // LLDB_UTILITY_CONSTSTRING_H
Index: lldb/include/lldb/Core/DataFileCache.h
===
--- lldb/include/lldb/Core/DataFileCache.h
+++ lldb/include/lldb/Core/DataFileCache.h
@@ -15,6 +15,7 @@
 #include "lldb/lldb-forward.h"
 #include "llvm/Support/Caching.h"
 #include 
+#include 
 
 namespace lldb_private {
 
@@ -190,7 +191,7 @@
 
 private:
   std::vector m_strings;
-  std::map m_string_to_offset;
+  std::unordered_map m_string_to_offset;
   /// Skip one byte to start the string table off with an empty string.
   uint32_t m_next_offset = 1;
 };
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122980: make ConstStringTable use std::unordered_map rather than std::map

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I feel like this came up in the past and there was a reason an unordered map 
couldn't work? Maybe I'm confusing this with something else. Added Pavel as he 
would certainly know.

Could we use a `llvm::DenseMap` here, as it's supposedly even faster than its 
STL counterpart?

(Adding myself as a reviewer too so this shows up in my review queue)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122980

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

The ConstString/StringPool is pretty hot so I'm very excited about making it 
faster.

I'm slightly concerned about the two hash values (the "full" hash vs the single 
byte one). That's not something that was introduced in this patch, but passing 
it around adds an opportunity to get it wrong.

I'm wondering if we could wrap this in a struct and pass that around:

  struct HashedStringRef {
const llvm::StringRef str;
const unsigned full_hash;
const uint8_t hash; 
HashedStringRef(llvm::StringRef s) : str(s), full_hash(djbHash(str)), 
hash(hash(str)) {}
  }

It looks like we always need both except in the StringMap implementation, but 
if I'm reading the code correctly we'll have constructed it in ConstString 
already anyway?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D122944: [lldb] Prevent object file plugins from messing with the data buffer if they don't match

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 419993.
JDevlieghere added a comment.
Herald added subscribers: pmatos, asb, MaskRay, aheejin, sbc100, emaste.

Prevent object files from changing the buffer by passing it by value


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

https://reviews.llvm.org/D122944

Files:
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
  lldb/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  lldb/source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
  lldb/source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
  lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.h
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.h

Index: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.h
===
--- lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.h
+++ lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.h
@@ -30,7 +30,7 @@
   }
 
   static ObjectFile *
-  CreateInstance(const lldb::ModuleSP &module_sp, lldb::DataBufferSP &data_sp,
+  CreateInstance(const lldb::ModuleSP &module_sp, lldb::DataBufferSP data_sp,
  lldb::offset_t data_offset, const FileSpec *file,
  lldb::offset_t file_offset, lldb::offset_t length);
 
Index: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
===
--- lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
+++ lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
@@ -88,7 +88,7 @@
 }
 
 ObjectFile *
-ObjectFileWasm::CreateInstance(const ModuleSP &module_sp, DataBufferSP &data_sp,
+ObjectFileWasm::CreateInstance(const ModuleSP &module_sp, DataBufferSP data_sp,
offset_t data_offset, const FileSpec *file,
offset_t file_offset, offset_t length) {
   Log *log = GetLog(LLDBLog::Object);
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -62,7 +62,7 @@
   static llvm::StringRef GetPluginDescriptionStatic();
 
   static ObjectFile *
-  CreateInstance(const lldb::ModuleSP &module_sp, lldb::DataBufferSP &data_sp,
+  CreateInstance(const lldb::ModuleSP &module_sp, lldb::DataBufferSP data_sp,
  lldb::offset_t data_offset, const lldb_private::FileSpec *file,
  lldb::offset_t offset, lldb::offset_t length);
 
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -77,12 +77,10 @@
  "(32 and 64 bit)";
 }
 
-ObjectFile *ObjectFilePECOFF::CreateInstance(const lldb::ModuleSP &module_sp,
- DataBufferSP &data_sp,
- lldb::offset_t data_offset,
- const lldb_private::FileSpec *file_p,
- lldb::offset_t file_offset,
- lldb::offset_t length) {
+ObjectFile *ObjectFilePECOFF::CreateInstance(
+const lldb::ModuleSP &module_sp, DataBufferSP data_sp,
+lldb::offset_t data_offset, const lldb_private::FileSpec *file_p,
+lldb::offset_t file_offset, lldb::offset_t length) {
   FileSpec file = file_p ? *file_p : FileSpec();
   if (!data_sp) {
 data_sp = MapFileData(file, length, file_offset);
Index: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.h
===
--- lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.h
+++ lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.h
@@ -31,7 +31,7 @@
   loadPDBFile(std::string PdbPath, llvm::BumpPtrAllocator &Allocator);
 
   static ObjectFile *
-  CreateInstance(const lldb::ModuleSP &module_sp, lldb::DataBufferSP &data_sp,
+  CreateInstance(const lldb::ModuleSP &module_sp, lldb::DataBufferSP data_sp,
  lldb::offset_t data_offset, const FileSpec *file,
  lldb::offset_t file_offset, lldb::offset_t length);
 
Index: lldb/source/Plugins/

[Lldb-commits] [PATCH] D122980: make ConstStringTable use DenseMap rather than std::map

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

llunak updated this revision to Diff 419996.
llunak retitled this revision from "make ConstStringTable use 
std::unordered_map rather  than std::map" to "make ConstStringTable use 
DenseMap rather  than std::map".
llunak edited the summary of this revision.
llunak added a comment.

In D122980#3424636 , @JDevlieghere 
wrote:

> Could we use a `llvm::DenseMap` here, as it's supposedly even faster than its 
> STL counterpart?

Yes, it is, I've updated the patch.


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

https://reviews.llvm.org/D122980

Files:
  lldb/include/lldb/Core/DataFileCache.h


Index: lldb/include/lldb/Core/DataFileCache.h
===
--- lldb/include/lldb/Core/DataFileCache.h
+++ lldb/include/lldb/Core/DataFileCache.h
@@ -13,6 +13,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/UUID.h"
 #include "lldb/lldb-forward.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Caching.h"
 #include 
 
@@ -190,7 +191,7 @@
 
 private:
   std::vector m_strings;
-  std::map m_string_to_offset;
+  llvm::DenseMap m_string_to_offset;
   /// Skip one byte to start the string table off with an empty string.
   uint32_t m_next_offset = 1;
 };


Index: lldb/include/lldb/Core/DataFileCache.h
===
--- lldb/include/lldb/Core/DataFileCache.h
+++ lldb/include/lldb/Core/DataFileCache.h
@@ -13,6 +13,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/UUID.h"
 #include "lldb/lldb-forward.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Caching.h"
 #include 
 
@@ -190,7 +191,7 @@
 
 private:
   std::vector m_strings;
-  std::map m_string_to_offset;
+  llvm::DenseMap m_string_to_offset;
   /// Skip one byte to start the string table off with an empty string.
   uint32_t m_next_offset = 1;
 };
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 1e5083a - [trace][intel pt] Handle better tsc in the decoder

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2022-04-02T11:06:26-07:00
New Revision: 1e5083a563f8aca294feda60dcb4a814623ba321

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

LOG: [trace][intel pt] Handle better tsc in the decoder

A problem that I introduced in the decoder is that I was considering TSC 
decoding
errors as actual instruction errors, which mean that the trace has a gap. This 
is
wrong because a TSC decoding error doesn't mean that there's a gap in the trace.
Instead, now I'm just counting how many of these errors happened and I'm using
the `dump info` command to check for this number.

Besides that, I refactored the decoder a little bit to make it simpler, more
readable, and to handle TSCs in a cleaner way.

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

Added: 


Modified: 
lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
lldb/test/API/commands/trace/TestTraceDumpInfo.py

Removed: 




diff  --git a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp 
b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
index 4ef689499188f..994d068810f1e 100644
--- a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
@@ -86,12 +86,7 @@ IntelPTInstruction::GetControlFlowType(lldb::addr_t 
next_load_address) const {
 
 ThreadSP DecodedThread::GetThread() { return m_thread_sp; }
 
-void DecodedThread::AppendInstruction(const pt_insn &insn) {
-  m_instructions.emplace_back(insn);
-}
-
-void DecodedThread::AppendInstruction(const pt_insn &insn, uint64_t tsc) {
-  m_instructions.emplace_back(insn);
+void DecodedThread::RecordTscForLastInstruction(uint64_t tsc) {
   if (!m_last_tsc || *m_last_tsc != tsc) {
 // In case the first instructions are errors or did not have a TSC, we'll
 // get a first valid TSC not in position 0. We can safely force these error
@@ -103,11 +98,38 @@ void DecodedThread::AppendInstruction(const pt_insn &insn, 
uint64_t tsc) {
   }
 }
 
+void DecodedThread::AppendInstruction(const pt_insn &insn) {
+  m_instructions.emplace_back(insn);
+}
+
+void DecodedThread::AppendInstruction(const pt_insn &insn, uint64_t tsc) {
+  AppendInstruction(insn);
+  RecordTscForLastInstruction(tsc);
+}
+
 void DecodedThread::AppendError(llvm::Error &&error) {
   m_errors.try_emplace(m_instructions.size(), toString(std::move(error)));
   m_instructions.emplace_back();
 }
 
+void DecodedThread::AppendError(llvm::Error &&error, uint64_t tsc) {
+  AppendError(std::move(error));
+  RecordTscForLastInstruction(tsc);
+}
+
+void DecodedThread::LibiptErrors::RecordError(int libipt_error_code) {
+  libipt_errors[pt_errstr(pt_errcode(libipt_error_code))]++;
+  total_count++;
+}
+
+void DecodedThread::RecordTscError(int libipt_error_code) {
+  m_tsc_errors.RecordError(libipt_error_code);
+}
+
+const DecodedThread::LibiptErrors &DecodedThread::GetTscErrors() const {
+  return m_tsc_errors;
+}
+
 ArrayRef DecodedThread::GetInstructions() const {
   return makeArrayRef(m_instructions);
 }
@@ -194,4 +216,4 @@ Optional 
DecodedThread::TscRange::Prev() {
   auto prev_it = m_it;
   --prev_it;
   return TscRange(prev_it, *m_decoded_thread);
-}
\ No newline at end of file
+}

diff  --git a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h 
b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
index acb67738f3a7e..030fea0b2fca9 100644
--- a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
+++ b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
@@ -155,6 +155,15 @@ class DecodedThread : public 
std::enable_shared_from_this {
 const DecodedThread *m_decoded_thread;
   };
 
+  // Struct holding counts for libipts errors;
+  struct LibiptErrors {
+// libipt error -> count
+llvm::DenseMap libipt_errors;
+int total_count = 0;
+
+void RecordError(int libipt_error_code);
+  };
+
   DecodedThread(lldb::ThreadSP thread_sp);
 
   /// Utility constructor that initializes the trace with a provided error.
@@ -195,6 +204,17 @@ class DecodedThread : public 
std::enable_shared_from_this {
   ///   points to a valid instruction.
   const char *GetErrorByInstructionIndex(size_t ins_idx);
 
+  /// Append a decoding error with a corresponding TSC.
+  void AppendError(llvm::Error &&error, uint64_t TSC);
+
+  /// Record an error decoding a TSC timestamp.
+  ///
+  /// See \a GetTscErrors() for more documentation.
+  ///
+  /// \param[in] libipt_error_code
+  ///   An error returned by the libipt library.
+  void RecordTscError(int libipt_error_code);
+
   /// Get a new cursor for the decoded thread.
   lldb::TraceCursorUP GetCursor();
 
@@ -207,6 +227,14 @@ class

[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

[Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1e5083a563f8: [trace][intel pt] Handle better tsc in the 
decoder (authored by Walter Erquinigo ).

Changed prior to commit:
  https://reviews.llvm.org/D122867?vs=419813&id=420001#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122867

Files:
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/test/API/commands/trace/TestTraceDumpInfo.py

Index: lldb/test/API/commands/trace/TestTraceDumpInfo.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInfo.py
+++ lldb/test/API/commands/trace/TestTraceDumpInfo.py
@@ -41,4 +41,6 @@
   Raw trace size: 4 KiB
   Total number of instructions: 21
   Total approximate memory usage: 0.98 KiB
-  Average memory usage per instruction: 48.00 bytes'''])
+  Average memory usage per instruction: 48.00 bytes
+
+  Number of TSC decoding errors: 0'''])
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -105,24 +105,31 @@
 
 void TraceIntelPT::DumpTraceInfo(Thread &thread, Stream &s, bool verbose) {
   Optional raw_size = GetRawTraceSize(thread);
-  s.Printf("\nthread #%u: tid = %" PRIu64, thread.GetIndexID(), thread.GetID());
+  s.Format("\nthread #{0}: tid = {1}", thread.GetIndexID(), thread.GetID());
   if (!raw_size) {
-s.Printf(", not traced\n");
+s << ", not traced\n";
 return;
   }
-  s.Printf("\n");
-
-  size_t insn_len = Decode(thread)->GetInstructions().size();
-  size_t mem_used = Decode(thread)->CalculateApproximateMemoryUsage();
-
-  s.Printf("  Raw trace size: %zu KiB\n", *raw_size / 1024);
-  s.Printf("  Total number of instructions: %zu\n", insn_len);
-  s.Printf("  Total approximate memory usage: %0.2lf KiB\n",
+  s << "\n";
+  DecodedThreadSP decoded_trace_sp = Decode(thread);
+  size_t insn_len = decoded_trace_sp->GetInstructions().size();
+  size_t mem_used = decoded_trace_sp->CalculateApproximateMemoryUsage();
+
+  s.Format("  Raw trace size: {0} KiB\n", *raw_size / 1024);
+  s.Format("  Total number of instructions: {0}\n", insn_len);
+  s.Format("  Total approximate memory usage: {0:2} KiB\n",
(double)mem_used / 1024);
   if (insn_len != 0)
-s.Printf("  Average memory usage per instruction: %0.2lf bytes\n",
+s.Format("  Average memory usage per instruction: {0:2} bytes\n",
  (double)mem_used / insn_len);
-  return;
+
+  const DecodedThread::LibiptErrors &tsc_errors =
+  decoded_trace_sp->GetTscErrors();
+  s.Format("\n  Number of TSC decoding errors: {0}\n", tsc_errors.total_count);
+  for (const auto &error_message_to_count : tsc_errors.libipt_errors) {
+s.Format("{0}: {1}\n", error_message_to_count.first,
+ error_message_to_count.second);
+  }
 }
 
 Optional TraceIntelPT::GetRawTraceSize(Thread &thread) {
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -90,6 +90,59 @@
   return 0;
 }
 
+// Simple struct used by the decoder to keep the state of the most
+// recent TSC and a flag indicating whether TSCs are enabled, not enabled
+// or we just don't yet.
+struct TscInfo {
+  uint64_t tsc = 0;
+  LazyBool has_tsc = eLazyBoolCalculate;
+
+  explicit operator bool() const { return has_tsc == eLazyBoolYes; }
+};
+
+/// Query the decoder for the most recent TSC timestamp and update
+/// tsc_info accordingly.
+void RefreshTscInfo(TscInfo &tsc_info, pt_insn_decoder &decoder,
+DecodedThread &decoded_thread) {
+  if (tsc_info.has_tsc == eLazyBoolNo)
+return;
+
+  uint64_t new_tsc;
+  if (int tsc_error = pt_insn_time(&decoder, &new_tsc, nullptr, nullptr)) {
+if (tsc_error == -pte_no_time) {
+  // We now know that the trace doesn't support TSC, so we won't try again.
+  // See
+  // https://github.com/intel/libipt/blob/master/doc/man/pt_qry_time.3.md
+  tsc_info.has_tsc = eLazyBoolNo;
+} else {
+  // We don't add TSC decoding errors in the decoded trace itself to prevent
+  // creating unnecessary gaps, but we can count how many of these errors
+  // happened. In this case we reuse the previous correct TSC we saw, as
+  // it's better than no TSC at all.
+  decoded_thread.RecordTscError(tsc_error);
+}
+  } else {
+tsc_info.tsc = new_tsc;
+tsc_info.has_tsc = eLazyBoolYes;
+  }
+}
+
+static void AppendError(DecodedThrea

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

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

llunak added a comment.

In D122974#3424654 , @JDevlieghere 
wrote:

> I'm slightly concerned about the two hash values (the "full" hash vs the 
> single byte one). That's not something that was introduced in this patch, but 
> passing it around adds an opportunity to get it wrong.

If this is aimed at me, then I don't know what you mean here. The single-byte 
hash is not passed around, it's used only locally in the two places that my 
change modifies and one more place where StringMap is not accessed (so no full 
hash needed there). I see no easy way for it to go wrong, and IMO the 
HashedStringRef struct would just complicate the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

LGTM from a LLDB perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122974

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