Subzero: Add basic ELFObjectWriter (text section, symtab, strtab, headers)

Subzero: Add basic ELFObjectWriter (text section, symtab, strtab, headers).

Able to write out the ELF file header w/ a text section,
a symbol table, and string table. Write text buffer
directly to file after translating each CFG.
This means that the header is written out early w/ fake
data and then we seek back and write the real header
at the very end.

Does not yet handle relocations, data, rodata, constant
pools, bss, or -ffunction-sections, more than 64K sections
or more than 2^24 symbols.

Numbers w/ current NOASSERT=1 build on 176.gcc:

w/out -elf-writer:
    0.233771 (21.1%): [ 1287] emit
    28MB .s file

w/ -elf-writer:
    0.051056 ( 5.6%): [ 1287] emit
    2.4MB .o file

BUG=none
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=08c3bcd66dc0101720da78c1d5f49230f4521c75

[jvoung (off chromium), 2014-11-10 20:15:40]

Patchset #5 (id:80001) has been deleted

[jvoung (off chromium), 2014-11-10 20:15:47]

Patchset #5 (id:100001) has been deleted

[jvoung (off chromium), 2014-11-20 22:33:48]

Patchset #2 (id:20001) has been deleted

[jvoung (off chromium), 2014-11-20 22:34:16]

Patchset #4 (id:120001) has been deleted

[jvoung (off chromium), 2014-11-20 22:34:29]

Patchset #3 (id:60001) has been deleted

[jvoung (off chromium), 2014-11-20 22:35:43]

Patchset #3 (id:140001) has been deleted

[jvoung (off chromium), 2014-11-20 22:38:40]

Patchset #2 (id:40001) has been deleted

[jvoung (off chromium), 2014-11-20 22:38:54]

Patchset #3 (id:180001) has been deleted

[jvoung (off chromium), 2014-11-20 22:39:04]

Patchset #5 (id:240001) has been deleted

[jvoung (off chromium), 2014-11-20 22:39:55]

Patchset #5 (id:260001) has been deleted

[jvoung (off chromium), 2014-11-21 00:09:21]

Patchset #6 (id:300001) has been deleted

[jvoung (off chromium), 2014-11-21 00:11:21]

jvoung@chromium.org changed reviewers:
+ kschimpf@chromium.org, stichnot@chromium.org

[jvoung (off chromium), 2014-11-21 00:11:55]

https://codereview.chromium.org/678533005/diff/280001/src/IceMemoryRegion.h
File src/IceMemoryRegion.h (left):

https://codereview.chromium.org/678533005/diff/280001/src/IceMemoryRegion.h#o...
src/IceMemoryRegion.h:61: void CopyFrom(size_t offset, const MemoryRegion &from)
const;
Ended up just passing the StringRef of the code buffer and writing it out
directly, instead of using CopyFrom, etc.

https://codereview.chromium.org/678533005/diff/320001/unittest/IceELFSectionT...
File unittest/IceELFSectionTest.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/unittest/IceELFSectionT...
unittest/IceELFSectionTest.cpp:20: // lollipop, and lipop are able to share
data, while the other strings do not.
Not sure how you guys feel about adding another type of test suite.

This isn't terribly important, and the elf_container.ll test will at least make
sure the strings make sense to llvm-readobj, but it seemed easier to test for
sharing this way.

[Jim Stichnoth, 2014-11-21 21:32:23]

https://codereview.chromium.org/678533005/diff/320001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceCfg.cpp#newcode457
src/IceCfg.cpp:457: // Transfer remaining fixups too. They may need their
offsets adjusted.
Is this a TODO, or ... ?

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:40: size_t Index = static_cast<size_t>(Arch);
Can you just directly use Arch as the index?

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:80: isELF64(Ctx.getTargetArch()) ? sizeof(Elf64_Sym)
: sizeof(Elf32_Sym);
Probably want to confirm that SymTabAlign and sizeof() calculations are correct
and stable across targets, compilers, etc.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:94: T *ELFObjectWriter::createSection(const IceString
&Name, Elf64_Word sh_type,
Capitalize arg names

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:110: auto UserIt = UserSections.begin();
Can UserIt/UserE be subsumed into a range-based for loop?

Also, it might be better not to use auto for RelIt/RelE since it requires some
searching for the reader to figure out the actual type.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:138: for (auto S : RelSections) {
same comment on auto

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:180: Elf64_Off
ELFObjectWriter::alignFileOffset(Elf64_Xword Align) {
The % operations could be more efficient if Align were guaranteed to be a power
of 2.  Probably not worth it if only function entries are aligned, but maybe
more so if it's used for bundle alignment?

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:191: // Need the function's declaration.
Is this a TODO, or, ... ?

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:231: const llvm::StringRef Data) {
(void)Data;
to silence unused parameter warning in the meantime

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:29: // sectinos and write them out.  Expected usage:
sections

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:53: // Add way to write/copy data symbols / set
initializers.
This comment line sounds more like part of the TODO than documenting the method?

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:88: T *createSection(const IceString &Name, Elf64_Word
sh_type,
Capitalize arg names

(hmm, on second thought I see these args are supposed to mimic LLVM field names,
so I guess it's OK...)

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:98: // (such as the size, if it wasn't already
incrementally updated),
end sentence with period

https://codereview.chromium.org/678533005/diff/320001/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFSection.h#new...
src/IceELFSection.h:47: static const SizeT NoSectionNumber;
I think you could do this instead, for slightly better clarity and efficiency:

enum { NoSectionNumber = std::numeric_limits<SizeT>::max() };

I guess one problem is that you lose the SizeT typing of the constant, though it
may not matter here since it looks like only equality comparisons are done.

Same for the other static const members below.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFSection.h#new...
src/IceELFSection.h:255: // E.g., "pop\\0" and "lollipop\0" can both use the
characters in
Did you mean for the backslashes to be doubled?  (and lollipop here has just one
backslash)

https://codereview.chromium.org/678533005/diff/320001/src/IceELFStreamer.h
File src/IceELFStreamer.h (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFStreamer.h#ne...
src/IceELFStreamer.h:66: const char Zeros[16] = {0};
Can this be static, to avoid initializing on the stack every time?

https://codereview.chromium.org/678533005/diff/320001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:101: if (!Ctx->getFlags().UseELFWriter) {
Instead of "if (!C) A; else B;", how about "if (C) B; else A;" ?

https://codereview.chromium.org/678533005/diff/320001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceTypes.def#newcode29
src/IceTypes.def:29: //#define X(tag, str)
update comment with new args

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:303: new raw_fd_ostream(OutputFilename.c_str(), ErrorInfo);
Should we disallow OutputFilename=="-" which is its default value?

The user can specify "./-" if they *really* want that filename. :)

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:315: Os = new raw_os_ostream(OutputFilename == "-" ? std::cout
: Ofs);
I know this was just copy/paste, but can we structure this with a single
comparison on OutputFilename=="-" in the !UseElfWriter branch, for clarity?

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:364: Os->flush();
Is it enough to just flush Os?  Should it be closed and/or deleted?

https://codereview.chromium.org/678533005/diff/320001/unittest/IceELFSectionT...
File unittest/IceELFSectionTest.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/unittest/IceELFSectionT...
unittest/IceELFSectionTest.cpp:14: #include "gtest/gtest.h"
Should this include go before the Ice*.h includes?  (and after the <> includes)

[jvoung (off chromium), 2014-11-24 21:35:47]

Thanks!

https://codereview.chromium.org/678533005/diff/320001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceCfg.cpp#newcode457
src/IceCfg.cpp:457: // Transfer remaining fixups too. They may need their
offsets adjusted.
On 2014/11/21 21:32:22, stichnot wrote:
> Is this a TODO, or ... ?

Yes, a TODO and made that explicit.

Currently the ELF writer doesn't know anything about the actual target, besides
the couple of extra entries in the .defs file. So it doesn't know about
target-specific relocations. The assembler has target-specific overrides, but
then it uses the relocation enums in IceFixups.h, which are generic.

So we need to do something there. Perhaps the assembler should specify the
target-specific relocation type right away. LLVM does keep a generic-ish fixup
layer, but that might be because it has to handle ELF, Macho and Coff. I'll look
at it more.

We might also need a target hook if we want the assembler to make another pass
through and handle function calls to the same section that can be fixed w/out
waiting on the linker, and then not bother writing out those relocations.
Otherwise, we can leave it to the linker (probably just start with that).

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:40: size_t Index = static_cast<size_t>(Arch);
On 2014/11/21 21:32:22, stichnot wrote:
> Can you just directly use Arch as the index?

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:80: isELF64(Ctx.getTargetArch()) ? sizeof(Elf64_Sym)
: sizeof(Elf32_Sym);
On 2014/11/21 21:32:22, stichnot wrote:
> Probably want to confirm that SymTabAlign and sizeof() calculations are
correct
> and stable across targets, compilers, etc.

Hmm, llvm instances where they use sizeof(Elf_Sym) -- but that is for 
yet another definition in include/llvm/Object/ELFTypes.h, and not this Elfxx_Sym
coming from Support/ELF.h.

There are also instances of sizeof(Elfxx_Shdr) and sizeof(Elfxx_Ehdr).

I'm thinking of keeping this, but adding a static assert here, so the sizeof
helps document what it represents while the hardcoded constant is just for the
assert.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:94: T *ELFObjectWriter::createSection(const IceString
&Name, Elf64_Word sh_type,
On 2014/11/21 21:32:22, stichnot wrote:
> Capitalize arg names

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:110: auto UserIt = UserSections.begin();
On 2014/11/21 21:32:22, stichnot wrote:
> Can UserIt/UserE be subsumed into a range-based for loop?
> 
> Also, it might be better not to use auto for RelIt/RelE since it requires some
> searching for the reader to figure out the actual type.

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:138: for (auto S : RelSections) {
On 2014/11/21 21:32:22, stichnot wrote:
> same comment on auto

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:180: Elf64_Off
ELFObjectWriter::alignFileOffset(Elf64_Xword Align) {
On 2014/11/21 21:32:22, stichnot wrote:
> The % operations could be more efficient if Align were guaranteed to be a
power
> of 2.  Probably not worth it if only function entries are aligned, but maybe
> more so if it's used for bundle alignment?

Good point -- the align isn't statically known to be a power of two, so the %
isn't so efficient.

I made this more like AssemblerX86::Align() which asserts that it's a power of
2.

For bundle alignment, I think that AssemblerX86::Align(), will end up being the
thing that is used (at least that knows the nop encodings).

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:191: // Need the function's declaration.
On 2014/11/21 21:32:22, stichnot wrote:
> Is this a TODO, or, ... ?

Done -- actually had a TODO in the .h file already, so removed this.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:231: const llvm::StringRef Data) {
On 2014/11/21 21:32:22, stichnot wrote:
> (void)Data;
> to silence unused parameter warning in the meantime

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:29: // sectinos and write them out.  Expected usage:
On 2014/11/21 21:32:22, stichnot wrote:
> sections

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:53: // Add way to write/copy data symbols / set
initializers.
On 2014/11/21 21:32:22, stichnot wrote:
> This comment line sounds more like part of the TODO than documenting the
method?

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:88: T *createSection(const IceString &Name, Elf64_Word
sh_type,
On 2014/11/21 21:32:22, stichnot wrote:
> Capitalize arg names
> 
> (hmm, on second thought I see these args are supposed to mimic LLVM field
names,
> so I guess it's OK...)

Hmm, I'll just go ahead and capitalize them. There's also some local vars like
"SymTabEntSize".

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:98: // (such as the size, if it wasn't already
incrementally updated),
On 2014/11/21 21:32:22, stichnot wrote:
> end sentence with period

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFSection.h#new...
src/IceELFSection.h:47: static const SizeT NoSectionNumber;
On 2014/11/21 21:32:22, stichnot wrote:
> I think you could do this instead, for slightly better clarity and efficiency:
> 
> enum { NoSectionNumber = std::numeric_limits<SizeT>::max() };
> 
> I guess one problem is that you lose the SizeT typing of the constant, though
it
> may not matter here since it looks like only equality comparisons are done.
> 
> Same for the other static const members below.

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceELFSection.h#new...
src/IceELFSection.h:255: // E.g., "pop\\0" and "lollipop\0" can both use the
characters in
On 2014/11/21 21:32:22, stichnot wrote:
> Did you mean for the backslashes to be doubled?  (and lollipop here has just
one
> backslash)

Hmm, I guess \0 should be fine -- making consistent. Or just omit them =)

https://codereview.chromium.org/678533005/diff/320001/src/IceELFStreamer.h
File src/IceELFStreamer.h (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFStreamer.h#ne...
src/IceELFStreamer.h:66: const char Zeros[16] = {0};
On 2014/11/21 21:32:22, stichnot wrote:
> Can this be static, to avoid initializing on the stack every time?

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:101: if (!Ctx->getFlags().UseELFWriter) {
On 2014/11/21 21:32:22, stichnot wrote:
> Instead of "if (!C) A; else B;", how about "if (C) B; else A;" ?

Done.

https://codereview.chromium.org/678533005/diff/320001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceTypes.def#newcode29
src/IceTypes.def:29: //#define X(tag, str)
On 2014/11/21 21:32:22, stichnot wrote:
> update comment with new args

Done.

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:303: new raw_fd_ostream(OutputFilename.c_str(), ErrorInfo);
On 2014/11/21 21:32:23, stichnot wrote:
> Should we disallow OutputFilename=="-" which is its default value?
> 
> The user can specify "./-" if they *really* want that filename. :)

Done -- streaming to stdout will have problems because of seeking back, so
disallowing this.

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:315: Os = new raw_os_ostream(OutputFilename == "-" ? std::cout
: Ofs);
On 2014/11/21 21:32:23, stichnot wrote:
> I know this was just copy/paste, but can we structure this with a single
> comparison on OutputFilename=="-" in the !UseElfWriter branch, for clarity?

Done. Did something similar for the Ls stream.

https://codereview.chromium.org/678533005/diff/320001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:364: Os->flush();
On 2014/11/21 21:32:23, stichnot wrote:
> Is it enough to just flush Os?  Should it be closed and/or deleted?

Switch Os and Ls to a unique_ptr, so it gets deleted and its dtor is run (which
for raw_fd_ostream/raw_os_ostream will flush).

https://codereview.chromium.org/678533005/diff/320001/unittest/IceELFSectionT...
File unittest/IceELFSectionTest.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/unittest/IceELFSectionT...
unittest/IceELFSectionTest.cpp:14: #include "gtest/gtest.h"
On 2014/11/21 21:32:23, stichnot wrote:
> Should this include go before the Ice*.h includes?  (and after the <>
includes)

Done.

[jvoung (off chromium), 2014-12-01 19:04:36]

ping

[Jim Stichnoth, 2014-12-01 22:41:06]

LGTM.  (And sorry for losing track of this.)

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/678533005/diff/320001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:80: isELF64(Ctx.getTargetArch()) ? sizeof(Elf64_Sym)
: sizeof(Elf32_Sym);
On 2014/11/24 21:35:46, jvoung wrote:
> On 2014/11/21 21:32:22, stichnot wrote:
> > Probably want to confirm that SymTabAlign and sizeof() calculations are
> correct
> > and stable across targets, compilers, etc.
> 
> Hmm, llvm instances where they use sizeof(Elf_Sym) -- but that is for 
> yet another definition in include/llvm/Object/ELFTypes.h, and not this
Elfxx_Sym
> coming from Support/ELF.h.
> 
> There are also instances of sizeof(Elfxx_Shdr) and sizeof(Elfxx_Ehdr).
> 
> I'm thinking of keeping this, but adding a static assert here, so the sizeof
> helps document what it represents while the hardcoded constant is just for the
> assert.

Cool, static_assert sounds good.

[jvoung (off chromium), 2014-12-02 01:55:21]

Committed patchset #11 (id:420001) manually as
08c3bcd66dc0101720da78c1d5f49230f4521c75 (presubmit successful).