Make fixed-size table base-address loads read-only

[jameysharp]

Fixes #8200

I'd appreciate careful review of my claim that the table base-address won't change as long as the table declaration has a fixed size. If I'm wrong about that then this change isn't safe.

[alexcrichton]

I believe this is correct, yes. Can you add an assertion here with a comment indicating why it's important to preserve the base pointer? That's I believe the only possible method where a table can change its base pointer, so having a comment there referencing this optimization I think would also be good.

[fitzgen]

Note that it is theoretically possible for the base pointer to remain constant even while supporting growth, if we are just growing within one large pre-allocated or pre-reserved memory region. It may or may not be worth adding support for this subtlety to cranelift-wasm.

[jameysharp]

Yeah, Nick, when I looked at the code Alex pointed at, I noticed that it looks like the pooling allocator never resizes the backing store for tables. So maybe we can thread that information through?

The decision of whether to use the readonly flag or not is in the wasmtime-cranelift crate so it's not a big deal to rely on Wasmtime internal details in making that decision. I haven't dug into how well connected those parts are though.

[alexcrichton]

What you're describing is I believe one property of "static" vs "dynamic" memories, namely that static memories never change their base pointer but dynamic memories are allowed. There's lots of knobs for that in Wasmtime but the problem here is we'd have to be able to classify static/dynamic at compile time and then make sure it matches the settings at runtime. Currently the pooling allocator isn't consulted at compile time, only runtime, so pooling-vs-not wouldn't be suitable for this optimization.

We could of course, though, add more flags and more config options like memories but for tables to control this.

[fitzgen]

Yep, I think that's something we may want to do in the long term.

[jameysharp]

Oh right, causality goes the wrong way here. I do like the idea of eventually adding a compile-time option for "no table backing-store resizes" that lets us set the readonly flag even for tables which might grow, and then turning table.grow requests that overflow the backing store into runtime errors.

Until we do that, though: Alex, I'm not sure what assertion I can add at that call to Vec::resize. I'm happy to add comments. But in the case where this PR requires non-moving backing stores, the resize call is guarded immediately above by checking that the new size does not exceed self.maximum(). So the only way it's reachable is if this function is called with a size delta of zero, and I think in that case it would be better to return earlier and always succeed.

What would you suggest doing there?

[alexcrichton]

Hm no you're right, I was assuming we had more context there than we actually do. I was envisioning an assert along the lines of "assert the type doesn't have max == Some(min)" but that can't be done.

In lieu of that I think it's ok to leave a comment on the check for max and say that this is needed by the spec but also for soundness in cranelift to enable this optimization. It's a bit redundant but it does seem good to have a mention of this optimization somewhere in grow

[jameysharp]

Okay, I've added stuff to the grow method in wasmtime-runtime. Would you give that part another look? I also rebased for other changes that have landed meanwhile.