Subzero: Embed the revision string into translated output.

Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the translated output.  As a side effect, it is also embedded into the Subzero translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long after the fact to know exactly which revision was pushed, e.g. when trying to reproduce a bug or crash.

2. A translated binary can be inspected to make sure Chrome used the expected revision of Subzero.  (And also to verify that pnacl-sz was used rather than pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem overly strict about global initializer expectations.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-08-06 23:09:01]

Description was changed from

==========
Subzero: Embed the Subzero revision into translated output.

BUG= none
==========

to

==========
Subzero: Embed the revision string into translated output.

BUG= none
==========

[Jim Stichnoth, 2016-08-06 23:26:26]

Description was changed from

==========
Subzero: Embed the revision string into translated output.

BUG= none
==========

to

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedeed into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)



BUG= none
==========

[Jim Stichnoth, 2016-08-06 23:32:19]

Description was changed from

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedeed into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)



BUG= none
==========

to

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedeed into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem
overly strict about global initializer expectations.

BUG= none
==========

[Jim Stichnoth, 2016-08-07 00:20:10]

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

[Jim Stichnoth, 2016-08-07 00:20:35]

Description was changed from

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedeed into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem
overly strict about global initializer expectations.

BUG= none
==========

to

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedded into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem
overly strict about global initializer expectations.

BUG= none
==========

[Jim Stichnoth, 2016-08-07 00:21:14]

Description was changed from

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedded into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem
overly strict about global initializer expectations.

BUG= none
==========

to

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedded into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug or crash.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem
overly strict about global initializer expectations.

BUG= none
==========

[John, 2016-08-08 11:47:29]

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

https://codereview.chromium.org/2218363002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:474: if (getFlags().getEmitRevision()) {
This method is supposed to emit globals, but it also may add something to the
global variable list. Is this really the best place to add this?

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp
File src/IceRevision.cpp (right):

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp#newcode17
src/IceRevision.cpp:17: #define XSTRINGIFY(x) STRINGIFY(x)
why the indirection? XSTRINGIFY could just #x its argument, right?

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.h
File src/IceRevision.h (right):

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.h#newcode23
src/IceRevision.h:23: // Note: It would be possible to declare this a constexpr
char * and put its
nit: constexpr char[] -- :P

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.h#newcode29
src/IceRevision.h:29: }
// namespace Ice

[Jim Stichnoth, 2016-08-08 14:46:18]

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

https://codereview.chromium.org/2218363002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:474: if (getFlags().getEmitRevision()) {
On 2016/08/08 11:47:28, John wrote:
> This method is supposed to emit globals, but it also may add something to the
> global variable list. Is this really the best place to add this?

A side effect of lowering globals is that Globals is cleared or "consumed".  See
the end of lowerGlobals().

Tying in here seems simplest given that:
  (1) lowering is initiated from different places depending on whether threads=0
or threads!=0
  (2) each target has to implement its own lowerGlobals()

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp
File src/IceRevision.cpp (right):

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp#newcode17
src/IceRevision.cpp:17: #define XSTRINGIFY(x) STRINGIFY(x)
On 2016/08/08 11:47:28, John wrote:
> why the indirection? XSTRINGIFY could just #x its argument, right?

I think this is one of the oddities of the preprocessor, explained in e.g.
http://stackoverflow.com/questions/16989730/stringification-how-does-it-work .

I tried various simplifications, and ended up with either a compilation error,
or the version reported as "Subzero_revision_SUBZERO_REVISION" instead of having
the git hash.

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.h
File src/IceRevision.h (right):

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.h#newcode23
src/IceRevision.h:23: // Note: It would be possible to declare this a constexpr
char * and put its
On 2016/08/08 11:47:28, John wrote:
> nit: constexpr char[] -- :P

Done.

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.h#newcode29
src/IceRevision.h:29: }
On 2016/08/08 11:47:28, John wrote:
> // namespace Ice

Done.

[John, 2016-08-08 14:55:36]

lgtm

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

https://codereview.chromium.org/2218363002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:474: if (getFlags().getEmitRevision()) {
On 2016/08/08 14:46:18, stichnot wrote:
> On 2016/08/08 11:47:28, John wrote:
> > This method is supposed to emit globals, but it also may add something to
the
> > global variable list. Is this really the best place to add this?
> 
> A side effect of lowering globals is that Globals is cleared or "consumed". 
See
> the end of lowerGlobals().
> 
> Tying in here seems simplest given that:
>   (1) lowering is initiated from different places depending on whether
threads=0
> or threads!=0
>   (2) each target has to implement its own lowerGlobals()

Or maybe just add the global in the GlobalContext's ctor/initialization?

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp
File src/IceRevision.cpp (right):

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp#newcode17
src/IceRevision.cpp:17: #define XSTRINGIFY(x) STRINGIFY(x)
On 2016/08/08 14:46:18, stichnot wrote:
> On 2016/08/08 11:47:28, John wrote:
> > why the indirection? XSTRINGIFY could just #x its argument, right?
> 
> I think this is one of the oddities of the preprocessor, explained in e.g.
> http://stackoverflow.com/questions/16989730/stringification-how-does-it-work .
> 
> I tried various simplifications, and ended up with either a compilation error,
> or the version reported as "Subzero_revision_SUBZERO_REVISION" instead of
having
> the git hash.

Oh, yeah! STRINGFY(x) #x stringfies x without expanding macros... you could
**probably** define SUBZERO_REVISION to be a string (add the escaped
double-quotes in the Makefiles), but this is better.

[Jim Stichnoth, 2016-08-08 17:48:10]

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

https://codereview.chromium.org/2218363002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:474: if (getFlags().getEmitRevision()) {
On 2016/08/08 14:55:36, John wrote:
> On 2016/08/08 14:46:18, stichnot wrote:
> > On 2016/08/08 11:47:28, John wrote:
> > > This method is supposed to emit globals, but it also may add something to
> the
> > > global variable list. Is this really the best place to add this?
> > 
> > A side effect of lowering globals is that Globals is cleared or "consumed". 
> See
> > the end of lowerGlobals().
> > 
> > Tying in here seems simplest given that:
> >   (1) lowering is initiated from different places depending on whether
> threads=0
> > or threads!=0
> >   (2) each target has to implement its own lowerGlobals()
> 
> Or maybe just add the global in the GlobalContext's ctor/initialization?

Brilliant!  Done.

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp
File src/IceRevision.cpp (right):

https://codereview.chromium.org/2218363002/diff/1/src/IceRevision.cpp#newcode17
src/IceRevision.cpp:17: #define XSTRINGIFY(x) STRINGIFY(x)
On 2016/08/08 14:55:36, John wrote:
> On 2016/08/08 14:46:18, stichnot wrote:
> > On 2016/08/08 11:47:28, John wrote:
> > > why the indirection? XSTRINGIFY could just #x its argument, right?
> > 
> > I think this is one of the oddities of the preprocessor, explained in e.g.
> > http://stackoverflow.com/questions/16989730/stringification-how-does-it-work
.
> > 
> > I tried various simplifications, and ended up with either a compilation
error,
> > or the version reported as "Subzero_revision_SUBZERO_REVISION" instead of
> having
> > the git hash.
> 
> Oh, yeah! STRINGFY(x) #x stringfies x without expanding macros... you could
> **probably** define SUBZERO_REVISION to be a string (add the escaped
> double-quotes in the Makefiles), but this is better.

Yeah - I kind of prefer dealing with quotes in the C++ code, rather than
subjecting it to the weirdness of shell and Makefile quoting/escaping.

[Jim Stichnoth, 2016-08-08 21:15:11]

Description was changed from

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedded into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug or crash.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem
overly strict about global initializer expectations.

BUG= none
==========

to

==========
Subzero: Embed the revision string into translated output.

Modify the Makefiles to pass in the current git hash, which is embedded into the
translated output.  As a side effect, it is also embedded into the Subzero
translator binary.  This is useful for two reasons:

1. The PNaCl component update process is somewhat manual, making it tricky long
after the fact to know exactly which revision was pushed, e.g. when trying to
reproduce a bug or crash.

2. A translated binary can be inspected to make sure Chrome used the expected
revision of Subzero.  (And also to verify that pnacl-sz was used rather than
pnacl-llc.)

The revision string is suppressed for lit tests, because a number of tests seem
overly strict about global initializer expectations.

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-08-08 21:15:13]

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