[Custom Descriptors] Keep trapping initializers#7693
Conversation
Update RemoveUnusedModuleElements to preserve traps in global and element segment initializers due to null or possibly-null descriptors passed to struct.new. For every struct.new that appears with a descriptor in an initializer, find the origin of the descrpitor value. If it is a null literal or a nullable import, keep the global containing the struct.new because it either will or will possibly trap.
| roots.emplace_back(ModuleElementKind::Global, global->name); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
More generally we could use walkModuleCode here, rather than manually walk segments and globals, but I suppose this is simpler atm, and it is unlikely we'll add more such global locations.
There was a problem hiding this comment.
I think there would be some complexity in figuring out what item to preserve if we used walkModuleCode. I agree the current code seems ok.
|
|
||
| // For each global we have processed, true if it might be null and false | ||
| // if it definitely is not null. | ||
| std::unordered_map<Name, bool> globalMaybeNullCache; |
There was a problem hiding this comment.
Is this worth it here? Other passes will make the wasm type non-nullable where possible.
There was a problem hiding this comment.
Yeah, maybe not. I'll remove it.
| // allocation. Cache the results to avoid searching the same globals | ||
| // again in the future. | ||
| auto* global = wasm.getGlobal(get->name); | ||
| std::vector<std::unordered_map<Name, bool>::iterator> cacheEntries; |
There was a problem hiding this comment.
| std::vector<std::unordered_map<Name, bool>::iterator> cacheEntries; |
| // | ||
|
|
||
| #include <memory> | ||
| #include <vector> |
There was a problem hiding this comment.
| #include <vector> |
There was a problem hiding this comment.
There are still lots of vectors in this file, so it seems best to keep this.
| if (auto* next = global->init->dynCast<GlobalGet>()) { | ||
| global = wasm.getGlobal(next->name); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I'm not sure we need this either, or the loop - if a global is simply a global.get of another, the optimizer will fold them. So we can just check the type of the global once to see if it is nullable.
There was a problem hiding this comment.
Oh, and just assume the worst if it's nullable? That seems risky. We run global-refining after remove-unused-module-elements in the default global optimizations, so we wouldn't end up removing these globals until the global optimizations are repeated. But I guess none of the function-parallel passes run in between would be affected, so it's probably ok?
There was a problem hiding this comment.
Good point that a single cycle might not be enough. Still, I think for GC content we can assume multiple cycles, so it's worth keeping the code simpler.
(And, as you say, the function passes are not affected anyhow even in a single cycle.)
| if (!curr->desc) { | ||
| return; | ||
| } | ||
| if (curr->desc->is<StructNew>()) { | ||
| return; | ||
| } | ||
| if (curr->desc->is<RefNull>()) { | ||
| mayTrap = true; | ||
| return; | ||
| } | ||
| if (curr->desc->is<GlobalGet>()) { | ||
| // Other optimizations will refine the type of the global to be | ||
| // non-nullable if it is not null, so we can just assume the worst if | ||
| // we see a nullable global here. | ||
| if (curr->desc->type.isNullable()) { | ||
| mayTrap = true; | ||
| } | ||
| return; | ||
| } | ||
| WASM_UNREACHABLE("unexpected descriptor"); |
There was a problem hiding this comment.
| if (!curr->desc) { | |
| return; | |
| } | |
| if (curr->desc->is<StructNew>()) { | |
| return; | |
| } | |
| if (curr->desc->is<RefNull>()) { | |
| mayTrap = true; | |
| return; | |
| } | |
| if (curr->desc->is<GlobalGet>()) { | |
| // Other optimizations will refine the type of the global to be | |
| // non-nullable if it is not null, so we can just assume the worst if | |
| // we see a nullable global here. | |
| if (curr->desc->type.isNullable()) { | |
| mayTrap = true; | |
| } | |
| return; | |
| } | |
| WASM_UNREACHABLE("unexpected descriptor"); | |
| if (curr->desc->type.isNullable()) { | |
| mayTrap = true; | |
| } |
I believe this handles all the cases?
Co-authored-by: Alon Zakai <azakai@google.com>
Update RemoveUnusedModuleElements to preserve traps in global and
element segment initializers due to null or possibly-null descriptors
passed to struct.new. For every struct.new that appears with a
descriptor in an initializer, find the origin of the descrpitor value.
If it is a null literal or a nullable import, keep the global containing
the struct.new because it either will or will possibly trap.