Conversation
|
nice! Thanks for fast fix. Hope this resolve that issue. |
| uint8_t code, | ||
| bool isAtomic) { | ||
| Load* curr; | ||
| auto* curr = allocator.alloc<Load>(); |
There was a problem hiding this comment.
Isn't this going to frequently leak an unused Load?
There was a problem hiding this comment.
Oh, heh, how silly... yes, I misread the code around this. Fix in a sec.
|
Unfortunately this doesn't fix our problem 😓 |
|
@MaxGraey I ran the custom fuzzer (that compares results before and after that commit landed, and which found this regression) for over 100,000 iterations more overnight, and found nothing. So this may be something you need to investigate on the AssemblyScript side. As long as you can get the initial wasm from the AS compiler, and the list of passes AS runs, you should be able to get a testcase. Then you can run the reducer on it, so it's ok if the initial testcase is very large. |
|
Oh, thanks for investigate time to this. But, if it's both versions (before and current) are correctness in term of inrpretation, just last one miss some opportunities for size reduction. Does this found by fuzzer? I thought fuzzer could found just interpretation mismatch |
|
This is a custom fuzzer. What it does is build using the commit before that one, and run optimizations on that, and see the size. Then it does all that with the commit where that change landed (I have a separate build dir for each one). Then it looks for any difference in the outputs - even one byte difference, in either direction, would be a "bug" that this custom fuzzer finds. That is how it found the regression that this PR fixes. It's very easy to add custom fuzzers to |
I see. Unfortunately this issue exists only on our custom pipeline but on binaryen's pipeline. I could reproduce our pass pipeline as separate PRs which forked before and after that refactorings. Hope this will help |
|
If you can get a me a testcase that uses binaryen.js, that would be good too. As long as it just uses things from the binaryen repo. If you can only get a testcase that uses the AS compiler, then that would be impractical for me to debug. If it's very hard for you to extract a testcase out from AS, then we should look into tools to help with that. One possibility is to resurrect the tracing API we used to have on the C API. That's hard to do on C, but could be done in JS, which means it could work for AS? But, hopefully there is a better way on the AS side. |
|
@MaxGraey I don't have any idea on what error you are experiencing on your side, but I think one thing that is different from the previous version and the current version is this part: https://github.com/WebAssembly/binaryen/pull/3332/files#r521353910 |
|
Thanks @aheejin! I will try investigate how affect this changes on our case |
We used to check if a load's sign matters before hashing it. If the load does
not extend, then the sign doesn't matter, and we ignored the value there. It
turns out that value could be garbage, as we didn't assign it in the binary
reader, if it wasn't relevant. In the rewrite this was missed, and actually it's
not really possible to do, since we have just a macro for the field, but not the
object it is on - which there may be more than one.
To fix this, just always assign the field. This is simpler anyhow, and avoids
confusion not just here but probably when debugging.
The testcase here is reduced from the fuzzer, and is not a 100% guarantee
to catch a read of uninitialized memory, but it can't hurt, and with ASan it
may be pretty consistent.
cc @MaxGraey