Allow subzero tests to pass without le32-nacl-objdump.

Allow subzero tests to pass without le32-nacl-objdump.

le32-nacl-objdump has been deprecated, and should no longer be used.
Instead, "arm-nacl-objdump" is being used. "le32-nacl-objdump" used to be
a hard link to "arm-nacl-objdump", but has since been deleted in NaCl's
"toolchain_build_pnacl.py" script.

R=phosek@chromium.org, stichnot@chromium.org

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

[Sean Klein, 2016-03-09 01:45:51]

Description was changed from

==========
Allow subzero tests to pass on machine without le32-nacl-objdump.

le32-nacl-objdump has been deprecated, and should no longer be used.
Instead, "arm-nacl-objdump" is being used. "le32-nacl-objdump" used to be
a symlink to "arm-nacl-objdump", but has since been deleted in NaCl's
"toolchain_build_pnacl.py" script.
==========

to

==========
Allow subzero tests to pass without le32-nacl-objdump.

le32-nacl-objdump has been deprecated, and should no longer be used.
Instead, "arm-nacl-objdump" is being used. "le32-nacl-objdump" used to be
a symlink to "arm-nacl-objdump", but has since been deleted in NaCl's
"toolchain_build_pnacl.py" script.
==========

[Sean Klein, 2016-03-09 01:45:52]

smklein@chromium.org changed reviewers:
+ phosek@chromium.org, stichnot@chromium.org

[Petr Hosek, 2016-03-09 02:17:07]

lgtm, thanks for fixing this

[Petr Hosek, 2016-03-09 02:38:08]

Just one correction, it used to be a *hardlink*.

[Sean Klein, 2016-03-09 02:38:46]

Description was changed from

==========
Allow subzero tests to pass without le32-nacl-objdump.

le32-nacl-objdump has been deprecated, and should no longer be used.
Instead, "arm-nacl-objdump" is being used. "le32-nacl-objdump" used to be
a symlink to "arm-nacl-objdump", but has since been deleted in NaCl's
"toolchain_build_pnacl.py" script.
==========

to

==========
Allow subzero tests to pass without le32-nacl-objdump.

le32-nacl-objdump has been deprecated, and should no longer be used.
Instead, "arm-nacl-objdump" is being used. "le32-nacl-objdump" used to be
a hard link to "arm-nacl-objdump", but has since been deleted in NaCl's
"toolchain_build_pnacl.py" script.
==========

[Jim Stichnoth, 2016-03-09 14:38:05]

otherwise lgtm

https://codereview.chromium.org/1776843002/diff/1/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1776843002/diff/1/pydir/run-pnacl-sz.py#newco...
pydir/run-pnacl-sz.py:199: cmd += (['&&', os.path.join(pnacl_bin_path,
'arm-nacl-objdump')] +
IIRC, originally the tests used the system objdump.  Later, we changed to
llvm-objdump, probably because different host systems might have different
versions of objdump with differences in output.  Sometime later, we switched to
le32-nacl-objdump because llvm-objdump was due to change its ability to
symbolize the output.

I guess "le32" was always a bit weird because there is no guarantee that the
host system or the generated code is le32, but calling out "arm" is even
weirder.

I tried changing this to i686 or x86_64, but it looks like those builds are
unaware of MIPS.

Could you maybe quarantine the weirdness by defining a helper function in this
file?  e.g.

  def GetObjdumpCmd():
    """Return a suitable objdump command."""
    return 'arm-nacl-objdump'

Later, if needed, this could be modified to construct the entire path and args
on a per-target basis, but for now I'm OK just returning a fixed string.

https://codereview.chromium.org/1776843002/diff/1/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/1776843002/diff/1/tests_lit/lit.cfg#newcode114
tests_lit/lit.cfg:114: 'arm-nacl-objdump',
This line can be removed entirely, after the elf_container.ll change.

https://codereview.chromium.org/1776843002/diff/1/tests_lit/llvm2ice_tests/el...
File tests_lit/llvm2ice_tests/elf_container.ll (right):

https://codereview.chromium.org/1776843002/diff/1/tests_lit/llvm2ice_tests/el...
tests_lit/llvm2ice_tests/elf_container.ll:20: ; RUN: %p2i -i %s --filetype=obj
--output %t --args -O2 --verbose none \
Add "--disassemble" before --args (and remove "--verbose none" while you're at
it)...

https://codereview.chromium.org/1776843002/diff/1/tests_lit/llvm2ice_tests/el...
tests_lit/llvm2ice_tests/elf_container.ll:22: ; RUN:   && arm-nacl-objdump -w -d
-r -Mintel %t \
... and remove this line entirely, because that is precisely what
"--disassemble" does.

[Sean Klein, 2016-03-09 18:27:44]

https://codereview.chromium.org/1776843002/diff/1/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1776843002/diff/1/pydir/run-pnacl-sz.py#newco...
pydir/run-pnacl-sz.py:199: cmd += (['&&', os.path.join(pnacl_bin_path,
'arm-nacl-objdump')] +
On 2016/03/09 14:38:04, stichnot wrote:
> IIRC, originally the tests used the system objdump.  Later, we changed to
> llvm-objdump, probably because different host systems might have different
> versions of objdump with differences in output.  Sometime later, we switched
to
> le32-nacl-objdump because llvm-objdump was due to change its ability to
> symbolize the output.
> 
> I guess "le32" was always a bit weird because there is no guarantee that the
> host system or the generated code is le32, but calling out "arm" is even
> weirder.
> 
> I tried changing this to i686 or x86_64, but it looks like those builds are
> unaware of MIPS.
> 
> Could you maybe quarantine the weirdness by defining a helper function in this
> file?  e.g.
> 
>   def GetObjdumpCmd():
>     """Return a suitable objdump command."""
>     return 'arm-nacl-objdump'
> 
> Later, if needed, this could be modified to construct the entire path and args
> on a per-target basis, but for now I'm OK just returning a fixed string.

Yeah, I agree that it's not very intuitive that "arm" happens to cover all
architectures we care about.

Updated to the "GetObjdumpCmd" function version.

https://codereview.chromium.org/1776843002/diff/1/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/1776843002/diff/1/tests_lit/lit.cfg#newcode114
tests_lit/lit.cfg:114: 'arm-nacl-objdump',
On 2016/03/09 14:38:04, stichnot wrote:
> This line can be removed entirely, after the elf_container.ll change.

Done.

https://codereview.chromium.org/1776843002/diff/1/tests_lit/llvm2ice_tests/el...
File tests_lit/llvm2ice_tests/elf_container.ll (right):

https://codereview.chromium.org/1776843002/diff/1/tests_lit/llvm2ice_tests/el...
tests_lit/llvm2ice_tests/elf_container.ll:20: ; RUN: %p2i -i %s --filetype=obj
--output %t --args -O2 --verbose none \
On 2016/03/09 14:38:05, stichnot wrote:
> Add "--disassemble" before --args (and remove "--verbose none" while you're at
> it)...

Done.

https://codereview.chromium.org/1776843002/diff/1/tests_lit/llvm2ice_tests/el...
tests_lit/llvm2ice_tests/elf_container.ll:22: ; RUN:   && arm-nacl-objdump -w -d
-r -Mintel %t \
On 2016/03/09 14:38:05, stichnot wrote:
> ... and remove this line entirely, because that is precisely what
> "--disassemble" does.

Done.

[Sean Klein, 2016-03-09 18:29:57]

Description was changed from

==========
Allow subzero tests to pass without le32-nacl-objdump.

le32-nacl-objdump has been deprecated, and should no longer be used.
Instead, "arm-nacl-objdump" is being used. "le32-nacl-objdump" used to be
a hard link to "arm-nacl-objdump", but has since been deleted in NaCl's
"toolchain_build_pnacl.py" script.
==========

to

==========
Allow subzero tests to pass without le32-nacl-objdump.

le32-nacl-objdump has been deprecated, and should no longer be used.
Instead, "arm-nacl-objdump" is being used. "le32-nacl-objdump" used to be
a hard link to "arm-nacl-objdump", but has since been deleted in NaCl's
"toolchain_build_pnacl.py" script.

R=phosek@chromium.org, stichnot@chromium.org

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

[Sean Klein, 2016-03-09 18:29:58]

Committed patchset #2 (id:20001) manually as
8a4675cecbde443db312f89bec7321e0614a51d5 (presubmit successful).