[OpenJDK 2D-Dev] [9] Review Request: 8080847 Copying of overlapping memory should be improved in java2d

Jim Graham james.graham at oracle.com
Thu May 28 00:02:10 UTC 2015 

I see no evidence that it can crash.  Again, undefined does not mean 
"can crash".  Someone joked on that thread that they should probably 
make memcpy crash on overlapping memory moves in a future release, but I 
don't see any evidence that anyone took them seriously.  The entire 
thread is more about whether or not they should make the "undefined" 
results consistent with historical versions of "undefined".

Having it switch from reliably, though unexpectedly, smearing one 
direction to reliably, and also unexpectedly, smearing in another 
direction - which is what the discussion you linked to is about - is 
consistent with "undefined".  I still see this as no reason to change 
the code.  They do discuss making memcpy default to memmove under the 
covers, but I didn't see buy-in for that and there were only vague 
references to "there is a patch already in flight that does what Bob 
said".  Even if it did default to memmove, then I still don't see that 
as a reason to change any code here.  It is still undefined since the 
macro doesn't deal with overlaps in the Y direction and it doesn't 
affect all platforms.

If we change the default blit to reliably always "do the right thing" 
regardless of blit direction then we set the expectation that drawing an 
image onto itself will always behave responsibly and then we invite the 
bug that it will fail for XOR, or alpha composites/extra alpha, or 
stretching or rotating.  Until we are ready to commit to that, I don't 
see any value in making this one version of blit "do the right thing".

For now, we do not promise any defined behavior for copying an image 
onto itself.  If we want to tackle that, then I'd support adding that 
guarantee to our behaviors, but it should be done comprehensively - not 
by making a change that only affects part of one implementation...

			...jim

On 5/27/2015 3:36 AM, Sergey Bylokhov wrote:
> On 26.05.15 21:27, Jim Graham wrote:
>> Undefined doesn't mean "may crash" in this case, it means that the
>> contents of memory may not match what you would expect if the regions
>> overlap because it is just a dump copy loop that does not do any
>> aliasing checks.
> Since it is undefined it can crash, but even if it is not, it can
> produce the different results for the same application on different cpu.
> Because it can copy data in any direction based on the current cpu. It
> is not a simple copy loop. see discussion [1] for example.
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=12518
>       https://sourceware.org/bugzilla/show_bug.cgi?id=12518#c3
>
> I still insist that it is the simplest fix, which will relieve us of
> randomness and will bring into accord with the native specification. It
> is similar to this rule: Swing should be accessed on the EDT.
> Application can work for decades and contradict this rule, but can be
> broken in any updates of Swing.
>
>>
>> Is there a way to silence the warning?  In this particular case we are
>> totally OK with the undefined behavior, in fact, the accidental
>> behavior that they are calling "undefined" because it is confusing to
>> most developers who don't know enough to worry about aliased memory
>> regions - is actually the behavior we want because it will match the
>> results of all of the other blits.
>>
>> If there is no way to silence the tool, then I'd recommend hard-coding
>> our own "dumb copy loop" instead so that the behavior continues to
>> match what memcpy should have been doing.
>>
>> Do not just fix this in the vertical direction as well - if you
>> continue on a path that makes the aliasing not happen then I will
>> insist that you modify all drawimage paths to all deal gracefully with
>> memory aliasing and write an extensive test suite to make sure that we
>> correctly manage the aliasing in all cases, all composite modes, the
>> bg versions as well as the non-bg versions, scaled and transformed
>> blits, etc.  If you are not prepared to do all of that, then we should
>> drop this attempt to fix a "bug" that is really code working as
>> (un)expected and focus instead on silencing the warning...
>>
>>             ...jim
>>
>> On 5/26/2015 4:34 AM, Sergey Bylokhov wrote:
>>> On 26.05.15 13:43, Jim Graham wrote:
>>>> What crash in memcpy?
>>> Simply because behavior of this function is undefined if the two arrays
>>> "to" and "from" overlap. Plus this clears an output for the tools like
>>> valgrind and some other issues can be found easily.
>>>
>>>> The issue you pointed to is about dealing with overlapping memory.
>>>> memcpy does not crash on overlapping memory copies, it just duplicates
>>>> data oddly in a way that most uses probably don't want.
>>>>
>>>> Also, the fix you gave only fixed the problem for the horizontal
>>>> direction, if the drawImage call were directed at 0,1 then we'd still
>>>> get all scan lines duplicated from the first...
>>> Right, I can take a look to this bug too.
>>>>
>>>>         ...jim
>>>>
>>>> On 5/25/2015 12:32 PM, Sergey Bylokhov wrote:
>>>>> Hi, Jim.
>>>>> Actually there is a difference in support: it works but result is a
>>>>> little bit wrong, and it does not work and crashes. This fix is not a
>>>>> general solution for the incorrect result of the blit+aliasing, but
>>>>> for
>>>>> the possible crash of memcpy especially after some improvements
>>>>> like in
>>>>> glibc. I think this still an issue.
>>>>>
>>>>>> I don't recall if we ever promised that this case would work without
>>>>>> aliasing issues.  I know that we went out of our way in the copyArea
>>>>>> method to prevent the aliasing issue, doing the blits piecemeal so
>>>>>> that they don't interfere with each other.  Further, while it may be
>>>>>> easy enough to just call memmove to have the libraray do this for us
>>>>>> in the IsoBlit case, other cases that don't fall into the IsoBlit
>>>>>> macro will not be similarly protected.  In particular, if you specify
>>>>>> an alpha value, you will not get this protection (at least not
>>>>>> without
>>>>>> a huge amount of work to overhaul the entire DrawImage pipeline).
>>>>>>
>>>>>> I would say that this would be OK if we planned to make this promise
>>>>>> about drawImage across all image formats and composition modes, but
>>>>>> that would be a far more complicated fix.  Until then, we should not
>>>>>> open this can of worms by modifying this one specific Blit case...
>>>>>>
>>>>>>             ...jim
>>>>>>
>>>>>> On 5/25/2015 5:35 AM, Sergey Bylokhov wrote:
>>>>>>> Hello.
>>>>>>> Please review the fix forjdk9.
>>>>>>>
>>>>>>> I found this issue during code review of another task, related to
>>>>>>> performance.
>>>>>>>
>>>>>>> The sample code below will call the IsomorphicCopy method which call
>>>>>>> memcpy on the overlapping memory(this is the simplest example)
>>>>>>>
>>>>>>>       BufferedImage img = new BufferedImage(100, 100,
>>>>>>> BufferedImage.TYPE_INT_ARGB_PRE);
>>>>>>>       Graphics2D g = img.createGraphics();
>>>>>>>       g.setComposite(AlphaComposite.Src);
>>>>>>>       g.drawImage(img, 0, 0, null);
>>>>>>>       g.dispose();
>>>>>>>
>>>>>>> http://linux.die.net/man/3/memcpy
>>>>>>> "The memcpy() function copies n bytes from memory area src to memory
>>>>>>> area dest. The memory areas must not overlap. Use memmove(3) if the
>>>>>>> memory areas do overlap"
>>>>>>>
>>>>>>>
>>>>>>> I can confirm this bug using valgrind and a program above:
>>>>>>> command:
>>>>>>> valgrind --smc-check=all --tool=memcheck --leak-check=full -v
>>>>>>> ./9/client/build/linux-x86_64-normal-server-fastdebug/images/jdk/bin/java
>>>>>>>
>>>>>>>
>>>>>>> -Xint
>>>>>>> Main
>>>>>>>
>>>>>>> output:
>>>>>>> ==60975== Source and destination overlap in memcpy(0xe1b8b4d8,
>>>>>>> 0xe1b8b4d8, 400)
>>>>>>> ==60975== at 0x4C2F71C: memcpy@@GLIBC_2.14 (in
>>>>>>> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>>>>>>> ==60975== by 0x1E0F504D: AnyIntIsomorphicCopy (in
>>>>>>> /moe/workspaces/jdk/9/client-work/build/linux-x86_64-normal-server-fastdebug/images/jdk/lib/amd64/libawt.so)
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ==60975== by 0x1E0F5DE8: Java_sun_java2d_loops_Blit_Blit (in
>>>>>>> /moe/workspaces/jdk/9/client-work/build/linux-x86_64-normal-server-fastdebug/images/jdk/lib/amd64/libawt.so)
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8080847
>>>>>>> Webrev can be found at:
>>>>>>> http://cr.openjdk.java.net/~serb/8080847/webrev.00
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>

More information about the 2d-dev mailing list