Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10826182:
Validator_ragel: move checkdecoder test from Makefile to scons (Closed)
|
Created:
8 years, 4 months ago by Vlad Shcherbina Modified:
8 years, 4 months ago CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionValidator_ragel: move checkdecoder test from Makefile to scons
Checkdecoder is a test that takes few hours and compares DFA-based decoder with objdump output on a number of enumerated instructions. This test is not included into any test suite and is only run manually
./scons dfacheckdecoder
It makes sense to only run it when changes to DFA are made (that is, after ./scons dfagen). Since dfagen is currently linux only, dfacheckdecoder is somewhat platform-dependent as well.
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=9435
Patch Set 1 #
Total comments: 12
Patch Set 2 : reaction to remarks #
Total comments: 21
Patch Set 3 : reaction to Nick's remarks #Patch Set 4 : following Mark's suggestion, obtain binutils from gerrit repo instead of deps/third-party #
Total comments: 2
Patch Set 5 : remove comment about unneeded patch #
Total comments: 18
Patch Set 6 : next round of minor changes #
Total comments: 4
Patch Set 7 : rebase #
Messages
Total messages: 13 (0 generated)
Obviously it does not work without binutils in third-party (which I will hopefully add to repo today), so I've only tried it locally so far. Please take a look at build.scons logic and style.
Few nits, o/w LGTM, but please wait for the review of some python expert https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:230: {'32': '', '64': '-m amd64'}[bits]) Please add explicit "-m ia32" https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:237: source=['unreviewed/one_instruction_x86_%s.rl'%bits, one_valid_instr_rl], Long line https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:250: unoptimized_env.Append(CFLAGS='-O0') # does it make any difference? I don't think it's big deal, really. The whole test runs for hour or so. -O0 saves may be minute from it's start (if even that). https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:257: gas =File("#/../third_party/binutils/as-new") Please rename it to as.linux as-new is some wart of the long objdump's history, and we may want to run this test on some other system, not just on Linux in the future https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:258: objdump =File("#/../third_party/binutils/objdump") This should be objdump.linux https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:276: '--nthreads=`cat /proc/cpuinfo | grep processor | wc -l` ' #TODO: remove Long line
https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:230: {'32': '', '64': '-m amd64'}[bits]) On 2012/08/07 09:24:22, khim wrote: > Please add explicit "-m ia32" Done. https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:237: source=['unreviewed/one_instruction_x86_%s.rl'%bits, one_valid_instr_rl], On 2012/08/07 09:24:22, khim wrote: > Long line Done. https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:250: unoptimized_env.Append(CFLAGS='-O0') # does it make any difference? On 2012/08/07 09:24:22, khim wrote: > I don't think it's big deal, really. > > The whole test runs for hour or so. -O0 saves may be minute from it's start (if > even that). Then I remove it to make config simpler. https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:257: gas =File("#/../third_party/binutils/as-new") On 2012/08/07 09:24:22, khim wrote: > Please rename it to as.linux > > as-new is some wart of the long objdump's history, and we may want to run this > test on some other system, not just on Linux in the future Done. https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:258: objdump =File("#/../third_party/binutils/objdump") On 2012/08/07 09:24:22, khim wrote: > This should be objdump.linux Done. https://chromiumcodereview.appspot.com/10826182/diff/1/src/trusted/validator_... src/trusted/validator_ragel/build.scons:276: '--nthreads=`cat /proc/cpuinfo | grep processor | wc -l` ' #TODO: remove On 2012/08/07 09:24:22, khim wrote: > Long line Done.
https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:33: Single empty line. The only exception is two lines between top level declarations (def / class). https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:56: caller_lib = 'dfa_validate_caller_x86_%s' % caller_lib_bits Just interpolate env.get('TARGET_FULLARCH') / env.get('TARGET_SUBARCH') https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:80: Single empty line. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:222: (one_valid_instr_rl,) = env.Command( In our codebase we typically don't have parenthesis around tuple unpacks. I don't think I've ever seen a tuple unpack with a single element, however, so I'll bless this for the sake of clarity (Python single element tuple syntax is ugly). Don't do this in general, however. Alternatively you could simply subscript the command which would dodge style issues for a little loss of clarity and looser semantics. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:223: target='one_valid_instruction_x86_%s.rl'%bits, Spaces around binary operators. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:224: source=[gen_dfa]+INST_DEFS, Dito. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:228: '-m %s')%( Dito. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:229: ' '.join('${SOURCES[%d]}'%(i+1) for i in range(len(INST_DEFS))), Dito times 2. I leave the rest up to you. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:248: action='python ${SOURCES[0]} <"${SOURCES[1]}" >"${TARGET}"' Having Python explicitly read and write the files would require less shell magic and make (eventual) cross platform compatibility easier. Prefer simple command lines that are just a sequence of arguments. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:257: gas =File("#/../third_party/binutils/as.linux") Spaces around "=" for assignments. Also: we're slightly inconsistent about this, but I believe ${SCONSTRUCT_DIR} is preferred to #. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:260: fast_temp_for_test = '/dev/shm' I am trying to keep to style issues, but isn't this taking us even farther from Windows compatibility? It seems like a bad idea to do this at the build level. Also, is this going to leak temp files?
https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:33: On 2012/08/07 18:36:14, Nick Bray wrote: > Single empty line. The only exception is two lines between top level > declarations (def / class). Done. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:56: caller_lib = 'dfa_validate_caller_x86_%s' % caller_lib_bits On 2012/08/07 18:36:14, Nick Bray wrote: > Just interpolate env.get('TARGET_FULLARCH') / env.get('TARGET_SUBARCH') That's tricky, because in some places it is referenced explicitly by name (which is not very beautiful). Also, this code already was there, I merely moved it a little bit. Is it ok if I put TODO here and leave it for another CL? https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:80: On 2012/08/07 18:36:14, Nick Bray wrote: > Single empty line. Done. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:222: (one_valid_instr_rl,) = env.Command( On 2012/08/07 18:36:14, Nick Bray wrote: > In our codebase we typically don't have parenthesis around tuple unpacks. I > don't think I've ever seen a tuple unpack with a single element, however, so > I'll bless this for the sake of clarity (Python single element tuple syntax is > ugly). Don't do this in general, however. > > Alternatively you could simply subscript the command which would dodge style > issues for a little loss of clarity and looser semantics. My thoughts :) So I'll leave it as it was. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:229: ' '.join('${SOURCES[%d]}'%(i+1) for i in range(len(INST_DEFS))), On 2012/08/07 18:36:14, Nick Bray wrote: > Dito times 2. I leave the rest up to you. Done (I hope). https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:257: gas =File("#/../third_party/binutils/as.linux") On 2012/08/07 18:36:14, Nick Bray wrote: > Spaces around "=" for assignments. > > Also: we're slightly inconsistent about this, but I believe > > ${SCONSTRUCT_DIR} > > is preferred to #. Oops, done. If I understand it correctly, ${SCONSTRUCT_DIR} is relative to build.scons, and I need the top-level. Perhaps, using sconstruct dir to reach third-party is not the best approach, but anyway I'm going to change the way binutils are obtained in the upcoming patchsets to this issue. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:260: fast_temp_for_test = '/dev/shm' On 2012/08/07 18:36:14, Nick Bray wrote: > I am trying to keep to style issues, but isn't this taking us even farther from > Windows compatibility? It seems like a bad idea to do this at the build level. > Also, is this going to leak temp files? This is a test for DFA-based decoder. It takes few hours to run, so it makes sense to only run it when DFA is generated (so it's not added to any test suits). Currently DFA can only be built on linux (and I don't think building it on other platforms is a priority). Constructed DFA can be used on any platform. Considering leaks - all temp files are deleted, at least when tests pass. Failed tests may leak files, but it's not very pressing concern because of limited use of this test. Is it enough if I just add TODO for now?
One nit o/w LGTM if Nick is happy with this
https://chromiumcodereview.appspot.com/10826182/diff/4010/src/trusted/validat... File src/trusted/validator_ragel/obtain_binutils.py (right): https://chromiumcodereview.appspot.com/10826182/diff/4010/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:24: # Add lfence/mfence/sfence - non-canonical encodings. After lond discussion we decided that this is not needed and it's not actually supported by the objdump release 6993545d5650fa77e75e9ae4b52c5703a53c53cc
https://chromiumcodereview.appspot.com/10826182/diff/4010/src/trusted/validat... File src/trusted/validator_ragel/obtain_binutils.py (right): https://chromiumcodereview.appspot.com/10826182/diff/4010/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:24: # Add lfence/mfence/sfence - non-canonical encodings. On 2012/08/08 13:40:24, khim wrote: > After lond discussion we decided that this is not needed and it's not actually > supported by the objdump release 6993545d5650fa77e75e9ae4b52c5703a53c53cc Done.
https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:257: gas =File("#/../third_party/binutils/as.linux") FYI ${SCONSTRUCT_DIR} is fixed relative to (the only) SConstruct, not the *.scons files. On 2012/08/08 08:19:30, Vlad Shcherbina wrote: > On 2012/08/07 18:36:14, Nick Bray wrote: > > Spaces around "=" for assignments. > > > > Also: we're slightly inconsistent about this, but I believe > > > > ${SCONSTRUCT_DIR} > > > > is preferred to #. > > Oops, done. > > If I understand it correctly, ${SCONSTRUCT_DIR} is relative to build.scons, and > I need the top-level. Perhaps, using sconstruct dir to reach third-party is not > the best approach, but anyway I'm going to change the way binutils are obtained > in the upcoming patchsets to this issue. https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:260: fast_temp_for_test = '/dev/shm' Add some comments about this test? Give some idea how long it takes to run and why it takes this long to run? I would not have expected that we had a test in SCons that takes hours. > This is a test for DFA-based decoder. It takes few hours to run, so it makes > sense to only run it when DFA is generated (so it's not added to any test > suits). Currently DFA can only be built on linux (and I don't think building it > on other platforms is a priority). Constructed DFA can be used on any platform. > > Considering leaks - all temp files are deleted, at least when tests pass. Failed > tests may leak files, but it's not very pressing concern because of limited use > of this test. Is it enough if I just add TODO for now? https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/build.scons:54: # interpolate env.get('TARGET_FULLARCH') / env.get('TARGET_SUBARCH') instead I think you're missing a piece of the puzzle. env.get('TARGET_FULLARCH') returns 'x86_32', 'x86_64', etc. env.get('TARGET_SUBARCH') will return '32' or '64'. You can interpolate these values directly (using Python's interpolation, not SCons') rather than calculating caller_lib_bits. It's equivalent but simpler. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... File src/trusted/validator_ragel/obtain_binutils.py (right): https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:1: #!/usr/bin/python Needs copyright header. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:5: %s <objdump> <gas> It doesn't seem like this is really a docstring, if you're interpolating it. Drop it down to the main function? https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:11: import sys Alphabetize. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:32: result = os.system(cmd) I have found that scripts like this benefit from: print print "Running: " + cmd print So you know what it's doing. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:35: exit(result) Use sys.exit instead - it is more standard and "exit" appears to be entirely undocumented. http://docs.python.org/library/functions.html I suspect it may not be an official part of the Python API, rather something from the interpreter loop is leaking in to __builtins__. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:53: # to please make, set the same modification time Capitalize, period at the end, possibly start the sentence with "Set" and move the beginning to the end. Also, why is this an issue? Comment. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:57: # they both are required to make binutils, Capitalize, period at end. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:66: os.chdir(old_dir) Why? The process terminates in this case. You probably want to finally: shutil.rmtree(CHECKOUT_DIR), however.
Nick, please take another look. Although I have provisional LGTМ from Victor, could you please explicitly make another one when you are happy with the CL? https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/3/src/trusted/validator_... src/trusted/validator_ragel/build.scons:260: fast_temp_for_test = '/dev/shm' On 2012/08/08 18:24:44, Nick Bray wrote: > Add some comments about this test? Give some idea how long it takes to run and > why it takes this long to run? I would not have expected that we had a test in > SCons that takes hours. > > > This is a test for DFA-based decoder. It takes few hours to run, so it makes > > sense to only run it when DFA is generated (so it's not added to any test > > suits). Currently DFA can only be built on linux (and I don't think building > it > > on other platforms is a priority). Constructed DFA can be used on any > platform. > > > > Considering leaks - all temp files are deleted, at least when tests pass. > Failed > > tests may leak files, but it's not very pressing concern because of limited > use > > of this test. Is it enough if I just add TODO for now? > Done. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/build.scons:54: # interpolate env.get('TARGET_FULLARCH') / env.get('TARGET_SUBARCH') instead On 2012/08/08 18:24:44, Nick Bray wrote: > I think you're missing a piece of the puzzle. > > env.get('TARGET_FULLARCH') returns 'x86_32', 'x86_64', etc. > env.get('TARGET_SUBARCH') will return '32' or '64'. > > You can interpolate these values directly (using Python's interpolation, not > SCons') rather than calculating caller_lib_bits. It's equivalent but simpler. Done. I used only env.get('TARGET_SUBARCH'), because env.get('TARGET_FULLARCH') returns 'x86-32' and I need 'x86_32'. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... File src/trusted/validator_ragel/obtain_binutils.py (right): https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:1: #!/usr/bin/python On 2012/08/08 18:24:44, Nick Bray wrote: > Needs copyright header. Done. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:5: %s <objdump> <gas> On 2012/08/08 18:24:44, Nick Bray wrote: > It doesn't seem like this is really a docstring, if you're interpolating it. > Drop it down to the main function? Made it proper docstring instead. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:11: import sys On 2012/08/08 18:24:44, Nick Bray wrote: > Alphabetize. Done. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:32: result = os.system(cmd) On 2012/08/08 18:24:44, Nick Bray wrote: > I have found that scripts like this benefit from: > > print > print "Running: " + cmd > print > > So you know what it's doing. Done. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:35: exit(result) On 2012/08/08 18:24:44, Nick Bray wrote: > Use sys.exit instead - it is more standard and "exit" appears to be entirely > undocumented. > http://docs.python.org/library/functions.html > I suspect it may not be an official part of the Python API, rather something > from the interpreter loop is leaking in to __builtins__. Oh, I never suspected that. Done. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:53: # to please make, set the same modification time On 2012/08/08 18:24:44, Nick Bray wrote: > Capitalize, period at the end, possibly start the sentence with "Set" and move > the beginning to the end. Also, why is this an issue? Comment. I rechecked, and it's actually not needed. So I removed it altogether. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:57: # they both are required to make binutils, On 2012/08/08 18:24:44, Nick Bray wrote: > Capitalize, period at end. Done. https://chromiumcodereview.appspot.com/10826182/diff/1008/src/trusted/validat... src/trusted/validator_ragel/obtain_binutils.py:66: os.chdir(old_dir) On 2012/08/08 18:24:44, Nick Bray wrote: > Why? The process terminates in this case. You probably want to finally: > shutil.rmtree(CHECKOUT_DIR), however. You are right. But I also need to chdir back because objdump and gas target locations could be relative.
LGTM w/ two non-binding suggestions. https://chromiumcodereview.appspot.com/10826182/diff/12006/src/trusted/valida... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/12006/src/trusted/valida... src/trusted/validator_ragel/build.scons:235: ' '.join('${SOURCES[%d]}' % (i + 1) Line breaking a generator expression makes it hard to read. I'd lift this into its own statement (outside the loop, even)? https://chromiumcodereview.appspot.com/10826182/diff/12006/src/trusted/valida... src/trusted/validator_ragel/build.scons:255: action='python ${SOURCES[0]} <"${SOURCES[1]}" >"${TARGET}"' Not critical for this CL, but I'd delegate the logic of reading and writing files to the script rather than using stdin/stdout redirection.
https://chromiumcodereview.appspot.com/10826182/diff/12006/src/trusted/valida... File src/trusted/validator_ragel/build.scons (right): https://chromiumcodereview.appspot.com/10826182/diff/12006/src/trusted/valida... src/trusted/validator_ragel/build.scons:235: ' '.join('${SOURCES[%d]}' % (i + 1) On 2012/08/09 20:16:20, Nick Bray wrote: > Line breaking a generator expression makes it hard to read. I'd lift this into > its own statement (outside the loop, even)? Since this construction relies on the fact that SOURCES are of the form [gen_dfa] + INST_DEFS, I don't want to move it away from it. https://chromiumcodereview.appspot.com/10826182/diff/12006/src/trusted/valida... src/trusted/validator_ragel/build.scons:255: action='python ${SOURCES[0]} <"${SOURCES[1]}" >"${TARGET}"' On 2012/08/09 20:16:20, Nick Bray wrote: > Not critical for this CL, but I'd delegate the logic of reading and writing > files to the script rather than using stdin/stdout redirection. I'll make it in a separate small CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
