[Lldb-commits] [PATCH] D115324: Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster.

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

The patch is slightly larger than I would prefer for a through review, but here's my first pass at it. I appreciate the difficulties in bootstrapping something like this incrementally, but I do see at least a couple of opportunities to make things smaller. I think the DataEncoder, FileSystem, Symbol, and Mangled changes could go into separate patch(es). I don't know if it's feasible to separate the construction of the symtab cache content, from the actual act of storing it in the cache, but if those two processes could be separated, it would help things a lot.



================
Comment at: lldb/source/Host/common/FileSystem.cpp:523
+  Status error;
+  if (IsDirectory(path))
+    error.SetErrorStringWithFormatv("\"{0}\" is a directory",
----------------
What's the purpose of this check ?


================
Comment at: lldb/source/Symbol/Symbol.cpp:645
+  file.AppendU16(m_type_data);
+  file.AppendU16((&m_type_data)[1]);
+  m_mangled.Encode(file, strtab);
----------------
This is quite dodgy. It would be better to change the storage so that we don't have to do this. Perhaps something like
```
union {
  uint16_t flag_storage;
  struct {
    uint16_t m_type_data_resolved : 1;
    ...
  };
};
```


================
Comment at: lldb/source/Symbol/Symtab.cpp:1220
+
+static const llvm::StringRef kIdentifierCStrMap("CMAP");
+
----------------
constexpr llvm::StringLiteral


================
Comment at: lldb/source/Symbol/Symtab.cpp:1256
+
+#define CURRENT_CACHE_VERSION 1
+/// The encoding format for the symbol table is as follows:
----------------
constexpr uint32_t


================
Comment at: lldb/source/Symbol/Symtab.cpp:1278-1282
+    // No object file, this case is used for unit testing only.
+    CacheSignature signature;
+    signature.m_mod_time = 123;
+    if (!signature.Encode(encoder))
+      return false;
----------------
I am troubled by code like this. Have you tried creating a mock object file in the test?


================
Comment at: lldb/source/Symbol/Symtab.cpp:1364
+    const uint8_t num_cstr_maps = data.GetU8(offset_ptr);
+    for (uint8_t i=0; i<num_cstr_maps; ++i) {
+      uint8_t type = data.GetU8(offset_ptr);
----------------
please clang-format the patch


================
Comment at: lldb/source/Utility/DataFileCache.cpp:10-13
+#include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleList.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Symbol/ObjectFile.h"
----------------
These are not appropriate dependencies for code in the utility library. Either reimplement it without them or move the class somewhere else.


================
Comment at: lldb/source/Utility/DataFileCache.cpp:51
+
+  auto add_buffer = [&](unsigned task, std::unique_ptr<llvm::MemoryBuffer> m) {
+    if (m_take_ownership)
----------------
`[&]` on a lambda that outlives the enclosing function is dangerous.


================
Comment at: lldb/source/Utility/DataFileCache.cpp:61-64
+    std::string error = toString(cache_or_err.takeError());
+    LLDB_LOG(log,
+             "failed to create lldb index cache directory \"{0}\": {1}",
+             path, error);
----------------
there's an LLDB_LOG_ERROR macro for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115324