Closed Bug 1672172 Opened 4 years ago Closed 4 years ago

Make SrcNotes relative to initial line/column

Categories

(Core :: JavaScript Engine, task, P3)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox82 --- wontfix
firefox83 --- wontfix
firefox84 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1672172 - Saturate column numbers at 1B characters. r?jandem!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672172 - Fail parse if JS line number exceeds 4B. r?jandem!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672172 - Make source notes relative to initial line/column. r?jandem!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672172 - Add tests for bytecode sharing. r?jandem!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1672172 - Show column numbers in jsshell src-note disassembly. r?arai!
47 bytes, text/x-phabricator-request
Details | Review

To increase opportunities for bytecode sharing, we can make the SrcNotes for ColSpan/SetLine relative to the SourceExtent line/column.

With the CompileOptions::column offset and uint32_t source length, we can
end up with column positions that are not representable in various parts of
the engine. Rather than throwing an error on overflow, this patch simply
makes it saturate. This avoids checks throughout the tokenizer. These column
numbers are used primarily for SpiderMonkey-developer logging and for error
stacks in devtools. The existing behaviour would lose sync of SrcNotes so
this saturation change is at least more consistent.

Fail parsing if line number (including initial offset) overflows the uint32_t
line number. Source length is capped at 4B characters so this change only
affects cases of inline-scripts with billions of lines which is not a
realistic case, even for generated code.

Depends on D94114

Update the SetLine / ColSpan source-notes to be relative to the script's
initial line and column number. This allows for more bytecode sharing.

Depends on D94115

Add tests for current bytecode sharing expectations to detect accidental
regressions in future.

Depends on D94116

Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/baa3bd02d664
Saturate column numbers at 1B characters. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b739f4a0e793
Fail parse if JS line number exceeds 4B. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c24562aac61b
Add tests for bytecode sharing. r=jandem
Regressions: 1672729
Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f800c9af17ef
Make source notes relative to initial line/column. r=jandem

Backed out changeset f800c9af17ef (bug 1672172) for bustage complaining about SourceNotes.h.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=f800c9af17ef05096acab5c3f10c11aa6320376f&tochange=c81c365a9616218b15035c19111a488b51252225&searchStr=build&selectedTaskRun=B85odFwTQzSfZHHTI7Kd0w.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/c81c365a9616218b15035c19111a488b51252225

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319577140&repo=autoland&lineNumber=17239

[task 2020-10-23T17:57:10.565Z] 17:57:10     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/js/src/gc'
[task 2020-10-23T17:57:10.566Z] 17:57:10     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 --target=arm-linux-androideabi -o StoreBuffer.o -c  -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DNDEBUG=1 -DTRIMMED=1 -DJS_CACHEIR_SPEW -DJS_STRUCTURED_SPEW -DJS_HAS_CTYPES -DFFI_BUILDING -DMOZ_LINKER -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/builds/worker/checkouts/gecko/js/src/gc -I/builds/worker/workspace/obj-build/js/src/gc -I/builds/worker/workspace/obj-build/js/src -I/builds/worker/checkouts/gecko/js/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/js/src/js-confdefs.h -Qunused-arguments -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include/arm-linux-androideabi -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include -gcc-toolchain /builds/worker/fetches/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 -D__ANDROID_API__=16 -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Werror=implicit-function-declaration -Wno-psabi -Wno-unknown-warning-option -mno-unaligned-access -fno-sized-deallocation -fno-aligned-new -fno-short-enums -fno-exceptions -fcrash-diagnostics-dir=/builds/worker/artifacts -stdlib=libstdc++ -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/fetches/android-ndk/sources/android/support/include -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++abi/include -march=armv7-a -mthumb -mfpu=neon -mfloat-abi=softfp -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -g -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing -Werror=format -Wno-shadow -fexperimental-new-pass-manager  -MD -MP -MF .deps/StoreBuffer.o.pp   /builds/worker/checkouts/gecko/js/src/gc/StoreBuffer.cpp
[task 2020-10-23T17:57:10.566Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/gc/StoreBuffer.cpp:7:
[task 2020-10-23T17:57:10.566Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/gc/StoreBuffer-inl.h:15:
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/gc/Heap-inl.h:13:
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/gc/Zone.h:23:
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/vm/JSFunction.h:20:
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/vm/JSObject.h:18:
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/vm/BytecodeUtil.h:31:
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  In file included from /builds/worker/checkouts/gecko/js/src/vm/SharedStencil.h:19:
[task 2020-10-23T17:57:10.567Z] 17:57:10    ERROR -  /builds/worker/checkouts/gecko/js/src/frontend/SourceNotes.h:254:40: error: comparison of integers of different signs: 'ptrdiff_t' (aka 'int') and 'const unsigned int' [-Werror,-Wsign-compare]
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -            toOperand(line, initialLine) > SrcNote::FourBytesOperandMask ? 4 : 1;
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  1 error generated.
[task 2020-10-23T17:57:10.567Z] 17:57:10    ERROR -  make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:676: StoreBuffer.o] Error 1
[task 2020-10-23T17:57:10.567Z] 17:57:10     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/js/src/gc'
Flags: needinfo?(tcampbell)

(In reply to Bogdan Tara[:bogdan_tara | bogdant] from comment #9)

Also seeing the following starting with the backed out changes:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319577880&repo=autoland&lineNumber=3977

Thanks! I've updated a fix for these devtools test failures.

Flags: needinfo?(tcampbell)
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b27951e02d6
Make source notes relative to initial line/column. r=jandem

Backed out changeset 0b27951e02d6 (bug 1672172) for browser_resources_* devtools failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=a07gAm0XRtaO-yDdC5CFJA.0&fromchange=0b27951e02d6d07e063b051095dbf762c3b8f356&searchStr=devtools&tochange=38441650feb9c01a9aa9f78b2ffe1c9416c4317e

Backout link: https://hg.mozilla.org/integration/autoland/rev/38441650feb9c01a9aa9f78b2ffe1c9416c4317e

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319612347&repo=autoland&lineNumber=3690

[task 2020-10-24T00:22:25.989Z] 00:22:25     INFO - TEST-START | devtools/shared/resources/tests/browser_resources_css_messages.js
[task 2020-10-24T00:23:11.017Z] 00:23:11     INFO - TEST-INFO | started process screenshot
[task 2020-10-24T00:23:11.075Z] 00:23:11     INFO - TEST-INFO | screenshot: exit 0
[task 2020-10-24T00:23:11.075Z] 00:23:11     INFO - Buffered messages logged at 00:22:25
[task 2020-10-24T00:23:11.075Z] 00:23:11     INFO - Entering test bound 
[task 2020-10-24T00:23:11.075Z] 00:23:11     INFO - Buffered messages logged at 00:22:26
[task 2020-10-24T00:23:11.075Z] 00:23:11     INFO - Adding a new tab with URL: http://localhost:50297/test_css_messages.html
[task 2020-10-24T00:23:11.075Z] 00:23:11     INFO - Buffered messages finished
[task 2020-10-24T00:23:11.075Z] 00:23:11     INFO - TEST-UNEXPECTED-FAIL | devtools/shared/resources/tests/browser_resources_css_messages.js | Test timed out - 
[task 2020-10-24T00:23:11.076Z] 00:23:11     INFO - Removing tab.
[task 2020-10-24T00:23:11.076Z] 00:23:11     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2020-10-24T00:23:11.076Z] 00:23:11     INFO - Got event: 'TabClose' on [object XULElement].
[task 2020-10-24T00:23:11.076Z] 00:23:11     INFO - Tab removed and finished closing
[task 2020-10-24T00:23:11.076Z] 00:23:11     INFO - TEST-PASS | devtools/shared/resources/tests/browser_resources_css_messages.js | The main process DevToolsServer has no pending connection when the test ends - 
[task 2020-10-24T00:23:11.086Z] 00:23:11     INFO - GECKO(7768) | MEMORY STAT | vsize 2104228MB | vsizeMaxContiguous 65012610MB | residentFast 297MB | heapAllocated 84MB
[task 2020-10-24T00:23:11.086Z] 00:23:11     INFO - TEST-OK | devtools/shared/resources/tests/browser_resources_css_messages.js | took 45098ms
[task 2020-10-24T00:23:11.105Z] 00:23:11     INFO - checking window state
[task 2020-10-24T00:23:11.114Z] 00:23:11     INFO - TEST-START | devtools/shared/resources/tests/browser_resources_document_events.js
[task 2020-10-24T00:23:11.735Z] 00:23:11     INFO - GECKO(7768) | MEMORY STAT | vsize 2104236MB | vsizeMaxContiguous 65012610MB | residentFast 301MB | heapAllocated 88MB
[task 2020-10-24T00:23:11.735Z] 00:23:11     INFO - TEST-OK | devtools/shared/resources/tests/browser_resources_document_events.js | took 623ms
[task 2020-10-24T00:23:11.744Z] 00:23:11     INFO - checking window state
[task 2020-10-24T00:23:11.763Z] 00:23:11     INFO - TEST-START | devtools/shared/resources/tests/browser_resources_error_messages.js
[task 2020-10-24T00:23:56.776Z] 00:23:56     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-10-24T00:23:56.776Z] 00:23:56     INFO - Buffered messages logged at 00:23:11
[task 2020-10-24T00:23:56.776Z] 00:23:56     INFO - Entering test bound 
[task 2020-10-24T00:23:56.777Z] 00:23:56     INFO - Adding a new tab with URL: http://localhost:50394/test_page_errors.html
[task 2020-10-24T00:23:56.777Z] 00:23:56     INFO - Buffered messages finished
[task 2020-10-24T00:23:56.777Z] 00:23:56     INFO - TEST-UNEXPECTED-FAIL | devtools/shared/resources/tests/browser_resources_error_messages.js | Test timed out - 
[task 2020-10-24T00:23:56.779Z] 00:23:56     INFO - Removing tab.
[task 2020-10-24T00:23:56.779Z] 00:23:56     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2020-10-24T00:23:56.789Z] 00:23:56     INFO - Got event: 'TabClose' on [object XULElement].
[task 2020-10-24T00:23:56.793Z] 00:23:56     INFO - Tab removed and finished closing
[task 2020-10-24T00:23:56.812Z] 00:23:56     INFO - TEST-PASS | devtools/shared/resources/tests/browser_resources_error_messages.js | The main process DevToolsServer has no pending connection when the test ends - 
[task 2020-10-24T00:23:56.841Z] 00:23:56     INFO - GECKO(7768) | MEMORY STAT | vsize 2104229MB | vsizeMaxContiguous 65158206MB | residentFast 296MB | heapAllocated 82MB
[task 2020-10-24T00:23:56.841Z] 00:23:56     INFO - TEST-OK | devtools/shared/resources/tests/browser_resources_error_messages.js | took 45081ms
Flags: needinfo?(tcampbell)

I looked into these latest failures and it seems like it was just bad timing with Bug 1663571 landing at same time and changing DNS behaviour on Windows (which affects the test cases ability to lookup long-form error messages from MDN). The tree still has some devtools intermittents, so I'll hold of on relanding right away.

Flags: needinfo?(tcampbell)
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2df411f95780
Make source notes relative to initial line/column. r=jandem
Keywords: leave-open

The shell function for dumping srcnotes also needs to be adjusted. I'll add another patch to this stack for it.

Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40c3783a0adf
Show column numbers in jsshell src-note disassembly. r=arai
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox82: --- → wontfix
status-firefox83: --- → wontfix
status-firefox84: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: