Create one-off tool pnacl-hack-memset

Create one-off tool pnacl-hack-memset

This tool takes a pexe, looks for calls to memset that (were) broken in
subzero, and does a patch to such calls.

That is, if the 2nd argument is a negative i8 (i.e. 0x80 through 0xff),
and the third argument is a constant, the code adds zero to the third
argument. This fix disables the (formerly) broken subzero optimization.

This hack is only necessary until about 10/31/16, when M54 gets released
for chromeos.

Note: Has command-line option --show-fixes if you want to see the calls
that were fixed.

BUG=None
R=stichnot@chromium.org

Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/5a8750930b705846f477de8c51a837e467ad875d

[Karl, 2016-08-22 21:19:24]

Description was changed from

==========
Create one-off tool pnacl-hack-memset

This tool takes a pexe, looks for calls to memset that (were) broken in
subzero, and does a patch to such calls.

That is, if the 2nd argument is a negative i8 (i.e. 0x80 through 0xff),
and the third argument is a constant, the code adds zero to the third
argument. This fix disables the (formerly) broken subzero optimization.

This hack is only necessary until about 10/31/16, when M54 gets released
for chromeos.

Note: Has command-line option --show-fixes if you want to see the calls
that were fixed.

BUG=None
==========

to

==========
Create one-off tool pnacl-hack-memset

This tool takes a pexe, looks for calls to memset that (were) broken in
subzero, and does a patch to such calls.

That is, if the 2nd argument is a negative i8 (i.e. 0x80 through 0xff),
and the third argument is a constant, the code adds zero to the third
argument. This fix disables the (formerly) broken subzero optimization.

This hack is only necessary until about 10/31/16, when M54 gets released
for chromeos.

Note: Has command-line option --show-fixes if you want to see the calls
that were fixed.

BUG=None
==========

[Karl, 2016-08-22 21:19:24]

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

[Jim Stichnoth, 2016-08-22 21:55:44]

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
File tools/pnacl-hack-memset/pnacl-hack-memset.cpp (right):

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:6: //===-- pnacl-thaw.cpp - The
low-level NaCl bitcode thawer ----------------===//
oops

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:59: static Module
*readBitcode(std::string &Filename, LLVMContext &Context,
Optional: remove "static" for consistency with remaining functions.

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:86: size_t
fixCallToMemset(CallInst &Call) {
Document return value

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:86: size_t
fixCallToMemset(CallInst &Call) {
Can this be CallInst *Call?  Makes it clearer at the call site that the arg may
be modified.

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:89: Value* Val =
Call.getArgOperand(1);
"Value *Val" rather than "Value* Val", right?
here and below

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:189: errs() << argv[0] << ": " <<
"Fixed " << ErrorCount << " calls to memset.\n";
80-col

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:189: errs() << argv[0] << ": " <<
"Fixed " << ErrorCount << " calls to memset.\n";
... << ": Fixed " << ...
i.e. concatenate strings here

[Karl, 2016-08-23 15:17:31]

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
File tools/pnacl-hack-memset/pnacl-hack-memset.cpp (right):

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:6: //===-- pnacl-thaw.cpp - The
low-level NaCl bitcode thawer ----------------===//
On 2016/08/22 21:55:44, stichnot wrote:
> oops

Corrected.

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:59: static Module
*readBitcode(std::string &Filename, LLVMContext &Context,
On 2016/08/22 21:55:43, stichnot wrote:
> Optional: remove "static" for consistency with remaining functions.

Done.

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:86: size_t
fixCallToMemset(CallInst &Call) {
On 2016/08/22 21:55:43, stichnot wrote:
> Document return value

Done.

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:89: Value* Val =
Call.getArgOperand(1);
On 2016/08/22 21:55:43, stichnot wrote:
> "Value *Val" rather than "Value* Val", right?
> here and below

Correct. Fixing. I was getting too used to the Chrome style, which does "Value*
Val".

https://codereview.chromium.org/2262373002/diff/1/tools/pnacl-hack-memset/pna...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:189: errs() << argv[0] << ": " <<
"Fixed " << ErrorCount << " calls to memset.\n";
On 2016/08/22 21:55:43, stichnot wrote:
> ... << ": Fixed " << ...
> i.e. concatenate strings here

Done.

[Jim Stichnoth, 2016-08-23 17:20:36]

If this were a permanent tool, I would ask for tests. :)

Otherwise lgtm.

https://codereview.chromium.org/2262373002/diff/20001/tools/pnacl-hack-memset...
File tools/pnacl-hack-memset/pnacl-hack-memset.cpp (right):

https://codereview.chromium.org/2262373002/diff/20001/tools/pnacl-hack-memset...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:6: //===-- pnacl-hack-memset.cpp -
Fix (interim) subzero bug -----------------===//
I would prefer capitalized Subzero... here and below

https://codereview.chromium.org/2262373002/diff/20001/tools/pnacl-hack-memset...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:10: // Fixes generated pexe's so
that a fixed fixed subzero bug (but not
Reflow to 80-col.  :)

"fixed fixed"

This first sentence doesn't actually parse.

s/an zero/a zero/

[Karl, 2016-08-23 17:44:01]

Description was changed from

==========
Create one-off tool pnacl-hack-memset

This tool takes a pexe, looks for calls to memset that (were) broken in
subzero, and does a patch to such calls.

That is, if the 2nd argument is a negative i8 (i.e. 0x80 through 0xff),
and the third argument is a constant, the code adds zero to the third
argument. This fix disables the (formerly) broken subzero optimization.

This hack is only necessary until about 10/31/16, when M54 gets released
for chromeos.

Note: Has command-line option --show-fixes if you want to see the calls
that were fixed.

BUG=None
==========

to

==========
Create one-off tool pnacl-hack-memset

This tool takes a pexe, looks for calls to memset that (were) broken in
subzero, and does a patch to such calls.

That is, if the 2nd argument is a negative i8 (i.e. 0x80 through 0xff),
and the third argument is a constant, the code adds zero to the third
argument. This fix disables the (formerly) broken subzero optimization.

This hack is only necessary until about 10/31/16, when M54 gets released
for chromeos.

Note: Has command-line option --show-fixes if you want to see the calls
that were fixed.

BUG=None
R=stichnot@chromium.org

Committed:
https://chromium.googlesource.com/native_client/pnacl-llvm/+/5a8750930b705846...
==========

[Karl, 2016-08-23 17:44:02]

Committed patchset #3 (id:40001) manually as
5a8750930b705846f477de8c51a837e467ad875d (presubmit successful).

[Karl, 2016-08-23 17:44:14]

https://codereview.chromium.org/2262373002/diff/20001/tools/pnacl-hack-memset...
File tools/pnacl-hack-memset/pnacl-hack-memset.cpp (right):

https://codereview.chromium.org/2262373002/diff/20001/tools/pnacl-hack-memset...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:6: //===-- pnacl-hack-memset.cpp -
Fix (interim) subzero bug -----------------===//
On 2016/08/23 17:20:36, stichnot wrote:
> I would prefer capitalized Subzero... here and below

Done.

https://codereview.chromium.org/2262373002/diff/20001/tools/pnacl-hack-memset...
tools/pnacl-hack-memset/pnacl-hack-memset.cpp:10: // Fixes generated pexe's so
that a fixed fixed subzero bug (but not
On 2016/08/23 17:20:36, stichnot wrote:
> Reflow to 80-col.  :)
> 
> "fixed fixed"
> 
> This first sentence doesn't actually parse.
> 
> s/an zero/a zero/

Done.