Page MenuHomePhabricator

ObjectFileELF: permit thread-local sections with overlapping file addresses
ClosedPublic

Authored by labath on Jul 25 2019, 7:38 AM.

Details

Summary

In an attempt to make file-address-based lookups more predictable, in D55998
we started ignoring sections which would result in file address
overlaps. It turns out this was too aggressive because thread-local
sections typically will have file addresses which apear to overlap
regular data/code. This does not cause a problem at runtime because
thread-local sections are loaded into memory using special logic, but it
can cause problems for lldb when trying to lookup objects by their file
address.

This patch changes ObjectFileELF to permit thread-local sections to
overlap regular ones by essentially giving them a separate address
space. It also makes them more symmetrical to regular sections by
creating container sections from PT_TLS segments.

Simultaneously, the patch changes the regular file address lookup logic
to ignore sections with the thread-specific bit set. I believe this is
what the users looking up file addresses would typically expect, as
looking up thread-local data generally requires more complex logic (e.g.
DWARF has a special opcode for that).

Event Timeline

labath created this revision.Jul 25 2019, 7:38 AM
clayborg added inline comments.Jul 25 2019, 3:10 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1874–1875

Maybe ask segment provider to get the next segment name?

ConstString Name(provider.GetNextSegmentName());

And have the llvm::formatv call be in a the VMAddressProvider::GetNextSegmentName()?

MaskRay added a comment.EditedJul 25 2019, 11:13 PM

It turns out this was too aggressive because thread-local
sections typically will have file addresses which apear to overlap
regular data/code. This does not cause a problem at runtime because
thread-local sections are loaded into memory using special logic, but it
can cause problems for lldb when trying to lookup objects by their file
address.

Yes :) This can happen with .tbss (SHT_NOBITS) overlapping another section (usually .init_array, but .got and .data are also possible). SHT_NOBITS sections are not allocated bytes in the file so they may overlap with a subsequent section.

lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml
69

.tbss 0x1000 NOBITS

.tdata 0x1010 PROGBITS

Move .tdata before .tbss (0xff0) to make the example more realistic?

.tdata has a larger address than .tbss. I think this is impossible in ld.bfd, but you can make .tbss go before .tdata with a broken lld linker script.

MaskRay added inline comments.Jul 26 2019, 12:45 AM
lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml
45

Do you mind explaining more how you'd like to improve file-address-based lookups for PT_TLS?

(lldb) image lookup -a 0x1010

Address: a.o[0x00001010] (a.o.PT_LOAD[0]..tdata + 0)

This is the current output before the change.

Yes PT_TLS can be seen as a separate address space. At runtime a TLS block is allocated (mmap) for each thread and they access TLS through their thread pointer. The address will be very different from the PT_LOAD address.

labath marked an inline comment as done.Jul 26 2019, 2:12 AM

Do you mind explaining more how you'd like to improve file-address-based lookups for PT_TLS?

I don't have this fully thought through (I was hoping this would develop as use cases start showing up), but...

Are you referring to the "image lookup" command specifically, or is it a more general question about the internals of lldb too?

Regarding "image lookup", the simplest way would be to add a "--tls" flag to look in the "tls" address space. Or even a more generic "address space" flag, as there are people interested in more address spaces. Or, we could just change the command to find and display multiple matches. But then the test here would need to be changed, as my main interest is that the correct address is found when evaluating DW_OP_addr and friends -- the "image lookup" thing is just a proxy.

As for internal interfaces, I guess similar options would be possible, but there I'm even more fuzzy about which ones are better because I don't know what are the ways in which this may be used. I know that the DW_OP_form_tls_address lookup currently completely ignores the "file" addresses of the sections and just straight to "load" addresses and real memory. This is not completely surprising as you need a thread to see thread local data, and if you have a thread, you have a live process to query. However, I can see how it might be interesting to be able to see the initial value of a thread local variable much like we can display the initial value of a global variable without launching a process. For this case, a flag to Section::ContainsFileAddress saying "yes, I want to look up in thread-local sections now" would suffice, but I don't know if this is the only use case...

lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml
69

I'll change the order here. The thing I was trying to test here is that addresses in .data are found regardless of whether it comes before or after a tls section. I think already having two TLS segments is somewhat unrealistic, and I could make it more real by splitting this into two tests, but it did not seem necessary, as lldb does not care about details like this.

labath updated this revision to Diff 211899.Jul 26 2019, 2:36 AM
labath marked 3 inline comments as done.
  • update according to review comments

Are you referring to the "image lookup" command specifically, or is it a more general question about the internals of lldb too?

Both:) This patch doesn't change the Address: a.o[0x00001010] (a.o.PT_LOAD[0]..tdata + 0) output so I was puzzled what this patch intends to do.

However, I can see how it might be interesting to be able to see the initial value of a thread local variable much like we can display the initial value of a global variable without launching a process. For this case, a flag to Section::ContainsFileAddress saying "yes, I want to look up in thread-local sections now" would suffice, but I don't know if this is the only use case...

Yes, inspecting the initial value of a thread-local variable is a use case. To that end, can this be done by introducing another member variable instead of overloading m_sections_up with a new purpose (adding PT_TLS)? If PT_TLS is recorded in a different variable, the change below can be deleted.

 bool Section::ContainsFileAddress(addr_t vm_addr) const {
   const addr_t file_addr = GetFileAddress();
-  if (file_addr != LLDB_INVALID_ADDRESS) {
+  if (file_addr != LLDB_INVALID_ADDRESS && !IsThreadSpecific()) {

(An adjacent pair of PT_LOAD segments can load the same file contents, e.g. PT_LOAD [0x150, 0x1234) and [0x1234, 0x1800) will transform to mmap calls with ranges [0, 0x2000) and [0x1000, 0x2000) at runtime if the runtime page size = 0x1000. They share one page in the file. If you ask what a specific offset in the file is mapped to, there can be multiple PT_LOAD segments (physical -> VMA is not unique). Fortunately the reverse mapping VMA -> physical offset can be treated as unique in practice ([p_vaddr,p_vaddr+p_memsz) ranges do not overlap).)

lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml
69

Multiple PT_TLS is unrealistic. None of glibc/musl/FreeBSD rtld supports more than 1 PT_TLS (they will just pick the last one and ignore the others).

MaskRay added inline comments.Jul 26 2019, 2:43 AM
lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml
62

.data = .tbss = 0x1010 is a more realistic scenario.

Normally, a SHT_PROGBITS section may overlap with a SHT_NOBITS section, but two SHT_PROGBITS sections do not overlap (ld has an on-by-default check ld --check-sections). Linkers allocate file bytes for SHT_PROGBITS sections so their occupied bytes cannot be reused by other sections (without fixing addresses with a linker script).

labath marked 2 inline comments as done.Jul 26 2019, 3:06 AM

Are you referring to the "image lookup" command specifically, or is it a more general question about the internals of lldb too?

Both:) This patch doesn't change the Address: a.o[0x00001010] (a.o.PT_LOAD[0]..tdata + 0) output so I was puzzled what this patch intends to do.

What do you mean by "doesn't change"? After this patch the addresses always resolve to the .data section..

However, I can see how it might be interesting to be able to see the initial value of a thread local variable much like we can display the initial value of a global variable without launching a process. For this case, a flag to Section::ContainsFileAddress saying "yes, I want to look up in thread-local sections now" would suffice, but I don't know if this is the only use case...

Yes, inspecting the initial value of a thread-local variable is a use case. To that end, can this be done by introducing another member variable instead of overloading m_sections_up with a new purpose (adding PT_TLS)? If PT_TLS is recorded in a different variable, the change below can be deleted.

I think that would be pretty significant departure from the current design of lldb. Lldb expects that the "section list" of a module will contain all of the module's sections (and before I started messing with these functions, it did). This includes non-loadable sections like .debug_info et al. While one could concieve a world where tls sections are in a special "tls" section list, I am not sure this is actually useful -- if we're going to think of the tls addresses as address spaces, then its reasonable to have more than two address spaces one day (there are people interested in that), and so we couldn't have a fixed set of section lists.

 bool Section::ContainsFileAddress(addr_t vm_addr) const {
   const addr_t file_addr = GetFileAddress();
-  if (file_addr != LLDB_INVALID_ADDRESS) {
+  if (file_addr != LLDB_INVALID_ADDRESS && !IsThreadSpecific()) {

(An adjacent pair of PT_LOAD segments can load the same file contents, e.g. PT_LOAD [0x150, 0x1234) and [0x1234, 0x1800) will transform to mmap calls with ranges [0, 0x2000) and [0x1000, 0x2000) at runtime if the runtime page size = 0x1000. They share one page in the file. If you ask what a specific offset in the file is mapped to, there can be multiple PT_LOAD segments (physical -> VMA is not unique). Fortunately the reverse mapping VMA -> physical offset can be treated as unique in practice ([p_vaddr,p_vaddr+p_memsz) ranges do not overlap).)

I am not 100% what you mean by this, but I think there's some confusion about names of things here. In lldb terms, a "file address" is the "load address, as it is written in the file. It is not the "physical offset within the file", which lldb calls "file offset". Unfortunately, this terminology has caused a lot of confusion in the past, but I don't know what would be the best way to resolve this. How does lld call these things? I guess there's less confusion there as lld does not have to care about real, memory, load addresses...

lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml
62

Interesting. I can that easily, but I'm wondering, do you know the reason for that? Is it just how it falls out of the default linker processing of things, or would something actually break if I assigned identical addresses to two SHT_PROGBITS sections?

69

Ok, so let's go for two tests then.

labath updated this revision to Diff 211904.Jul 26 2019, 3:17 AM

Split the test into two

MaskRay accepted this revision.Jul 26 2019, 5:28 AM

Are you referring to the "image lookup" command specifically, or is it a more general question about the internals of lldb too?

Both:) This patch doesn't change the Address: a.o[0x00001010] (a.o.PT_LOAD[0]..tdata + 0) output so I was puzzled what this patch intends to do.

What do you mean by "doesn't change"? After this patch the addresses always resolve to the .data section..

However, I can see how it might be interesting to be able to see the initial value of a thread local variable much like we can display the initial value of a global variable without launching a process. For this case, a flag to Section::ContainsFileAddress saying "yes, I want to look up in thread-local sections now" would suffice, but I don't know if this is the only use case...

Yes, inspecting the initial value of a thread-local variable is a use case. To that end, can this be done by introducing another member variable instead of overloading m_sections_up with a new purpose (adding PT_TLS)? If PT_TLS is recorded in a different variable, the change below can be deleted.

I think that would be pretty significant departure from the current design of lldb. Lldb expects that the "section list" of a module will contain all of the module's sections (and before I started messing with these functions, it did). This includes non-loadable sections like .debug_info et al. While one could concieve a world where tls sections are in a special "tls" section list, I am not sure this is actually useful -- if we're going to think of the tls addresses as address spaces, then its reasonable to have more than two address spaces one day (there are people interested in that), and so we couldn't have a fixed set of section lists.

I see. If that would be a significant departure, the current approach should be the choice. I didn't non-SHF_ALLOC sections are also in the list. If .debug_info et all are in the list, I don't see any problem to have PT_TLS in the list since those PT_LOAD are already in the list.

 bool Section::ContainsFileAddress(addr_t vm_addr) const {
   const addr_t file_addr = GetFileAddress();
-  if (file_addr != LLDB_INVALID_ADDRESS) {
+  if (file_addr != LLDB_INVALID_ADDRESS && !IsThreadSpecific()) {

(An adjacent pair of PT_LOAD segments can load the same file contents, e.g. PT_LOAD [0x150, 0x1234) and [0x1234, 0x1800) will transform to mmap calls with ranges [0, 0x2000) and [0x1000, 0x2000) at runtime if the runtime page size = 0x1000. They share one page in the file. If you ask what a specific offset in the file is mapped to, there can be multiple PT_LOAD segments (physical -> VMA is not unique). Fortunately the reverse mapping VMA -> physical offset can be treated as unique in practice ([p_vaddr,p_vaddr+p_memsz) ranges do not overlap).)

I am not 100% what you mean by this, but I think there's some confusion about names of things here. In lldb terms, a "file address" is the "load address, as it is written in the file. It is not the "physical offset within the file", which lldb calls "file offset". Unfortunately, this terminology has caused a lot of confusion in the past, but I don't know what would be the best way to resolve this. How does lld call these things? I guess there's less confusion there as lld does not have to care about real, memory, load addresses...

Maybe we can refer to these things with ELF terminology: p_offset (offsets in the file)/p_vaddr (VMA)/p_paddr (LMA)...

lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml
62

https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L2412

SHT_PROGBITS sections occupy space in the file but SHT_NOBITS sections don't. The linker doesn't allocate the same byte for different sections, unless you fix the VMA/LMA with a linker script. So usually SHT_PROGBITS sections cannot overlap.

This revision is now accepted and ready to land.Jul 26 2019, 5:28 AM

Thanks for sharing your knowledge about linkers and dynamic loaders. I have found it very useful.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 3:04 AM

I'm seeing some really weird behavior for the following two tests and I'm honestly kind of puzzled.

ObjectFile/ELF/PT_LOAD-overlap-PT_TLS.yaml
ObjectFile/ELF/PT_TLS-overlap-PT_LOAD.yaml

They fail in the same way for a standalone build, both on macOS (http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/1009/) and Linux (https://ci.swift.org/view/swift-master-rebranch/job/oss-lldb-master-rebranch-incremental-linux-ubuntu-18_04/8/).

For a standalone build the image lookups returns:

(lldb) image lookup -a 0x1000
      Address: PT_LOAD-overlap-PT_TLS.yaml.tmp[0x00001000] (PT_LOAD-overlap-PT_TLS.yaml.tmp..tbss + 0)

While for a regular in-tree build the image lookup returns:

(lldb) image lookup -a 0x1000
      Address: PT_LOAD-overlap-PT_TLS.yaml.tmp[0x00001000] (PT_LOAD-overlap-PT_TLS.yaml.tmp.PT_LOAD[0]..data + 0)

I scp'd the binaries between my local machine and the bot and they don't affect the outcome, it's just lldb that's different... Pavel, can you think of *anything* that might cause this?

It took me a while, but I tracked this down to the lack of % in front of lldb in the RUN: commands. We don't have an "lldb" substitution, so this ends up running whatever it finds on the path. Normally this does not matter because we add the build dir to the lit path, but for some reason this is not happening in a standalone build (wild guess: probably we just add the "llvm" build dir).