Write out global initializers and data rel directly to ELF file.

Write out global initializers and data rel directly to ELF file.

The local symbol relocations are a bit different from
llvm-mc, which are section-relative. E.g., instead "bytes",
it will be ".data + offsetof(bytes, .data)". So the
contents of the text/data/rodata sections can also differ
since the offsets written in place are different.

Still need to fill the symbol table with undefined
symbols (e.g., memset, and szrt lib functions) before
trying to link.


BUG=none
R=kschimpf@google.com, stichnot@chromium.org

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

[jvoung (off chromium), 2015-01-27 01:46:09]

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

[jvoung (off chromium), 2015-01-27 01:46:28]

https://codereview.chromium.org/874353006/diff/60001/src/IceELFSection.cpp
File src/IceELFSection.cpp (right):

https://codereview.chromium.org/874353006/diff/60001/src/IceELFSection.cpp#ne...
src/IceELFSection.cpp:106: if (Binding == STB_LOCAL)
oops

https://codereview.chromium.org/874353006/diff/60001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/874353006/diff/60001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:161: typedef std::vector<char> DataVecType;
Hopefully this is okay -- I changed it mostly so that I can easily convert it to
a StringRef and use raw_ostream << StringRef.

[Jim Stichnoth, 2015-01-27 16:44:56]

https://codereview.chromium.org/874353006/diff/60001/src/IceELFSection.cpp
File src/IceELFSection.cpp (right):

https://codereview.chromium.org/874353006/diff/60001/src/IceELFSection.cpp#ne...
src/IceELFSection.cpp:106: if (Binding == STB_LOCAL)
On 2015/01/27 01:46:28, jvoung wrote:
> oops

Hmm.  Too bad LLVM didn't name the enums that define these values...

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (left):

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:263: llvm_unreachable("TODO");
Yay! :)

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:226: bool IsELF64 = isELF64(Ctx.getTargetArch());
Move this into the if() block?  Maybe SectionName as well, thanks to the
refactoring?

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:279: // TODO(jvoung): dedupe, use the GlobalContext
version
Should be possible now. :)

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:291: for (auto Var : Vars) {
I think you should use VariableDeclaration *Var instead of auto Var since the
type isn't cumbersome (as opposed to something ugly like
std::unique_ptr<VariableDeclaration *>) and the type isn't immediately apparent
without digging up the definition of VariableDeclarationList.

E.g., I think the "auto &SectionList" below is reasonable.

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:307: SectionList.reserve(Vars.size());
Could you document relevant functions as to whether they are expected to be
called just once (or a small bounded number of times) during a pexe translation,
versus potentially many times due to e.g. -ffunction-sections or
-fdata-sections?

"Relevant" means functions that may result in a significant number of malloc()
calls due to containers using the default allocator.  This may lead to
scalability issues, and it's better to try fixing them at the source rather than
relying on special malloc implementations (though that may be useful as well).

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:368: llvm_unreachable("Unknown SectionType");
llvm::report_fatal_error()?

https://codereview.chromium.org/874353006/diff/80001/src/IceELFSection.cpp
File src/IceELFSection.cpp (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceELFSection.cpp#ne...
src/IceELFSection.cpp:61: Elf64_Xword AlignDiff = Align - Mod;
I've seen this pattern (which I think takes a bit of head-scratching to convince
oneself of its correctness) in a few other places, e.g.
ELFObjectWriter::alignFileOffset() and AssemblerX86::alignFunction(), maybe
more.  Would it be practical to refactor it?

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:260: virtual void lowerGlobalsELF(const
VariableDeclarationList &Vars) = 0;
After you rebase, you'll find a new method here - lowerConstants().  Some
renaming or refactoring may be in order, I'm not completely sure at this point.

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (left):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:502: virtual void lower(const VariableDeclaration
&Var) final;
Thanks! :)

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/elf_container.ll (right):

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/elf_container.ll:54: ; TEXT-RELOCS-NEXT: R_386_32
.L$float$1
I get a little nervous when I see these constant pool labels like .L$float$1 and
realize that the names derive from the order the constants are added to the
pool.  A small change to something in target lowering could change all these
label names, and many lit tests would need to be updated.

What do you think about naming the label based on the constant value?  E.g., in
this case .L$float$1 could become .L$float$3FF3AE14 .

(Something to consider later, not necessarily part of this CL.)

[jvoung (off chromium), 2015-01-28 17:46:23]

Thanks!

https://codereview.chromium.org/874353006/diff/60001/src/IceELFSection.cpp
File src/IceELFSection.cpp (right):

https://codereview.chromium.org/874353006/diff/60001/src/IceELFSection.cpp#ne...
src/IceELFSection.cpp:106: if (Binding == STB_LOCAL)
On 2015/01/27 16:44:55, stichnot wrote:
> On 2015/01/27 01:46:28, jvoung wrote:
> > oops
> 
> Hmm.  Too bad LLVM didn't name the enums that define these values...

Yeah, and unfortunately LLVM doesn't have a second copy of these where the enum
is named =)

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:226: bool IsELF64 = isELF64(Ctx.getTargetArch());
On 2015/01/27 16:44:55, stichnot wrote:
> Move this into the if() block?  Maybe SectionName as well, thanks to the
> refactoring?

Done.

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:279: // TODO(jvoung): dedupe, use the GlobalContext
version
On 2015/01/27 16:44:55, stichnot wrote:
> Should be possible now. :)

Sweet -- Done.

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:291: for (auto Var : Vars) {
On 2015/01/27 16:44:55, stichnot wrote:
> I think you should use VariableDeclaration *Var instead of auto Var since the
> type isn't cumbersome (as opposed to something ugly like
> std::unique_ptr<VariableDeclaration *>) and the type isn't immediately
apparent
> without digging up the definition of VariableDeclarationList.
> 
> E.g., I think the "auto &SectionList" below is reasonable.

Done -- still getting used to this fancy new "auto" thing =)

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:307: SectionList.reserve(Vars.size());
On 2015/01/27 16:44:55, stichnot wrote:
> Could you document relevant functions as to whether they are expected to be
> called just once (or a small bounded number of times) during a pexe
translation,
> versus potentially many times due to e.g. -ffunction-sections or
> -fdata-sections?

Good point -- I've added some comments in the .h file.


> "Relevant" means functions that may result in a significant number of malloc()
> calls due to containers using the default allocator.  This may lead to
> scalability issues, and it's better to try fixing them at the source rather
than
> relying on special malloc implementations (though that may be useful as well).


Yes, I am a little worried about inefficiencies here. Need to check the timings
more...

By fixing at the source, do you mean for example, partition the
globals earlier (while parsing) rather than make a new set of vector
and then copying here?

Also, some of the maps (like the symtab and strtab) could probably be
unordered-map or vector, and then sorting happens later. Also wasn't sure about
the possiblity of copying strings around...

https://codereview.chromium.org/874353006/diff/80001/src/IceELFObjectWriter.c...
src/IceELFObjectWriter.cpp:368: llvm_unreachable("Unknown SectionType");
On 2015/01/27 16:44:55, stichnot wrote:
> llvm::report_fatal_error()?

Done.

https://codereview.chromium.org/874353006/diff/80001/src/IceELFSection.cpp
File src/IceELFSection.cpp (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceELFSection.cpp#ne...
src/IceELFSection.cpp:61: Elf64_Xword AlignDiff = Align - Mod;
On 2015/01/27 16:44:55, stichnot wrote:
> I've seen this pattern (which I think takes a bit of head-scratching to
convince
> oneself of its correctness) in a few other places, e.g.
> ELFObjectWriter::alignFileOffset() and AssemblerX86::alignFunction(), maybe
> more.  Would it be practical to refactor it?

How about having something like OffsetToAlignment() (or whatever name) for
calculating the diff, and then the padding adjustment just uses that diff (but
is different for each function)? That'll hide the math at least.

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:260: virtual void lowerGlobalsELF(const
VariableDeclarationList &Vars) = 0;
On 2015/01/27 16:44:55, stichnot wrote:
> After you rebase, you'll find a new method here - lowerConstants().  Some
> renaming or refactoring may be in order, I'm not completely sure at this
point.

lower() has also become lowerInit() and the class is now TargetGlobalLowering.
I've renamed this to lowerInitELF, but yeah... the names could be better?

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (left):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:502: virtual void lower(const VariableDeclaration
&Var) final;
On 2015/01/27 16:44:55, stichnot wrote:
> Thanks! :)

Acknowledged.

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/elf_container.ll (right):

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/elf_container.ll:54: ; TEXT-RELOCS-NEXT: R_386_32
.L$float$1
On 2015/01/27 16:44:56, stichnot wrote:
> I get a little nervous when I see these constant pool labels like .L$float$1
and
> realize that the names derive from the order the constants are added to the
> pool.  A small change to something in target lowering could change all these
> label names, and many lit tests would need to be updated.
> 
> What do you think about naming the label based on the constant value?  E.g.,
in
> this case .L$float$1 could become .L$float$3FF3AE14 .
> 
> (Something to consider later, not necessarily part of this CL.)


Hmm that's a good point. That could work. Hopefully the overhead isn't much
assuming very few constants. Otherwise I imagine the labels will usually use all
8 or 16 hex digits?

[Jim Stichnoth, 2015-01-28 20:35:08]

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:260: virtual void lowerGlobalsELF(const
VariableDeclarationList &Vars) = 0;
On 2015/01/28 17:46:22, jvoung wrote:
> On 2015/01/27 16:44:55, stichnot wrote:
> > After you rebase, you'll find a new method here - lowerConstants().  Some
> > renaming or refactoring may be in order, I'm not completely sure at this
> point.
> 
> lower() has also become lowerInit() and the class is now TargetGlobalLowering.
> I've renamed this to lowerInitELF, but yeah... the names could be better?

Yeah... If you feel inspired, go ahead and make the changes in this CL.  They
should be pretty limited.

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/elf_container.ll (right):

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/elf_container.ll:54: ; TEXT-RELOCS-NEXT: R_386_32
.L$float$1
On 2015/01/28 17:46:22, jvoung wrote:
> On 2015/01/27 16:44:56, stichnot wrote:
> > I get a little nervous when I see these constant pool labels like .L$float$1
> and
> > realize that the names derive from the order the constants are added to the
> > pool.  A small change to something in target lowering could change all these
> > label names, and many lit tests would need to be updated.
> > 
> > What do you think about naming the label based on the constant value?  E.g.,
> in
> > this case .L$float$1 could become .L$float$3FF3AE14 .
> > 
> > (Something to consider later, not necessarily part of this CL.)
> 
> 
> Hmm that's a good point. That could work. Hopefully the overhead isn't much
> assuming very few constants. Otherwise I imagine the labels will usually use
all
> 8 or 16 hex digits?

Yeah, at least for FP constants, the labels would probably be long.  And there
could be a lot of them if some variant of constant blinding is used.

Another possibility is to sort them before emission and use their sorted
position in the name.  This adds a bit of extra overhead at the end, and the
overhead can't be hidden because the translation threads have to complete before
the constant pool can be emitted.  Sorting could be enabled only for lit
testing, but then that kind of defeats the purpose of testing, so I'd probably
keep sorting always enabled.

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:301: for (size_t I =
ELFObjectWriter::BaseSectionType;
Consider doing something like this to make the second pass through the array
look more similar to the first pass:

SectionType I = 0;
for (auto &SectionList : VarsBySection)
  writeDataOfType(I++, SectionList, RelocationKind, IsELF64);

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:316: for (auto *Var : Vars) {
auto --> VariableDeclaration

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:364: for (auto *Var : Vars) {
auto --> VariableDeclaration

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:34: // (3) writeFunctionCode (may invoke once per
function)
Why "may"? Just based on the method signature, it seems like it *must* be
invoked once per function definition.

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:35: // (4) writeConstantPool (may invoke once per type)
"(invoke at most once per primitive type)"

Would that be accurate?

https://codereview.chromium.org/874353006/diff/120001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceUtils.h#newcode58
src/IceUtils.h:58: assert(llvm::isPowerOf2_32(Align));
isPowerOf2_64 ?

[Jim Stichnoth, 2015-01-28 20:54:31]

stichnot@chromium.org changed reviewers:
+ kschimpf@google.com
- kschimpf@chromium.org

[Karl, 2015-01-28 21:18:46]

I didn't see any other issues besides the existing comments by Jim.

[jvoung (off chromium), 2015-01-28 23:37:53]

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:260: virtual void lowerGlobalsELF(const
VariableDeclarationList &Vars) = 0;
On 2015/01/28 20:35:08, stichnot wrote:
> On 2015/01/28 17:46:22, jvoung wrote:
> > On 2015/01/27 16:44:55, stichnot wrote:
> > > After you rebase, you'll find a new method here - lowerConstants().  Some
> > > renaming or refactoring may be in order, I'm not completely sure at this
> > point.
> > 
> > lower() has also become lowerInit() and the class is now
TargetGlobalLowering.
> > I've renamed this to lowerInitELF, but yeah... the names could be better?
> 
> Yeah... If you feel inspired, go ahead and make the changes in this CL.  They
> should be pretty limited.


I guess I kind of like lowerGlobal() since it's called from
Translator::lowerGlobals()... though that's a little redundant w/ the "Global"
in TargetGlobalLowering.

Also pluralized the ELF version since it takes the whole list.

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/elf_container.ll (right):

https://codereview.chromium.org/874353006/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/elf_container.ll:54: ; TEXT-RELOCS-NEXT: R_386_32
.L$float$1
On 2015/01/28 20:35:08, stichnot wrote:
> On 2015/01/28 17:46:22, jvoung wrote:
> > On 2015/01/27 16:44:56, stichnot wrote:
> > > I get a little nervous when I see these constant pool labels like
.L$float$1
> > and
> > > realize that the names derive from the order the constants are added to
the
> > > pool.  A small change to something in target lowering could change all
these
> > > label names, and many lit tests would need to be updated.
> > > 
> > > What do you think about naming the label based on the constant value? 
E.g.,
> > in
> > > this case .L$float$1 could become .L$float$3FF3AE14 .
> > > 
> > > (Something to consider later, not necessarily part of this CL.)
> > 
> > 
> > Hmm that's a good point. That could work. Hopefully the overhead isn't much
> > assuming very few constants. Otherwise I imagine the labels will usually use
> all
> > 8 or 16 hex digits?
> 
> Yeah, at least for FP constants, the labels would probably be long.  And there
> could be a lot of them if some variant of constant blinding is used.
> 
> Another possibility is to sort them before emission and use their sorted
> position in the name.  This adds a bit of extra overhead at the end, and the
> overhead can't be hidden because the translation threads have to complete
before
> the constant pool can be emitted.  Sorting could be enabled only for lit
> testing, but then that kind of defeats the purpose of testing, so I'd probably
> keep sorting always enabled.

Hmm, sorting could also make the data section content predictable... though
maybe the test shouldn't be grepping for the data section content like it is
now:

line 311 ; CHECK:       0000: A0709D3F 00000080

etc.

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:301: for (size_t I =
ELFObjectWriter::BaseSectionType;
On 2015/01/28 20:35:08, stichnot wrote:
> Consider doing something like this to make the second pass through the array
> look more similar to the first pass:
> 
> SectionType I = 0;
> for (auto &SectionList : VarsBySection)
>   writeDataOfType(I++, SectionList, RelocationKind, IsELF64);

Done sort of -- If I is type SectionType, then I++ gives:

cannot increment expression of enum type

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:316: for (auto *Var : Vars) {
On 2015/01/28 20:35:08, stichnot wrote:
> auto --> VariableDeclaration

Done.

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:364: for (auto *Var : Vars) {
On 2015/01/28 20:35:08, stichnot wrote:
> auto --> VariableDeclaration

Done.

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:34: // (3) writeFunctionCode (may invoke once per
function)
On 2015/01/28 20:35:08, stichnot wrote:
> Why "may"? Just based on the method signature, it seems like it *must* be
> invoked once per function definition.

Done. Hmm yes =)

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:35: // (4) writeConstantPool (may invoke once per type)
On 2015/01/28 20:35:08, stichnot wrote:
> "(invoke at most once per primitive type)"
> 
> Would that be accurate?

Yes, or how about "must invoke once per pooled primitive type"?

Probably not worth fdata-section'ing these, and that might defeat the
constant-merging (not fully sure if that's based of the section name).

https://codereview.chromium.org/874353006/diff/120001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceUtils.h#newcode58
src/IceUtils.h:58: assert(llvm::isPowerOf2_32(Align));
On 2015/01/28 20:35:08, stichnot wrote:
> isPowerOf2_64 ?

Done.

[Jim Stichnoth, 2015-01-29 00:38:14]

lgtm

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:260: virtual void lowerGlobalsELF(const
VariableDeclarationList &Vars) = 0;
On 2015/01/28 23:37:52, jvoung wrote:
> On 2015/01/28 20:35:08, stichnot wrote:
> > On 2015/01/28 17:46:22, jvoung wrote:
> > > On 2015/01/27 16:44:55, stichnot wrote:
> > > > After you rebase, you'll find a new method here - lowerConstants(). 
Some
> > > > renaming or refactoring may be in order, I'm not completely sure at this
> > > point.
> > > 
> > > lower() has also become lowerInit() and the class is now
> TargetGlobalLowering.
> > > I've renamed this to lowerInitELF, but yeah... the names could be better?
> > 
> > Yeah... If you feel inspired, go ahead and make the changes in this CL. 
They
> > should be pretty limited.
> 
> 
> I guess I kind of like lowerGlobal() since it's called from
> Translator::lowerGlobals()... though that's a little redundant w/ the "Global"
> in TargetGlobalLowering.
> 
> Also pluralized the ELF version since it takes the whole list.

OK.  I also should have explicitly mentioned that I also think
"TargetGlobalLowering" isn't necessary the best name choice given its current
functionality.  I wonder if something like "TargetDataLowering" would be better?
 Or other ideas?

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:301: for (size_t I =
ELFObjectWriter::BaseSectionType;
On 2015/01/28 23:37:52, jvoung wrote:
> On 2015/01/28 20:35:08, stichnot wrote:
> > Consider doing something like this to make the second pass through the array
> > look more similar to the first pass:
> > 
> > SectionType I = 0;
> > for (auto &SectionList : VarsBySection)
> >   writeDataOfType(I++, SectionList, RelocationKind, IsELF64);
> 
> Done sort of -- If I is type SectionType, then I++ gives:
> 
> cannot increment expression of enum type

BTW, JF pointed out this would be possible using an enum *class*, with the
drawback that you don't get the enum values directly injected into the parent
namespace.

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter.h
File src/IceELFObjectWriter.h (right):

https://codereview.chromium.org/874353006/diff/120001/src/IceELFObjectWriter....
src/IceELFObjectWriter.h:35: // (4) writeConstantPool (may invoke once per type)
On 2015/01/28 23:37:53, jvoung wrote:
> Probably not worth fdata-section'ing these, and that might defeat the
> constant-merging (not fully sure if that's based of the section name).

I agree - my main purpose of data-sections was to allow szbuild.py
frankenbinaries, and that doesn't apply to the internal constant pool entries.

[jvoung (off chromium), 2015-01-29 19:33:04]

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/874353006/diff/80001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:260: virtual void lowerGlobalsELF(const
VariableDeclarationList &Vars) = 0;
On 2015/01/29 00:38:14, stichnot wrote:
> On 2015/01/28 23:37:52, jvoung wrote:
> > On 2015/01/28 20:35:08, stichnot wrote:
> > > On 2015/01/28 17:46:22, jvoung wrote:
> > > > On 2015/01/27 16:44:55, stichnot wrote:
> > > > > After you rebase, you'll find a new method here - lowerConstants(). 
> Some
> > > > > renaming or refactoring may be in order, I'm not completely sure at
this
> > > > point.
> > > > 
> > > > lower() has also become lowerInit() and the class is now
> > TargetGlobalLowering.
> > > > I've renamed this to lowerInitELF, but yeah... the names could be
better?
> > > 
> > > Yeah... If you feel inspired, go ahead and make the changes in this CL. 
> They
> > > should be pretty limited.
> > 
> > 
> > I guess I kind of like lowerGlobal() since it's called from
> > Translator::lowerGlobals()... though that's a little redundant w/ the
"Global"
> > in TargetGlobalLowering.
> > 
> > Also pluralized the ELF version since it takes the whole list.
> 
> OK.  I also should have explicitly mentioned that I also think
> "TargetGlobalLowering" isn't necessary the best name choice given its current
> functionality.  I wonder if something like "TargetDataLowering" would be
better?
>  Or other ideas?

I was actually thinking TargetDataLowering also. I'll make that change for now.

[Karl, 2015-01-29 21:35:27]

lgtm

[jvoung (off chromium), 2015-01-29 22:42:44]

Committed patchset #11 (id:200001) manually as
72984d881d7afb8d890380348061a3177b609e89 (presubmit successful).

[JF, 2015-01-31 17:49:53]

jfb@chromium.org changed reviewers:
+ jfb@chromium.org

[JF, 2015-01-31 17:49:54]

https://codereview.chromium.org/874353006/diff/200001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/874353006/diff/200001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:308: void
ELFObjectWriter::writeDataOfType(SectionType SectionType,
This made the Windows build sad:

c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:
In member function 'void
Ice::ELFObjectWriter::writeDataOfType(Ice::ELFObjectWriter::SectionType, const
VariableDeclarationList&, Ice::FixupKind, bool)':
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:323:8:
error: 'SectionType' is not a class, namespace, or enumeration
   case SectionType::ROData: {
        ^
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:336:8:
error: 'SectionType' is not a class, namespace, or enumeration
   case SectionType::Data: {
        ^
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:348:8:
error: 'SectionType' is not a class, namespace, or enumeration
   case SectionType::BSS: {
        ^
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:358:8:
error: 'SectionType' is not a class, namespace, or enumeration
   case SectionType::NumSectionTypes:
        ^
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:322:10:
warning: enumeration value 'ROData' not handled in switch [-Wswitch]
   switch (SectionType) {
          ^
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:322:10:
warning: enumeration value 'Data' not handled in switch [-Wswitch]
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:322:10:
warning: enumeration value 'BSS' not handled in switch [-Wswitch]
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:322:10:
warning: enumeration value 'NumSectionTypes' not handled in switch [-Wswitch]
c:/b/build/slave/nacl-toolchain/build/native_client/toolchain_build/src/subzero/src/IceELFObjectWriter.cpp:377:26:
error: 'SectionType' is not a class, namespace, or enumeration
       if (SectionType == SectionType::ROData)
                          ^


Fixing in:
  https://codereview.chromium.org/891953002/