Subzero: Fix a cleanup error after the IceString removal CL.

Subzero: Fix a cleanup error after the IceString removal CL.

This corrects a "cleanup" mistake in line 414/417 of https://codereview.chromium.org/1838753002/diff/120001/src/IceTargetLoweringX8664.cpp .

This wasn't caught before because "make presubmit" doesn't run any sandboxed x86-64 tests.  So we add that to the presubmit script.

In addition, the "make check-spec" is changed to run each spec component via the "--run" flag of szbuild_spec2k.py.  Otherwise, the makefile was hard-coding running the native binary setup instead of the sandboxed or nonsfi setup.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-04-01 23:08:35]

Description was changed from

==========
Subzero: Fix a cleanup error after the IceString removal CL.

BUG= none
==========

to

==========
Subzero: Fix a cleanup error after the IceString removal CL.

This corrects a "cleanup" mistake in line 414/417 of
https://codereview.chromium.org/1838753002/diff/120001/src/IceTargetLoweringX...
.

This wasn't caught before because "make presubmit" doesn't run any sandboxed
x86-64 tests.  So we add that to the presubmit script.

In addition, the "make check-spec" is changed to run each spec component via the
"--run" flag of szbuild_spec2k.py.  Otherwise, the makefile was hard-coding
running the native binary setup instead of the sandboxed or nonsfi setup.

BUG= none
==========

[Jim Stichnoth, 2016-04-01 23:08:35]

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

[John, 2016-04-02 00:58:12]

lgtm

What was the error that this is fixing? I remember adding this test while
implementing the x86-64 sandbox, and I'm surprised by the fix.

Optional: Can you add a lit test?

[Jim Stichnoth, 2016-04-02 01:25:26]

On 2016/04/02 00:58:12, John wrote:
> lgtm
> 
> What was the error that this is fixing? I remember adding this test while
> implementing the x86-64 sandbox, and I'm surprised by the fix.
> 
> Optional: Can you add a lit test?

Thanks.  I'll answer the question after landing, in the interest of time.

[Jim Stichnoth, 2016-04-02 01:26:19]

Description was changed from

==========
Subzero: Fix a cleanup error after the IceString removal CL.

This corrects a "cleanup" mistake in line 414/417 of
https://codereview.chromium.org/1838753002/diff/120001/src/IceTargetLoweringX...
.

This wasn't caught before because "make presubmit" doesn't run any sandboxed
x86-64 tests.  So we add that to the presubmit script.

In addition, the "make check-spec" is changed to run each spec component via the
"--run" flag of szbuild_spec2k.py.  Otherwise, the makefile was hard-coding
running the native binary setup instead of the sandboxed or nonsfi setup.

BUG= none
==========

to

==========
Subzero: Fix a cleanup error after the IceString removal CL.

This corrects a "cleanup" mistake in line 414/417 of
https://codereview.chromium.org/1838753002/diff/120001/src/IceTargetLoweringX...
.

This wasn't caught before because "make presubmit" doesn't run any sandboxed
x86-64 tests.  So we add that to the presubmit script.

In addition, the "make check-spec" is changed to run each spec component via the
"--run" flag of szbuild_spec2k.py.  Otherwise, the makefile was hard-coding
running the native binary setup instead of the sandboxed or nonsfi setup.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-04-02 01:26:21]

Committed patchset #1 (id:1) manually as
45f7700f3d51ba04c1d82b04a58b2cfeeb830441 (presubmit successful).

[Jim Stichnoth, 2016-04-02 02:23:08]

On 2016/04/02 00:58:12, John wrote:
> lgtm
> 
> What was the error that this is fixing? I remember adding this test while
> implementing the x86-64 sandbox, and I'm surprised by the fix.

The original code in IceTargetLoweringX8664.cpp computed whether a
ConstantRelocatable address calculation can be done directly versus requiring an
lea instruction, via the test:

  NeedsLea = CR->getName() != "" || CR->getOffset() < 0;

It turns out that CR->getName() is *never* empty, and neither of us could recall
how it might have been empty in the original CL that introduced it.  Since the
IceString removal CL makes these names optional, that getName() test needed to
be collapsed:

  NeedsLea = true || CR->getOffset() < 0;

which I further simplified to:

  NeedsLea = CR->getOffset() < 0;

Oops!  It should have actually simplified to:

  NeedsLea = true;

As a result, there were some direct dereferences in the generated code that
should have been an lea.