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
--
Best regards, Sergey.
