Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1686963002:
[Interpreter] Print constant pool in generate-bytecode-expectations (Closed)
|
Created:
4 years, 10 months ago by Stefano Sanfilippo Modified:
4 years, 10 months ago 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] Print constant pool in generate-bytecode-expectations
This is a follow-up to https://crrev.com/1671863002, adding the
capability to print the contents of the constant pool. The expected
type of the pool is taken from command line, and it's either:
* string/int/double: assume all constants have the specified type.
This way, we can emit a meaningful representation, e.g. a quoted
string for type string and so on. All the constants in the pool must
have the same type, otherwise one or more CHECK() will fail and the
program will eventually crash.
* mixed: print the InstanceType tag instead of the actual value.
This is the choice for those tests where the type of the constants in
the pool is not uniform, however only a type tag is printed, not the
actual value of the entries. SMIs are an exception, since they do not
have an InstanceType tag, so kInstanceTypeDontCare is printed instead.
In addition to that, functions Print{ExpectedSnippet,BytecodeSequence}
have been extracted with no functional change. It's just for improving
readability, since the code is becoming quite long.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/db52dbbbfe10cbabfd74e1882ddf3c0b3c0ecf03
Cr-Commit-Position: refs/heads/master@{#33888}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Use AsEscapedUC16ForJSON for escaping UC16 strings. #Patch Set 3 : Address reviewers' comments. #Messages
Total messages: 18 (10 generated)
Description was changed from ========== [Interpreter] Print constant pool contents in generate-bytecode-expectations This is a followup to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a followup to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ==========
ssanfilippo@chromium.org changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
Description was changed from ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a followup to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a follow-up to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a follow-up to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a follow-up to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Now we can print the contents of the constant pool. PTAL.
lgtm with some comments, thanks. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:193: } nit - move these PrintQuotedUC16Char and QuoteCStrin above PrintBytecodeOperand (general helpers functions first, then the element printer functions). https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:201: }; nit - move this to the top of the namespace https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:203: void PrintConstantString(std::ostream& stream, i::String* constant_string) { /s/PrintConstantString/PrintV8String/ /s/constant_string/string/ https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:217: PrintConstantString(std::cout, i::String::cast(*constant)); /s/std::cout/stream https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:250: if (num_constants > 0) { I would be fine with just always emitting a '{}' even if the num_constants is empty, which would avoid you having to special case the handler stuff below. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:276: void PrintFrameSize(std::ostream& stream, int frame_size) { nit - order the functions in the same order as they are called below (i.e. PrintFrameSize, PrintBytecodeSequence, PrintConstantPool. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:307: << kIndent << ' ' << bytecode_array->length() << ",\n" move printing of parameter_count into PrintFrameSize, and print the bytecode length and " {\n" in PrintBytecodeSequence. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:322: // nit - remove the comment (other than the TODO) given the comment above about always emitting the '{}'. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:330: void PrintExpectedSnippet(ConstantPoolType parsed_constant_pool_type, /s/parsed_constant_pool_type/constant_pool_type/
https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:193: } On 2016/02/10 15:20:32, rmcilroy wrote: > nit - move these PrintQuotedUC16Char and QuoteCStrin above PrintBytecodeOperand > (general helpers functions first, then the element printer functions). Done. Also, please notice that I got rid of the former. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:201: }; On 2016/02/10 15:20:32, rmcilroy wrote: > nit - move this to the top of the namespace Done. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:203: void PrintConstantString(std::ostream& stream, i::String* constant_string) { On 2016/02/10 15:20:32, rmcilroy wrote: > /s/PrintConstantString/PrintV8String/ > /s/constant_string/string/ Done. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:217: PrintConstantString(std::cout, i::String::cast(*constant)); On 2016/02/10 15:20:32, rmcilroy wrote: > /s/std::cout/stream Done. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:250: if (num_constants > 0) { On 2016/02/10 15:20:32, rmcilroy wrote: > I would be fine with just always emitting a '{}' even if the num_constants is > empty, which would avoid you having to special case the handler stuff below. You are right, although I'd prefer to skip the {} part since the test cases actually put 0 without the {}. In the end, the extra code for handling that case boils down to: if (constant_pool->lenght() > 0 && /* have handlers */) { stream << ",\n" << kIndent << "{},\n"; } WDYT? https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:276: void PrintFrameSize(std::ostream& stream, int frame_size) { On 2016/02/10 15:20:32, rmcilroy wrote: > nit - order the functions in the same order as they are called below (i.e. > PrintFrameSize, PrintBytecodeSequence, PrintConstantPool. Done. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:307: << kIndent << ' ' << bytecode_array->length() << ",\n" On 2016/02/10 15:20:32, rmcilroy wrote: > move printing of parameter_count into PrintFrameSize, and print the bytecode > length and " {\n" in PrintBytecodeSequence. Done. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:322: // On 2016/02/10 15:20:32, rmcilroy wrote: > nit - remove the comment (other than the TODO) given the comment above about > always emitting the '{}'. Done. https://codereview.chromium.org/1686963002/diff/40001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:330: void PrintExpectedSnippet(ConstantPoolType parsed_constant_pool_type, On 2016/02/10 15:20:32, rmcilroy wrote: > /s/parsed_constant_pool_type/constant_pool_type/ Done.
This is looking good. Is there anything that prevents deriving the constant pool type from the generated constant pool rather than asking a human for it?
The CQ bit was checked by ssanfilippo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1686963002/#ps70001 (title: "Address reviewers' comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686963002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686963002/70001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a follow-up to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a follow-up to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #3 (id:70001)
Message was sent while issue was closed.
Committed patchset #3 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a follow-up to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Print constant pool in generate-bytecode-expectations This is a follow-up to https://crrev.com/1671863002, adding the capability to print the contents of the constant pool. The expected type of the pool is taken from command line, and it's either: * string/int/double: assume all constants have the specified type. This way, we can emit a meaningful representation, e.g. a quoted string for type string and so on. All the constants in the pool must have the same type, otherwise one or more CHECK() will fail and the program will eventually crash. * mixed: print the InstanceType tag instead of the actual value. This is the choice for those tests where the type of the constants in the pool is not uniform, however only a type tag is printed, not the actual value of the entries. SMIs are an exception, since they do not have an InstanceType tag, so kInstanceTypeDontCare is printed instead. In addition to that, functions Print{ExpectedSnippet,BytecodeSequence} have been extracted with no functional change. It's just for improving readability, since the code is becoming quite long. BUG=v8:4280 LOG=N Committed: https://crrev.com/db52dbbbfe10cbabfd74e1882ddf3c0b3c0ecf03 Cr-Commit-Position: refs/heads/master@{#33888} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/db52dbbbfe10cbabfd74e1882ddf3c0b3c0ecf03 Cr-Commit-Position: refs/heads/master@{#33888} |
