[wasm] Fix nullptr deref in Liftoff debug side table coverage [v8/v8 : main]
Tejas Vasisht (Gerrit)
Tejas Vasisht added 2 comments![]( "Open in Gerrit")
[wasm] Fix nullptr deref in GenerateLiftoffDebugSideTable with --wasm-code-coverageTejas VasishtPlease try to make this first line shorter (see https://chromium.googlesource.com/chromium/src/+/HEAD/docs/contributing.md#uploading-a-change-for-review).
Done
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tejas Vasisht (Gerrit)
Tejas Vasisht voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tejas Vasisht (Gerrit)
Tejas Vasisht added 1 comment![]( "Open in Gerrit")
Hello clemensb@, paolosev@. Updated the fix based on the CQ failures. The guard fixed the null dereference, but caused mismatched PC offsets. The dummy compilation now reuses the existing coverage data so its layout matches the original code. Also rebased onto the latest main.
Related details
- Clemens Backes
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 1 comment![]( "Open in Gerrit")
if (V8_UNLIKELY(coverage_instrumentation_ && (opcode != kExprLoop))) {Why this change?
Related details
- Clemens Backes
- Tejas Vasisht
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Hello clemensb@, paolosev@. Updated the fix based on the CQ failures. The guard fixed the null dereference, but caused mismatched PC offsets. The dummy compilation now reuses the existing coverage data so its layout matches the original code. Also rebased onto the latest main.
What do you mean by "caused mismatched PC offsets"? The entries in the debug side table did not match any more? How does that depend on code coverage?
If there's a good reason to enforce the emission of code coverage instrumentation when generating the debug side table, then I think it's an acceptable solution, but I'd like to understand a bit better why the simple solution does not work. Maybe there's a bug elsewhere.
Related details
- Tejas Vasisht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tejas Vasisht (Gerrit)
Tejas Vasisht added 1 comment![]( "Open in Gerrit")
Clemens BackesHello clemensb@, paolosev@. Updated the fix based on the CQ failures. The guard fixed the null dereference, but caused mismatched PC offsets. The dummy compilation now reuses the existing coverage data so its layout matches the original code. Also rebased onto the latest main.
What do you mean by "caused mismatched PC offsets"? The entries in the debug side table did not match any more? How does that depend on code coverage?
If there's a good reason to enforce the emission of code coverage instrumentation when generating the debug side table, then I think it's an acceptable solution, but I'd like to understand a bit better why the simple solution does not work. Maybe there's a bug elsewhere.
The original Liftoff compilation emits emit_inc_i32_at instructions for coverage. Those instructions change the PC offsets where debug-side-table entries are registered. GenerateLiftoffDebugSideTable() recompiles the function and uses that new table to inspect the original code. If regeneration skips coverage instrumentation, its offsets no longer match the original coverage-instrumented code.
Reusing the existing coverage data makes both compilations emit the same layout.
Related details
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |