Closed Bug 1205081 Opened 9 years ago Closed 9 years ago

Imprecise results from check_vanilla_allocations.py

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jolesen, Assigned: jolesen)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
> - 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)
(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)
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.
detecting errors in 'nm -A' output.
Attachment #8661912 - Flags: review?(n.nethercote)
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+
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.
(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.
Attachment #8661827 - Attachment is obsolete: true
[ 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 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+
Thanks!
Attachment #8662110 - Attachment is obsolete: true
Attachment #8662112 - Flags: review?(n.nethercote)
Keywords: checkin-needed
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)
https://hg.mozilla.org/mozilla-central/rev/76f495de5f78
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Blocks: 1206187
You need to log in before you can comment on or make changes to this bug.