Created Ice::Instrumentation base class and accompanying hooks.

Created Ice::Instrumentation base class and accompanying hooks.

Also added a command line flag for AddressSanitizer

BUG=None
R=kschimpf@google.com

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

[tlively, 2016-06-06 21:46:30]

Description was changed from

==========
Created Ice::Instrumentation base class and accompanying hooks.

Also added a command line flag for AddressSanitizer

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

to

==========
Created Ice::Instrumentation base class and accompanying hooks.

Also added a command line flag for AddressSanitizer

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

[tlively, 2016-06-06 21:46:31]

tlively@google.com changed reviewers:
+ kschimpf@google.com

[Karl, 2016-06-06 22:27:03]

Here is my initial feedback on this CL.

https://codereview.chromium.org/2042063002/diff/1/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceCfg.cpp#newcode242
src/IceCfg.cpp:242: // TODO(tlively): Never sanitize in the minimal build.
Conditionalize this on BuildDefs::minimal.

This code base prefers not to run unnecessary code in a minimal build. Hence,
replace with

   if (!BuildDefs.minimal()) {
     getContext() ...
     dump(...)
   }

https://codereview.chromium.org/2042063002/diff/1/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceCompileServer.cpp#ne...
src/IceCompileServer.cpp:245: // TODO(tlively): Make this instantiate an
instrumentation subclass
Conditionalize on BuildDefs.minimal()

https://codereview.chromium.org/2042063002/diff/1/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceCompiler.cpp#newcode27
src/IceCompiler.cpp:27: #include "IceInstrumentation.h"
Why is this included. It doesn't appear to be used.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:490: if (Instrumentor)
Again, enclose code inside a conditional on BuildDefs::minimal().

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:432: void setInstrumentation(class Instrumentation
*Instr) {
Conditionalize body on BuildDefs.minimal().

Also, make it clear that it will take ownership of the argument by making the
argument a unique_ptr.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:433: Instrumentor = std::unique_ptr<class
Instrumentation>(Instr);
Drop the "class" from the template, it shouldn't be needed.

If you change the argument as suggested above, you may need to enclose
Instr with std::move(Instr). The compiler will let you know if it is needed by
many (hard to follow) template instantiation errors.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp#...
src/IceInstrumentation.cpp:1: //===- subzero/src/IceInstrumentation.cpp - ICE
intrumentation framework --===//
Spelling intrumentation?

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp#...
src/IceInstrumentation.cpp:31: instrumentFuncStart(Context);
Since there is no implementation details, it is hard to decide if this is right.
Nor, there is no documentation (i.e. comments) in the header file to conclude
the meaning of this function.

However, you are calling it inside a basic block loop, yet the name implies it
is a start for a function. What are you trying to do?

Also, if I see start, I would expect an end.  If it is somehow setting the focus
on node None, then how about the name instrumentNodeFocus()?

Or possibly focusNode()?

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h
File src/IceInstrumentation.h (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:22: #pragma clang diagnostic ignored
"-Wunused-parameter"
Don't change command line flags within a file, especially an include file.  By
adding this pragma, you have turned off this feature for all later includes!

Rather, fix this as follows:

Replace method headings of methods like

   void foo(t1 arg1, ....) {}

with

   void foo(t1 arg1, ...) {
     (void) arg1;
     ...
    }

The "(void) arg1;" statement will automatically turn off the warning.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:25: #ifndef SUBZERO_SRC_ICEASANLOWERING_H
The coding convention is that the define name should follow the file path,
substituting '_' for all punctuation, and capitalizing letters.

This naming convention is more likely to avoid naming conflicts than some
arbitrarily chosen name.

Hence, use SUBZERO_SRC_ICEINSTRUMENTATION_H

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:39: protected:
Generally, we don't like introducing fields before the public API. The public
API should come first, so that it is easy to find.

[Roland McGrath, 2016-06-06 22:28:44]

mcgrathr@chromium.org changed reviewers:
+ mcgrathr@chromium.org

[Roland McGrath, 2016-06-06 22:28:45]

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:433: Instrumentor = std::unique_ptr<class
Instrumentation>(Instr);
The class keyword is superfluous in both these uses.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:569: std::unique_ptr<Instrumentation> Instrumentor =
Isn't that what the default constructor would do?  Why have an initializer here?

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp#...
src/IceInstrumentation.cpp:92: default:
This will silently ignore any other kinds of instructions.
If there are no other kinds, then the default: is unnecessary and leaving it out
should ensure you get a compiler warning for failing to handle a new kind if one
is added.  If there are other kinds that you explicitly do not want to do any
instrumentation for, then listing them all here with a case Inst:Foo: (all
falling through to the same break;) makes it clear that you really intended to
do nothing for those kinds.  Then you can leave out the default: to make sure
you get warnings in case you forgot any kinds.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h
File src/IceInstrumentation.h (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:17: /// functions is has a trivial default
implementation to make subclasses more
s/is has/has/

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:22: #pragma clang diagnostic ignored
"-Wunused-parameter"
Why is this outside the include guard?
And do you really want this in a header file??  Won't it implicitly affect every
source file that does #include "IceInstrumentation.h"?

[tlively, 2016-06-07 00:43:06]

https://codereview.chromium.org/2042063002/diff/1/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceCfg.cpp#newcode242
src/IceCfg.cpp:242: // TODO(tlively): Never sanitize in the minimal build.
On 2016/06/06 22:27:02, Karl wrote:
> Conditionalize this on BuildDefs::minimal.
> 
> This code base prefers not to run unnecessary code in a minimal build. Hence,
> replace with
> 
>    if (!BuildDefs.minimal()) {
>      getContext() ...
>      dump(...)
>    }

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceCompileServer.cpp#ne...
src/IceCompileServer.cpp:245: // TODO(tlively): Make this instantiate an
instrumentation subclass
On 2016/06/06 22:27:02, Karl wrote:
> Conditionalize on BuildDefs.minimal()

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceCompiler.cpp#newcode27
src/IceCompiler.cpp:27: #include "IceInstrumentation.h"
On 2016/06/06 22:27:02, Karl wrote:
> Why is this included. It doesn't appear to be used.

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:490: if (Instrumentor)
On 2016/06/06 22:27:02, Karl wrote:
> Again, enclose code inside a conditional on BuildDefs::minimal().

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:432: void setInstrumentation(class Instrumentation
*Instr) {
On 2016/06/06 22:27:02, Karl wrote:
> Conditionalize body on BuildDefs.minimal().
> 
> Also, make it clear that it will take ownership of the argument by making the
> argument a unique_ptr.

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:433: Instrumentor = std::unique_ptr<class
Instrumentation>(Instr);
On 2016/06/06 22:27:02, Karl wrote:
> Drop the "class" from the template, it shouldn't be needed.
> 
> If you change the argument as suggested above, you may need to enclose
> Instr with std::move(Instr). The compiler will let you know if it is needed by
> many (hard to follow) template instantiation errors.

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:433: Instrumentor = std::unique_ptr<class
Instrumentation>(Instr);
On 2016/06/06 22:28:45, Roland McGrath wrote:
> The class keyword is superfluous in both these uses.

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceGlobalContext.h#newc...
src/IceGlobalContext.h:569: std::unique_ptr<Instrumentation> Instrumentor =
On 2016/06/06 22:28:45, Roland McGrath wrote:
> Isn't that what the default constructor would do?  Why have an initializer
here?

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp#...
src/IceInstrumentation.cpp:1: //===- subzero/src/IceInstrumentation.cpp - ICE
intrumentation framework --===//
On 2016/06/06 22:27:03, Karl wrote:
> Spelling intrumentation?

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp#...
src/IceInstrumentation.cpp:31: instrumentFuncStart(Context);
On 2016/06/06 22:27:02, Karl wrote:
> Since there is no implementation details, it is hard to decide if this is
right.
> Nor, there is no documentation (i.e. comments) in the header file to conclude
> the meaning of this function.
> 
> However, you are calling it inside a basic block loop, yet the name implies it
> is a start for a function. What are you trying to do?
> 
> Also, if I see start, I would expect an end.  If it is somehow setting the
focus
> on node None, then how about the name instrumentNodeFocus()?
> 
> Or possibly focusNode()?

This call should have been outside the loop. Fixed.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.cpp#...
src/IceInstrumentation.cpp:92: default:
On 2016/06/06 22:28:45, Roland McGrath wrote:
> This will silently ignore any other kinds of instructions.
> If there are no other kinds, then the default: is unnecessary and leaving it
out
> should ensure you get a compiler warning for failing to handle a new kind if
one
> is added.  If there are other kinds that you explicitly do not want to do any
> instrumentation for, then listing them all here with a case Inst:Foo: (all
> falling through to the same break;) makes it clear that you really intended to
> do nothing for those kinds.  Then you can leave out the default: to make sure
> you get warnings in case you forgot any kinds.

I added an assert with documentation of why the other cases are left out. Only
high-level instructions should be instrumentable.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h
File src/IceInstrumentation.h (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:17: /// functions is has a trivial default
implementation to make subclasses more
On 2016/06/06 22:28:45, Roland McGrath wrote:
> s/is has/has/

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:22: #pragma clang diagnostic ignored
"-Wunused-parameter"
On 2016/06/06 22:27:03, Karl wrote:
> Don't change command line flags within a file, especially an include file.  By
> adding this pragma, you have turned off this feature for all later includes!
> 
> Rather, fix this as follows:
> 
> Replace method headings of methods like
> 
>    void foo(t1 arg1, ....) {}
> 
> with
> 
>    void foo(t1 arg1, ...) {
>      (void) arg1;
>      ...
>     }
> 
> The "(void) arg1;" statement will automatically turn off the warning.

This fix makes the code much larger and uglier. What I meant to do here is

#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunused-parameter"
#endif // __clang__

... lots of unused parameters ...

#ifdef __clang__
#pragma clang diagnostic pop
#endif // __clang__

This is used in IceCompileServer.cpp. Would that be ok here or should I accept
the code growth and void the parameters as suggested?

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:25: #ifndef SUBZERO_SRC_ICEASANLOWERING_H
On 2016/06/06 22:27:03, Karl wrote:
> The coding convention is that the define name should follow the file path,
> substituting '_' for all punctuation, and capitalizing letters.
> 
> This naming convention is more likely to avoid naming conflicts than some
> arbitrarily chosen name.
> 
> Hence, use SUBZERO_SRC_ICEINSTRUMENTATION_H

Done.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:39: protected:
On 2016/06/06 22:27:03, Karl wrote:
> Generally, we don't like introducing fields before the public API. The public
> API should come first, so that it is easy to find.

Done.

[Karl, 2016-06-07 14:42:22]

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

[Karl, 2016-06-07 14:42:41]

Also added Jim to the reviewers list.

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h
File src/IceInstrumentation.h (right):

https://codereview.chromium.org/2042063002/diff/1/src/IceInstrumentation.h#ne...
src/IceInstrumentation.h:22: #pragma clang diagnostic ignored
"-Wunused-parameter"
On 2016/06/07 00:43:06, tlively wrote:
> On 2016/06/06 22:27:03, Karl wrote:
> > Don't change command line flags within a file, especially an include file. 
By
> > adding this pragma, you have turned off this feature for all later includes!
> > 
> > Rather, fix this as follows:
> > 
> > Replace method headings of methods like
> > 
> >    void foo(t1 arg1, ....) {}
> > 
> > with
> > 
> >    void foo(t1 arg1, ...) {
> >      (void) arg1;
> >      ...
> >     }
> > 
> > The "(void) arg1;" statement will automatically turn off the warning.
> 
> This fix makes the code much larger and uglier. What I meant to do here is
> 
> #ifdef __clang__
> #pragma clang diagnostic push
> #pragma clang diagnostic ignored "-Wunused-parameter"
> #endif // __clang__
> 
> ... lots of unused parameters ...
> 
> #ifdef __clang__
> #pragma clang diagnostic pop
> #endif // __clang__
> 
> This is used in IceCompileServer.cpp. Would that be ok here or should I accept
> the code growth and void the parameters as suggested?

Please do NOT add code that changes command line arguments. While using push/pop
localizes it, it doesn't follow coding conventions.

Please add "(void) param;" instructions to the function bodies to remove the
error messages.

https://codereview.chromium.org/2042063002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceCfg.cpp#newcode243
src/IceCfg.cpp:243: if (!BuildDefs::minimal()) {
Shouldn't these also be under a CL flag for sanitizing?

https://codereview.chromium.org/2042063002/diff/20001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:248: Ctx->setInstrumentation(std::move(Instr));
Out of curiosity, could this be simplified to:

  Ctx->setInstrumentation(new Instrumentation(Ctx.get());

https://codereview.chromium.org/2042063002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:490: if (!BuildDefs::minimal() && Instrumentor)
Shouldn't this also be based on a command line flag?

I would expect that at some point, one may want to control instrumenting both
ASAN and MSAM. What is not clear by the way the code is written, how this code
has been designed for this possibility.

If you expect the Instrumentor to be conditional on command-line flags (and
hence only one instrumentor), this should be made clearer with comments on the
Instrument class. If you expect to use different instrument classes for each,
then why does the instrument class have virtual functions?

https://codereview.chromium.org/2042063002/diff/20001/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceInstrumentation....
src/IceInstrumentation.cpp:44: Inst *Instr = iteratorToInst(Context.getCur());
You probably should check to verify the instruction hasn't been deleted. The
model of instructions is that a pass may delete an instruction. When this
happens, the instruction isn't actually deleted, it is just marked as deleted.

While your sanitizer may run first now (avoiding this issue), it is better to be
safe, in case some new pass is added at a later date.

Add check on

   !Instr->isDeleted()

[tlively, 2016-06-07 18:14:42]

https://codereview.chromium.org/2042063002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceCfg.cpp#newcode243
src/IceCfg.cpp:243: if (!BuildDefs::minimal()) {
On 2016/06/07 14:42:40, Karl wrote:
> Shouldn't these also be under a CL flag for sanitizing?

Done.

https://codereview.chromium.org/2042063002/diff/20001/src/IceCompileServer.cpp
File src/IceCompileServer.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceCompileServer.cp...
src/IceCompileServer.cpp:248: Ctx->setInstrumentation(std::move(Instr));
On 2016/06/07 14:42:40, Karl wrote:
> Out of curiosity, could this be simplified to:
> 
>   Ctx->setInstrumentation(new Instrumentation(Ctx.get());

I just tried this and a couple variations but the compiler didn't like them.

https://codereview.chromium.org/2042063002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:490: if (!BuildDefs::minimal() && Instrumentor)
On 2016/06/07 14:42:40, Karl wrote:
> Shouldn't this also be based on a command line flag?
> 
> I would expect that at some point, one may want to control instrumenting both
> ASAN and MSAM. What is not clear by the way the code is written, how this code
> has been designed for this possibility.
> 
> If you expect the Instrumentor to be conditional on command-line flags (and
> hence only one instrumentor), this should be made clearer with comments on the
> Instrument class. If you expect to use different instrument classes for each,
> then why does the instrument class have virtual functions?

If no instrumentation has occurred then Instrumentor will be false. Added more
detail to top of IceInstrumentation.h explaining model.

https://codereview.chromium.org/2042063002/diff/20001/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2042063002/diff/20001/src/IceInstrumentation....
src/IceInstrumentation.cpp:44: Inst *Instr = iteratorToInst(Context.getCur());
On 2016/06/07 14:42:41, Karl wrote:
> You probably should check to verify the instruction hasn't been deleted. The
> model of instructions is that a pass may delete an instruction. When this
> happens, the instruction isn't actually deleted, it is just marked as deleted.
> 
> While your sanitizer may run first now (avoiding this issue), it is better to
be
> safe, in case some new pass is added at a later date.
> 
> Add check on
> 
>    !Instr->isDeleted()

Context.advanceNext() skips deleted instructions. I Moved advanceCur() and
advanceNext() into the loop to be more clear and added a comment explaining this
issue.

[Karl, 2016-06-07 20:50:56]

lgtm

[tlively, 2016-06-07 20:55:02]

Description was changed from

==========
Created Ice::Instrumentation base class and accompanying hooks.

Also added a command line flag for AddressSanitizer

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

to

==========
Created Ice::Instrumentation base class and accompanying hooks.

Also added a command line flag for AddressSanitizer

BUG=None
R=kschimpf@google.com

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

[tlively, 2016-06-07 20:55:04]

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