[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] JDK-8176287 :[macosx] The print test crashed with Nimbus L&F
From: Philip Race <philip.race () oracle ! com>
Date: 2017-03-15 18:02:49
Message-ID: 58C981C9.9050003 () oracle ! com
[Download RAW message or body]
I wondered that too but since it appeared we then had a loop
that explicitly initialised all elements it should not matter.
But I supposed it was sort of an accidental byproduct of trying
different things.
-phil.
On 3/15/17, 10:57 AM, Sergey Bylokhov wrote:
>>
>> On 3/14/2017 9:51 PM, Sergey Bylokhov wrote:
>>>
>>>> 14 марта 2017 г., в 14:41, Philip Race <philip.race@oracle.com
>>>> <mailto:philip.race@oracle.com>> написал(а):
>>>>
>>>> >Since this is mac specific code, I guess VS2010 will not play any
>>>> part in building this.
>>>>
>>>> Ah, yes :-)
>>>>
>>>> Updated fix looks OK.
>>>
>>> Should we memset the data allocated via malloc(calloc was used before)?
>>>
>> Should be a good practice to do it, I guess .
>> Modified webrev adding memset:
>> http://cr.openjdk.java.net/~psadhukhan/8176287/webrev.02/
>> <http://cr.openjdk.java.net/%7Epsadhukhan/8176287/webrev.02/>
>
> This looks fine to me, but I wonder why the calloc was replaced by
> malloc?
>
>>
>> Regards
>> Prasanta
>>>>
>>>> -phil.
>>>>
>>>> On 3/14/17, 2:31 AM, Prasanta Sadhukhan wrote:
>>>>>
>>>>> JPRT 8u build resulted in failure, so I had to modify at 2 other
>>>>> places.
>>>>>
>>>>> QuartzSurfaceData.m:287 and QuartzSurfaceData.m:328
>>>>> Other things remains same.
>>>>> http://cr.openjdk.java.net/~psadhukhan/8176287/webrev.01/
>>>>>
>>>>> Regards
>>>>> Prasanta
>>>>> On 3/14/2017 10:47 AM, Philip Race wrote:
>>>>>>
>>>>>>
>>>>>> On 3/13/17, 10:14 PM, Prasanta Sadhukhan wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 3/14/2017 10:24 AM, Philip Race wrote:
>>>>>>>> The problem seems to have been that you were allocating zero
>>>>>>>> bytes in the old code :
>>>>>>>> 950 CGFloat* colors= (CGFloat*)calloc(0, sizeof(CGFloat)*length);
>>>>>>>>
>>>>>>>> 960 qsdo->gradientInfo->colordata = (CGFloat*)calloc(0, sizeof(CGFloat)*4*length);
>>>>>>>>
>>>>>>>> Regarding the new code, whilst it seems like it fixes the problem I have a nit
>>>>>>>> 937 int i;
>>>>>>>> 938 for (i=0; i<length; i++)
>>>>>>>>
>>>>>>>> Since this code appears at the start of a block I'd expect all
>>>>>>>> compilers to be happy with
>>>>>>>> for (int i=0; i<length; i++)
>>>>>>>>
>>>>>>>> is this not so ? Assuming yes, pls fix before push.
>>>>>>> Yes, it should be ok. I got a problem with jdk8u JPRT build
>>>>>>> (during earlier backport) citing C99 compiler failure but I
>>>>>>> guess that was because variable was declared not at blockstart.
>>>>>>> Will again do a JPRT and if its ok, I will push with this change.
>>>>>>
>>>>>> Testing the 8u backport via JPRT is good since that will use
>>>>>> VS2010 which
>>>>>> wins the "most likely to barf" award on such an issue.
>>>>>>
>>>>>> -phil
>>>>>>>>
>>>>>>>> Also I wonder if the regression test we created for LGP passes
>>>>>>>> only because it is "short".
>>>>>>>> Perhaps later we can improve on that.
>>>>>>>>
>>>>>>>> The fix will also need to be backported since the original fix
>>>>>>>> was backported.
>>>>>>>>
>>>>>>> ok.
>>>>>>>> So "+1" with those comments ..
>>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Regards
>>>>>>> Prasanta
>>>>>>>> -phil.
>>>>>>>>
>>>>>>>> On 3/12/17, 11:49 PM, Prasanta Sadhukhan wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>> Please review a jck print test crash fix for jdk9. The issue
>>>>>>>>> was seen with only Nimbus L&F which seems to use Linear
>>>>>>>>> gradient path
>>>>>>>>> and not in other L&F (such as Aqua) .
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176287
>>>>>>>>> webrev: http://cr.openjdk.java.net/~psadhukhan/8176287/webrev.00/
>>>>>>>>>
>>>>>>>>> Linear Gradient path collects the gradient colors and
>>>>>>>>> fractions values in native obtained from Java and allocates
>>>>>>>>> several arrays to store the same in setupGradient() method.
>>>>>>>>> It seems even after being freed, in subsequent call to the
>>>>>>>>> same gradient path routine, it may get the same allocated
>>>>>>>>> pointer the next time the array is allocated causing it to
>>>>>>>>> crash citing "memory being modified after freed".
>>>>>>>>>
>>>>>>>>> Optimise setupGradient() method to allocate fewer pointer. The
>>>>>>>>> JCK test works now.
>>>>>>>>> Also, the JDK-8162796 testcase LinearGradientPrintingTest and
>>>>>>>>> RadialGradientPrintingTest works with this optimisation.
>>>>>>>>>
>>>>>>>>> Regards
>>>>>>>>> Prasanta
>>>>>>>
>>>>>
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
I wondered that too but since it appeared we then had a loop<br>
that explicitly initialised all elements it should not matter.<br>
<br>
But I supposed it was sort of an accidental byproduct of trying
different things.<br>
<br>
-phil.<br>
<br>
On 3/15/17, 10:57 AM, Sergey Bylokhov wrote:
<blockquote
cite="mid:3FFB08F7-F271-44AB-BBC0-75F85746AA11@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><br class="">
<div class="moz-cite-prefix">On 3/14/2017 9:51 PM, Sergey
Bylokhov wrote:<br class="">
</div>
<blockquote
cite="mid:AA800756-9F73-4702-AF4A-DF363FDA1F8F@oracle.com"
type="cite" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8" class="">
<br class="">
<div class="">
<blockquote type="cite" class="">
<div class="">14 марта 2017 г., в 14:41, Philip Race
<<a moz-do-not-send="true"
href="mailto:philip.race@oracle.com" \
class="">philip.race@oracle.com</a>>
написал(а):</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
>Since this is mac specific code, I guess
VS2010 will not play any part in building this.<br
class="">
<br class="">
Ah, yes :-)<br class="">
<br class="">
Updated fix looks OK.<br class="">
</div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">Should we memset the data allocated via
malloc(calloc was used before)?</div>
<br class="">
</div>
</blockquote>
Should be a good practice to do it, I guess .<br class="">
Modified webrev adding memset:<br class="">
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epsadhukhan/8176287/webrev.02/"
class="">http://cr.openjdk.java.net/~psadhukhan/8176287/webrev.02/</a><br
class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>This looks fine to me, but I wonder why the calloc was
replaced by malloc? </div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""> <br
class="">
Regards<br class="">
Prasanta<br class="">
<blockquote
cite="mid:AA800756-9F73-4702-AF4A-DF363FDA1F8F@oracle.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""> <br
class="">
-phil.<br class="">
<br class="">
On 3/14/17, 2:31 AM, Prasanta Sadhukhan wrote:
<blockquote
cite="mid:0bd81696-1e17-9321-30ae-d414d9a279a0@oracle.com"
type="cite" class="">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type" class="">
<p class="">JPRT 8u build resulted in failure,
so I had to modify at 2 other places.</p>
<pre class="">QuartzSurfaceData.m:287 and \
QuartzSurfaceData.m:328</pre> Other things remains same.<br class="">
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="http://cr.openjdk.java.net/%7Epsadhukhan/8176287/webrev.01/">http://cr.openjdk.java.net/~psadhukhan/8176287/webrev.01/</a><br
class="">
<br class="">
Regards<br class="">
Prasanta<br class="">
<div class="moz-cite-prefix">On 3/14/2017
10:47 AM, Philip Race wrote:<br class="">
</div>
<blockquote
cite="mid:58C77CF9.4070908@oracle.com"
type="cite" class="">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type" class="">
<br class="">
<br class="">
On 3/13/17, 10:14 PM, Prasanta Sadhukhan
wrote:
<blockquote
\
cite="mid:3238af15-a7fc-836d-b5d1-e64e8676087b@oracle.com" type="cite" class="">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type" class="">
<p class=""><br class="">
</p>
<br class="">
<div class="moz-cite-prefix">On 3/14/2017
10:24 AM, Philip Race wrote:<br class="">
</div>
<blockquote
cite="mid:58C77774.90600@oracle.com"
type="cite" class="">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type" class="">
The problem seems to have been that you
were allocating zero bytes in the old
code :<br class="">
<meta http-equiv="content-type"
content="text/html; charset=UTF-8"
class="">
<pre class=""><span class="changed"> 950 \
CGFloat* colors= (CGFloat*)calloc(0, sizeof(CGFloat)*length);
</span></pre>
<meta http-equiv="content-type"
content="text/html; charset=UTF-8"
class="">
<pre class=""><span class="changed">960 \
qsdo->gradientInfo->colordata = (CGFloat*)calloc(0, sizeof(CGFloat)*4*length);
Regarding the new code, whilst it seems like it fixes the problem I have a nit
</span></pre>
937 int i;<br class="">
938 for (i=0; i<length;
i++)<br class="">
<br class="">
Since this code appears at the start of
a block I'd expect all compilers to be
happy with<br class="">
for (int i=0; i<length; i++)<br
class="">
<br class="">
is this not so ? Assuming yes, pls fix
before push.<br class="">
</blockquote>
Yes, it should be ok. I got a problem with
jdk8u JPRT build (during earlier backport)
citing C99 compiler failure but I guess
that was because variable was declared not
at blockstart.<br class="">
Will again do a JPRT and if its ok, I will
push with this change.<br class="">
</blockquote>
<br class="">
Testing the 8u backport via JPRT is good
since that will use VS2010 which<br class="">
wins the "most likely to barf" award on such
an issue.<br class="">
<br class="">
-phil<br class="">
<blockquote
\
cite="mid:3238af15-a7fc-836d-b5d1-e64e8676087b@oracle.com" type="cite" class="">
<blockquote
cite="mid:58C77774.90600@oracle.com"
type="cite" class=""> <br class="">
Also I wonder if the regression test we
created for LGP passes only because it
is "short".<br class="">
Perhaps later we can improve on that.<br
class="">
<br class="">
The fix will also need to be backported
since the original fix was backported.<br
class="">
<br class="">
</blockquote>
ok.<br class="">
<blockquote
cite="mid:58C77774.90600@oracle.com"
type="cite" class=""> So "+1" with those
comments ..<br class="">
<br class="">
</blockquote>
Thanks<br class="">
<br class="">
Regards<br class="">
Prasanta<br class="">
<blockquote
cite="mid:58C77774.90600@oracle.com"
type="cite" class=""> -phil.<br class="">
<br class="">
On 3/12/17, 11:49 PM, Prasanta Sadhukhan
wrote:
<blockquote
\
cite="mid:cd19de59-2d2c-9e0b-a553-6e83102abdfd@oracle.com" type="cite" class="">Hi \
All, <br class="">
<br class="">
Please review a jck print test crash
fix for jdk9. The issue was seen with
only Nimbus L&F which seems to use
Linear gradient path <br class="">
and not in other L&F (such as
Aqua) . <br class="">
<br class="">
Bug: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="https://bugs.openjdk.java.net/browse/JDK-8176287">https://bugs.openjdk.java.net/browse/JDK-8176287</a>
<br class="">
webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="http://cr.openjdk.java.net/%7Epsadhukhan/8176287/webrev.00/">http://cr.openjdk.java.net/~psadhukhan/8176287/webrev.00/</a>
<br class="">
<br class="">
Linear Gradient path collects the
gradient colors and fractions values
in native obtained from Java and
allocates several arrays to store the
same in setupGradient() method. <br
class="">
It seems even after being freed, in
subsequent call to the same gradient
path routine, it may get the same
allocated pointer the next time the
array is allocated causing it to crash
citing "memory being modified after
freed". <br class="">
<br class="">
Optimise setupGradient() method to
allocate fewer pointer. The JCK test
works now. <br class="">
Also, the JDK-8162796 testcase
LinearGradientPrintingTest and
RadialGradientPrintingTest works with
this optimisation. <br class="">
<br class="">
Regards <br class="">
Prasanta <br class="">
</blockquote>
</blockquote>
<br class="">
</blockquote>
</blockquote>
<br class="">
</blockquote>
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic