Library bytecode verification unittests #1505

tcare · 2016-08-27T00:41:53Z

Reintroduces bytecode verification unittests for library code. We previously had this check in SD days, but it was complicated and called external scripts to work. The new version uses just RL to compare against the header file directly. A number of changes had to be made:

Removed non-standard %REGRESS% psuedo environment variable. Instead, test files and baselines are relative paths based on the directory containing the rlexe.xml.
Fixed the relative path behavior of runtests.py. Also added a guard to the timezone set so runtests.py works on Windows again.
Fixed an issue where ch would generate different bytecode than jshost due to an implicit fscrNoAsmJs parser flag in ScriptContext. We don't need this flag on the initial entry, so it can be removed from the JSRT entrypoint for library bytecode generation.
Disabled using the release version for serialized library bytecode, instead using a fixed version (all 0's). This is OK as long as we check the bytecode integrity in unit tests.
Added an implicit disable_jit tag exclusion when not running disable JIT; this allows us to run some test only under disable JIT and not normally.
Added a new bytecode directory with the xml for the tests.

Due to the changes to serialization all of the library bytecode headers needed to be changed.

tcare · 2016-08-27T00:49:09Z

@curtisman @MikeHolman @ianwjhalliday please review

FYI @digitalinfinity @dilijev

dilijev · 2016-08-27T00:49:56Z

lib/Runtime/ByteCode/ByteCodeSerializer.cpp

+            V1.value = 0;
+            V2.value = 0;
+            V3.value = 0;
+            V4.value = 0;


For releases, we need to make sure we have the GUIDs in the ByteCode so that ReleaseVersioningScheme can be applied. In that scenario, can we also ensure consistent bytecode between Chakra and ChakraCore.dll

Wait, I understand now. For library code, we don't want to do the GUID check? We still need to make sure the ByteCode is updated when it is supposed to be. How else can we specifically test this?

In reply to: 76506572 [](ancestors = 76506572)

For the library code it doesn't really matter, as long as the unit tests catch the incorrectness. We usually catch this in unit tests when we fail unit tests, but we don't always catch them right now. E.g. the jshost/ch inconsistency fixed in this change. The unit tests are guaranteed to catch the bytecode changes.

dilijev · 2016-08-27T00:51:44Z

//-------------------------------------------------------------------------------------------------------

Remove this file?

Refers to: test/out:1 in bdf71d1. [](commit_id = bdf71d1, deletion_comment = False)

tcare · 2016-08-27T00:52:24Z

Oops thanks for the catch.

dilijev · 2016-08-27T00:55:55Z

test/ByteCode/rlexe.xml

+  <test>
+    <default>
+      <tags>exclude_dynapogo,exclude_amd64,exclude_arm,require_backend</tags>
+      <compile-flags>-GenerateLibraryByteCodeHeader</compile-flags>


-GenerateLibraryByteCodeHeader [](start = 6, length = 61)

So the plan is to use a test to generate the entire bytecode and check for differences, and then alert the user to the fact that the bytecode needs to be updated?

Since we've already got the binaries handy, this is a good approach.

I think right now the way we regen bytecode depends on scripts. Since we're talking about eventually replacing rl.exe anyway, I don't want to suggest that we put that functionality into rl.exe, but we should define a unified way to generate bytecode.

We used to use scripts in SD but never ported it over to git. There essentially doesn't need to be a script; it was doing what RL does. The only real change is that instead of writing to a file I write to stdout.

tcare · 2016-08-27T00:55:57Z

Looks like Jenkins is having trouble with release and crossplat. I'll investigate next week.

dilijev · 2016-08-27T00:57:26Z

@tcare
PreFAST warnings from release builds on Windows.
testrunner.py doesn't support %VAR% syntax so it can't interpret the test definitions.

dilijev · 2016-08-27T00:59:47Z

test/ByteCode/rlexe.xml

+    <default>
+      <tags>exclude_dynapogo,exclude_amd64,exclude_arm,require_backend</tags>
+      <compile-flags>-GenerateLibraryByteCodeHeader</compile-flags>
+      <files>%REGRESS%\..\lib\Runtime\Library\InJavascript\Intl.js</files>


%REGRESS% [](start = 13, length = 9)

I think paths are relative to the test location so you can just use ..\..\lib\

This doesn't work for using jshost to test core with existing scripts. I'll see if I can work around it..

Good point. Simplest solution is to make testrunner.py support %VAR%

tcare · 2016-08-29T17:39:16Z

Closing for now until cross plat is resolved.

dilijev · 2016-08-29T17:44:07Z

@tcare Why not keep it open to track it (do we think this will take a long time to resolve?)

tcare · 2016-08-29T17:46:14Z

It'll be considerably different. I want to move away from %REGRESS%. Ideally I would move away from RL too ;)

Reintroduces bytecode verification unittests for library code. We previously had this check in SD days, but it was complicated and called external scripts to work. The new version uses just RL to compare against the header file directly. A number of changes had to be made: - Removed non-standard %REGRESS% psuedo environment variable. Instead, test files and baselines are relative paths based on the directory containing the rlexe.xml. - Fixed the relative path behavior of runtests.py. Also added a guard to the timezone set so runtests.py works on Windows again. - Fixed an issue where ch would generate different bytecode than jshost due to an implicit fscrNoAsmJs parser flag in ScriptContext. We don't need this flag on the initial entry, so it can be removed from the JSRT entrypoint for library bytecode generation. - Disabled using the release version for serialized library bytecode, instead using a fixed version (all 0's). This is OK as long as we check the bytecode integrity in unit tests. - Added an implicit disable_jit tag exclusion when not running disable JIT; this allows us to run some test only under disable JIT and not normally. - Added a new bytecode directory with the xml for the tests. Due to the changes to serialization all of the library bytecode headers needed to be changed.

tcare · 2016-08-30T23:19:59Z

Reopened.

%REGRESS% is gone. We now use the test folder as the working path.

@jianchun can you please review the runtests.py changes?

tcare · 2016-08-30T23:30:28Z

Looks like runtests.py needs more work. Unfortunately we don't have a good buddy build mechanism for Jenkins at the moment. I'll ping back when I update.

tcare · 2016-08-31T21:55:17Z

Moving out serialization work to #1520

msftclas added the cla-not-required label Aug 27, 2016

tcare force-pushed the bytecode branch 2 times, most recently from 1179a94 to bdf71d1 Compare August 27, 2016 00:47

dilijev reviewed Aug 27, 2016
View reviewed changes

tcare closed this Aug 29, 2016

tcare reopened this Aug 30, 2016

msftclas added the cla-not-required label Aug 30, 2016

tcare force-pushed the bytecode branch from bdf71d1 to 13294ba Compare August 30, 2016 23:13

tcare closed this Aug 31, 2016

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade bluebird from 3.5.1 to 3.7.2 saurabharch/ChakraCore#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library bytecode verification unittests #1505

Library bytecode verification unittests #1505

tcare commented Aug 27, 2016 •

edited

tcare commented Aug 27, 2016

dilijev Aug 27, 2016

dilijev Aug 27, 2016

tcare Aug 27, 2016

dilijev commented Aug 27, 2016

tcare commented Aug 27, 2016

dilijev Aug 27, 2016

tcare Aug 27, 2016

tcare commented Aug 27, 2016

dilijev commented Aug 27, 2016 •

edited

dilijev Aug 27, 2016 •

edited

tcare Aug 27, 2016 •

edited

dilijev Aug 27, 2016

tcare commented Aug 29, 2016

dilijev commented Aug 29, 2016

tcare commented Aug 29, 2016

tcare commented Aug 30, 2016

tcare commented Aug 30, 2016

tcare commented Aug 31, 2016

Library bytecode verification unittests #1505

Library bytecode verification unittests #1505

Conversation

tcare commented Aug 27, 2016 • edited

tcare commented Aug 27, 2016

dilijev Aug 27, 2016

Choose a reason for hiding this comment

dilijev Aug 27, 2016

Choose a reason for hiding this comment

tcare Aug 27, 2016

Choose a reason for hiding this comment

dilijev commented Aug 27, 2016

tcare commented Aug 27, 2016

dilijev Aug 27, 2016

Choose a reason for hiding this comment

tcare Aug 27, 2016

Choose a reason for hiding this comment

tcare commented Aug 27, 2016

dilijev commented Aug 27, 2016 • edited

dilijev Aug 27, 2016 • edited

Choose a reason for hiding this comment

tcare Aug 27, 2016 • edited

Choose a reason for hiding this comment

dilijev Aug 27, 2016

Choose a reason for hiding this comment

tcare commented Aug 29, 2016

dilijev commented Aug 29, 2016

tcare commented Aug 29, 2016

tcare commented Aug 30, 2016

tcare commented Aug 30, 2016

tcare commented Aug 31, 2016

tcare commented Aug 27, 2016 •

edited

dilijev commented Aug 27, 2016 •

edited

dilijev Aug 27, 2016 •

edited

tcare Aug 27, 2016 •

edited