splashscreen.so is missing pnggccrd.c

Fri Aug 29 10:30:14 UTC 2008

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 <martinrb at google.com> 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 <Anthony.Petrov at sun.com> 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 <Anthony.Petrov at sun.com>
>>>> 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.
> 