[Lldb-commits] [PATCH] D63540: Fix lookup of symbols at the same address with no size vs. size

Mon Jul 1 02:13:12 PDT 2019

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Looks good to me, but I'd suggest waiting a while to give @clayborg a chance to respond...

================
Comment at: lldb/lit/SymbolFile/sizeless-symbol.test:4
+
+image lookup --address 1
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful
----------------
jankratochvil wrote:
> labath wrote:
> > As you're fixing this bug by changing the way symbol size is computed, it would be nice to also put (and check it's output) a "image dump symtab" command here. That would also make it clear what are the assumptions the following "lookup" commands are operating under.
> I only hope the `image dump symtab` output ordering is stable.
I'm pretty sure these are printed in the same order as present in .symtab. I'd be more worried about the order in which the symbols get emitted into the symtab in the first place. Theoretically this might change due to some patches to the assembler, but it should not change spuriously...

================
Comment at: lldb/lit/SymbolFile/sizeless-symbol.test:10
+image dump symtab
+# CHECK:Index   UserID DSX Type            File Address/Value Load Address       Size               Flags      Name
+# CHECK-NEXT:------- ------ --- --------------- ------------------ ------------------ ------------------ ---------- ----------------------------------
----------------
Nit: Please line up this table by inserting a couple of spaces between ":" and "Index".

================
Comment at: lldb/source/Symbol/Symtab.cpp:904
+        // same address. So do not set size of a non-last zero-sized symbol.
+        if (i == num_entries - 1 || m_file_addr_to_index.GetMutableEntryAtIndex(i + 1)->GetRangeBase() != curr_base_addr) {
           const RangeVector<addr_t, addr_t>::Entry *containing_section =
----------------
Please clang-format this.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63540