Page MenuHomePhabricator

Unconditionally accept symbol sizes from elf
ClosedPublic

Authored by tberghammer on Jan 14 2016, 5:52 AM.

Details

Summary

Unconditionally accept symbol sizes from elf

The ELF symbol table always contain the size of the symbols so we
don't have to try to guess them based on the address of the next
symbol (it is needed for mach-o).

The change fixes an issue when a symbol is removed after a 0 size
symbol (e.g. because the second one is not public) what previously
caused the symbol lookup algorithm to end up with showing the 0 size
symbol even for the later addresses (what are not part of any symbol).
That symbol lookup error can confuse the user and also confuses the
current stack unwinder.

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Unconditionally accept symbol sizes from .dynsym.
tberghammer updated this object.
tberghammer added a reviewer: clayborg.
tberghammer added a subscriber: lldb-commits.
clayborg edited edge metadata.Jan 14 2016, 9:28 AM

So does this mean any symbols whose byte size is zero will always have a byte size of zero when parsed from a .dynsym section? What kinds of symbols have a byte size of zero in the .dynsym? Seems like function symbols should always have a valid byte size no? Why is the confusing the unwinder? How does the unwinder treat symbols differently when they have a byte size of zero after your fix and how does this fix your issues?

Yes it will prevent the artificial size calculation for symbols with 0 size in .dynsym. I think all function and data symbols should have a valid size so I am not sure why we have this symbol size calculation in the first place but I assume it is to work around some old compiler bug.

You can end up with a 0 size symbol if you specify a symbol in assembly without specifying it's type and size (found it in some android framework code).

.global foo
foo:
<any data or code>

The unwinder treat symbols with 0 size the same way as it treats with valid size. The difference is that it treats an address differently if we have a symbol for it and if we don't have a symbol for it. In the first case it calculates the unwind info for the given function while in the second case it tries to find unwind information fitting for the given address.

The current implementation with extending the length of the symbol have a few problem:

  • We will display the wrong function name to the user in the backtrace what is annoying (worse then not showing a function name)
  • If we end up with an incorrect symbol name then we will use the unwind information for that symbol what won't be valid at the location where the code is currently stopped

It would be interesting to see if there are any relocations or any other hints to help make correct function bounds from a stripped (.dynsym only) ELF file. In MachO we have a LC_FUNCTION_STARTS load command (kind of like an ELF note) that contains all start addresses of all functions even if we have all private symbols stripped. It might be worth checking if there is anything in ELF that could help us determine function starts for a given binary, then this wouldn't be an issue right? A few things I can think of are the EH frame FDEs can give you all function bounds for all functions that have unwind info. Relocations might be able to help you, but they might just be noise.

I looked through the sections we have in a striped elf file and non of them have any information what would tell us the start address of the functions (it isn't needed in runtime so it is removed to decrease the size).

Relocations won't really help because they will only reference the start of the public/global functions where we already have the start address from dynsym.

.eh_frame is a useful information source, but it isn't complete for several reasons. It is only present for non leaf functions (if the compiler is smart enough) and one eh_frame entry can belong to several functions (very common on arm where we use .ARM.exidx instead of .eh_frame). Currently if we don't have a symbol name for a function then we try to create a fake symbol for it based on the eh_frame but this is the behavior what is stopped by the extension of the 0 sized symbols because the symbol size extension will cover addresses where we originally didn't have any symbol. With leaving the 0 size symbols valid I would like to get this behavior back ti work (it works when the last symbol before the address had a non 0 size).

Do you remember what was the original reason for changing the size of the symbols from 0 to the address of the next symbol? I think it would be a good idea to completely remove that logic but I am not sure if it would break somebody or not.

clayborg requested changes to this revision.Jan 14 2016, 11:07 AM
clayborg edited edge metadata.

I looked through the sections we have in a striped elf file and non of them have any information what would tell us the start address of the functions (it isn't needed in runtime so it is removed to decrease the size).

Relocations won't really help because they will only reference the start of the public/global functions where we already have the start address from dynsym.

.eh_frame is a useful information source, but it isn't complete for several reasons. It is only present for non leaf functions (if the compiler is smart enough) and one eh_frame entry can belong to several functions (very common on arm where we use .ARM.exidx instead of .eh_frame). Currently if we don't have a symbol name for a function then we try to create a fake symbol for it based on the eh_frame but this is the behavior what is stopped by the extension of the 0 sized symbols because the symbol size extension will cover addresses where we originally didn't have any symbol. With leaving the 0 size symbols valid I would like to get this behavior back ti work (it works when the last symbol before the address had a non 0 size).

Should we try to use EH frame set the size of any zero sizes symbols?

Do you remember what was the original reason for changing the size of the symbols from 0 to the address of the next symbol? I think it would be a good idea to completely remove that logic but I am not sure if it would break somebody or not.

This was done for MachO because we don't have sizes in our symbol table. You can probably just remove it and say that the size is valid always because ELF does have a symbol size. We needed to do this in MachO, but it isn't necessary in ELF. So try removing it and running the test suite, if all passes, then just make it so for all symbols (both .symtab and .dynsym).

This revision now requires changes to proceed.Jan 14 2016, 11:07 AM
tberghammer retitled this revision from Unconditionally accept symbol sizes from .dynsym to Unconditionally accept symbol sizes from elf.
tberghammer updated this object.
tberghammer edited edge metadata.

I disabled the symbol size calculation for ELF and it worked for all configuration I tested (Linux x86_64/i386, clang-3.5/gcc-4.8.4) so I think we should go ahead with this solution. We can look for something different if we hit any issue.

clayborg requested changes to this revision.Jan 15 2016, 9:49 AM
clayborg edited edge metadata.

See inlined comments.

source/Symbol/Symtab.cpp
974–978 ↗(On Diff #44981)

You can remove this if statement right? Symbol byte size will always be valid no?

1070–1071 ↗(On Diff #44981)

Why do we need to check this? Won't "m_file_addr_to_index.FindEntryThatContains(file_addr);" already check this for us?

This revision now requires changes to proceed.Jan 15 2016, 9:49 AM
tberghammer added inline comments.Jan 15 2016, 9:59 AM
source/Symbol/Symtab.cpp
974–978 ↗(On Diff #44981)

I case of ELF all synbol size will be valid but I think this code is used for mach-o as well where they won't. The condition is kind of saying "if (mach-o)"

1070–1071 ↗(On Diff #44981)

The m_file_addr_to_index initialized with extending all 0 byte entry until the next entry so it can handle mach-o as well and because of this an entry can cover a larger address range then it's symbol.

An alternative implementation would be to sort the symbols based on address. Then calculate the size for all of them if they are not valid (mach-o) and finally generate the entries based on that. That way we can get rid of this condition but Symtab::InitAddressIndexes would become more complicated and most likely a bit less efficient.

clayborg accepted this revision.Jan 15 2016, 10:24 AM
clayborg edited edge metadata.

Never mind, sorry, missed that this was in Symbol.cpp, I was still thinking of the ObjectFileELF...

This revision is now accepted and ready to land.Jan 15 2016, 10:24 AM
This revision was automatically updated to reflect the committed changes.