Closed
Bug 1205081
Opened 9 years ago
Closed 9 years ago
Imprecise results from check_vanilla_allocations.py
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: jolesen, Assigned: jolesen)
References
Details
Attachments
(1 file, 3 obsolete files)
5.07 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
The script in config/check_vanilla_allocations.py is run as part of "make check" when building SpiderMonkey on Linux. There is a couple of problems with this script: - The regex looking for source filenames in the output from "nm -u -C -l" does not match source files with a '-' in their name. Files like Debugger-vixl.cpp are not checked. - The source file names reported by "nm -l" are not accurate when building with --enable-debug --enable-optimize. For example, the reference to "operator new(unsigned long)" in jsutil.cpp is reported as coming from ThreadLocal.h:135 when building with GCC-4.9 on Linux. It is more accurate to use "nm -u -C -A" and look for symbol references from the object file jsutil.o. Unfortunately, this way we lose the line number information when the check fails.
Comment 1•9 years ago
|
||
> - The regex looking for source filenames in the output from "nm -u -C -l" > does not match source files with a '-' in their name. Files like > Debugger-vixl.cpp are not checked. Well spotted! > - The source file names reported by "nm -l" are not accurate when building > with --enable-debug --enable-optimize. For example, the reference to > "operator new(unsigned long)" in jsutil.cpp is reported as coming from > ThreadLocal.h:135 when building with GCC-4.9 on Linux. Does this compromise correctness, or does it just make the output on failure less useful?
Flags: needinfo?(jolesen)
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1) > > - The source file names reported by "nm -l" are not accurate when building > > with --enable-debug --enable-optimize. For example, the reference to > > "operator new(unsigned long)" in jsutil.cpp is reported as coming from > > ThreadLocal.h:135 when building with GCC-4.9 on Linux. > > Does this compromise correctness, or does it just make the output on failure > less useful? It affects correctness. The jsutil.cpp source file has deliberate references to operator new etc that the script requires to be present. This is to avoid the failure mode where all the symbol references are somehow lost, and the test would do nothing. When the jsutil.cpp references to operator new are reported as coming from ThreadLocal.h, the vanilla allocations test (and thus make check) fails. It is also worth noting that "nm -l" is quite slow: jolesen@debian:~/gecko-dev$ time nm -u -C -l obj-x86dev/js/src/libjs_static.a > /dev/null real 0m22.576s user 0m21.268s sys 0m0.140s jolesen@debian:~/gecko-dev$ time nm -u -C -A obj-x86dev/js/src/libjs_static.a > /dev/null real 0m0.213s user 0m0.180s sys 0m0.020s I have a patch that fixes both of these issues.
Flags: needinfo?(jolesen)
Assignee | ||
Comment 3•9 years ago
|
||
information in optimized builds. Fix a problem with the filename regex so we can also detect symbol references in from source files with a hyphen in their name. This is a first version of the patch which has less precise error reporting. I think we should still run 'nm -l', but only after detecting an error in the 'nm -A' output. That way we can still get the very usefil line number information for the offending operator new references.
Assignee | ||
Comment 4•9 years ago
|
||
detecting errors in 'nm -A' output.
Assignee | ||
Updated•9 years ago
|
Attachment #8661912 -
Flags: review?(n.nethercote)
Comment 5•9 years ago
|
||
Comment on attachment 8661912 [details] [diff] [review] Updated patch that runs 'nm -l' the report line number information after Review of attachment 8661912 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for fixing this. I hope it wasn't too painful. The speed improvement is nice. Have you tested it by deliberately inserting calls to |operator new| in additional files and making sure it fails? If not, please do. ::: config/check_vanilla_allocations.py @@ +176,5 @@ > + > + for line in lines: > + m = re.search(alloc_lines_re, line) > + if m: > + print('check_vanilla_allocations.py: %30s at %s' % (m.group(1), m.group(3))) Nit: I'd use format() instead of '%'. Or even just use print() without special formatting.
Attachment #8661912 -
Flags: review?(n.nethercote) → review+
Comment 6•9 years ago
|
||
BTW, if you want to mark a patch as obsolete, click on its "Details" link, then click on the "edit details" link in the attachment view, then select the "obsolete" checkbox. Alternatively, if you use |hg mq| and |hg bzexport| and you export a patch that has the same name as a previous patch, the old copy will be obsoleted automatically. This is the approach I use and I find it much nicer than attaching patches by hand.
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5) > Have you tested it by deliberately inserting calls to |operator new| in > additional files and making sure it fails? If not, please do. Building with --enable-simulator=arm64, the script reports some errors: /home/jolesen/gecko-dev/obj-a64simdev/_virtualenv/bin/python /home/jolesen/gecko-dev/config/check_vanilla_allocations.py libjs_static.a TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'operator new[](unsigned' present in Unified_cpp_js_src7.o TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'operator new(unsigned' present in Unified_cpp_js_src7.o check_vanilla_allocations.py: Source lines with allocation calls: check_vanilla_allocations.py: Accurate in unoptimized builds; jsutil.cpp expected. check_vanilla_allocations.py: memalign at /home/jolesen/gecko-dev/js/src/jsutil.cpp:95 check_vanilla_allocations.py: operator new[](unsigned long) at /home/jolesen/gecko-dev/js/src/jsutil.cpp:94 check_vanilla_allocations.py: operator new(unsigned long) at /home/jolesen/gecko-dev/js/src/jsutil.cpp:91 check_vanilla_allocations.py: operator new[](unsigned long) at /home/jolesen/gecko-dev/js/src/jit/arm64/vixl/Simulator-vixl.cpp:2794 check_vanilla_allocations.py: operator new(unsigned long) at /home/jolesen/gecko-dev/js/src/jit/arm64/vixl/Debugger-vixl.cpp:533 Building without the simulator, the test passes. > ::: config/check_vanilla_allocations.py > @@ +176,5 @@ > > + > > + for line in lines: > > + m = re.search(alloc_lines_re, line) > > + if m: > > + print('check_vanilla_allocations.py: %30s at %s' % (m.group(1), m.group(3))) > > Nit: I'd use format() instead of '%'. Or even just use print() without > special formatting. Thanks, I'll fix it.
Assignee | ||
Updated•9 years ago
|
Attachment #8661827 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
[ I'm using 'git bz attach -e HEAD' which can also obsolete a previous patch, but I forgot to edit the #Obsoletes: line. ]
Attachment #8661912 -
Attachment is obsolete: true
Attachment #8662110 -
Flags: review?(n.nethercote)
Comment 9•9 years ago
|
||
Comment on attachment 8662110 [details] [diff] [review] Updated patch: Use basic string concatenation instead of the '%' operator. Review of attachment 8662110 [details] [diff] [review]: ----------------------------------------------------------------- For the future: when someone gives r+ and they have only minor comments, it generally means that they trust you to make the requested changes and they don't need to review the updated patch. E.g. if I asked for big changes and I wanted to see the revised version I would have given f+ or r-. But no harm to look again in this case, and better to err on the side of over-checking :) Thank you. ::: config/check_vanilla_allocations.py @@ +176,5 @@ > + > + for line in lines: > + m = re.search(alloc_lines_re, line) > + if m: > + print('check_vanilla_allocations.py: ' + m.group(1) + ' called at ' + m.group(3)) You could even do: > print('check_vanilla_allocations.py:', m.group(1), 'called at', m.group(3)) but now I'm really nit-picking :)
Attachment #8662110 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Thanks!
Attachment #8662110 -
Attachment is obsolete: true
Attachment #8662112 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8662112 [details] [diff] [review] The same thing occurred to me seconds after submitting the revised patch. Remove review request. This was minor changes to an already reviewed patch.
Attachment #8662112 -
Flags: review?(n.nethercote)
Updated•9 years ago
|
Attachment #8662112 -
Flags: review+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/76f495de5f78
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76f495de5f78
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•