Hi Martin,
The AWT folks think we need to guard this CPPFLAGS setting with the
following condition:
ifneq ($(PLATFORM), windows)
...
endif
We don't use gcc on Windows, and everything works just fine with MS VS.
Please feel free to submit the patch with this change, and please add
OpenJDK user 'art' to the reviewers list. Given this modification
applied, I'm approving the fix. The push is going to be performed by
Artem (CC'ed). Thanks in advance!
--
best regards,
Anthony
On 08/29/2008 01:58 AM Martin Buchholz wrote:
Hi Anthony,
I investigated yet a bit more, and finally am sure about the underlying cause:
Standard gcc does not define __MMX__, but ours does,
perhaps because it has been configured to make MMX support the default.
$ gcc -E -dM main.c | grep __MMX__
$ /usr/local/google/fauxJDKCompiler/gcc -E -dM main.c | grep __MMX__
#define __MMX__ 1
By default, pngconf.h uses mmx instructions if __MMX__ is defined.
But this can be disabled by -DPNG_NO_MMX_CODE.
Since the existing code in OpenJDK depends on MMX not being used
(since pnggccrd.c is never compiled) -DPNG_NO_MMX_CODE should
be added to CPPFLAGS in the splashscreen makefile unconditionally.
Requesting permission to push...
You could file a new bug, or we could reuse 6613927, at your option.
Martin
# HG changeset patch
# User martin
# Date 1219960566 25200
# Node ID 198b5e82cd0fa4b42cafabce3e763e2a2cb3888c
# Parent 1267605489211c6c162bb246f653759e933bd06e
6613927: Compilation of splashscreen png library failed on Ubuntu 7.04 (64bit)
Summary: Define -DPNG_NO_MMX_CODE unconditionally, not just on 64-bit Linux
Reviewed-by: anthony
diff --git a/make/sun/splashscreen/Makefile b/make/sun/splashscreen/Makefile
--- a/make/sun/splashscreen/Makefile
+++ b/make/sun/splashscreen/Makefile
@@ -85,13 +85,6 @@
CPPFLAGS += -I$(PLATFORM_SRC)/native/$(PKGDIR)/splashscreen
-I$(SHARE_SRC)/native/$(PKGDIR)/splashscreen
CPPFLAGS += -I$(SHARE_SRC)/native/$(PKGDIR)/image/jpeg
-I$(SHARE_SRC)/native/java/util/zip/zlib-1.1.3
-ifeq ($(PLATFORM), linux)
- ifeq ($(ARCH_DATA_MODEL), 64)
- # 64-bit gcc has problems compiling MMX instructions.
- # Google it for more details. Possibly the newer versions of
- # the PNG-library and/or the new compiler will not need this
- # option in the future.
- CPPFLAGS += -DPNG_NO_MMX_CODE
- endif
-endif
-
+# Shun the less than portable MMX assembly code in pnggccrd.c,
+# and use alternative implementations in C.
+CPPFLAGS += -DPNG_NO_MMX_CODE
On Thu, Aug 28, 2008 at 9:33 AM, Martin Buchholz <[EMAIL PROTECTED]> wrote:
I'm thinking:
- the MMX support is in pnggccrd.c,
- but that file is never compiled in OpenJDK
- so... how could png ever work when PNG_NO_MMX_CODE is not defined,
on any platform?
I am suspicious that png splashscreens are actually broken
without my proposed patch. Has this actually been tested?
I googled a little and have a theory.
"-z defs" is supposed to be ignored for shared libs
(but why are we using it to build libsplashscreen.so?)
but very recent gcc toolchains don't ignore it the way they
perhaps should.
E.g. Here's a debian bug report
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=489142
On hppa -Wl,-z,defs is not ignored for shared libraries
But using "-z defs" is actually good engineering practice,
since it catches problems at link time instead of run time.
Whether or not it's a bug in my toolchain, my toolchain
detects the unsatisfied symbols in the link.
There's a good chance that my change will be necessary
to build OpenJDK on future stock Linux systems.
Martin
On Thu, Aug 28, 2008 at 3:11 AM, Anthony Petrov <[EMAIL PROTECTED]> wrote:
Hello Martin,
Thank you for the patch. Though there's a little concern: how does the bare
libpng deal with this issue when compiled using the toolchain you use? I
mean, the problem is not reproducible when compiling on a "regular" 32-bit
system, so it might be important to identify the root cause of this issue.
Could you please take a look at the libpng source (the version that was
forked for the purposes of OpenJDK is 1.2.18) and try compiling it with your
tools (by the way, what are they?)? If this is the problem with this
particular version of libpng, and newer versions do not experience this
problem, then this might be probably a justification for updating our
version of libpng rather than disabling the usage of MMX for all platforms.
What do you think?
--
best regards,
Anthony
On 08/28/2008 02:23 AM Martin Buchholz wrote:
Anthony,
Thanks for your patience; you were entirely right,
it was indeed necessary to define PNG_NO_MMX_CODE.
Since pnggccrd.c is never compiled, and it contains the MMX code,
the only correct thing appears to be to define this macro unconditionally.
The obvious patch follows (tested on 32-bit Ubuntu Linux with
very recent gcc toolchain)
(Has anyone actually tested png splashscreen images on 32 or 64-bit
Linux?)
# HG changeset patch
# User martin
# Date 1219875308 25200
# Node ID 86b160e4bc1aae927aa71dfd712bb2365ebc0e1c
# Parent 1267605489211c6c162bb246f653759e933bd06e
6613927: Compilation of splashscreen png library failed on Ubuntu 7.04
(64bit)
Summary: Define -DPNG_NO_MMX_CODE unconditionally, not just on 64-bit
Linux
Reviewed-by: anthony
diff --git a/make/sun/splashscreen/Makefile
b/make/sun/splashscreen/Makefile
--- a/make/sun/splashscreen/Makefile
+++ b/make/sun/splashscreen/Makefile
@@ -85,13 +85,6 @@
CPPFLAGS += -I$(PLATFORM_SRC)/native/$(PKGDIR)/splashscreen
-I$(SHARE_SRC)/native/$(PKGDIR)/splashscreen
CPPFLAGS += -I$(SHARE_SRC)/native/$(PKGDIR)/image/jpeg
-I$(SHARE_SRC)/native/java/util/zip/zlib-1.1.3
-ifeq ($(PLATFORM), linux)
- ifeq ($(ARCH_DATA_MODEL), 64)
- # 64-bit gcc has problems compiling MMX instructions.
- # Google it for more details. Possibly the newer versions of
- # the PNG-library and/or the new compiler will not need this
- # option in the future.
- CPPFLAGS += -DPNG_NO_MMX_CODE
- endif
-endif
-
+# Shun the less than portable MMX assembly code in pnggccrd.c.
+# Newer versions of the PNG-library may not need this option.
+CPPFLAGS += -DPNG_NO_MMX_CODE
Martin
On Wed, Aug 27, 2008 at 5:48 AM, Anthony Petrov <[EMAIL PROTECTED]>
wrote:
Hi Martin,
On 08/25/2008 08:17 PM Martin Buchholz wrote:
Thanks for the link, but...
- The fix for 6613927 is applied in my workspace
And it gets activated when building on a 64-bit system only. Could you
please take a look at the make/sun/splashscreen/Makefile file and make
sure
it defines the "CPPFLAGS += -DPNG_NO_MMX_CODE" in every case. It is
really
very interesting if this will resolve the issue. Thank you in advance.
