RFR/advice: 8054292 : Code comments leak in fastdebug builds

[Vladimir Kozlov]

Hi David,

Yes, the clean solution but more memory usage would be to duplicate 
strings (and corresponding structures) when we copy them instead of 
copying only a pointer. Then we will not get dangling pointers when we 
always free them in destructors.

I agree that current changes are fragile and reactive to results of 
testing. It could be long tail of additional fixes after this as we find 
other cases. Can you estimate how much memory we use for strings per 
compilation? If it is not much we can go with "clean solution" to copy 
stings and free in destructors.

You added _code.free_strings() in C1 but you missed the same case in C2 
when compilation is bailed out and strings are not copied to nmethod.

CodeBuffer::free_strings() should set _Strings to NULL after freeing them.

Indention is wrong in new code - should be 2 spaces.
And no spaces around arrow:  "cb -> ".

In scratch_emit_size() you should free strings instead of setting NULL 
since they are not copied at that place.

Thanks,
Vladimir

On 8/13/14 9:13 AM, David Chase wrote:
> webrev: http://cr.openjdk.java.net/~drchase/8054292/webrev-for-leak-checks.04/
> bug: https://bugs.openjdk.java.net/browse/JDK-8054292
>
> There’s a problem with CodeBuffers leaking strings, and three possible solutions,
> and I’m looking for advice/discussion as to which solution might be best.
>
> The root cause is that though a CodeBuffer is supposed to exist for resource
> management purposes, it was not properly coded that way.  It’s currently coded
> to leak code comment strings, but the “clean fix” (free strings in destructor) might
> cause dangling pointers, and it’s difficult to prove that it won’t because it’s unlikely
> that I have tested down all the squirrelly paths
>
> I think there are three possible fixes with different risks:
>
> 1) “Clean fix”, reclaim resources in destructor.  Risk: dangling pointer, bites us probably when
> someone is debugging and prints something, otherwise the code comment strings are ignored.
> That bug will be annoying as heck when it happens.
>
> 2) “Dutch Boy with asserts”, zero/free strings at critical points in the lifecycle of a CodeBuffer,
> assert if a CodeBuffer is destroyed with unzeroed stings.  Risk: we are still leaking, and a test will
> fail, probably when execution goes down some weird and untested path.  But we’ll get an hs_err
> that explains the problem, so we can take steps to repair it.
>
> 3) “Dutch boy without asserts”, same as 2), but with no asserts.  Instead of failing, it will
> sometimes leak (but this will probably not occur that often) but it will not dangle pointers
> and it will not crash for leaking.
>
> The webrev illustrates the (potential) changes for all three solutions.
>
> I started by trying to identify all sources of leaks (to prove that there was a leak, in particular)
> by zeroing the string pointer when it was copied out, and then asserting it was zero in the
> destructor. A leak, or failure to identify a point where strings are copied out, is then identified
> by a crash.
>
> Next I tried the clean fix (a one-liner on top of the changes for the one-leak-at-a-time code),
> to see if it would cause any crashes.  This was not entirely satisfactory since the strings are
> for debugging purposes and most tests don’t check them; I did a run of jtreg with
> PrintOptoAssembly turned on to see if this would provoke a crash, and it did not.
>
> Testing:
>
> jtreg of compiler, gc, runtime
> jtreg of compiler w/ PrintOptoAssembly and “clean fix”.
> ute vm.quick.testlist
>
> I tried to run kitchensink, but it failed for lack of jrockit.qa.stress.kitchensink.process.stress.main .
> Local ute command was:
>
>    bin/ute -jdk $ff -testbase local/testbase/bigapps/Kitchensink -test bigapps/Kitchensink -env EXEC_DURATION=960
>
> and I had previously done
>
>    bin/ute fetch all -site east -jdkrelease 9
>
> Any ideas what I might be doing wrong with ute?
> This was a problem on both Linux (Ubuntu) and MacOS (Mavericks)
>
> I’m leaning towards “clean fix”, partly because I never saw anything that looked like creation of a dangling pointer.
>