Implemented linking to a dummy ASan runtime

Implemented linking to a dummy ASan runtime

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com

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

[tlively, 2016-06-14 23:52:54]

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

[Jim Stichnoth, 2016-06-15 00:19:35]

https://codereview.chromium.org/2068593003/diff/20001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/2068593003/diff/20001/pydir/szbuild.py#newcod...
pydir/szbuild.py:272: exit()
exit(1)
I think

https://codereview.chromium.org/2068593003/diff/20001/pydir/szbuild.py#newcod...
pydir/szbuild.py:273: if not '-fsanitize-address' in args.sz_args:
Is this check really necessary?  I don't think there's any precedent here for
de-duping args, so this just seems to add complexity / raise questions.

https://codereview.chromium.org/2068593003/diff/40001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2068593003/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:119: constexpr bool HasTailCall = false;
Newer code in Subzero uses a documentation idiom for these false constexprs:

  constexpr bool NoTailCall = false;
  ... InstCall::create(..., NoTailCall);

The initialization is a bit weird to read (double negative), but the create()
call is quite clear.

[tlively, 2016-06-15 00:26:31]

https://codereview.chromium.org/2068593003/diff/20001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/2068593003/diff/20001/pydir/szbuild.py#newcod...
pydir/szbuild.py:272: exit()
On 2016/06/15 00:19:35, stichnot wrote:
> exit(1)
> I think

Done.

https://codereview.chromium.org/2068593003/diff/20001/pydir/szbuild.py#newcod...
pydir/szbuild.py:273: if not '-fsanitize-address' in args.sz_args:
On 2016/06/15 00:19:35, stichnot wrote:
> Is this check really necessary?  I don't think there's any precedent here for
> de-duping args, so this just seems to add complexity / raise questions.

Done.

[Jim Stichnoth, 2016-06-15 05:21:16]

https://codereview.chromium.org/2068593003/diff/40001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/2068593003/diff/40001/pydir/szbuild.py#newcod...
pydir/szbuild.py:274: args.sz_args.append('-fsanitize-address')
I think you still want to append -fsanitize-address (just not bother to try to
de-dupe).

[Karl, 2016-06-15 14:19:28]

https://codereview.chromium.org/2068593003/diff/60001/tests_lit/asan_tests/st...
File tests_lit/asan_tests/startinitcall.ll (right):

https://codereview.chromium.org/2068593003/diff/60001/tests_lit/asan_tests/st...
tests_lit/asan_tests/startinitcall.ll:14: ; DUMP-NEXT: define internal void
@notStart(
Wouldn't be safer to make this the DUMP-LABEL entry, since it has the name of
the function. By using that as the label, you protect this test from function
reordering during compilation,
or other forms of code insertion.

https://codereview.chromium.org/2068593003/diff/60001/tests_lit/asan_tests/st...
tests_lit/asan_tests/startinitcall.ll:25: ; DUMP-NEXT: define void @_start(
Same here.

[tlively, 2016-06-15 16:32:00]

https://codereview.chromium.org/2068593003/diff/40001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/2068593003/diff/40001/pydir/szbuild.py#newcod...
pydir/szbuild.py:274: args.sz_args.append('-fsanitize-address')
On 2016/06/15 05:21:16, stichnot wrote:
> I think you still want to append -fsanitize-address (just not bother to try to
> de-dupe).

Done.

https://codereview.chromium.org/2068593003/diff/40001/src/IceASanInstrumentat...
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2068593003/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:119: constexpr bool HasTailCall = false;
On 2016/06/15 00:19:35, stichnot wrote:
> Newer code in Subzero uses a documentation idiom for these false constexprs:
> 
>   constexpr bool NoTailCall = false;
>   ... InstCall::create(..., NoTailCall);
> 
> The initialization is a bit weird to read (double negative), but the create()
> call is quite clear.

Done.

https://codereview.chromium.org/2068593003/diff/60001/tests_lit/asan_tests/st...
File tests_lit/asan_tests/startinitcall.ll (right):

https://codereview.chromium.org/2068593003/diff/60001/tests_lit/asan_tests/st...
tests_lit/asan_tests/startinitcall.ll:14: ; DUMP-NEXT: define internal void
@notStart(
On 2016/06/15 14:19:28, Karl wrote:
> Wouldn't be safer to make this the DUMP-LABEL entry, since it has the name of
> the function. By using that as the label, you protect this test from function
> reordering during compilation,
> or other forms of code insertion.

Done.

https://codereview.chromium.org/2068593003/diff/60001/tests_lit/asan_tests/st...
tests_lit/asan_tests/startinitcall.ll:25: ; DUMP-NEXT: define void @_start(
On 2016/06/15 14:19:28, Karl wrote:
> Same here.

Done.

[Karl, 2016-06-15 16:38:34]

lgtm

[tlively, 2016-06-15 17:00:28]

Description was changed from

==========
Implemented linking to a dummy ASan runtime

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Implemented linking to a dummy ASan runtime

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com

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

[tlively, 2016-06-15 17:00:30]

Committed patchset #5 (id:80001) manually as
4e81fe0a5429c103509c8380de8dc7d779f67134 (presubmit successful).