Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent __has_builtin compiler differences from causing divergence #7836

Merged
merged 2 commits into from
Jun 27, 2023
Merged

Prevent __has_builtin compiler differences from causing divergence #7836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c0a9a4
test(xsnap): check trivial snapshot golden hashes
michaelfig May 25, 2023
a2b48e1
build(xsnap): use `-D__has_builtin(x)=1` to avoid divergence
michaelfig May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/xsnap/src/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ async function main(args, { env, stdout, spawn, fs, os }) {
`MODDABLE=${ModdableSDK.MODDABLE}`,
`GOAL=${goal}`,
`XSNAP_VERSION=${pkg.version}`,
`CC=cc "-D__has_builtin(x)=1"`,
'-f',
'xsnap-worker.mk',
],
Expand Down
15 changes: 15 additions & 0 deletions packages/xsnap/test/snapshots/test-xsnap.js.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Snapshot report for `test/test-xsnap.js`

The actual snapshot is saved in `test-xsnap.js.snap`.

Generated by [AVA](https://avajs.dev).

## produce golden snapshot hashes

> no evaluations

'91d30e59c1a087d58bb6d8eefcf1262e99e59cfc249222ab25f881ac642437e5'

> smallish safeInteger multiplication doesn't spill to XS_NUMBER_KIND

'4b48f6c58c08bb757efd3b8fb21891a386bdc5bfbae6803c8cb7df108e553ace'
Binary file added packages/xsnap/test/snapshots/test-xsnap.js.snap
View file
Open in desktop
Binary file not shown.
50 changes: 50 additions & 0 deletions packages/xsnap/test/test-xsnap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import '@endo/init/debug.js';
// eslint-disable-next-line import/no-extraneous-dependencies
import test from 'ava';

import { createHash } from 'crypto';
import * as proc from 'child_process';
import * as os from 'os';
import fs from 'fs';
Expand Down Expand Up @@ -302,6 +303,55 @@ const writeAndReadSnapshot = async (t, snapshotUseFs) => {
test('write and read snapshot (use FS)', writeAndReadSnapshot, true);
test('write and read snapshot (use stream)', writeAndReadSnapshot, false);

test('produce golden snapshot hashes', async t => {
t.log(`\
The snapshot hashes produced by this test were created from this package's
version of xsnap compiled for and run on Agoric's supported (within-consensus)
platforms.

The snapshot will change (and the test will fail) if xsnap or this platform
is not compatible with this predefined consensus. This is likely to happen
in the future when xsnap is upgraded, in which case there will need to be
special accommodation for the new version, not just generating new golden
hashes.
`);
const toEvals = [
[`no evaluations`, ''],
[
`smallish safeInteger multiplication doesn't spill to XS_NUMBER_KIND`,
`globalThis.bazinga = 100; globalThis.bazinga *= 1_000_000;`,
],
];
for await (const [description, toEval] of toEvals) {
t.log(description);
const messages = [];
async function handleCommand(message) {
messages.push(decode(message));
return new Uint8Array();
}

const vat0 = await xsnap({
...options(io),
handleCommand,
snapshotUseFs: false,
});
if (toEval) {
await vat0.evaluate(toEval);
}

const hash = createHash('sha256');
for await (const buf of vat0.makeSnapshotStream()) {
hash.update(buf);
}
await vat0.close();

const hexHash = hash.digest('hex');
t.log(`${description} produces golden hash ${hexHash}`);
t.snapshot(hexHash, description);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a note here to remind future test-runners of what we're asserting.

This test snapshot was created from a version of xsnap that was manually observed to avoid the spill (because it has the __builtin and can safely tell when it remains within the safe range of integers). We cannot test for the spill/not-spill directly without either parsing the xsnap heap snapshot, or writing C test code that performs a multiplication and then inspects the .kind field of the returned value. But we know that any change to the spill/non-spill behavior will change the heap snapshot hash.

The snapshot will change (and the test will fail) if either:

  • XS is changed in a way that changes the spill behavior (e.g. maybe they give up on that optimization for some reason)
  • XS is changed in any other way that changes the heap snapshot

It probably can't change in response to the compiler having/not-having the builtins, because the -D change means the not-having-builtins cases will simply fail to link, well before we get to this test case. And it won't change in response to the compiler having/not-having __has_builtin(), because the -D means we completely ignore the normal __has_builtin().

So as automated tests go, it's a bit too sensitive to things that 1: will happen, eventually, and 2: aren't what we're trying to assert. That's fine, especially since checking the real thing is too hard, but we should add a note, so when the test fails and the developer is considering ava's --update-snapshots, they can tell why this test was using a snapshot in the first place.

t.deepEqual(messages, [], `${description} messages`);
}
});

test('execute immediately after makeSnapshotStream', async t => {
const messages = [];
async function handleCommand(message) {
Expand Down
Loading