[PATCH] D55864: [elfabi] Write program headers, .dynamic, .dynstr, and .shstrtab

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich marked an inline comment as done.
jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:599
+    // Define the TLS section.
+    TlsSec.Name = ".tls";
+    TlsSec.Shdr = [this](Elf_Shdr& Shdr) {
----------------
MaskRay wrote:
> jakehehrlich wrote:
> > MaskRay wrote:
> > > Which architecture uses the section name `.tls`?
> > > 
> > > Did you mean `.tbss`?
> > .tbss would imply that this is writable when it isn't. In practice read-only data is already thread safe so there is no need for a read only TLS section. There is also in general no reason to have a read-only NOBITS section since the content is just zeros. This means there aren't good names to use for what I'm doing here. These section names don't matter functionally however. I'm open to many other names. I just went for short names where a name used in the wild didn't exist.
> > In practice read-only data is already thread safe so there is no need for a read only TLS section. 
> 
> So why do you need the non-writable `.tss`?
I don't I don't really need it, but I want to minimize size and complexity. The requirements that I found (which might be minimal)

1) You must have a .dynamic section if you have a soname, or dt_needed. Right now this is added even if no soname or needed libraries exist.
2) To test with, inspect, and work with a file with a .dynamic section you need a PT_DYNAMIC segment
3) To have a PT_DYNAMIC segment you need a PT_LOAD which covers it. Permissions on either of these seem not to matter
4) If you have TLS symbols defined in your module you need a TLS section.
5) Using a TLS nobits saves space

I have not observed that a TLS segment is needed but I haven't really gotten to testing that out (I will remove it if it isn't needed). I'd like to have only one PT_LOAD that covers everything, not just the PT_DYNAMIC segment (though that is an option). That means .dynstr and .dynsym as well as .dynamic, the TLS section, and the other definition section. For aesthetic reasons I think the permissions on the PT_LOAD should match the permissions on the sections that it covers. Consequently all sections should have the same permissions if we want only one PT_LOAD. The TLS section is arguably not even covered by the PT_LOAD so it might be fair to give it different permissions and call it .tbss but this doesn't give us a proper name for the read only NOBITS section that critically *does* need to be covered by the PT_LOAD segment.

So its an issue of aesthetic tradeoffs (*cough* bikeshed *cough*) in the face of some other motivated issues (having one PT_LOAD and using SHT_NOBITS where possible). There are a few worlds I think exist that try their best to meet the aesthetics work out nicely.

1) Make the PT_LOAD writable and thus .dynsym and .dynstr as well (note that .dynamic is sometimes read only and sometimes writeable)
2) Make the PT_LOAD read only and have read-only NOBITS sections in both TLS and non-TLS varities (the current choice) but not have good names and the idea of these existing is otherwise senseless and thus no previous examples of these exist
3) Make the PT_LOAD read only, keep the non-TLS read-only NOBITS, but make the TLS section writeable so that we can call it ".tbss" in an aesthetically pleasing way.
4) Make the PT_LOAD read only, keep the sections read-only, and call them ".rodata" and ".tbss" anyhow even if those things each imply (by convention) a false fact about our sections. As an adendum we could choose slightly different names as well.

By choosing new names the aesthetic I violate is that I'm just making up section names (which happens a lot). I think anything else violates something I personally consider more fundamental like consistency, permissions consistency, using standard permissions for the standard sections we do have, etc... I think there's also an argument to be made that using differing section names makes these sorts of binaries easily recognizable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55864/new/

https://reviews.llvm.org/D55864