RemoveUnusedModuleElements: Handle null table segments#8884
Conversation
aheejin
left a comment
aheejin
left a comment
There was a problem hiding this comment.
I wonder if this applies to wasm-split too? We recently started to splitting element segments, so...
| } else { | ||
| // A related situation is a table with element segments that write | ||
| // nulls: they might overwrite a function, causing a trap, if traps can | ||
| // happen, so we must preserve such element segments. | ||
| for (auto& elem : info.typeElems[type.getBottom()]) { | ||
| reference({ModuleElementKind::ElementSegment, elem}); | ||
| } |
There was a problem hiding this comment.
Can't this happen even if table->init exists? Should this be in else branch?
There was a problem hiding this comment.
It can, but the first loop is in allElems, and the second on typeElems of a specific type. So the first loop does more, and if we do it, we don't need the second. I'll add a comment.
| // need to read one place. Bottom types are stored without subtyping, | ||
| // however: adding a null to all its supertypes would mean adding it to all | ||
| // function types, which is wasteful and adds no information; as a result, | ||
| // we need to check for nulls specifically and not rely on subtyping. |
There was a problem hiding this comment.
I think I can get why we can't remove null segments, but I'm not sure how that's related to bottom types and subtyping. The test doesn't have anything related either. Is this two different PRs together?
There was a problem hiding this comment.
Sorry, this just uses the fact that nulls always have the bottom type. And nothing else (no function) has that bottom type.
So when we see a null, we just use that type to note it in the data. When we see a function, we not only note it, but also its supertypes - but here, we don't do that, for the reasons in the comment (that the supertypes of the bottom type are literally all the function types).
There was a problem hiding this comment.
(This might be less confusing if we called "bottom type" instead "null type", which I personally prefer)
Hmm, I'm not familiar enough with wasm-split, but if it tracks values in tables then it needs to track nulls too (if the nulls could have an effect like overwriting). |
|
Fuzzer found another bug related to this one - let me see if I can fix both together, in a nice way. |
|
Ok, sorry for the churn, but this is now in a final state. The problem here is more general: not only nulls can be written, causing traps, but also functions of the wrong kind. We have to keep such writes around in both cases, if they can matter. That means that if one segment tramples another, we must be careful what we remove: if one segment wrote something that does not trap, then later, writing something that does trap will change the behavior (without that first segment writing something valid, we trap anyhow). To handle this, check for segment overlap. If there is any, keep segments around, it traps can happen. This is simple, and while not optimal, it seems good enough: many users use traps-never-happen anyhow, and in general, overlapping segments is very rare (compilers should not be emitting them). |
|
Sorry, will take a look |
| ;; We also require traps-never-happen, as an elem with non-constant offset can | ||
| ;; trap, normally. When both closed world and tnh, we remove elem $elem. | ||
| (module | ||
| ;; TNH__: (type $func (func)) |
There was a problem hiding this comment.
This TNH__ doesn't seem to exist in the RUN lines?
| ;; trap, normally. When both closed world and tnh, we remove elem $elem. | ||
| (module | ||
| ;; TNH__: (type $func (func)) | ||
| ;; CHECK: (type $func (func)) |
There was a problem hiding this comment.
How about making this CLOSED_WORLD to be contrasted with OPEN_WORLD below?
| (table $table 6 6 funcref) | ||
|
|
||
| ;; OPEN_WORLD: (elem $elem (global.get $offset) $other) | ||
| (elem $elem (global.get $offset) $other) |
There was a problem hiding this comment.
Nit: How about elem $first because the next elem is $second?
| ;; remove segments from the table. | ||
| (module | ||
| ;; CHECK: (type $func (func)) | ||
| ;; TNH__: (type $func (func)) |
There was a problem hiding this comment.
Just curious, why the __ after TNH?
| // tramples it with a null or with a function of another type, then if we | ||
| // call that index with the right type for A, the trampling causes a trap - | ||
| // so we cannot remove the trampling segment, even though it has nothing | ||
| // can can be called, forcing us to keep it just to preserve the trap. |
There was a problem hiding this comment.
| // can can be called, forcing us to keep it just to preserve the trap. | |
| // that can be called, forcing us to keep it just to preserve the trap. |
| // A related situation is a table with element segments that trample | ||
| // valid values with nulls or functions of the wrong type for a call: | ||
| // when they overwrite a valid function, they cause a trap, which we | ||
| // must preserve if traps can. |
There was a problem hiding this comment.
Nit: How about moving this before if, because this is a separate case and not the part of Table::init thing?
2 participants
When an elem segment has a null, we must keep it around as it may be
needed to cause the program to trap (if it overwrites a function that would
not have trapped, had it been called).