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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 220273.
kwk added a comment.

- [LLDB][ELF] Fixup for comments in D67390 
- Change test expectation to find 2 instead of 1 symbol


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390

Files:
  lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c
  lldb/lit/Modules/ELF/Inputs/load-symtab-and-dynsym.c
  lldb/lit/Modules/ELF/load-from-dynsym-alone.test
  lldb/lit/Modules/ELF/load-symtab-and-dynsym.test
  lldb/lit/helper/toolchain.py
  
lldb/packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/unittests/Target/ModuleCacheTest.cpp

Index: lldb/unittests/Target/ModuleCacheTest.cpp
===
--- lldb/unittests/Target/ModuleCacheTest.cpp
+++ lldb/unittests/Target/ModuleCacheTest.cpp
@@ -129,7 +129,7 @@
   ASSERT_TRUE(bool(module_sp));
 
   SymbolContextList sc_list;
-  EXPECT_EQ(1u, module_sp->FindFunctionSymbols(ConstString("boom"),
+  EXPECT_EQ(2u, module_sp->FindFunctionSymbols(ConstString("boom"),
eFunctionNameTypeFull, sc_list));
   EXPECT_STREQ(GetDummyRemotePath().GetCString(),
module_sp->GetPlatformFileSpec().GetCString());
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -2644,24 +2644,33 @@
 
 // Sharable objects and dynamic executables usually have 2 distinct symbol
 // tables, one named ".symtab", and the other ".dynsym". The dynsym is a
-// smaller version of the symtab that only contains global symbols. The
-// information found in the dynsym is therefore also found in the symtab,
-// while the reverse is not necessarily true.
+// smaller version of the symtab that only contains global symbols.
+// 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.
 Section *symtab =
 section_list->FindSectionByType(eSectionTypeELFSymbolTable, true).get();
-if (!symtab) {
-  // The symtab section is non-allocable and can be stripped, so if it
-  // doesn't exist then use the dynsym section which should always be
-  // there.
-  symtab =
-  section_list->FindSectionByType(eSectionTypeELFDynamicSymbols, true)
-  .get();
-}
 if (symtab) {
   m_symtab_up.reset(new Symtab(symtab->GetObjectFile()));
   symbol_id += ParseSymbolTable(m_symtab_up.get(), symbol_id, symtab);
 }
 
+// The symtab section is non-allocable and can be stripped, while the dynsym
+// section which should always be always be there. If both exist we load
+// both to support the minidebuginfo case. Otherwise we just load the dynsym
+// section.
+Section *dynsym =
+section_list->FindSectionByType(eSectionTypeELFDynamicSymbols, true)
+.get();
+if (dynsym) {
+  if (!m_symtab_up) {
+auto sec = symtab ? symtab : dynsym;
+m_symtab_up.reset(new Symtab(sec->GetObjectFile()));
+  }
+  symbol_id += ParseSymbolTable(m_symtab_up.get(), symbol_id, dynsym);
+}
+
 // DT_JMPREL
 //  If present, this entry's d_ptr member holds the address of
 //  relocation
Index: lldb/packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
===
--- lldb/packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
+++ lldb/packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
@@ -273,7 +273,7 @@
 "a_function should not exist yet",
 error=True,
 matching=False,
-patterns=["1 match found"])
+patterns=["2 matches found"])
 
 # Use lldb 'process load' to load the dylib.
 self.expect(
@@ -298,7 +298,7 @@
 "image lookup -n a_function",
 "a_function should now exist",
 patterns=[
-"1 match found .*%s" %
+"2 matches found .*%s" %
 dylibName])
 
 # Use lldb 'process unload' to unload the dylib.
Index: lldb/lit/helper/toolchain.py
===
--- lldb/lit/helper/toolchain.py
+++ lldb/lit/helper/toolchain.py
@@ -126,6 +126,6 @@
 
 support_tools = ['yaml2obj', 'obj2yaml', 'llvm-pdbutil',
  'llvm-mc', 'llvm-readobj', 'llvm-objdump',
- 'llvm-objcopy', 'lli']
+

[Lldb-commits] [PATCH] D66791: [lldb][ELF] Read symbols from .gnu_debugdata sect.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/lit/Modules/ELF/minidebuginfo-corrupt-xz.yaml:1
+# REQUIRES: system-linux, lzma
+

kwk wrote:
> labath wrote:
> > system-linux shouldn't be required here. One of the reasons I wanted to 
> > have these tests is that they can run on any system, not just on linux.
> @labath what about the other file 
> `minidebuginfo-set-and-hit-breakpoint.test`. Remove `system-linux` from there 
> as well?
You need to restrict that test somehow, because it only has a chance for 
working on elf platforms (as you're compiling binaries for the host and running 
them). It probably *should* work on all elf platforms, but we currently don't 
have a good way of expressing that and we're just restricting to system-linux 
currently.

If you feel adventurous you can try introducing a `system-elf` feature (or 
something), and then changing this test (and possibly others) to use that 
instead, but please do that as a separate patch.



Comment at: lldb/source/Host/common/LZMA.cpp:129
+  uint64_t uncompressedSize = 0;
+  auto err = getUncompressedSize(InputBuffer, uncompressedSize);
+  if (err)

kwk wrote:
> labath wrote:
> > MaskRay wrote:
> > > if (auto err = ...)
> > >   return err;
> > I have a feeling this pattern is actually more common in lldb. I tend to 
> > use this one, but I am fine with either.
> So what now? I had `getCompressedSize` return an error before and changed it 
> upon request to expected. I can live with both but like the expected thing a 
> bit better in terms of: this goes in and this comes out mentality.
My interpretation of @MaskRay's comment was that he wanted you do write 
something like 
```
if (auto err = uncompressedSize.takeError())
  return err;
```
That is orthogonal to what type is returned by `getCompressedSize`, and is 
indeed a more common pattern in at least some parts of llvm. I am personally 
fine with both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits