Skip to content

Commit 6656dfa

Browse files
authoredFeb 24, 2021
Fix hashing of a use of a name without the context/target for it (#3601)
Before this we would assert on hashing e.g. (br $x) by itself, without the context so we recognized the name $x. Somehow that was not an issue until delegate, we just happened to not hash such things. I believe I remember that @aheejin noticed this issue before, but given we didn't have a testcase, we deferred fixing it - now is the time, I guess, as with delegate it is easy to get e.g. CodeFolding to hash a Try with a delegate. Issue found by emscripten-core/emscripten#13485
1 parent 49a906d commit 6656dfa

File tree

2 files changed

+31
-8
lines changed

2 files changed

+31
-8
lines changed
 

‎src/ir/ExpressionAnalyzer.cpp

+21-8
Original file line numberDiff line numberDiff line change
@@ -331,15 +331,28 @@ size_t ExpressionAnalyzer::hash(Expression* curr) {
331331
}
332332
}
333333
void visitScopeName(Name curr) {
334-
if (curr.is()) { // try's delegate target can be null
335-
// Names are relative, we give the same hash for
336-
// (block $x (br $x))
337-
// (block $y (br $y))
338-
static_assert(sizeof(Index) == sizeof(int32_t),
339-
"wasm64 will need changes here");
340-
assert(internalNames.find(curr) != internalNames.end());
341-
rehash(digest, internalNames[curr]);
334+
// We consider 3 cases here, and prefix a hash value of 0, 1, or 2 to
335+
// maximally differentiate them.
336+
337+
// Try's delegate target can be null.
338+
if (!curr.is()) {
339+
rehash(digest, 0);
340+
return;
341+
}
342+
// Names are relative, we give the same hash for
343+
// (block $x (br $x))
344+
// (block $y (br $y))
345+
// But if the name is not known to us, hash the absolute one.
346+
if (!internalNames.count(curr)) {
347+
rehash(digest, 1);
348+
// Perform the same hashing as a generic name.
349+
visitNonScopeName(curr);
350+
return;
342351
}
352+
rehash(digest, 2);
353+
static_assert(sizeof(Index) == sizeof(int32_t),
354+
"wasm64 will need changes here");
355+
rehash(digest, internalNames[curr]);
343356
}
344357
void visitNonScopeName(Name curr) { rehash(digest, uint64_t(curr.str)); }
345358
void visitType(Type curr) { rehash(digest, curr.getID()); }

‎test/example/hash.cpp

+10
Original file line numberDiff line numberDiff line change
@@ -138,5 +138,15 @@ int main() {
138138
y.index = 11;
139139
assertNotEqual(x, y);
140140
}
141+
{
142+
// It is ok to hash something that refers to an unknown name, like a break
143+
// without the outside context that it branches to. And different names
144+
// should have different hashes.
145+
Break x;
146+
x.name = "foo";
147+
Break y;
148+
y.name = "bar";
149+
assertNotEqual(x, y);
150+
}
141151
std::cout << "success.\n";
142152
}

0 commit comments

Comments
 (0)
Failed to load comments.