Adjust allocation strategy for TinyGo-derived modules so they don't immediately crash.

[erikrose]

Should fix fastly/Viceroy#491. Please see that (plus commit messages) for context.

[alexcrichton]

Can you detail a bit more here why this is necessary? We've so far survived with zero language-specific configurations/hacks (AFAIK) throughout component toolchain tooling and personally I'd like to see it remain that way. More specifically, why can't this be fixed in the Go-specific tooling? Or why can't this be an opt-in option that the Go tooling passes?

[erikrose]

Can you detail a bit more here why this is necessary?

Absolutely! Basically, we would like to run existing TinyGo-compiled modules as components. So, while an upstream fix would be ideal for the future, we have a back catalog of compiled artifacts to somehow make work.

TinyGo makes an assumption in its memory management that violates no specs but conflicts with assumptions made by the memory.grow-based allocator in the adaptation code. Specifically, the memory.grow happens (as far as I can infer), and then TinyGo says "How big is the heap? I'll take all that for my own!". Then it immediately stomps on the adapter's State struct. Heck, if you squint, you might even conclude our adaptation code is making an invalid assumption.

The fix for new code should be something like having TinyGo export a cabi_realloc (which it does if you target WASIp2). That'll hit existing happy paths in the adaptation code and shouldn't require changes from us. OTOH, one way to look at it is that TinyGo already does export a cabi_realloc, except with the wrong name and type signature. This patch detects that and wraps it to work.

Of course, it would be perfectly possible to pre-process our TinyGo modules outside wit-component: we could inject the same manufactured cabi_realloc right into the customer module and then go down the usual adaptation paths. The advantages to doing it in wit-component are…

1. It saves complexity (passes, branches, etc.) for Fastly and other similar folks.

2. It doesn't modify the customer module, which feels like a moral good to me.

3. It solves this pretty hairy crash transparently for others who need to run arbitrary third-party code as components.

   Admittedly, it's an unsatisfying breach of elegance, but the alternative is for everyone to independently discover, diagnose, and search out the solution to this. I did try to structure it in the most generic way possible, with the name of the malloc-like routine a variable and the TinyGo detection off by itself, because I can forsee us discovering the same troublesome assumption in other languages. The name tiny_go did persist in some field names because I thought it most clearly answered "why the heck is this here?", but we could genericize those and exile mention of TinyGo to comments if that makes it more palatable.

Alternative solutions are most welcome! But others we could gin up were ickier, and hopefully WASIp2 will relegate all this to an historical footnote.

[alexcrichton]

I agree this is probably the best place to solve this, but to put this into context as a reviewer:

• The title/description of this PR don't meaningfully explain what's going on in the commit. While the description points to Empty TinyGo project crashes under component adapter fastly/Viceroy#491 that's an issue on a separate project that also isn't a clear "this is why this is being done this way in wasm-tools". Basically as a reviewer coming to read this there's no direction of where this is going, so I'm left to my own devices to reverse-engineer that all from the code.
• The code itself documents what it's doing but it doesn't meanigfully explain why it's doing so. That's fine as such documentation is more relevant for a PR/commit messages but I'm still left wondering why this is being done.
• There are no tests in this PR so there's no meaningful way to explore what modification is being done beyond I'm relatively confident that it's not changing anything pre-existing.

It unfortunately also doesn't help that the encode function is a beast of a function that's already very difficult to understand what's happening with realloc. That's not your fault of course but this is going to unfortuantely make matters worse as the realloc logic is getting more complicated.

This can then also all be coupled with the fact that while we all want components to work generally there's still a needle to thread in what's supported where. While I don't believe this PR is doing this, at one extreme it's not reasonable to saddle an open source project with all the legacy burdens and baggage of a specific embedding. The other extreme of blissfully ignoring reality is also not appropriate for an open source project as well, and inevitably the balance is going to be somewhere in the middle.

At the end of the day if no one knows how or is willing to update the TinyGo toolchain that's an extremely burdensome requirement to carry. Support for a new platform like components is almost always best done with a give-and-take style approach where embedders, toolchains, languages, etc, are all in a balance of what concerns are handled where. If a toolchain cannot budge from a single position then it, in my opinion, needs to be an extremely high value target to justify saddling everyone else with that burden.

Basically this is my rationale for "I think this is best done with an opt-in flag". I don't want the baggage here to proliferate to other toolchains and other languages by default. We already have a means of solving the problems you're encountering for other languages and for a variety of reasons it sounds like TinyGo and/or the surrounding support isn't going to implement those solutions. Given that, I at least personally believe that from an open source project perspective the best way to support this is to land this here but behind an off-by-default flags the explicitly requires users to activate. That's where documentation can be updated etc.

---

On the slightly more technical front, I'm more-or-less taking you at face value that this is the best place to solve this. I do not personally have the energy to boot up on everything TinyGo-related and see if I have a different way forward for this. From my current understanding I don't know, for example, if this is a temporary hack for preexisting modules or a permanent feature intended for all future productions of TinyGo modules. If the former, just for preexisting modules, that feels understandable to me. If the latter, all future modules, I don't really have any intuition for why that is the case.

[erikrose]

Tests are on the way! I had hoped to have them laid in before anyone got around to reviewing, but they proved trickier than expected, and my attention was divided. I apologize for the delay. Please feel free to look away while I finish them. I will turn this back into a Draft until then.

For this PR's motivation and problem statement, beyond what's in the initial commit message, I'd reference this comment block in fallback_realloc_scheme(). That explains why we trigger for TinyGo specifically. The comments on ReallocScheme leave out mention of TinyGo, since the schemes are meant to be potentially generic. Instead, they give an overview of what each scheme does, Malloc being the new one.

I share your concern that encode() is a beast! I preserved existing behavior and improved the commenting on existing code where it was difficult to reverse-engineer. But there are still spots where the logic escapes me—that's partly why I didn't attempt a larger, clarifying refactor, beyond such effort being outside the scope of this PR. It certainly could use such a refactor, though; there's too much state blowing around in there. One possibility I experimented with was encapsulating the state involved with tracking functions added by the adapter: map.funcs, func_names, num_func_imports. At first glance, these all have to change in parallel, and there's an opportunity to enforce that. However, my first few attempts weren't a clear win for comprehension, and there were exceptions, which is another reason I put it aside. Something like that, if it can be made a clear win, would be a good future PR.

From my current understanding I don't know, for example, if this is a temporary hack for preexisting modules or a permanent feature intended for all future productions of TinyGo modules. If the former, just for preexisting modules, that feels understandable to me.

It's just for preexisting modules. New builds should target WASIp2, e.g. GOOS=wasip2 GOARCH=wasm tinygo build -o main.wasm main.go. That provides a cabi_realloc(), which leads the adaptation code down a happier path, cued by ReallocScheme::Realloc, which uses the provided cabi_realloc() to communicate the adapter's storage use (for its State struct) to Go's GC.

if no one knows how or is willing to update the TinyGo toolchain

The TinyGo wasm toolchain is well maintained, having p1 and p2 support as well as a dedicated maintainer on Fastly's staff. I think any ambiguity about the allocation of responsiblity among tools is an accident of timing. As I understand it, providing cabi_realloc() as part of the adapted module is a WASIp2 affordance; toolchains that know about only p1 generally don't provide it. There is blurring around this due to some needs of Fermyon before p2 came out, but conversations with @sunfishcode have persuaded me that p2 is the historically intended cutover point.

Basically this is my rationale for "I think this is best done with an opt-in flag". I don't want the baggage here to proliferate to other toolchains and other languages by default.

Me neither. That's why the baggage is so narrowly scoped. It activates only (1) if TinyGo is in the producers section and (2) no cabi_realloc() is provided (so we're talking about a p1 module here with no awareness of p2) and (3) a malloc() is exported and (4) malloc() has the expected signature. There is thus a vanishingly small chance of it kicking in spuriously. And it will naturally veto itself as people move to p2 builds and beyond. Thus, I would like to have this on by default, rather than making every person encountering an opaque panic bounce off Stack Overflow to find the flag.

Let me know if I can fill in more details about anything, and I'm eager to hear your feedback either way. (Take your time, as I'll be out Thursday and Friday.) Thanks!

[alexcrichton]

I'm sorry if I sound like a broken record but I still don't really understand the fundamental reason as to "why" for this. Strategies not working with memory.grow are a known quantity (this used to plauge wasi-libc) so I understand why that's not an alternative. Questions I'm still left with are:

• With wasi-libc when memory.grow didn't work we fixed wasi-libc to make it work, why can't something be done with TinyGo?
• Why does TinyGo not export cabi_realloc? I understand it not being a default export on a WASIp1 target but is there no hook in any SDK or something like that to place it? This has been solved elsewhere (e.g. wit-bindgen) where the output there is explicitly targeting a component through the WASIp1 target so cabi_realloc is emitted.

Something like that, if it can be made a clear win, would be a good future PR.

Yes to be clear I don't want to saddle you with the burden of improving this function. I mostly wanted to point out that I, probably like everyone else, have no room in my head for permanently understanding how this function works which means I have to reread it every time and it continues to be more of a beast than before.

toolchains that know about only p1 generally don't provide it

Part of my point is that TinyGo is not alone here yet this problem has been solved for all other languages. Rust and C, for example, have WASIp1 toolchains that don't support cabi_realloc and yet both WASIp1 targets are suitable for compentizing through the use of SDKs, libraries, etc. That's the "give and take" I was mentioning where one tool isn't responsible for all the solutions (wit-component) but instead I think it works best when responsibility is spread out. This gives way to my questions above of I don't understand why what worked for other languages is not suitable for TinyGo.

There is thus a vanishingly small chance of it kicking in spuriously. And it will naturally veto itself as people move to p2 builds and beyond. Thus, I would like to have this on by default, rather than making every person encountering an opaque panic bounce off Stack Overflow to find the flag.

Personally I'm not swayed by this argument in that the implementation here looks a lot like user agents for browsers. If a toolchain in the future is struggling with this then it'd probably just pretend its TinyGo to get this code path to activate. In that sense to me it's an active downside to sniff the producers section and use that to determine whether this code path is active or not. I basically don't want to be party to a future where everyone's pretending to be TinyGo because that's easier than changing this tool or the language/sdk/etc.

[erikrose]

I'm sorry if I sound like a broken record but I still don't really understand the fundamental reason as to "why" for this.

No problem. Apologies if I repeat things you've already heard as well.

First, let me reiterate that our motivation is to support legacy wasm binaries. So fixes to SDKs or other pre-compilation tooling won't help.

Strategies not working with memory.grow are a known quantity (this used to plauge wasi-libc) so I understand why that's not an alternative. Questions I'm still left with are:

• With wasi-libc when memory.grow didn't work we fixed wasi-libc to make it work, why can't something be done with TinyGo?

Can you explain or point to that fix? Maybe there is indeed some insight I can reuse. I wasn't able to turn up anything definite or applicable after a brief search.

I spent the morning digging into TinyGo's memory management code and the adaptation code, looking for an alternative approach. The crux is that TinyGo assumes it owns the entire heap, from __heap_base (which comes from the linker) onward, so it clobbers the adapter's stack and State, even though they get stored in one new memory.grown page each. It also likes to call memory.grow itself, so any sort of __heap_end symbol won't help.

Therefore, there's no way for the adapter to store its stuff at the far end of memory. But what about the near end?

It might be possible to sock limited-length things (like the stack and State) away in low memory, at __heap_base itself, and then increment __heap_base so TinyGo never knows about that memory. This would have the advantage of working for every language and not being TinyGo-specific. It all comes down to whether adapters think they can dynamically allocate RAM later (via cabi_realloc()). If they do, this won't work, and calling TinyGo's malloc() or similar is the only way.

Indeed, I believe adapters have every right to call cabi_realloc() whenever they wish. With the WASIp1 adapter, the State allocation is done lazily and is triggered by all/most WASI calls (through State::with()). While it so happens that that state (and the stack) are singletons and thus could live in a reserved, known-size space, there's no reason to expect all conceivable adapters to be so accomodating.

Thus, as far as I can see, calling TinyGo's malloc() is the only workable approach.

• Why does TinyGo not export cabi_realloc? I understand it not being a default export on a WASIp1 target but is there no hook in any SDK or something like that to place it? This has been solved elsewhere (e.g. wit-bindgen) where the output there is explicitly targeting a component through the WASIp1 target so cabi_realloc is emitted.

I like contracts too! :-D That is indeed the way forward, but it doesn't help legacy binaries.

There is thus a vanishingly small chance of it kicking in spuriously. And it will naturally veto itself as people move to p2 builds and beyond. Thus, I would like to have this on by default, rather than making every person encountering an opaque panic bounce off Stack Overflow to find the flag.

Personally I'm not swayed by this argument in that the implementation here looks a lot like user agents for browsers. ... I basically don't want to be party to a future where everyone's pretending to be TinyGo because that's easier than changing this tool or the language/sdk/etc.

Yes, it certainly is an heuristic. If runtime crashing of componentized WASIp1 TinyGo programs is less ugly than the heuristic (and potential future misuse), we can fall back to an off-by-default flag, something like --realloc_via_malloc=name_of_malloc_routine. I only hope that the crashing remains prompt, as it has been in the few cases we've tested. But I don't see any reason that is guaranteed. Is that the tradeoff you'd prefer? Any insights from other memory.grow-afflicted languages? Thank you again.

[alexcrichton]

let me reiterate that our motivation is to support legacy wasm binaries

What I mostly don't understand about this is squaring this comment with:

If runtime crashing of componentized WASIp1 TinyGo programs is less ugly than the heuristic

For this latter point of crashing, I'm not sure what precisely you're trying to convey here. I'm no fan of crashing or subtle corruptions either, and I in no way want to perpetuate things. Questions I am still left with, however, are:

• If this is purely preexisting binaries, why is a flag a problem? Is there no mechanism anywhere internally to insert a flag?
• If this is to prevent others from tripping over this problem, why is this purely only about preexisting binaries?

Basically these two motivations seem to be at odds. If fixing future cases is problematic that's where my questions about fixing SDKs and/or the toolchain come from. If fixing future cases is not necessary then that's where my motivation for making this a flag comes in.

Can you explain or point to that fix?

The original fix was at WebAssembly/wasi-libc#377

Therefore, there's no way for the adapter to store its stuff at the far end of memory. But what about the near end?

This is not something I've actually considered before. My main hesitation is that the adapter is trying to assume as little as possible about the input module as it can to be as flexible as it can. You're correct though that if we can assume __heap_base is respected and present then we could just increase that by a bit and give the adapter that area. That would require refactoring/implementation work both here and in the adapter, but it would be doable.

Thus, as far as I can see, calling TinyGo's malloc() is the only workable approach.

I will again sort of come back to my original point here, but either way I don't necesarily agree with this. If only preexisting modules matter then there's the option of (a) using malloc or (b) using __heap_base extension like you bring up. In practice there is a single adapter in this world and it's the p1-to-p2 adapter, and no other exist outside of testing. In that sense I don't think there's any need to pidgeonhole ourselves into a corner for use cases that don't exist, aka if something works for the p1-to-p2 adapter I would consider that the bar to clear.

If future modules also matter then it comes back to the point of asking why can't cabi_realloc be added somewhere. That would also mean that malloc is not the only possible approach.

I'll note I do realize that the __heap_base-based solution would be more investment than you're probably willing to put into this. I personally think it's important to use that as a deciding factor between choosing how best to solve things, but at the same time I don't want to ignore possible solutions just because they're more work than another solution (especially when they score higher on the technical merits metrics as this sort of would IMO)

I only hope that the crashing remains prompt

Well, personally, I also hope that everyone's able to solve their problem space in wasm quickly and efficiently and without pain. Apart from an emotional desire though I'd prefer to measure what's happening here based on its technical merits instead.

Going back to the start of this comment, I do not know how to square what you're implying here. On one hand you're saying that only preexisting modules matter. On the other hand you're saying that if this is off-by-default then future developers will be plagued with hours of debugging due to forgetting this flag. I don't know how to connect these two situations because it seems like there is a single entity in control of all the modules which can turn on the flag irrespective of whatever the defaults are of wasm-tools-the-CLI itself.

Is that the tradeoff you'd prefer?

My guess is that this PR is taking more time and effort than you were originally expecting. If that's frustrating you I apologize for that, but at the same time at least in my own experience that's how OSS works out sometimes. What I can do here is reiterate that I'm not trying to be some nefarious entity whose sole purpose is to hobble the TinyGo ecosystem. Rather I see myself as the primary maintainer of this repository and tooling within. To do my job effectively I need to understand code landing in this repository, and I do not understand this code (specifically the requirement this is on-by-default). I'm trying to square this understanding and a textual medium of comments over PRs is only but so good at resolving differences, so I'm doing my best.

[keithw]

I don't know if my piping up here is helpful, so please feel free to ignore, but it seems like maybe all parties could live with a compromise where wit-component uses the proposed TinyGo producer-sniffing and export list to detect the likely problem and print a warning or error, but that's it.

And, the affected communities can create a tool (maybe part of wasm-tools or maybe not) that pre-processes the affected modules and adds a cabi_realloc export, so that the resulting modules no longer trigger the warning/error and work correctly.

That could solve the issue where "the alternative is for everyone to independently discover, diagnose, and search out the solution to this" without special-casing on a producer name to change wit-component's behavior.

[alexcrichton]

That sounds reasonable to me!

[erikrose]

I'm fine falling back to rewriting the main module. There are 3 projects in my realm of responsibility that need this fix, so I'll need to factor it up someplace. Would you like it in wasm-tools?

If yes, it seems like we might as well save people a step and activate it with a --realloc_using_malloc flag passed to component new. But instead of inserting a cabi_realloc() in the adapter module as my current patch does, we'd insert it in the main module and leave the adaptation code (gc.rs) alone, which avoids disturbing the large and ill-understood encode() method. And nothing turns on by default; we just throw a warning.

Does that sound good?

I'll answer a few hanging threads just for completeness:

• If this is purely preexisting binaries, why is a flag a problem? Is there no mechanism anywhere internally to insert a flag?
• If this is to prevent others from tripping over this problem, why is this purely only about preexisting binaries?

It's solely about preexisting binaries. Perhaps I just assume more parties have troves of preexisting third-party binaries than actually do; perhaps Fastly is unusual in this regard.

If only preexisting modules matter then there's the option of (a) using malloc or (b) using __heap_base extension like you bring up.

Yep, if we don't care about unlimited-size allocations (i.e. there's really only 1 adapter of consequence in the world), B could totally work. As you say, I'm not psyched to go tearing things apart even moreso at the moment, since this fix is part of a larger effort I need to get back to and since I fear breaking non-TinyGo things with a wider-scoped change. But let's hold it in reserve in case we need it in the future.

My guess is that this PR is taking more time and effort than you were originally expecting. If that's frustrating you I apologize for that, but at the same time at least in my own experience that's how OSS works out sometimes.

Again, no worries. I've been on both sides of this fence many times.

Would you have a few minutes today to meet synchronously and make sure we're aligned? (I also want to make sure I'm not coming across like a nefarious interloper out to ruin your codebase.)

[alexcrichton]

I talked with @erikrose and our conclusions (correct me if I'm wrong) were:

• Keep the TinyGo detection here, but just as a reusable function for other crates to use.
• Internally the behavior of falling back to malloc will be a new option on ComponentEncoder, effectively a CLI flag for the wasm-tools CLI itself.
• I'll help out if needed to ensure this has a test for it too.

[erikrose]

Alex and I had a 20-minute talk and decided to…

• Leave the gc.rs realloc_via_malloc code as is.
• Put it behind an off-be-default --realloc-via-malloc flag which, when on, gins up a malloc()-based cabi_realloc() iff {a cabi_realloc() isn't already in the main module} and {a suitable malloc() is}.
• Not throw a warning when we detect a TinyGo module, because down the road we could want to optionally silence that warning, and that just leads to a profusion of flags. It's not clear yet that many people will run into this problem. We hope they'll all migrate to TinyGo's wasip2 target before components become widespread.
• Expose the TinyGo-detection code as an uncalled (but tested) public routine that I (and others, should they exist) can call from third-party tooling to decide whether to pass the --realloc-via-malloc flag.
• Have these productive synchronous discussions earlier in the future. :-)

[erikrose]

@alexcrichton Early indications are that Big Go has the same problem as TinyGo, but it's less amenable to this solution, since it exposes no malloc(). We're trying an approach where we move the adapter state and stack off the heap altogether, instead sticking it in the outermost stack frame. If this works, it should solve the problem for both Go and TinyGo, along with any other language that comes along, without any dependence on __heap_base or exposed exports. In fact, it might actually simplify the adapter code a bit.

So maybe you should go ahead and release wit-component while we experiment.