Source maps: Fix locations without debug info between ones that do #5906
Conversation
| uint32_t position = nextDebugLocation.availablePos + positionDelta; | ||
|
|
||
| nextDebugLocation.previousPos = nextDebugLocation.availablePos; | ||
| nextDebugLocation.availablePos = position; |
There was a problem hiding this comment.
This is not new logic - just moved up from below (since we also want it for a 1-length entry).
There was a problem hiding this comment.
@kripken
Hi Alon,
does this optimization only impact the debug info read process? how about the BinaryenFunctionSetDebugLocation API? I think AssemblyScript is using this API for debug info generation, it also have the smeared issue for the shadow stack check.
for more detail: AssemblyScript/assemblyscript#2704
CC @dcodeIO @MaxGraey @HerrCai0907
There was a problem hiding this comment.
This part of the code, or this PR as a whole?
The PR as a whole affects binary read and write. We now write 1-length segments to prevent smearing, and we read them properly as well.
SetDebugLocation should still be fine. That sets debug info on a single expression. This PR fixes things in binary reading and writing, which due to smearing would cause things to get debug info when they should not. See the testcase in the PR for an example of a bug that is fixed.
Codecov Report
@@ Coverage Diff @@
## main #5906 +/- ##
==========================================
+ Coverage 42.60% 42.61% +0.01%
==========================================
Files 484 484
Lines 74829 74845 +16
Branches 11922 11926 +4
==========================================
+ Hits 31881 31896 +15
Misses 39751 39751
- Partials 3197 3198 +1
|
|
To be extra safe I also manually verified that the Chrome devtools can read a file emitted with this, with no errors or warnings. |
|
This seems to have broken |
|
Sorry, I tried all the tests with |
After WebAssembly/binaryen#5906 Binaryen can emit such entries, which emsymbolizer should not error on.
…ebAssembly#5906) Previously we did nothing for instructions without debug info. So if we had one that did, followed by one that didn't, the one that didn't could get "smeared" with the debug info of the first. Source map locations are the start of segments, apparently, and so if we say a location has info then all others after it will as well, until the next segment. To fix that, add support for source map entries with just one field, the binary location. Such entries have no debug info (no file:line:col), and though the source maps spec is not very clear on this, this seems like the right way to prevent this problem: to stop a segment with debug info by starting a new one without, when we know we don't want that info any more. That is, before this PR we could have this: ;; file.cpp:10:1 (nop) ;; binary offset 5 in wasm (nop) ;; binary offset 6 in wasm and the second nop would get the same debug annotation, since we just have one source map segment, [5, file.cpp, 10, 1] // start at offset 5 in wasm With this PR, we emit: [5, file.cpp, 10, 1] // start at offset 5 in wasm; file.cpp:10:1 [6] // start at offset 6 in wasm; no debug info This does add 7% to source map sizes, however, since we add those 1-length segments now, but that seems unavoidable to fix this bug. To implement this, add a new field that says if the next location in the source map has debug info or not, and use that.
3 participants
Previously we did nothing for instructions without debug info. So if we had one
that did, followed by one that didn't, the one that didn't could get "smeared" with
the debug info of the first. Source map locations are the start of segments,
apparently, and so if we say a location has info then all others after it will as well,
until the next segment.
To fix that, add support for source map entries with just one field, the binary
location. Such entries have no debug info (no
file:line:col), and though the sourcemaps spec is not very clear on this, this seems like the right way to prevent this
problem: to stop a segment with debug info by starting a new one without, when
we know we don't want that info any more. That is, before this PR we could have
this:
and the second
nopwould get the same debug annotation, since we just haveone source map segment,
With this PR, we emit:
This causes no errors on the emscripten source map tests in addition to the ones
in this repo, so I am somewhat confident about it. It does add 7% to source map
sizes, however, since we add those 1-length segments now, but that seems
unavoidable to fix this bug. (This was reported by a Java user that was seeing an
obviously incorrect stack trace, which was due to such smearing.)
To implement this, add a new field that says if the next location in the source map
has debug info or not, and use that.