Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(7)

Issue 1688383003: [Interpreter] Change the output format of generate-bytecode-expectations. (Closed)

Created:
4 years, 10 months ago by Stefano Sanfilippo
Modified:
4 years, 10 months ago
Reviewers:
oth, rmcilroy
CC:
v8-reviews_googlegroups.com, oth, rmcilroy
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[Interpreter] Change the output format of generate-bytecode-expectations. Now the tool produces a far more readable output format, which bears a lot of resemblance to YAML. In fact, the output should be machine parseable as such, one document per testcase. However, the output format may be subject to changes in future, so don't rely on this property. In general, the output format has been optimized for producing a meaningful textual diff, while keeping a decent readability as well. Therefore, not everything is as compact as it could be, e.g. for an empty const pool we get: constant pool: [ ] instead of: constant pool: [] Also, trailing commas are always inserted in lists. Additionally, now the tool accepts its output format as input. When operating in this mode, all the snippets are extracted, processed and the output is then emitted as usual. If nothing has changed, the output should match the input. This is very useful for catching bugs in the bytecode generation by running a textual diff against a known-good file. The core (namely bytecode-expectations.cc) has been extracted from the original cc file, which provides the utility as usual. The definitions in the matching header of the library have been moved into the v8::internal::interpreter namespace. The library exposes a class ExpectationPrinter, with a method PrintExpectation, which takes a test snippet as input, and writes the formatted expectation to the supplied stream. One might then use a std::stringstream to retrieve the results as a string and run it through a diff utility. BUG=v8:4280 LOG=N Committed: https://crrev.com/e082ebdbf3f352d55850c87dff3d5d01d772655b Cr-Commit-Position: refs/heads/master@{#33997}

Patch Set 1 #

Patch Set 2 : Do not create a static lib target. #

Total comments: 17

Patch Set 3 : Change bytecode-expectations API. #

Patch Set 4 : Address reviewers' comments. #

Patch Set 5 : Fix help message. #

Total comments: 42

Patch Set 6 : #

Patch Set 7 : Second round of fixes. #

Patch Set 8 : Rename program options, move methods out of class. #

Total comments: 26

Patch Set 9 : Third round of reviews + fix all lint warnings. #

Patch Set 10 : Simplify indent stripping with a CHECK, rebase changed cctest.gyp #

Patch Set 11 : Fix Clang on Mac OS X compiler error. #

Patch Set 12 : Fix MSVC on Windows compiler error. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+356 lines, -552 lines) Patch
M test/cctest/cctest.gyp View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -1 line 0 comments Download
A test/cctest/interpreter/bytecode-expectations-printer.h View 1 2 3 4 5 6 7 8 1 chunk +87 lines, -0 lines 0 comments Download
A + test/cctest/interpreter/bytecode-expectations-printer.cc View 1 2 3 4 5 6 7 8 8 chunks +92 lines, -244 lines 0 comments Download
M test/cctest/interpreter/generate-bytecode-expectations.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +172 lines, -307 lines 0 comments Download

Messages

Total messages: 43 (23 generated)
Stefano Sanfilippo
Here is the same utility, with a better output format. The core has been moved ...
4 years, 10 months ago (2016-02-12 12:17:27 UTC) #5
rmcilroy
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp#newcode365 test/cctest/cctest.gyp:365: '../../test/cctest/interpreter/bytecode-expectations.cc', Also include the .h file https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter/bytecode-expectations.h File test/cctest/interpreter/bytecode-expectations.h ...
4 years, 10 months ago (2016-02-12 12:33:11 UTC) #7
Stefano Sanfilippo
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/cctest.gyp#newcode365 test/cctest/cctest.gyp:365: '../../test/cctest/interpreter/bytecode-expectations.cc', On 2016/02/12 12:33:10, rmcilroy wrote: > Also include ...
4 years, 10 months ago (2016-02-12 14:24:00 UTC) #8
oth
A few minor comments, but looking good. https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter/generate-bytecode-expectations.cc File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter/generate-bytecode-expectations.cc#newcode46 test/cctest/interpreter/generate-bytecode-expectations.cc:46: bool ReadNextSnippet(std::string* ...
4 years, 10 months ago (2016-02-12 14:40:14 UTC) #11
Stefano Sanfilippo
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter/generate-bytecode-expectations.cc File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter/generate-bytecode-expectations.cc#newcode46 test/cctest/interpreter/generate-bytecode-expectations.cc:46: bool ReadNextSnippet(std::string* string, std::istream& stream) { On 2016/02/12 14:40:14, ...
4 years, 10 months ago (2016-02-12 14:49:53 UTC) #13
rmcilroy
https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interpreter/bytecode-expectations.cc File test/cctest/interpreter/bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interpreter/bytecode-expectations.cc#newcode25 test/cctest/interpreter/bytecode-expectations.cc:25: i::Isolate* GetInternalIsolate(v8::Isolate* isolate) { Move this to generate-bytecode-expectations.cc and ...
4 years, 10 months ago (2016-02-12 15:29:06 UTC) #14
rmcilroy
https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter/generate-bytecode-expectations.cc File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/40001/test/cctest/interpreter/generate-bytecode-expectations.cc#newcode57 test/cctest/interpreter/generate-bytecode-expectations.cc:57: // Skip two leading spaces we added, if the ...
4 years, 10 months ago (2016-02-12 15:59:05 UTC) #15
Stefano Sanfilippo
https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interpreter/bytecode-expectations.cc File test/cctest/interpreter/bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interpreter/bytecode-expectations.cc#newcode25 test/cctest/interpreter/bytecode-expectations.cc:25: i::Isolate* GetInternalIsolate(v8::Isolate* isolate) { On 2016/02/12 15:29:05, rmcilroy wrote: ...
4 years, 10 months ago (2016-02-12 18:29:41 UTC) #16
rmcilroy
LGTM once comments are addressed. Thanks. https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interpreter/bytecode-expectations.cc File test/cctest/interpreter/bytecode-expectations.cc (right): https://codereview.chromium.org/1688383003/diff/100001/test/cctest/interpreter/bytecode-expectations.cc#newcode25 test/cctest/interpreter/bytecode-expectations.cc:25: i::Isolate* GetInternalIsolate(v8::Isolate* isolate) ...
4 years, 10 months ago (2016-02-15 11:15:20 UTC) #17
Stefano Sanfilippo
https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interpreter/bytecode-expectations-printer.cc File test/cctest/interpreter/bytecode-expectations-printer.cc (right): https://codereview.chromium.org/1688383003/diff/160001/test/cctest/interpreter/bytecode-expectations-printer.cc#newcode27 test/cctest/interpreter/bytecode-expectations-printer.cc:27: i::Isolate* BytecodeExpectationsPrinter::GetInternalIsolate() const { On 2016/02/15 11:15:19, rmcilroy wrote: ...
4 years, 10 months ago (2016-02-15 13:26:34 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688383003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/200001
4 years, 10 months ago (2016-02-15 14:29:43 UTC) #20
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/15454)
4 years, 10 months ago (2016-02-15 14:33:13 UTC) #22
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688383003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/220001
4 years, 10 months ago (2016-02-15 14:45:37 UTC) #24
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compile_rel/builds/10388)
4 years, 10 months ago (2016-02-15 14:49:47 UTC) #26
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688383003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/240001
4 years, 10 months ago (2016-02-15 14:52:16 UTC) #28
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-15 15:09:36 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688383003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/240001
4 years, 10 months ago (2016-02-15 15:15:19 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688383003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688383003/240001
4 years, 10 months ago (2016-02-15 15:18:55 UTC) #39
commit-bot: I haz the power
Committed patchset #12 (id:240001)
4 years, 10 months ago (2016-02-15 15:20:26 UTC) #41
commit-bot: I haz the power
4 years, 10 months ago (2016-02-15 15:20:39 UTC) #43
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/e082ebdbf3f352d55850c87dff3d5d01d772655b
Cr-Commit-Position: refs/heads/master@{#33997}

Powered by Google App Engine
This is Rietveld 408576698