This is an archive of the discontinued LLVM Phabricator instance.

ELFObjectWriter: optimize suffices in strtab
ClosedPublic

Authored by hans on Apr 28 2014, 6:03 PM.

Details

Summary

We already do this for shstrtab, so might as well do it for strtab. This extracts the code for this into a separate class. It might be overkill to have this in a separate file, but I figured maybe this is useful for other object formats and lld. If not, we can just move it into ELFObjectWriter.cpp.

I mostly wanted to do this for the general principle, but it does save a little bit on object file size. I tried this on a clang bootstrap and saved 0.54% on the sum of object file sizes (1.14 MB out of 212 MB for a release build).

Diff Detail

Event Timeline

hans updated this revision to Diff 8900.Apr 28 2014, 6:03 PM
hans retitled this revision from to ELFObjectWriter: optimize suffices in strtab.
hans updated this object.
hans edited the test plan for this revision. (Show Details)
hans added a subscriber: Unknown Object (MLST).
rafael accepted this revision.Apr 29 2014, 4:18 PM
rafael added a reviewer: rafael.
rafael added a subscriber: rafael.

I am in favour of moving this to its own file, by why ADT? All the listed potential users would be using lib/Object, so it can stay there, no?

LGTM with the class being somewhere in lib/Object.

lib/Support/StringTableBuilder.cpp
35

Starting the table with a null character makes this ELF specific? That is fine for now, but please comment.

test/MC/ELF/symref.s
116

Just remove the number. The printed name is all that the test needs. These are leftover from the old and horrible elf-dump script.

This revision is now accepted and ready to land.Apr 29 2014, 4:18 PM
silvas added a subscriber: silvas.Apr 29 2014, 5:26 PM

Please cross-reference this from the TODO above StringTableBuilder in tools/yaml2obj/yaml2elf.h

include/llvm/ADT/StringTableBuilder.h
18–19

Please add a class comment indicating that this is primarily a convenience for doing suffix deduplication.

silvas added inline comments.Apr 29 2014, 5:33 PM
include/llvm/ADT/StringTableBuilder.h
18–19

also, you probably want to call out the very finicky nature of finalize(), since the behavior of the class changes completely before and after it is called (it would be cleaner and safer to have two separate classes, but I think that might complicate things enough to be a net loss)

hans added a comment.Apr 30 2014, 9:26 AM

I am in favour of moving this to its own file, by why ADT? All the listed potential users would be using lib/Object, so it can stay there, no?

Yes, lib/Object is of course the right place. Moved it there now.

LGTM with the class being somewhere in lib/Object.

Thanks! Landing..

Please cross-reference this from the TODO above StringTableBuilder in tools/yaml2obj/yaml2elf.h

Done. I'll see if I can hook this up in a follow-up patch.

include/llvm/ADT/StringTableBuilder.h
18–19

Done.

18–19

I figured the asserts make it pretty clear what's going on, but I've also expanded the comments a bit.

lib/Support/StringTableBuilder.cpp
35

Done.

test/MC/ELF/symref.s
116

Done.

hans closed this revision.Apr 30 2014, 10:56 AM

This was committed in r207670.