Closed
Bug 354966
Opened 18 years ago
Closed 18 years ago
x86_64 can't assemble pnggccrd.c in libpng-1.2.12
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
VERIFIED
FIXED
People
(Reporter: glennrp+bmo, Assigned: glennrp+bmo)
References
Details
Attachments
(1 file, 1 obsolete file)
1.24 KB,
patch
|
pavlov
:
review+
tor
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•18 years ago
|
||
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.
Assignee | ||
Comment 2•18 years ago
|
||
Sylvain: Please try the patch on your x86_64 platform.
Comment 3•18 years ago
|
||
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.
Attachment #240776 -
Attachment is obsolete: true
Assignee | ||
Comment 4•18 years ago
|
||
Comment on attachment 240780 [details] [diff] [review] patch, v2 sylvain: thanks! pav: r?
Attachment #240780 -
Flags: superreview?(tor)
Attachment #240780 -
Flags: review?(pavlov)
Comment 5•18 years ago
|
||
(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•18 years ago
|
||
Just an update on my last reply: after I put #define PNG_NO_MMX_CODE directly in mozpngconf.h it now works.
Assignee | ||
Comment 7•18 years ago
|
||
Comment on attachment 240780 [details] [diff] [review] patch, v2 Cancelling requests for review since Sylvain says it doesn't work.
Attachment #240780 -
Flags: superreview?(tor)
Attachment #240780 -
Flags: review?(pavlov)
Comment 8•18 years ago
|
||
(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 daedalus144@yahoo.com instead ;-)
Assignee | ||
Comment 9•18 years ago
|
||
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•18 years ago
|
||
Oops! I forgot to run autoconf. The patch is working fine now.
Assignee | ||
Updated•18 years ago
|
Attachment #240780 -
Flags: review?(pavlov)
Updated•18 years ago
|
Attachment #240780 -
Flags: review?(pavlov) → review+
Assignee | ||
Comment 11•18 years ago
|
||
Comment on attachment 240780 [details] [diff] [review] patch, v2 tor: sr?
Attachment #240780 -
Flags: superreview?(tor)
Attachment #240780 -
Flags: superreview?(tor) → superreview+
I checked this in because my builds break without it.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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...)
(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...)
Assignee | ||
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•