Don't create sections for SHN_ABS symbols in ELF files.

[clayborg]

Summary

Symbols that have the section index of SHN_ABS were previously creating extra top level sections that contained the value of the symbol as if the symbol's value was an address. As far as I can tell, these symbol's values are not addresses, even if they do have a size. To make matters worse, adding these extra sections can stop address lookups from succeeding if the symbol's value + size overlaps with an existing section as these sections get mapped into memory when the image is loaded by the dynamic loader. This can cause stack frames to appear empty as the address lookup fails completely.

This patch:

• doesn't create a section for any SHN_ABS symbols
• makes symbols that are absolute have values that are not addresses
• add accessors to SBSymbol to get the value and size of a symbol as raw integers. Prevoiusly there was no way to access a symbol's value from a SBSymbol because the only accessors were:
  
  SBAddress SBSymbol::GetStartAddress(); SBAddress SBSymbol::GetEndAddress();
  
  and these accessors would return an invalid SBAddress if the symbol's value wasn't an address
• Adds a test to ensure no ".absolute.<symbol-name>" sections are created
• Adds a test to test the new SBSymbol APIs

[labath]

I have somewhat mixed feelings about this.

On one hand, I was never too happy with how we're creating these funny extra sections, so I can't say I would be sad to see that go. OTOH, I can't really agree with the assertion that these symbols do not represent "addresses". All the ELF spec says about them is that they are not subject to relocation. That makes them rather ill-suited for describing the locations of objects in the usual scenarios. However, these symbols can be (and, I believe, are) used in the embedded world for describing objects that are architecturally defined to reside at a particular memory address (the 16-bit dos/bios world was full of those). However, one can imagine something similar being done in userspace as well (for example, if the kernel provides some data at a fixed virtual address). We envisioned something similar when we added support for this. In our case, the object/function in question was generated by a JIT, which knew the exact address at which it placed the jitted code.

However, AFAIK, that project never materialized, and we are not relying on these symbols for that. Tagging @dsrbecky to confirm. Assuming that's the case, and given that gdb does not handle these symbols in this way either (at least, I haven't been able to make it do that), I think we can go forward with removing them.

lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py

28–29

As I said in the overall comment, I cannot agree with this statement without reservations.

And in any case, I think prose like this is better left for the commit message. The test should just state what it does (although that's fairly obvious here), not what it used to do.

76–80

This check isn't particularly useful, since you've completely deleted the code which created those sections. If something similar is ever reintroduced, it's quite possible it would use a different section name, and so it wouldn't catch this.

I don't think it's necessary, but if you want, I think it'd be better to run something like image lookup -a 0xffffffff80000000 and check that it does not produce any matches, as that's what you're really interested in.

[clayborg]

In D131705#3726248, @labath wrote:

I have somewhat mixed feelings about this.

On one hand, I was never too happy with how we're creating these funny extra sections, so I can't say I would be sad to see that go. OTOH, I can't really agree with the assertion that these symbols do not represent "addresses". All the ELF spec says about them is that they are not subject to relocation. That makes them rather ill-suited for describing the locations of objects in the usual scenarios. However, these symbols can be (and, I believe, are) used in the embedded world for describing objects that are architecturally defined to reside at a particular memory address (the 16-bit dos/bios world was full of those). However, one can imagine something similar being done in userspace as well (for example, if the kernel provides some data at a fixed virtual address). We envisioned something similar when we added support for this. In our case, the object/function in question was generated by a JIT, which knew the exact address at which it placed the jitted code.

I am fine trying to see if these symbols exist within an existing section and if they do, then resolve the symbol as a section + offset within an existing section value if you think that is the way to go. If they don't exist in any sections, then do what I am doing by just making them have no section. But really, why would you ever use a SHN_ABS symbol when you can make a real symbol that _is_ defined to be in a section? That is what puzzled me. The documentation in the ELF was not helpful, as you stated it just says these symbols have no relocations. but if you do have such a symbol, then if the section it does exist in does get relocated or moved at runtime, what good is the symbol and why not create an actual symbol in an actual section?

However, AFAIK, that project never materialized, and we are not relying on these symbols for that. Tagging @dsrbecky to confirm. Assuming that's the case, and given that gdb does not handle these symbols in this way either (at least, I haven't been able to make it do that), I think we can go forward with removing them.

When you say "we can go forward with removing them", do you mean not add them at all as symbols? I wouldn't want to remove a symbol, I would rather keep it like I have added it now if possible.

[clayborg]

lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py

28–29

I can remove this part of the comment.

76–80

Sounds good as long as we don't start trying to resolve the symbol as mentioned

[labath]

In D131705#3726772, @clayborg wrote:

In D131705#3726248, @labath wrote:

I have somewhat mixed feelings about this.

On one hand, I was never too happy with how we're creating these funny extra sections, so I can't say I would be sad to see that go. OTOH, I can't really agree with the assertion that these symbols do not represent "addresses". All the ELF spec says about them is that they are not subject to relocation. That makes them rather ill-suited for describing the locations of objects in the usual scenarios. However, these symbols can be (and, I believe, are) used in the embedded world for describing objects that are architecturally defined to reside at a particular memory address (the 16-bit dos/bios world was full of those). However, one can imagine something similar being done in userspace as well (for example, if the kernel provides some data at a fixed virtual address). We envisioned something similar when we added support for this. In our case, the object/function in question was generated by a JIT, which knew the exact address at which it placed the jitted code.

I am fine trying to see if these symbols exist within an existing section and if they do, then resolve the symbol as a section + offset within an existing section value if you think that is the way to go. If they don't exist in any sections, then do what I am doing by just making them have no section. But really, why would you ever use a SHN_ABS symbol when you can make a real symbol that _is_ defined to be in a section? That is what puzzled me. The documentation in the ELF was not helpful, as you stated it just says these symbols have no relocations. but if you do have such a symbol, then if the section it does exist in does get relocated or moved at runtime, what good is the symbol and why not create an actual symbol in an actual section?

I think that the reason this doesn't make sense is that the initial assumption (every symbol must reside in a section) is incorrect. That needs to be true if you want to relocate symbols, as relocation works on the level of sections. However, if the symbols value is not supposed to be affected by relocations, then it doesn't make sense for it to be in any particular section -- that's why elf introduces the special SHN_ABS "section" index. After everything is relocated and loaded into memory, it may end up pointing to some loaded section (as known to the tooling), or it may not. It does not make sense to ask the question "what happens if the section the symbol is in is relocated" because the symbol creator explicitly said it does not want the symbols value to be affected by relocation nor for them to be associated with any particular section.

The reason why we introduced these fake sections is that lldb also assumed (and I don't really blame it) that every resolvable address must belong to a section, and we wanted to be able to resolve address that are described by these symbols. However, I have now confirmed that this functionality is not used (and I suspect the reason was that other tools were having problems understanding this as well), so I think it's fine to remove it.

However, AFAIK, that project never materialized, and we are not relying on these symbols for that. Tagging @dsrbecky to confirm. Assuming that's the case, and given that gdb does not handle these symbols in this way either (at least, I haven't been able to make it do that), I think we can go forward with removing them.

When you say "we can go forward with removing them", do you mean not add them at all as symbols? I wouldn't want to remove a symbol, I would rather keep it like I have added it now if possible.

I meant removing the extra fake sections. I agree that keeping the symbols is desirable.

[alvinhochun]

lldb/source/Commands/CommandObjectTarget.cpp

1550

This IndentMore added is missing a matching IndentLess call.

1551–1556

May I ask, is it expected for this to print only the value and size but not the name of the symbol?

Also, in some COFF binaries linked by LLD there are a bunch of absolute symbols (e.g. __guard_flags) and when they hit this code path all they print is Value: 0xffffffffffffffff.

llvm-readobj shows the symbol as:

Symbol {
  Name: __guard_flags
  Value: 2147550464
  Section: IMAGE_SYM_ABSOLUTE (-1)
  BaseType: Null (0x0)
  ComplexType: Null (0x0)
  StorageClass: External (0x2)
  AuxSymbolCount: 0
}

Though neither values are correct. I expect the value to be 0x10500. I haven't looked into this yet. Any idea what might be going on?

[clayborg]

lldb/source/Commands/CommandObjectTarget.cpp

1550

Yes, a fix is needed. Feel free to post a patch, or I will try to get to this soon.

1551–1556

We should be printing the name. I guess in the "symbol->ValueIsAddress()" case it would print the name from the symbolication of the address. Should be an easy fix, feel free to submit a patch, or I can try to get to it when i have some time.

I would look at the lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp and check the "ObjectFilePECOFF::ParseSymtab(Symtab &symtab)" method. It must not be creating the symbols correctly from the symbol table. Any fixes for the PECOFF object file plug-in would be appreciated if you find any issues or things we can do better. Back in the day I had a hard time getting any symbol table the show up in the PECOFF file, but that was a long time ago and I wasn't super familiar with Windows binaries.

[alvinhochun]

lldb/source/Commands/CommandObjectTarget.cpp

1551–1556

Thanks for the reply. I can make a patch for this and the indentation issue.

I did find out how to get lldb to load COFF absolute symbols, but I think there may be a separate issue with LLD actually writing the wrong value to the file in the first place. Anyway I will try to fix that separately.

lldb/bindings/interface/SBSymbol.i

public:
     GetInstructions (lldb::SBTarget target, const char *flavor_string);
 
     SBAddress
     GetStartAddress ();
 
     SBAddress
     GetEndAddress ();
 
+    uint64_t GetValue();
+
+    uint64_t GetSize();
+
     uint32_t
     GetPrologueByteSize ();
 
     SymbolType
     GetType ();
 
     bool
     GetDescription (lldb::SBStream &description);

lldb/include/lldb/API/SBSymbol.h

public:
 
   const char *GetMangledName() const;
 
   lldb::SBInstructionList GetInstructions(lldb::SBTarget target);
 
   lldb::SBInstructionList GetInstructions(lldb::SBTarget target,
                                           const char *flavor_string);
 
+  /// Get the start address of this symbol
+  ///
+  /// \returns
+  ///   If the symbol's value is not an address, an invalid SBAddress object
+  ///   will be returned. If the symbol's value is an address, a valid SBAddress
+  ///   object will be returned.
   SBAddress GetStartAddress();
 
+  /// Get the end address of this symbol
+  ///
+  /// \returns
+  ///   If the symbol's value is not an address, an invalid SBAddress object
+  ///   will be returned. If the symbol's value is an address, a valid SBAddress
+  ///   object will be returned.
   SBAddress GetEndAddress();
 
+  /// Get the raw value of a symbol.
+  ///
+  /// This accessor allows direct access to the symbol's value from the symbol
+  /// table regardless of what the value is. The value can be a file address or
+  /// it can be an integer value that depends on what the symbol's type is. Some
+  /// symbol values are not addresses, but absolute values or integer values
+  /// that can be mean different things. The GetStartAddress() accessor will
+  /// only return a valid SBAddress if the symbol's value is an address, so this
+  /// accessor provides a way to access the symbol's value when the value is
+  /// not an address.
+  ///
+  /// \returns
+  ///   Returns the raw integer value of a symbol from the symbol table.
+  uint64_t GetValue();
+
+  /// Get the size of the symbol.
+  ///
+  /// This accessor allows direct access to the symbol's size from the symbol
+  /// table regardless of what the value is (address or integer value).
+  ///
+  /// \returns
+  ///   Returns the size of a symbol from the symbol table.
+  uint64_t GetSize();
+
   uint32_t GetPrologueByteSize();
 
   SymbolType GetType();
 
   bool operator==(const lldb::SBSymbol &rhs) const;
 
   bool operator!=(const lldb::SBSymbol &rhs) const;
 

lldb/include/lldb/Symbol/Symbol.h

Address GetAddress() const {
     // GetAddress() accessor, we need to hand out an invalid address if the
     // symbol's value isn't an address.
     if (ValueIsAddress())
       return m_addr_range.GetBaseAddress();
     else
       return Address();
   }
 
+  /// Get the raw value of the symbol from the symbol table.
+  ///
+  /// If the symbol's value is an address, return the file address, else return
+  /// the raw value that is stored in the m_addr_range. If the base address has
+  /// no section, then getting the file address will return the correct value
+  /// as it will return the offset in the base address which is the value.
+  uint64_t GetRawValue() const {
+    return m_addr_range.GetBaseAddress().GetFileAddress();
+  }
+
   // When a symbol's value isn't an address, we need to access the raw value.
   // This function will ensure this symbol's value isn't an address and return
   // the integer value if this checks out, otherwise it will return
   // "fail_value" if the symbol is an address value.
   uint64_t GetIntegerValue(uint64_t fail_value = 0) const {
     if (ValueIsAddress()) {
       // This symbol's value is an address. Use Symbol::GetAddress() to get the
       // address.

lldb/source/API/SBSymbol.cpp

if (m_opaque_ptr && m_opaque_ptr->ValueIsAddress()) {
     if (range_size > 0) {
       addr.SetAddress(m_opaque_ptr->GetAddressRef());
       addr->Slide(m_opaque_ptr->GetByteSize());
     }
   }
   return addr;
 }
 
+uint64_t SBSymbol::GetValue() {
+  LLDB_INSTRUMENT_VA(this);
+  if (m_opaque_ptr)
+    return m_opaque_ptr->GetRawValue();
+  return 0;
+}
+
+uint64_t SBSymbol::GetSize() {
+  LLDB_INSTRUMENT_VA(this);
+  if (m_opaque_ptr && m_opaque_ptr->GetByteSizeIsValid())
+    return m_opaque_ptr->GetByteSize();
+  return 0;
+}
+
 uint32_t SBSymbol::GetPrologueByteSize() {
   LLDB_INSTRUMENT_VA(this);
 
   if (m_opaque_ptr)
     return m_opaque_ptr->GetPrologueByteSize();
   return 0;
 }
 

lldb/source/Commands/CommandObjectTarget.cpp

if (num_matches > 0) {
     strm.Indent();
     strm.Printf("%u symbols match %s'%s' in ", num_matches,
                 name_is_regex ? "the regular expression " : "", name);
     DumpFullpath(strm, &module->GetFileSpec(), 0);
     strm.PutCString(":\n");
     strm.IndentMore();
     for (uint32_t i = 0; i < num_matches; ++i) {
       Symbol *symbol = symtab->SymbolAtIndex(match_indexes[i]);
-      if (symbol && symbol->ValueIsAddress()) {
+      if (symbol) {
+        if (symbol->ValueIsAddress()) {
           DumpAddress(
               interpreter.GetExecutionContext().GetBestExecutionContextScope(),
               symbol->GetAddressRef(), verbose, all_ranges, strm);
+        } else {
+          strm.IndentMore();
+          strm.Indent("    Value: ");
+          strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetRawValue());
+          if (symbol->GetByteSizeIsValid()) {
+            strm.Indent("    Size: ");
+            strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetByteSize());
+          }
+        }
       }
     }
     strm.IndentLess();
   }
   return num_matches;
 }
 
 static void DumpSymbolContextList(ExecutionContextScope *exe_scope,

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (arch.IsValid()) {
         }
       }
     }
 
     // symbol_value_offset may contain 0 for ARM symbols or -1 for THUMB
     // symbols. See above for more details.
     uint64_t symbol_value = symbol.st_value + symbol_value_offset;
 
-    if (symbol_section_sp == nullptr && shndx == SHN_ABS &&
-        symbol.st_size != 0) {
-      // We don't have a section for a symbol with non-zero size. Create a new
-      // section for it so the address range covered by the symbol is also
-      // covered by the module (represented through the section list). It is
-      // needed so module lookup for the addresses covered by this symbol will
-      // be successfull. This case happens for absolute symbols.
-      ConstString fake_section_name(std::string(".absolute.") + symbol_name);
-      symbol_section_sp =
-          std::make_shared<Section>(module_sp, this, SHN_ABS, fake_section_name,
-                                    eSectionTypeAbsoluteAddress, symbol_value,
-                                    symbol.st_size, 0, 0, 0, SHF_ALLOC);
-
-      module_section_list->AddSection(symbol_section_sp);
-      section_list->AddSection(symbol_section_sp);
-    }
-
     if (symbol_section_sp &&
         CalculateType() != ObjectFile::Type::eTypeObjectFile)
       symbol_value -= symbol_section_sp->GetFileAddress();
 
     if (symbol_section_sp && module_section_list &&
         module_section_list != section_list) {
       ConstString sect_name = symbol_section_sp->GetName();
       auto section_it = section_name_to_section.find(sect_name.GetCString());

lldb/source/Symbol/Symbol.cpp

void Symbol::Clear() {
   m_contains_linker_annotations = false;
   m_is_weak = false;
   m_type = eSymbolTypeInvalid;
   m_flags = 0;
   m_addr_range.Clear();
 }
 
 bool Symbol::ValueIsAddress() const {
-  return m_addr_range.GetBaseAddress().GetSection().get() != nullptr ||
-         m_type == eSymbolTypeAbsolute;
+  return (bool)m_addr_range.GetBaseAddress().GetSection();
 }
 
 ConstString Symbol::GetDisplayName() const {
   return GetMangled().GetDisplayDemangledName();
 }
 
 ConstString Symbol::GetReExportedSymbolName() const {
   if (m_type == eSymbolTypeReExported) {

lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py

+"""
+Test absolute symbols in ELF files to make sure they don't create sections and
+to verify that symbol values and size can still be accessed via SBSymbol APIs.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import os
+
+class TestAbsoluteSymbol(TestBase):
+
+    @no_debug_info_test
+    def test_absolute_symbol(self):
+        '''
+            Load an ELF file that contains two symbols:
+            - "absolute" which is a symbol with the section SHN_ABS
+            - "main" which is a code symbol in .text
+
+            Index   st_name    st_value           st_size            st_info                             st_other st_shndx Name
+            ======= ---------- ------------------ ------------------ ----------------------------------- -------- -------- ===========================
+            [    0] 0x00000000 0x0000000000000000 0x0000000000000000 0x00 (STB_LOCAL      STT_NOTYPE   ) 0x00            0
+            [    1] 0x00000001 0x0000000000001000 0x0000000000000004 0x12 (STB_GLOBAL     STT_FUNC     ) 0x00            1 main
+            [    2] 0x00000006 0xffffffff80000000 0x0000000000000009 0x10 (STB_GLOBAL     STT_NOTYPE   ) 0x00      SHN_ABS absolute
+
+            We used to create sections for symbols whose section ID was SHN_ABS
+            and this caused problems as the new sections could interfere with
+            with address resolution. Absolute symbols' values are not addresses
+            and should not be treated this way.
+
+            New APIs were added to SBSymbol to allow access to the raw integer
+            value and size of symbols so symbols whose value was not an address
+            could be accessed. Prior to this commit, you could only call:
+
+            SBAddress SBSymbol::GetStartAddress()
+            SBAddress SBSymbol::GetEndAddress()
+
+            If the symbol's value was not an address, you couldn't access the
+            raw value because the above accessors would return invalid SBAddress
+            objects if the value wasn't an address. New APIs were added for this:
+
+            uint64_t SBSymbol::GetValue()
+            uint64_t SBSymbol::GetSize();
+        '''
+        src_dir = self.getSourceDir()
+        yaml_path = os.path.join(src_dir, "absolute.yaml")
+        yaml_base, ext = os.path.splitext(yaml_path)
+        obj_path = self.getBuildArtifact("a.out")
+        self.yaml2obj(yaml_path, obj_path)
+
+        # Create a target with the object file we just created from YAML
+        target = self.dbg.CreateTarget(obj_path)
+        self.assertTrue(target, VALID_TARGET)
+
+        module = target.modules[0]
+
+        # Make sure the 'main' symbol is valid and has address values. Also make
+        # sure we can access the raw file address and size via the new APIS.
+        symbol = module.FindSymbol('main')
+        self.assertTrue(symbol.IsValid())
+        self.assertTrue(symbol.GetStartAddress().IsValid())
+        self.assertTrue(symbol.GetEndAddress().IsValid())
+        self.assertEqual(symbol.GetValue(), 0x1000)
+        self.assertEqual(symbol.GetSize(), 0x4)
+
+        # Make sure the 'absolute' symbol is valid and has no address values.
+        # Also make sure we can access the raw file address and size via the new
+        # APIS.
+        symbol = module.FindSymbol('absolute')
+        self.assertTrue(symbol.IsValid())
+        self.assertFalse(symbol.GetStartAddress().IsValid())
+        self.assertFalse(symbol.GetEndAddress().IsValid())
+        self.assertEqual(symbol.GetValue(), 0xffffffff80000000)
+        self.assertEqual(symbol.GetSize(), 9)
+
+        # Make sure no sections were created for the absolute symbol with a
+        # prefix of ".absolute." followed by the symbol name as they interfere
+        # with address lookups if they are treated like real sections.
+        for section in module.sections:
+            self.assertTrue(section.GetName() != ".absolute.absolute")

lldb/test/API/python_api/absolute_symbol/absolute.yaml

+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  OSABI:           ELFOSABI_FREEBSD
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0xFFFFFFFF8037C000
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x4
+    Content:         "c3c3c3c3"
+ProgramHeaders:
+  - Type: PT_LOAD
+    Flags: [ PF_X, PF_R ]
+    VAddr: 0x1000
+    PAddr: 0x1000
+    Align: 0x4
+    FirstSec: .text
+    LastSec:  .text
+Symbols:
+  - Name:            main
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Value:           0x1000
+    Size:            0x4
+  - Name:            absolute
+    Index:           SHN_ABS
+    Binding:         STB_GLOBAL
+    Value:           0xFFFFFFFF80000000
+    Size:            0x9
+

lldb/test/Shell/SymbolFile/absolute-symbol.test

 # RUN: yaml2obj %s -o %t.o
 # RUN: %lldb -b -o 'target modules lookup -s absolute_symbol' %t.o | FileCheck %s
 # CHECK: 1 symbols match 'absolute_symbol'
-# CHECK:   Address: 0x0000000012345678 (0x0000000012345678)
-# CHECK:   Summary: 0x0000000012345678
+# CHECK:   Value: 0x0000000012345678
 # Created from:
 #   .globl absolute_symbol
 #   absolute_symbol = 0x12345678
 --- !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x100000C
   cpusubtype:      0x0
- n_strx:          1
       n_desc:          0
       n_value:         305419896
   StringTable:
     - ''
     - absolute_symbol
     - ltmp0
     - ''
 ...
-