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