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

Tue Sep 10 12:42:29 PDT 2019

JDevlieghere added inline comments.

================
Comment at: lldb/lit/Modules/ELF/load-from-dynsym-alone.test:24
+# Remove functionInDynsym symbol from .symtab (will leave symbol in .dynsym intact)
+# RUN: llvm-strip --strip-symbol=functionInDynsym %t.binary
+
----------------
Please also add `llvm-strip` to the list of support tools (`toolchain.py:127`).

================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2645

     // Sharable objects and dynamic executables usually have 2 distinct symbol
     // tables, one named ".symtab", and the other ".dynsym". The dynsym is a
----------------
Can you motivate the need for this change? This comment seems to suggest that reading the symtab table should be sufficient as it should contain all the information from the dynsym. If that is not true, it would be worth updating this comment. 

================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2657

+    // The symtab section is non-allocable and can be stripped. If both, .symtab
+    // and .dynsym exist, we load both. And if only .dymsym exists, we load it
----------------
Why did you remove the last part of the original comment? This seemed to be the most useful part... The newly added sentences explain what we are doing (which is relatively clear from the code). I'd rather see a comment explaining "why" something needs to happen. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390