Start unit testing for functions in nc_inst_state.c

Start unit testing for functions in nc_inst_state.c

BUG= http://code.google.com/p/nativeclient/issues/detail?id=1613
TEST=src/trusted/validator_x86/nc_inst_state_Tests.cc
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=5018

[Brad Chen, 2011-04-21 20:36:22]

I appreciate your initiative in putting together a unit testing framework here,
for this critical part of the trusted code base. One specific question for now.

http://codereview.chromium.org/6883091/diff/7001/src/trusted/validator_x86/nc...
File src/trusted/validator_x86/nc_inst_state.c (right):

http://codereview.chromium.org/6883091/diff/7001/src/trusted/validator_x86/nc...
src/trusted/validator_x86/nc_inst_state.c:22: #include
"native_client/src/trusted/validator_x86/nc_inst_state_statics.c"
This seems slightly scary, although I'm at a loss for identifying anything
specific. Is there a better way of doing this?

[Karl, 2011-04-21 21:11:45]

http://codereview.chromium.org/6883091/diff/7001/src/trusted/validator_x86/nc...
File src/trusted/validator_x86/nc_inst_state.c (right):

http://codereview.chromium.org/6883091/diff/7001/src/trusted/validator_x86/nc...
src/trusted/validator_x86/nc_inst_state.c:22: #include
"native_client/src/trusted/validator_x86/nc_inst_state_statics.c"
On 2011/04/21 20:36:22, Brad Chen wrote:
> This seems slightly scary, although I'm at a loss for identifying anything
> specific. Is there a better way of doing this?


I talked to Cliff about this, and we both agreed that this was a reasonable
solution for testing static functions. What I've done is include it in both the
source nc_inst_state.cc and the tests in nc_inst_state_Tests.cc

[Karl, 2011-04-25 21:38:16]

Is anyone looking at this?

[Brad Chen, 2011-04-25 21:47:27]

LGTM. A couple nits below.

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
File src/trusted/validator_x86/nc_inst_state_Tests.cc (right):

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
src/trusted/validator_x86/nc_inst_state_Tests.cc:7: // Unit tests for code in
nc_inst_state.cc (and nc_inst_state_statics.c).
What was that notation you were going to use to indicate something as not being
part of the TCB? It should appear here someplace, right?

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
File src/trusted/validator_x86/nc_inst_state_statics.c (right):

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
src/trusted/validator_x86/nc_inst_state_statics.c:12: */
It would probably make sense to put in a #define protection as in .h files in
this file; that will help some readers recognize that it is included rather than
compiled.

[Mark Seaborn, 2011-04-25 23:29:48]

http://codereview.chromium.org/6883091/diff/2024/src/trusted/validator_x86/Re...
File src/trusted/validator_x86/RexPrefixes.h (right):

http://codereview.chromium.org/6883091/diff/2024/src/trusted/validator_x86/Re...
src/trusted/validator_x86/RexPrefixes.h:1: /*
This file's name does not follow our naming conventions.

http://codereview.chromium.org/6883091/diff/2024/src/trusted/validator_x86/nc...
File src/trusted/validator_x86/nc_inst_state_Tests.cc (right):

http://codereview.chromium.org/6883091/diff/2024/src/trusted/validator_x86/nc...
src/trusted/validator_x86/nc_inst_state_Tests.cc:1: /*
Why are you using non-standard naming for this file?  i.e. why is "Tests"
capitalised?

[Mark Seaborn, 2011-04-25 23:33:59]

This change fails on the Mac OS X 10.5 buildbot with:

/b/build/slave/mac10.5-m32-n32-opt/build/native_client/src/trusted/validator_x86/nc_inst_state_statics.c:
In member function 'void<unnamed>::NcInstStateTests::dummy()':
/b/build/slave/mac10.5-m32-n32-opt/build/native_client/src/trusted/validator_x86/nc_inst_state_statics.c:793:
warning: 'prefix_desc$next_length_adjustment' is used uninitialized in this
function
/b/build/slave/mac10.5-m32-n32-opt/build/native_client/src/trusted/validator_x86/nc_inst_state_statics.c:800:
warning: 'prefix_desc$matched_prefix' is used uninitialized in this function
scons: ***
[scons-out/opt-mac-x86-32/obj/src/trusted/validator_x86/nc_inst_state_Tests.o]
Error 1

[Karl, 2011-04-25 23:35:32]

Karl Schimpf



On Mon, Apr 25, 2011 at 4:33 PM, <mseaborn@chromium.org> wrote:

> This change fails on the Mac OS X 10.5 buildbot with:
>
>
>
/b/build/slave/mac10.5-m32-n32-opt/build/native_client/src/trusted/validator_x86/nc_inst_state_statics.c:
> In member function 'void<unnamed>::NcInstStateTests::dummy()':
>
>
/b/build/slave/mac10.5-m32-n32-opt/build/native_client/src/trusted/validator_x86/nc_inst_state_statics.c:793:
> warning: 'prefix_desc$next_length_adjustment' is used uninitialized in this
> function
>
>
/b/build/slave/mac10.5-m32-n32-opt/build/native_client/src/trusted/validator_x86/nc_inst_state_statics.c:800:
> warning: 'prefix_desc$matched_prefix' is used uninitialized in this
> function
> scons: ***
>
> [scons-out/opt-mac-x86-32/obj/src/trusted/validator_x86/nc_inst_state_Tests.o]
> Error 1
>
>
I see that. I've submitted revision 5022 to fix the problem.


>
>
> http://codereview.chromium.org/6883091/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Karl, 2011-06-24 18:15:00]

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
File src/trusted/validator_x86/nc_inst_state_Tests.cc (right):

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
src/trusted/validator_x86/nc_inst_state_Tests.cc:7: // Unit tests for code in
nc_inst_state.cc (and nc_inst_state_statics.c).
On 2011/04/25 21:47:27, Brad Chen wrote:
> What was that notation you were going to use to indicate something as not
being
> part of the TCB? It should appear here someplace, right?

Done.

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
File src/trusted/validator_x86/nc_inst_state_statics.c (right):

http://codereview.chromium.org/6883091/diff/2004/src/trusted/validator_x86/nc...
src/trusted/validator_x86/nc_inst_state_statics.c:12: */
On 2011/04/25 21:47:27, Brad Chen wrote:
> It would probably make sense to put in a #define protection as in .h files in
> this file; that will help some readers recognize that it is included rather
than
> compiled.

Done.