bulbazord added a comment.
This patch effectively introduces a file format to cache lldb internal state.
While the tests and the code do give some information for what it looks like,
some documentation about this format would be nice.
I like this idea a lot! My comments were kind of surface level but should help
fix some of the rough edges of the patch.
================
Comment at: lldb/include/lldb/Core/Module.h:879
StatsDuration &GetSymtabParseTime() { return m_symtab_parse_time; }
-
/// Accessor for the symbol table index time metric.
----------------
nit: unnecessary whitespace change
================
Comment at: lldb/include/lldb/Core/Module.h:951
+ ///
+ /// The hash should be enough to indentify the file on disk and the
+ /// architecture of the file. If the module represents an object inside of a
----------------
typo: `indentify` -> `identify`
================
Comment at: lldb/include/lldb/Core/Module.h:959
+ /// from the same file
+ /// - a .o file inside of a BSD archive. Each .o file will have a object name
+ /// and object offset that should produce a unique hash. The object offset
----------------
`have a object name` -> `have an object name`
================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:710-713
+ lldb::addr_t m_file_offset;
+ /// The length of this object file if it is known. This can be zero if length
+ /// is unknown or can't be determined.
+ lldb::addr_t m_length;
----------------
Curious to know, why `lldb::addr_t` instead of something like `lldb::offset_t`?
================
Comment at: lldb/include/lldb/Symbol/Symbol.h:21-22
+ class FileWriter;
+}
+}
+
----------------
nit: you have a namespace comment when you do this in an earlier file but not
here. what does clang-format do?
================
Comment at: lldb/source/Core/Mangled.cpp:411-412
+ MangledOnly = 2u,
+ /// If we have a Mangled object with two different names that are not related
+ /// then we need to save both strings.
+ MangledAndDemangled = 3u
----------------
What does "two different names that are not related" mean here?
================
Comment at: lldb/source/Core/Module.cpp:65
+
#include <cassert>
----------------
nit: unrelated whitespace change
================
Comment at: lldb/source/Core/Module.cpp:1706
+ // Append the object name to the path as a directory. The object name will be
+ // valie if we have an object file from a BSD archive like "foo.a(bar.o)".
+ if (m_object_name)
----------------
typo: `valie` -> `valid`
================
Comment at: lldb/source/Host/common/FileSystem.cpp:134
+ return false;
+ auto file_or_err = Open(file_spec, File::OpenOptions::eOpenOptionReadWrite,
lldb::eFilePermissionsUserRead | lldb::eFilePermissionsUserWrite,
/*should_close_fd=*/true);
+ if (file_or_err)
----------------
Could you break this line into multiple lines?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113789/new/
https://reviews.llvm.org/D113789
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
