Fix lookup of symbols at the same address with no size vs. size

[jankratochvil]

Summary

The Fedora problem has been fixed by D63540.
But as reported by @omjavaid it regressed arm32: Ubuntu Xenial, Bionic and Debian Buster
I found it also reproducible with on Fedora in chroot with ubuntu-18.04-server-cloudimg-armhf.
The regression is due to:

GetAddressClass fails to recognized 0x102f0 as a code address:

PASS:
(lldb) p (void)sync()
GetAddressClass:0x102f1
GetAddressClass:0x102f1=(null) ValueIsAddress=1 section_type=1
GetAddressClass:0x96040
GetAddressClass:0x96040=__mmap ValueIsAddress=1 section_type=1
GetAddressClass:0x102f1
GetAddressClass:0x102f1=(null) ValueIsAddress=1 section_type=1
GetAddressClass:0x102f0
GetAddressClass:0x102f0=(null) ValueIsAddress=1 section_type=1
...

FAIL:
(lldb) p (void)sync()
GetAddressClass:0x102f1
GetAddressClass:0x102f1=_start ValueIsAddress=1 section_type=1
GetAddressClass:0x96040
GetAddressClass:0x96040=__mmap ValueIsAddress=1 section_type=1
GetAddressClass:0x102f1
GetAddressClass:0x102f1=_start ValueIsAddress=1 section_type=1
GetAddressClass:0x102f0
...

That is due to:

symtab.fail:[   11]     12     Invalid         0x00000000000102f0                    0x0000000000000000 0x00000003
symtab.fail:[   66]     99   X Code            0x00000000000102f0                    0x0000000000000030 0x00000012 _start
symtab.pass:[   11]     12     Invalid         0x00000000000102f0                    0x0000000000000030 0x00000003
symtab.pass:[   66]     99   X Code            0x00000000000102f0                    0x0000000000000030 0x00000012 _start

The difference is in the 'Invalid' symbol which is:

Num:    Value  Size Type    Bind   Vis      Ndx Name
 12: 000102f0     0 SECTION LOCAL  DEFAULT   12

Apparently ARM32 relies on that section symbol to have proper size. I do not see how Symtab::InitAddressIndexes could handle STT_SECTION in a special way as that is ELF-type specific Symbol characteristics:

uint32_t m_flags; // A copy of the flags from the original symbol table, the
                  // ObjectFile plug-in can interpret these

[clayborg]

lldb/source/Symbol/Symtab.cpp

898–900

So if we have N symbols for address 0x1000 with size of zero and we have one with a valid size, do we leave all symbols with zero size stay with a size of zero? I was confused by the comment and wanted to clarify.

[jankratochvil]

lldb/source/Symbol/Symtab.cpp

898–900

Yes. It sets size for at most one of the zero-sized symbols and it does that only in the case there is no sizeful symbol between them.
Maybe it should set all the zero-sized symbols in such case.
But that still would not help the arm32 regression as in that case there is one section-type zero-sized symbol and one sizeful regular symbol.

[labath]

Apparently ARM32 relies on that section symbol to have proper size. I do not see how Symtab::InitAddressIndexes could handle STT_SECTION in a special way as that is ELF-type specific Symbol characteristics:

Right, so I think that the thing we need to fix is the ObjectFileELF::GetAddressClass implementation. There's no requirement that this function needs to be implemented on top of the Symtab class. In fact, this isn't really true even now, as the function falls back on the hand-maintained map in m_address_class_map if the base implementation fails. Maybe we ought to move this logic up front, so that it takes precedence over the base implementation? I'm not sure about that, as I am not that familiar with how the arm/thumb determination is supposed to work, but @omjavaid might be able to help here: How are the arm/thumb mapping symbols supposed to work exactly? What's the "official" algorithm for checking whether a piece of the .text section is arm or thumb?

lldb/lit/SymbolFile/Inputs/sizeless-symbol.s

+        .text
+        .byte   0
+sizeless:
+sizeful:
+        .byte   0
+        .byte   0
+sizeend:
+        .size   sizeful, sizeend - sizeful

lldb/lit/SymbolFile/sizeless-symbol.test

+# Some targets do not have the .size directive.
+# RUN: %clang -target x86_64-unknown-unknown-elf %S/Inputs/sizeless-symbol.s -c -o %t.o
+# RUN: %lldb %t.o -s %s -o quit | FileCheck %s
+
+image lookup --address 1
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful
+image lookup --address 2
+# CHECK: Summary: sizeless-symbol.test.tmp.o`sizeful + 1
+image dump symtab
+# CHECK:     Index   UserID DSX Type            File Address/Value Load Address       Size               Flags      Name
+# CHECK-NEXT:------- ------ --- --------------- ------------------ ------------------ ------------------ ---------- ----------------------------------
+# CHECK-NEXT:[    0]      1     Code            0x0000000000000003                    0x0000000000000000 0x00000000 sizeend
+# CHECK-NEXT:[    1]      2     Code            0x0000000000000001                    0x0000000000000002 0x00000000 sizeful
+# CHECK-NEXT:[    2]      3     Code            0x0000000000000001                    0x0000000000000000 0x00000000 sizeless

lldb/source/Symbol/Symtab.cpp

if (num_entries > 0) {
 
       // Iterate through the FileRangeToIndexMap and fill in the size for any
       // entries that didn't already have a size from the Symbol (e.g. if we
       // have a plain linker symbol with an address only, instead of debug info
       // where we get an address and a size and a type, etc.)
       for (size_t i = 0; i < num_entries; i++) {
         FileRangeToIndexMap::Entry *entry =
             m_file_addr_to_index.GetMutableEntryAtIndex(i);
-        if (entry->GetByteSize() == 0) {
-          addr_t curr_base_addr = entry->GetRangeBase();
+        if (entry->GetByteSize() > 0)
+          continue;
+        // Symbols with non-zero size will show after zero-sized symbols on the
+        // 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 =
               section_ranges.FindEntryThatContains(curr_base_addr);
 
           // Use the end of the section as the default max size of the symbol
           addr_t sym_size = 0;
           if (containing_section) {
             sym_size =
                 containing_section->GetByteSize() -