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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
