354966 - x86_64 can't assemble pnggccrd.c in libpng-1.2.12

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

It was reported in bug #334110 that after libpng-1.2.12 was checked in, the x86_64 platform cannot compile/assemble pnggccrd.c:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1159571460.22166.gz

The problem has been known to the libpng developers, and they deal with it by doing a trial compile of pnggccrd.c in their configure script, and if it fails, they include "#define PNG_NO_MMX_CODE" in a header file.

The simplest solution is just to put #define PNG_NO_MMX_CODE in mozpngconf.h,
but that would sacrifice decoding speed on MMX platforms.

Comment 1

[Glenn Randers-Pehrson]

Attachment: patch configure.in to do a trial compile of pnggccrd.c

Patch to make configure.in do a trial compile of pnggccrd.c, similar to what is done in libpng's configure script.  If the compile fails, it defines the PNG_NO_MMX_CODE flag.

Comment 2

[Glenn Randers-Pehrson]

Sylvain: Please try the patch on your x86_64 platform.

Comment 3

[Sylvain Pasche]

Attachment: patch, v2

Compiling pnggccrd.c was not failing as it should. I found out that some defines were missing so that the assembly instructions were not executed. I updated the patch by defining MOZ_PNG_READ which does the trick.

Comment 4

[Glenn Randers-Pehrson]

Comment on attachment 240780 [details] [diff] [review]
patch, v2

sylvain: thanks!
pav: r?

Comment 5

[daedalus144]

(In reply to comment #4)
> (From update of attachment 240780 [details] [diff] [review] [edit])
> sylvain: thanks!
> pav: r?
> 

Tried patch v2

Got a similar assembler error:

gcc -o pngrio.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux2.6\" -DOSARCH=\"Linux\" -DBUILD_ID=2006100112 -I/root/source/mozilla/modules/libimg/png -I../../../dist/include/zlib -I../../../dist/include   -I../../../dist/include/png -I../../../dist/include/nspr  -DMOZ_PNG_READ -DMOZ_PNG_WRITE  -I../../../dist/sdk/include    -fPIC -O2 -march=athlon64 -pipe  -Wall -W -Wno-unused -Wpointer-arith -Wcast-align -Wno-long-long -pedantic -O2 -march=athlon64 -pipe -pthread -pipe  -DNDEBUG -DTRIMMED -O2 -march=athlon64 -pipe  -O2 -march=athlon64 -pipe  -include ../../../mozilla-config.h -DMOZILLA_CLIENT -Wp,-MD,.deps/pngrio.pp /root/source/mozilla/modules/libimg/png/pngrio.c
{standard input}: Assembler messages:
{standard input}:19: Error: suffix or operands invalid for `push'
{standard input}:20: Error: suffix or operands invalid for `push'
{standard input}:21: Error: suffix or operands invalid for `push'
{standard input}:22: Error: suffix or operands invalid for `pushf'
{standard input}:23: Error: suffix or operands invalid for `pop'
{standard input}:26: Error: suffix or operands invalid for `push'
{standard input}:27: Error: suffix or operands invalid for `popf'
{standard input}:28: Error: suffix or operands invalid for `pushf'
{standard input}:29: Error: suffix or operands invalid for `pop'
{standard input}:30: Error: suffix or operands invalid for `push'
{standard input}:31: Error: suffix or operands invalid for `popf'
{standard input}:49: Error: suffix or operands invalid for `pop'
{standard input}:50: Error: suffix or operands invalid for `pop'
{standard input}:51: Error: suffix or operands invalid for `pop'
{standard input}:124: Error: suffix or operands invalid for `push'
{standard input}:210: Error: suffix or operands invalid for `pop'
gmake[5]: *** [pnggccrd.o] Error 1
gmake[5]: *** Waiting for unfinished jobs....

Comment 6

[daedalus144]

Just an update on my last reply: after I put #define PNG_NO_MMX_CODE directly in mozpngconf.h it now works.

Comment 7

[Glenn Randers-Pehrson]

Comment on attachment 240780 [details] [diff] [review]
patch, v2

Cancelling requests for review since Sylvain says it doesn't work.

Comment 8

[Sylvain Pasche]

(In reply to comment #5)
> Tried patch v2
> 
> Got a similar assembler error:

Are you sure you ran autoconf2.13 after patching configure.in?

(In reply to comment #7)
> (From update of attachment 240780 [details] [diff] [review] [edit])
> Cancelling requests for review since Sylvain says it doesn't work.

I guess you mean [e-mail] instead ;-)

Comment 9

[Glenn Randers-Pehrson]

sylvain:  Sorry, yes I meant daedelus.
daedelus:
This line
  -I../../../dist/include/nspr  -DMOZ_PNG_READ -DMOZ_PNG_WRITE
should have read
  -I../../../dist/include/nspr  -DMOZ_PNG_READ -DPNG_NO_MMX_CODE -DMOZ_PNG_WRITE

In the output from "configure" what was the line regarding pnggccrd.c?
It should have been
Checking if pnggccrd.c can be compiles without PNG_NO_MMX_CODE...no

Maybe the flag got lost while appending the  -DMOZ_PNG_WRITE flag.
Or maybe as Sylvain suggested you need to run autoconf.

Comment 10

[daedalus144]

Oops! I forgot to run autoconf. The patch is working fine now.

Comment 11

[Glenn Randers-Pehrson]

Comment on attachment 240780 [details] [diff] [review]
patch, v2

tor: sr?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked this in because my builds break without it.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch seems like a bad idea, since it makes it easy to accidentally disable the optimizations by introducing a compilation error in them.

It seems like we should be defining PNG_NO_MMX_CODE for the cases where we *expect* the code not to compile, so we actually get compiler errors telling us about unexpected compilation failure.

Since this is x86 assembly, shouldn't we just define it for everything that's not x86?  (That said, it would be nice if libpng had x86_64 assembly too...)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(That said, if we're only taking libpng dumps and never modifying them, it's probably reasonable to leave it, since it's more maintainable than tracking what's really supported.  That said, I'd think libpng itself ought to...)