1080462 - Valgrind detects leaks involving enableSPSProfilingWithSlowAssertions and js::jit::CodeGeneratorShared::generateCompactNativeToBytecodeMap

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function() {
    enableSPSProfilingWithSlowAssertions()
    for (z in [0]) {}
})()

causes the following leaks on m-c changeset f0bb13ef0ee4 with --ion-eager --no-threads at js::jit::CodeGeneratorShared::generateCompactNativeToBytecodeMap.

==32489== 8 bytes in 1 blocks are definitely lost in loss record 14 of 63
==32489==    at 0x4C2ABBD: malloc (vg_replace_malloc.c:296)
==32489==    by 0x687F5E: js_malloc (Utility.h:98)
==32489==    by 0x687F5E: js_pod_malloc<JSScript*> (Utility.h:559)
==32489==    by 0x687F5E: pod_malloc<JSScript*> (MallocProvider.h:62)
==32489==    by 0x687F5E: js::jit::CodeGeneratorShared::createNativeToBytecodeScriptList(JSContext*) (CodeGenerator-shared.cpp:569)
==32489==    by 0x688087: js::jit::CodeGeneratorShared::generateCompactNativeToBytecodeMap(JSContext*, js::jit::JitCode*) (CodeGenerator-shared.cpp:600)
/snip

==32489== 60 bytes in 1 blocks are definitely lost in loss record 37 of 63
==32489==    at 0x4C2ABBD: malloc (vg_replace_malloc.c:296)
==32489==    by 0x688115: js_malloc (Utility.h:98)
==32489==    by 0x688115: js_pod_malloc<unsigned char> (Utility.h:559)
==32489==    by 0x688115: pod_malloc<unsigned char> (MallocProvider.h:62)
==32489==    by 0x688115: js::jit::CodeGeneratorShared::generateCompactNativeToBytecodeMap(JSContext*, js::jit::JitCode*) (CodeGenerator-shared.cpp:623)
==32489==    by 0x5731F4: js::jit::CodeGenerator::link(JSContext*, js::types::CompilerConstraintList*) (CodeGenerator.cpp:7541)
/snip

Debug configure options:

AR=ar sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --disable-debug --enable-optimize=-O1 --enable-nspr-build --enable-more-deterministic --enable-valgrind --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/0f71056d9b6f
user:        Kannan Vijayan
date:        Wed Aug 13 11:59:52 2014 -0400
summary:     Bug 1004831 - Part 2 - Add compact map representation for native to bytecode mappings. r=luke,h4writer

Kannan, is bug 1004831 a possible regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: log

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-1080462.patch

Hey Gary, can you tell me if this fixes it?  It should.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8502573 [details] [diff] [review]
fix-bug-1080462.patch

No, it does not fix all of the issues. Log coming up.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: log

Here is the log when run against a binary compiled with the patch.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: bug-1080462.patch

This should fix that.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8509596 [details] [diff] [review]
bug-1080462.patch

Nope.

Btw, you can install Valgrind 3.10.0 on Linux and run:

valgrind --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --leak-check=full --errors-for-leak-kinds=definite --show-leak-kinds=definite ./js --ion-eager --no-threads 1080462.js

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: log

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: bug-1081374.patch

Found it.  Memory leak due to not properly destroying native2bytecode mapping table entries when they are removed.

Cleaned up leaks in a couple of error paths as well.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8510534 [details] [diff] [review]
bug-1081374.patch

Review of attachment 8510534 [details] [diff] [review]:
-----------------------------------------------------------------

Instead of doing "js_free(nativeToBytecodeScriptList_);" everywhere. Can we make nativeToBytecodeScriptList_ an UniquePtr. As a result when UniquePtr is removed it will automatically free the content? This would remove the need of js_free to be correctly placed.

What is the reason to put nativeToBytecodeScriptList_ and nativeToBytecodeMap_ are item in CodeGeneratorShared. Wouldn't it make more sense the function generateCompactNativeToBytecodeMap returns this as outparams?

Comment 10

[Hannes Verschore [:h4writer]]

@Djvj: can you elaborate on the two questions above, before I finish the review?

Comment 11

[Kannan Vijayan [:djvj]]

(In reply to Hannes Verschore [:h4writer] from comment #9)
> Comment on attachment 8510534 [details] [diff] [review]
> bug-1081374.patch
> 
> Review of attachment 8510534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Instead of doing "js_free(nativeToBytecodeScriptList_);" everywhere. Can we
> make nativeToBytecodeScriptList_ an UniquePtr. As a result when UniquePtr is
> removed it will automatically free the content? This would remove the need
> of js_free to be correctly placed.

I didn't think the number of places where js_free got introduced was that much.  I don't mind going the UniquePtr route, though.. just didn't feel it necessary to use it here.  I can change this.

> 
> What is the reason to put nativeToBytecodeScriptList_ and
> nativeToBytecodeMap_ are item in CodeGeneratorShared. Wouldn't it make more
> sense the function generateCompactNativeToBytecodeMap returns this as
> outparams?

There are several pieces of info generated by generateCompactNativeToBytecodeMap - the compact map, the script list, the list's size, the map size, and the offset into the map where the table is located (the ultimate "region pointer" that is extracted from the map points into the middle of the map data - specifically at the nativeToBytecodeTableOffset_).

One option is to stick all of these into a struct and have that be filled-in as an outparam to this function.

Comment 12

[Kannan Vijayan [:djvj]]

(In reply to Hannes Verschore [:h4writer] from comment #10)
> @Djvj: can you elaborate on the two questions above, before I finish the
> review?

Was the response in comment 11 sufficient?

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8510534 [details] [diff] [review]
bug-1081374.patch

Review of attachment 8510534 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Kannan Vijayan [:djvj] from comment #11)
> (In reply to Hannes Verschore [:h4writer] from comment #9)
> > Comment on attachment 8510534 [details] [diff] [review]
> > bug-1081374.patch
> > 
> > Review of attachment 8510534 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Instead of doing "js_free(nativeToBytecodeScriptList_);" everywhere. Can we
> > make nativeToBytecodeScriptList_ an UniquePtr. As a result when UniquePtr is
> > removed it will automatically free the content? This would remove the need
> > of js_free to be correctly placed.
> 
> I didn't think the number of places where js_free got introduced was that
> much.  I don't mind going the UniquePtr route, though.. just didn't feel it
> necessary to use it here.  I can change this.
> 
> > 
> > What is the reason to put nativeToBytecodeScriptList_ and
> > nativeToBytecodeMap_ are item in CodeGeneratorShared. Wouldn't it make more
> > sense the function generateCompactNativeToBytecodeMap returns this as
> > outparams?
> 
> There are several pieces of info generated by
> generateCompactNativeToBytecodeMap - the compact map, the script list, the
> list's size, the map size, and the offset into the map where the table is
> located (the ultimate "region pointer" that is extracted from the map points
> into the middle of the map data - specifically at the
> nativeToBytecodeTableOffset_).
> 
> One option is to stick all of these into a struct and have that be filled-in
> as an outparam to this function.

Yeah! A struct output with UniquePtr would definitely make this more safe. As in: make it harder to regress this again by not adding the right js_free's.
It would get free'd by default.
Can you add a follow-up bug to do this?

I'll r+ in the meantime. I'm not sure if this need to get backported, but the changes in this patch is smaller than the follow-up bug. So might be easier to backport (if needed?).

Comment 14

[Kannan Vijayan [:djvj]]

Try run looks good: https://hg.mozilla.org/try/rev/ca4fb71374f4
Pushed:  https://hg.mozilla.org/integration/mozilla-inbound/rev/0dae8ac74ab4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes during startup cache precompilation during the build packaging step, which usually points to xpcshell bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78f5069f7c2

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3823280&repo=mozilla-inbound

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Lots of random jit-test failures too:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3824426&repo=mozilla-inbound

Comment 17

[Kannan Vijayan [:djvj]]

Repushed after fixes and green try run (https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7edafbc93825).

 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4352c2b4fc4

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4352c2b4fc4