[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Bunch of random nits.

Looks like a great addition. I know debug load time is a common annoyance.



================
Comment at: lldb/docs/use/ondemand.rst:1
+On Demand Symbols
+=================
----------------
If you can find a logical place for it, maybe define what "hydration" means in this context.

Developers can find the meaning from the source code but if "hydration" appears in logs it would be good for users to have an idea what that means.

Something like "the process of <whatever hydration means> (referred to internally as "hydration")".


================
Comment at: lldb/docs/use/ondemand.rst:92
+symbol tables. This can cause breakpoint setting by function name to fail when
+previosly it wouldn't fail.
+
----------------
"previously"


================
Comment at: lldb/include/lldb/Symbol/SymbolFileOnDemand.h:31
+/// symbols. Any client can on demand hydrate the underlying
+/// SymbolFile via SetForceLoad() API.
+class SymbolFileOnDemand : public lldb_private::SymbolFile {
----------------
Where is this call defined, and by API do you mean API like https://lldb.llvm.org/design/sbapi.html or API as in API of the SymbolFile class?


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:47
+lldb::LanguageType SymbolFileOnDemand::ParseLanguage(CompileUnit &comp_unit) {
+  if (!this->m_debug_info_enabled) {
+    Log *log = GetLog();
----------------
Is there a specific reason to use "this" explicitly instead of just `!m_debug_info_enabled`?

Something to do with the wrapping of the underlying SymbolFile perhaps.


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:54
+        LLDB_LOG(log, "Language {0} would return if hydrated.",
+                 eLanguageTypeUnknown);
+    }
----------------
This should be `langType` shouldn't it?


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:110
+             __FUNCTION__);
+    // Not early exit.
+    return false;
----------------
What's the meaning of this comment?


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:133
+      if (optimized) {
+        LLDB_LOG(log, "Optimized would return if hydrated.");
+      }
----------------
Would return <some value> if hydrated?


================
Comment at: lldb/test/Shell/SymbolFile/OnDemand/symbolic-breakpoint.test:1
+
+# Test shows that symbolic function breakpoint works with LLDB on demand symbol loading.
----------------
Stray newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631