Instrumented local variables and implemented runtime.

Instrumented local variables and implemented 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=1fd80c722c1a2d4c31d43a0df35ccbd49d9c2b5a

[tlively, 2016-06-23 21:23:53]

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

[Karl, 2016-06-23 21:55:52]

Some initial comments. The C code should be cleaned up to make it more readable.

I also did not see a test added to test the fix for deleted alloc's. Can you put
one in (use the small test you had and
test the effects when compiled with ASAN).

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode28
runtime/szrt_asan.c:28: #define MEM2SHADOW(p) (((uintptr_t)(p) >> 3) +
shadow_offset)
Create a symbolic name for 3 (like kByteLog2). Then replace corresponding uses.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode42
runtime/szrt_asan.c:42: void __asan_init(void) {
Since __asan_malloc assumes that

   RS_SIZE >= sizeof(void*)

and 

   RS_SIZE >= sizeof(size_t)

Why not add an assert check (with appropriate comment) to verify assumptions?

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode44
runtime/szrt_asan.c:44: size_t length = (sizeof(void *) == 4) ? 1 << 29 :
0x0000100000000000;
Again, create a symbolic name for 29?
Also, you may want to write the then and else parts as:

       ? size_t(1) << kShadowBoundary32 : size_t(1) << kShadowBoundary64

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode87
runtime/szrt_asan.c:87: *(void **)rz_left = rz_right;
Since types like

    (void **)
and
    (size_t *)

occurs multiple times, why not typedef then and given them symbolic names?

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode109
runtime/szrt_asan.c:109: assert((uintptr_t)end % 8 == 0);
CHAR_BIT instead of 8?

Also, consider defining a macro for this (byte-aligned) test, so that it it more
readable?

Maybe IS_BYTE_ALIGNED?

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode110
runtime/szrt_asan.c:110: assert(size < 64);
Why the magic number 64?

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode117
runtime/szrt_asan.c:117: ptr += size % 8;
How about defining a macro for cases of (X % 8) and use it everywhere it occurs?

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode138
runtime/szrt_asan.c:138: }
In general, this code looks right. However, it is hard to read because it has
lots of low-level calculations and casts. Try to clean this up
by adding macros that capture common usages, so that it is easier to read.

[tlively, 2016-06-24 05:39:22]

Cleaned up the C runtime. Still need to fix redzones to be on the left and right
of allocations, since the "left" redzone is currently being allocated to the
right of the other allocations. After that fix I will add in the test.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode28
runtime/szrt_asan.c:28: #define MEM2SHADOW(p) (((uintptr_t)(p) >> 3) +
shadow_offset)
On 2016/06/23 21:55:52, Karl wrote:
> Create a symbolic name for 3 (like kByteLog2). Then replace corresponding
uses.

Done.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode42
runtime/szrt_asan.c:42: void __asan_init(void) {
On 2016/06/23 21:55:51, Karl wrote:
> Since __asan_malloc assumes that
> 
>    RS_SIZE >= sizeof(void*)
> 
> and 
> 
>    RS_SIZE >= sizeof(size_t)
> 
> Why not add an assert check (with appropriate comment) to verify assumptions?

Done.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode44
runtime/szrt_asan.c:44: size_t length = (sizeof(void *) == 4) ? 1 << 29 :
0x0000100000000000;
On 2016/06/23 21:55:51, Karl wrote:
> Again, create a symbolic name for 29?
> Also, you may want to write the then and else parts as:
> 
>        ? size_t(1) << kShadowBoundary32 : size_t(1) << kShadowBoundary64
> 

Done.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode87
runtime/szrt_asan.c:87: *(void **)rz_left = rz_right;
On 2016/06/23 21:55:52, Karl wrote:
> Since types like
> 
>     (void **)
> and
>     (size_t *)
> 
> occurs multiple times, why not typedef then and given them symbolic names?

What sort of names might be appropriate for these types?

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode109
runtime/szrt_asan.c:109: assert((uintptr_t)end % 8 == 0);
On 2016/06/23 21:55:52, Karl wrote:
> CHAR_BIT instead of 8?
> 
> Also, consider defining a macro for this (byte-aligned) test, so that it it
more
> readable?
> 
> Maybe IS_BYTE_ALIGNED?

Done.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode110
runtime/szrt_asan.c:110: assert(size < 64);
On 2016/06/23 21:55:52, Karl wrote:
> Why the magic number 64?

Done.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode117
runtime/szrt_asan.c:117: ptr += size % 8;
On 2016/06/23 21:55:52, Karl wrote:
> How about defining a macro for cases of (X % 8) and use it everywhere it
occurs?

Done.

https://codereview.chromium.org/2095763002/diff/1/runtime/szrt_asan.c#newcode138
runtime/szrt_asan.c:138: }
On 2016/06/23 21:55:51, Karl wrote:
> In general, this code looks right. However, it is hard to read because it has
> lots of low-level calculations and casts. Try to clean this up
> by adding macros that capture common usages, so that it is easier to read.

Done, and with the changes I briefly tested changing the scale and redzone size
and that worked well.

[tlively, 2016-06-25 02:09:38]

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp#newcode657
src/IceCfg.cpp:657: return A1->getAlignInBytes() >= A2->getAlignInBytes();
This fixes the problem of the leftmost redzone being placed on the right with
-O2 and on the left with -Om1. Now it is always placed on the left. I updated
existing tests to reflect this change, and all the cross tests still pass.

[Jim Stichnoth, 2016-06-25 17:14:13]

https://codereview.chromium.org/2095763002/diff/40001/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2095763002/diff/40001/runtime/szrt_asan.c#new...
runtime/szrt_asan.c:30: #define SHADOW_SCALE ((size_t)1 << SHADOW_SCALE_LOG2)
Are you sure about the relative precedence of cast versus shift?  I'm not. :) 
Use extra parentheses to be safe.

Or, you can probably get away with "1u" instead of "(size_t)1".

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

https://codereview.chromium.org/2095763002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:280: InstRet *Instr) {
You can just use "InstRet *) {" and avoid the void-cast wart below.

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

https://codereview.chromium.org/2095763002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:24: #include "IceInst.h"
I'm not 100% sure about this, but it looks like you may be able to avoid adding
this include simply by forward-declaring the Inst classes that aren't already
forward-declared in IceDefs.h.  I think it's best to avoid includes within .h
files, because eventually it blows up with a maze of circular dependencies.

See below...

https://codereview.chromium.org/2095763002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:48: void instrumentRet(LoweringContext &Context,
InstRet *Instr) override;
You could either forward-declare InstRet somewhere above:

  class InstRet;

or forward-declare it right here:

  void instrumentRet(..., class InstRet *Instr) override;

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp#newcode774
src/IceCfg.cpp:774: if (llvm::isa<InstAlloca>(&Instr) && !Instr.isDeleted()) {
Can you use the "standard" pattern here?

  if (Instr.isDeleted())
    continue;
  if (llvm::isa<InstAlloca>(&Instr)) {

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp#newcode795
src/IceCfg.cpp:795: for (Inst &Instr : EntryNode->getInsts()) {
Same thing here, start the loop with a isDeleted() check.

In the future, this will make it a bit easier to apply a non-deleted instruction
iterator adapter.

[tlively, 2016-06-27 17:03:58]

https://codereview.chromium.org/2095763002/diff/40001/runtime/szrt_asan.c
File runtime/szrt_asan.c (right):

https://codereview.chromium.org/2095763002/diff/40001/runtime/szrt_asan.c#new...
runtime/szrt_asan.c:30: #define SHADOW_SCALE ((size_t)1 << SHADOW_SCALE_LOG2)
On 2016/06/25 17:14:13, stichnot wrote:
> Are you sure about the relative precedence of cast versus shift?  I'm not. :) 
> Use extra parentheses to be safe.
> 
> Or, you can probably get away with "1u" instead of "(size_t)1".

Simplified here, and verified operator precedence for MEM2SHADOW and SHADOW2MEM.

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

https://codereview.chromium.org/2095763002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.cpp:280: InstRet *Instr) {
On 2016/06/25 17:14:13, stichnot wrote:
> You can just use "InstRet *) {" and avoid the void-cast wart below.

Done.

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

https://codereview.chromium.org/2095763002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:24: #include "IceInst.h"
On 2016/06/25 17:14:13, stichnot wrote:
> I'm not 100% sure about this, but it looks like you may be able to avoid
adding
> this include simply by forward-declaring the Inst classes that aren't already
> forward-declared in IceDefs.h.  I think it's best to avoid includes within .h
> files, because eventually it blows up with a maze of circular dependencies.
> 
> See below...

Done.

https://codereview.chromium.org/2095763002/diff/40001/src/IceASanInstrumentat...
src/IceASanInstrumentation.h:48: void instrumentRet(LoweringContext &Context,
InstRet *Instr) override;
On 2016/06/25 17:14:13, stichnot wrote:
> You could either forward-declare InstRet somewhere above:
> 
>   class InstRet;
> 
> or forward-declare it right here:
> 
>   void instrumentRet(..., class InstRet *Instr) override;

This turned out to be unnecessary, probably because they are forward declared in
IceInstrumentation.h

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp#newcode774
src/IceCfg.cpp:774: if (llvm::isa<InstAlloca>(&Instr) && !Instr.isDeleted()) {
On 2016/06/25 17:14:13, stichnot wrote:
> Can you use the "standard" pattern here?
> 
>   if (Instr.isDeleted())
>     continue;
>   if (llvm::isa<InstAlloca>(&Instr)) {

Done.

https://codereview.chromium.org/2095763002/diff/40001/src/IceCfg.cpp#newcode795
src/IceCfg.cpp:795: for (Inst &Instr : EntryNode->getInsts()) {
On 2016/06/25 17:14:13, stichnot wrote:
> Same thing here, start the loop with a isDeleted() check.
> 
> In the future, this will make it a bit easier to apply a non-deleted
instruction
> iterator adapter.

Done.

[Karl, 2016-06-27 17:31:07]

https://codereview.chromium.org/2095763002/diff/60001/tests_lit/asan_tests/al...
File tests_lit/asan_tests/alignment.ll (right):

https://codereview.chromium.org/2095763002/diff/60001/tests_lit/asan_tests/al...
tests_lit/asan_tests/alignment.ll:1: ; Translate with -fsanitize-address -O2 to
test alignment
If all RUN commands require a flag, you can use REQUIRES to check each flag.
That is, use:

; REQUIRES: target_X8632, no_allow_minimal

or

; REQUIRES: target_X8632
; REQUIRES: no_allow_minimal

This way, you do not need to add the %if protections, simplifying the
readability of the RUN commands.

Also, is there a reason you are using -O2? In general, tests should be -Om1 so
that they are more stable (i.e. don't get effected by optimizations).

If -O2 is required, because you are expecting an effect by optimization, please
document.

[tlively, 2016-06-27 20:10:29]

https://codereview.chromium.org/2095763002/diff/60001/tests_lit/asan_tests/al...
File tests_lit/asan_tests/alignment.ll (right):

https://codereview.chromium.org/2095763002/diff/60001/tests_lit/asan_tests/al...
tests_lit/asan_tests/alignment.ll:1: ; Translate with -fsanitize-address -O2 to
test alignment
On 2016/06/27 17:31:07, Karl wrote:
> If all RUN commands require a flag, you can use REQUIRES to check each flag.
> That is, use:
> 
> ; REQUIRES: target_X8632, no_allow_minimal
> 
> or
> 
> ; REQUIRES: target_X8632
> ; REQUIRES: no_allow_minimal
> 
> This way, you do not need to add the %if protections, simplifying the
> readability of the RUN commands.
> 
> Also, is there a reason you are using -O2? In general, tests should be -Om1 so
> that they are more stable (i.e. don't get effected by optimizations).
> 
> If -O2 is required, because you are expecting an effect by optimization,
please
> document.

Done.

[Karl, 2016-06-27 21:38:10]

lgtm

https://codereview.chromium.org/2095763002/diff/80001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2095763002/diff/80001/src/IceCfg.cpp#newcode656
src/IceCfg.cpp:656: return A1->getAlignInBytes() >= A2->getAlignInBytes();
For stable sort, shouldn't this be >?

[tlively, 2016-06-27 21:47:23]

Description was changed from

==========
Instrumented local variables and implemented runtime.

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

to

==========
Instrumented local variables and implemented 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-27 21:47:25]

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

[tlively, 2016-06-27 22:13:24]

https://codereview.chromium.org/2095763002/diff/80001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2095763002/diff/80001/src/IceCfg.cpp#newcode656
src/IceCfg.cpp:656: return A1->getAlignInBytes() >= A2->getAlignInBytes();
On 2016/06/27 21:38:10, Karl wrote:
> For stable sort, shouldn't this be >?

Done.