Embedded SDK hash in kernel binaries

[mkustermann]

There are several layers of compatibility in the VM regarding to kernel. One is whether the VM can correctly parse Kernel files. Another is whether the kernel file actually has the right information inside it (e.g. the right async transformer has run, the right classes/fields/variables are in it).

The second point does not have any validation atm and can lead to customers accidentally using kernel files of an older version, which the VM can still parse, but the kernel files might have the wrong information in it (e.g. old async transformer ran instead of new one).

To make such cases not crash the VM and instead give a nicer error message, we want the VM to validate that the Kernel it consumes comes from the same Dart SDK commit as the VM was built at.

We already have support for embedding a hash in the VM which validates that e.g. AOT snapshots given to the VM were produced by gen_snapshot from the same sources.

=> We would like to extend this to kernel files and embed a hash on those.

/cc @mraleph @johnniwinther

[alexmarkov]

It is not clear why we can't bump kernel version when making incompatible changes in transformations to solve this problem. While format remains intact, if a change is not backwards compatible we should definitely bump kernel version and adjust minimum supported version in the VM so VM doesn't pretend it is supporting older versions.

I agree that there is a value in bumping version automatically, but there are also disadvantages. The vast majority of changes are actually binary compatible, which provides more flexibility during development (for example consider bisecting/debugging a crash in gen_snapshot in Dart SDK repo on a dill file which was generated once in flutter). I think it would be useful to have an option to suppress this check if needed.

In the past, there was a requirement to allow 1-2 week version skew most of the time, which we fulfilled with backwards compatible support of older kernel files in the VM. Incompatible changes were extremely rare and only when it was impossible to make a soft transition. While there might not be such requirement right now, hash-based versioning would prevent that in future if it is actually needed somewhere. I guess we will find out what actually relies on backwards compatibility of kernel binaries only when we roll new versioning scheme.

[mkustermann]

There's two situations: If we have to be compatible (due to customers needs) some breaking changes become really hard (or not feasible at all) to roll out. If we don't have to maintain compatibility (no customer needs it), not providing it will significantly improve the VM's team productivity when doing breaking changes and provides our team with more flexibility. With no real downsides. (As you say, for VM development we can provide a way to opt-out of the verification)

Right now it seems that there are no known customer needs for maintaining compatibility. We will test this assumption with this work. If there happens to be cases where customers depend on compatibility, we will now actually know - which is great :)

Right now the only enforcement we have is the kernel binary format itself. There is no enforcement of ABI, kernel transformers that run, etc. We have had several occurrences of customers accidentally using incompatible kernel files which the VM didn't refuse, where the VM crashed in various ways. This caused a lot of wasted time to debug issues!

So we want to enforce it, which we can only do by embedding a hash/version in the kernel file and making the VM verify it when it consumes kernel. Using a manually maintained number can be error prone if forgotten to be updated and also slows down development (we know that out of experience with the abi bot).

Maybe @mraleph @a-siva have a different opinion?

[jensjoha]

It's also worth remembering that we can initialize from a dill and reuse the parts that's unchanged. This is for instance done in flutter.
Adding some "hash" that has to match for the vm to be able to load it, would mean that we would either create some file that the VM couldn't consume, or that we'd have to check that hash as well as the binary version. Neither seems good to me, and bumping the binary version seems like the right choice to me.

That it can be "error prone" - in my opinion - is not a good excuse. If nothing else you should be able to write a test that reminds you (e.g. have a file with your hash tied up to a binary version, if the hash doesn't match throw an error telling the user to bump the version and update the hash-to-version mapping or similar).

[mraleph]

From my perspective the benefits of having this mechanism outweigh negatives - for majority of use cases we need to be able to verify total consistency VM + kernel configuration without allowing any version skew.

I think having this mechanism overall can lead to healthier user experience where our tools can automatically recover (e.g. ignore kernel binary which can't be used and produce a fresh one) or produce meaningful error messages.

I don't think Kernel version number gives us enough information for that because it only covers Kernel AST format itself and does not cover core libraries or custom transformers applied to Kernel. AFAIK @johnniwinther was on board with this proposal.

So overall I think we should move forward with tightening this up - if just to detect situations where people rely on VM allowing some version skew (which is not something that we have documented anywhere!) - though potentially maintain an escape hatch if we discover late in the roll process that we need to allow some version skew.

[jensjoha]

Pros:

• Happens automatically

Cons:

• Happens automatically, even when not needed
• Would require to rebuild all sdk etc for every change
• Would for all intends and purpose be a "subversion" of the "binary version" that every tool reading and writing dill files would have to know about and deal with.
• Isn't the new sdk compiled with an old version of the VM?

I don't think Kernel version number gives us enough information for that because it only covers Kernel AST format itself

But you can certainly update it despite not having changed the actual format.
I still fail to see why you can't get the same "security" from a test...

[mraleph]

Happens automatically, even when not needed

Would require to rebuild all sdk etc for every change

I am not sure why this is needed. Are you talking about development environment? If you are working on a change locally we can set things in such a way that this check is either disabled or is using base commit hash for your change - this means you don't have to rebuild anything.

Isn't the new sdk compiled with an old version of the VM?

It is compiled using new CFE running on old version of the VM. The key here is that files produced by the new CFE should only be then consumed by the new VM, not old VM.

I still fail to see why you can't get the same "security" from a test...

I think I don't understand how this test would look like. Could you elaborate a bit more?

I think the problem we are facing right now is that we don't really have a very clear definition of the ABI boundary, so we can't just write a simple test for it (we can slowly expand the test whenever we hit a incompatibility issue during the roll - but I don't think this is the right approach).

[jensjoha]

Happens automatically, even when not needed

Would require to rebuild all sdk etc for every change

I am not sure why this is needed. Are you talking about development environment? If you are working on a change locally we can set things in such a way that this check is either disabled or is using base commit hash for your change - this means you don't have to rebuild anything.

If I update from commit n to commit n+1 the way I understand what's written here is that it should throw away the old dills and create new ones. (even doing local commits)

Isn't the new sdk compiled with an old version of the VM?

It is compiled using new CFE running on old version of the VM. The key here is that files produced by the new CFE should only be then consumed by the new VM, not old VM.

The way I see it, there's two options:

• It gets the hash from the VM --- that way you wouldn't be able to create it using an old VM.
• It has the hash build in to its sources --- that way you would have to have it committed in a file --- like the binary version.

I still fail to see why you can't get the same "security" from a test...

I think I don't understand how this test would look like. Could you elaborate a bit more?

I think the problem we are facing right now is that we don't really have a very clear definition of the ABI boundary, so we can't just write a simple test for it (we can slowly expand the test whenever we hit a incompatibility issue during the roll - but I don't think this is the right approach).

Ah. So we know that there is something that we can change that might change the compatibility, but we don't know what that something is so the "solution" is to assume it's everything?

I'm all for making stuff blow up less (and if that includes having more information in the dill that's fine by me (although it also has to be easily checkable on the dart side which shouldn't accept it either)), but this feels like "shooting sparrows with cannons" (Danish saying that I'm not sure exists in English).

[mraleph]

If I update from commit n to commit n+1 the way I understand what's written here is that it should throw away the old dills and create new ones. (even doing local commits)

We can make it work for local commits - but if you rebase your local branch then you need to through out your local dills.

We can also give you an escape hatch for local CFE development, e.g. if you just read and write dill and don't run them on the VM then you can dills around without regenerating them.

The way I see it, there's two options

It has the hash build in to its sources --- that way you would have to have it committed in a file --- like the binary version.

It can also get the has from git command itself. I think precise mechanism here is TBD by involved teams - we need something that works for SDK development, SDK releases as well as SDK rolls into other repositories.

but this feels like "shooting sparrows with cannons" (Danish saying that I'm not sure exists in English).

Yes, but the sparrow is guaranteed to be kaput after this.

Ideally, we need to have a really well defined and statically enforced API between all involved components. Unfortunately we are not in the ideal situation - and getting it fully under control would require considerable effort. So we need some solid but relatively easy solution to prevent those pesky sparrows version skews from complicating rolls in confusing ways.

[jensjoha]

It can also get the has from git command itself. I think precise mechanism here is TBD by involved teams - we need something that works for SDK development, SDK releases as well as SDK rolls into other repositories.

This would only work when checked out via git, and it would require to run the git command every time which is certainly going to slow things down.

but this feels like "shooting sparrows with cannons" (Danish saying that I'm not sure exists in English).

Yes, but the sparrow is guaranteed to be kaput after this.

I'm guessing there's been an actual bug in regards to the async transformation since that's what's mentioned above. And yes, this would probably have "guaranteed" to have cought/avoided that. There'll probably still be sort of similar problems up the alley of customers accidentially using wrong dills that it doesn't fix though.

[mraleph]

I'm guessing there's been an actual bug in regards to the async transformation since that's what's mentioned above. And yes, this would probably have "guaranteed" to have cought/avoided that.

Note that there have been at least 3-4 similar issues in the past couple months - most of which took time to understand and triage. So this is not happening just because there was a single issue - it is happening because similar issues occur repeatedly.

[natebosch]

Right now it seems that there are no known customer needs for maintaining compatibility.

Some tools do use forwards compatibility of snapshots as an optimization. It would be unfortunate to never be able to have that optimization and alway shave to re-snapshot across SDK versions, but that would be strongly preferred over having no indication of incompatibility until there is a runtime error.

[a-siva]

Embedding the SDK hash inside the Kernel file, checking for it and throwing an error makes kernel files almost similar to AOT snapshots in terms of version compatibility and is stricter than the proposal of using version numbers in the kernel file and providing compatibility until there is a breaking change, the versions have to be checked by the VM as proposed in #38969
Incompatible async transformations like the one defined above would be considered breaking and would require a version bump, of course there is the issue of how do we guarantee that the version is bumped when a breaking change is made (the purpose of the ABI compatibility bot was meant to detect this, maybe it is not doing the job).