[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

lgtm. The reason I requested this to be put separately, is because it is implemented in a way that kicks in even without minidebuginfo. I think this is fine, because entries can be removed from the symtab even without the whole minidebuginfo business. While this format will likely be the only real user of these partial symtabs, in theory, there isn't anything stopping someone from removing symtab entries independently of that. While unlikely, I see no harm in supporting that, if it does not incur any extra maintenance costs.



================
Comment at: lldb/lit/Modules/ELF/load-from-dynsym-alone.test:14
+
+# We want to keep the symbol "functionInDynsym" in the .dynamic section and not
+# have it put the default .symtab section.
----------------
s/.dynamic/.dynsym/


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2650-2652
+    // One exception to the above rule is when we have minidebuginfo embedded
+    // into a compressed .gnu_debugdata section. This section contains a .symtab
+    // from which all symbols already contained in the .dynsym are stripped.
----------------
How about we make this less layered, and rephrase the existing comment a bit: "Information in the dynsym section is *usually* also found in the symtab, but this is not required as symtab entries can be removed after linking. The minidebuginfo format makes use of this facility to create smaller symbol tables.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2668-2671
+      if (!m_symtab_up) {
+        auto sec = symtab ? symtab : dynsym;
+        m_symtab_up.reset(new Symtab(sec->GetObjectFile()));
+      }
----------------
I wouldn't bother with this. You can just unconditionally create a Symtab object before you start parsing any symbol tables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390