Subzero. Uses fixups to calculate addend to relocations.

Subzero. Uses fixups to calculate addend to relocations.

This CL modifies the ELF emission so the addends are calculated during
object file creation, and not during function code emission.

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

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

[John, 2016-02-03 19:20:50]

jpp@chromium.org changed reviewers:
+ eholk@chromium.org, kschimpf@google.com, sehr@chromium.org,
stichnot@chromium.org

[Karl, 2016-02-03 21:03:05]

lgtm, but I'm not really sure about the changes to x86.

https://codereview.chromium.org/1669443002/diff/80001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1669443002/diff/80001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:623: }
Add blank line?

https://codereview.chromium.org/1669443002/diff/80001/src/IceFixups.cpp
File src/IceFixups.cpp (right):

https://codereview.chromium.org/1669443002/diff/80001/src/IceFixups.cpp#newco...
src/IceFixups.cpp:90: Str << Offset;
This works, but is not obvious.

[John, 2016-02-03 21:25:20]

https://codereview.chromium.org/1669443002/diff/80001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1669443002/diff/80001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:623: }
On 2016/02/03 21:03:05, Karl wrote:
> Add blank line?

Done.

https://codereview.chromium.org/1669443002/diff/80001/src/IceFixups.cpp
File src/IceFixups.cpp (right):

https://codereview.chromium.org/1669443002/diff/80001/src/IceFixups.cpp#newco...
src/IceFixups.cpp:90: Str << Offset;
On 2016/02/03 21:03:05, Karl wrote:
> This works, but is not obvious.

Agreed, done.

[Jim Stichnoth, 2016-02-03 23:18:27]

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerARM32....
src/IceAssemblerARM32.cpp:513: bool IsGoodOffset =
canEncodeBranchOffset(Offset);
Maybe just:
  assert(canEncodeBranchOffset(Offset));

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:137: auto *Fixup =
this->createFixup(Traits::FK_PcRel, label);
AssemblerFixup *Fixup
here and below

https://codereview.chromium.org/1669443002/diff/100001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:266: for (const auto *Fixup : Fixups) {
const AssemblerFixup *Fixup

https://codereview.chromium.org/1669443002/diff/100001/src/IceFixups.cpp
File src/IceFixups.cpp (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceFixups.cpp#newc...
src/IceFixups.cpp:70: // The old behavior was x86 specific: emit expected a
relocatable addend to
I don't think it makes sense to document old behavior like this in the code. 
(It reminds me of when people put dates in comments without the year, or the
standard phone menu admonishment "Please listen carefully, as our options have
changed.")  It's better documented as a comment in the code review, so consider
that done. :)

https://codereview.chromium.org/1669443002/diff/100001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:406: const auto *Dest = this->getDest();
const Variable *

https://codereview.chromium.org/1669443002/diff/100001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:418: const auto *Dest = this->getDest();
const Variable *

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX863...
File unittest/AssemblerX8632/TestUtil.h (right):

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX863...
unittest/AssemblerX8632/TestUtil.h:734: for (auto *Fixup :
assembler()->fixups()) {
const auto *Fixup, right?

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX866...
File unittest/AssemblerX8664/TestUtil.h (right):

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX866...
unittest/AssemblerX8664/TestUtil.h:918: for (auto *Fixup :
assembler()->fixups()) {
const auto * ?

[Jim Stichnoth, 2016-02-03 23:20:10]

Sorry, I meant to "otherwise lgtm".

[John, 2016-02-04 18:28:50]

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerARM32....
src/IceAssemblerARM32.cpp:513: bool IsGoodOffset =
canEncodeBranchOffset(Offset);
On 2016/02/03 23:18:27, stichnot wrote:
> Maybe just:
>   assert(canEncodeBranchOffset(Offset));

Done.

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:137: auto *Fixup =
this->createFixup(Traits::FK_PcRel, label);
On 2016/02/03 23:18:27, stichnot wrote:
> AssemblerFixup *Fixup
> here and below

Discussed offline. Leaving as is.

https://codereview.chromium.org/1669443002/diff/100001/src/IceELFObjectWriter...
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceELFObjectWriter...
src/IceELFObjectWriter.cpp:266: for (const auto *Fixup : Fixups) {
On 2016/02/03 23:18:27, stichnot wrote:
> const AssemblerFixup *Fixup

Discussed offline. Leaving as is.

https://codereview.chromium.org/1669443002/diff/100001/src/IceFixups.cpp
File src/IceFixups.cpp (right):

https://codereview.chromium.org/1669443002/diff/100001/src/IceFixups.cpp#newc...
src/IceFixups.cpp:70: // The old behavior was x86 specific: emit expected a
relocatable addend to
On 2016/02/03 23:18:27, stichnot wrote:
> I don't think it makes sense to document old behavior like this in the code. 
> (It reminds me of when people put dates in comments without the year, or the
> standard phone menu admonishment "Please listen carefully, as our options have
> changed.")  It's better documented as a comment in the code review, so
consider
> that done. :)

Comment removed. assert left behind with an error message.

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX863...
File unittest/AssemblerX8632/TestUtil.h (right):

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX863...
unittest/AssemblerX8632/TestUtil.h:734: for (auto *Fixup :
assembler()->fixups()) {
On 2016/02/03 23:18:27, stichnot wrote:
> const auto *Fixup, right?

Done.

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX866...
File unittest/AssemblerX8664/TestUtil.h (right):

https://codereview.chromium.org/1669443002/diff/100001/unittest/AssemblerX866...
unittest/AssemblerX8664/TestUtil.h:918: for (auto *Fixup :
assembler()->fixups()) {
On 2016/02/03 23:18:27, stichnot wrote:
> const auto * ?

Done.

[John, 2016-02-04 18:35:24]

Description was changed from

==========
Subzero. Uses fixups to calculate addend to relocations.

This CL modifies the ELF emission so the addends are calculated during
object file creation, and not during function code emission.

BUG=
==========

to

==========
Subzero. Uses fixups to calculate addend to relocations.

This CL modifies the ELF emission so the addends are calculated during
object file creation, and not during function code emission.

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

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

[John, 2016-02-04 18:35:25]

Committed patchset #6 (id:100001) manually as
6e8d3fae541c3e3aa1cb213a9db3a8155edcdbe8 (presubmit successful).