Subzero: Improved quality of ASan error messages

Subzero: Improved quality of ASan error messages

Added load/store and stack/heap/global information.

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=fb068e840bbb28a5ac552619661c9a6171ba42db

[tlively, 2016-08-04 02:17:36]

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

[Karl, 2016-08-04 14:42:24]

Otherwise LGTM.

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

https://codereview.chromium.org/2211733002/diff/1/runtime/szrt_asan.c#newcode85
runtime/szrt_asan.c:85: switch (access) {
Why not use a global array with these values, and just index?

Example:

static char* kAccessNames[] = {
  "load from",
  "store to"
};

and then replace the switch with:

accesstype = kAccessNames[access];

https://codereview.chromium.org/2211733002/diff/1/runtime/szrt_asan.c#newcode95
runtime/szrt_asan.c:95: switch (shadow_val) {
Similar here, replace with something like:

memtype = kMemTypeNames[MEMTYPE_INDEX(shadow_val)];

and then up after GLOBAL_POISON_VAL

define:

#define MEMTYPE_INDEX(x) (-1 - (x))

[Jim Stichnoth, 2016-08-04 14:55:10]

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

https://codereview.chromium.org/2211733002/diff/1/runtime/szrt_asan.c#newcode85
runtime/szrt_asan.c:85: switch (access) {
On 2016/08/04 14:42:24, Karl wrote:
> Why not use a global array with these values, and just index?
> 
> Example:
> 
> static char* kAccessNames[] = {
>   "load from",
>   "store to"
> };
> 
> and then replace the switch with:
> 
> accesstype = kAccessNames[access];

That sounds appealing, but note that it will probably need to x-macros magic to
be done properly.

https://codereview.chromium.org/2211733002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2211733002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:39: constexpr char RzStackPoison = -1;
It would be nice if this "-1" could be defined in a common place that the
runtime source file also includes, to keep the values from getting out of sync.

Also, just a question -- I don't see uses of the -2 or -3 constants in the
compiler source code, to correspond to those in the runtime file -- are those
purely local to the runtime functions?

[tlively, 2016-08-04 17:20:47]

On 2016/08/04 14:55:10, stichnot wrote:

>
https://codereview.chromium.org/2211733002/diff/1/src/IceASanInstrumentation....
> src/IceASanInstrumentation.cpp:39: constexpr char RzStackPoison = -1;
> It would be nice if this "-1" could be defined in a common place that the
> runtime source file also includes, to keep the values from getting out of
sync.
> 
> Also, just a question -- I don't see uses of the -2 or -3 constants in the
> compiler source code, to correspond to those in the runtime file -- are those
> purely local to the runtime functions?

That's right, only the runtime poisons globals and heap objects, so the compiler
source only uses -1.

That being said, there are a number of other common values such as the location
and scale of the shadow memory that could be placed in a shared location. I just
don't know what that location would be, or what it would look like since the C
runtime and C++ source have such different styles.

[tlively, 2016-08-04 17:58:22]

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

https://codereview.chromium.org/2211733002/diff/1/runtime/szrt_asan.c#newcode85
runtime/szrt_asan.c:85: switch (access) {
On 2016/08/04 14:42:24, Karl wrote:
> Why not use a global array with these values, and just index?
> 
> Example:
> 
> static char* kAccessNames[] = {
>   "load from",
>   "store to"
> };
> 
> and then replace the switch with:
> 
> accesstype = kAccessNames[access];

Done.

https://codereview.chromium.org/2211733002/diff/1/runtime/szrt_asan.c#newcode95
runtime/szrt_asan.c:95: switch (shadow_val) {
On 2016/08/04 14:42:24, Karl wrote:
> Similar here, replace with something like:
> 
> memtype = kMemTypeNames[MEMTYPE_INDEX(shadow_val)];
> 
> and then up after GLOBAL_POISON_VAL
> 
> define:
> 
> #define MEMTYPE_INDEX(x) (-1 - (x))

Done.

[Karl, 2016-08-04 18:01:42]

I don't see a new patch set?

[native-client-reviews_googlegroups.com, 2016-08-04 18:07:11]

Fixed.

On Thu, Aug 4, 2016 at 11:01 AM, <kschimpf@google.com> wrote:

> I don't see a new patch set?
>
> https://codereview.chromium.org/2211733002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Karl, 2016-08-04 18:21:06]

lgtm

[tlively, 2016-08-04 18:35:10]

Description was changed from

==========
Subzero: Improved quality of ASan error messages

Added load/store and stack/heap/global information.

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

to

==========
Subzero: Improved quality of ASan error messages

Added load/store and stack/heap/global information.

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-08-04 18:35:11]

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