Page MenuHomePhabricator

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

Authored by clayborg on Aug 11 2022, 11:16 AM.

Details

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

Diff Detail

Event Timeline

clayborg created this revision.Aug 11 2022, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 11:16 AM
Herald added a subscriber: emaste. · View Herald Transcript
clayborg requested review of this revision.Aug 11 2022, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 11:16 AM

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
29–30

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.

77–81

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.

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 added inline comments.Aug 16 2022, 11:05 AM
lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py
29–30

I can remove this part of the comment.

77–81

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

labath accepted this revision.Aug 17 2022, 8:52 AM

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.

This revision is now accepted and ready to land.Aug 17 2022, 8:52 AM
This revision was automatically updated to reflect the committed changes.
alvinhochun added inline comments.
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 added inline comments.Sep 19 2022, 12:13 PM
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 added inline comments.Sep 22 2022, 5:18 AM
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.