731597 - lzma code does not build cleanly via xplat on 32-bit Mac OS X, nor on linux

Status: RESOLVED FIXED

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

After checking in the patches for Bug 729336, I am seeing a host of build errors.

On Mac:

../other-licenses/lzma/CpuArch.c: In function ‘MyCPUID’:
../other-licenses/lzma/CpuArch.c:75: error: can't find a register in class ‘BREG’ while reloading ‘asm’
../other-licenses/lzma/CpuArch.c:75: error: ‘asm’ operand has impossible constraints
make: *** [other-licenses/lzma/CpuArch.o] Error 1

On Linux:

g++ -m32 -o shell/avmshell eval/eval-abc.o eval/eval-avmplus.o eval/eval-cogen.o eval/eval-cogen-expr.o eval/eval-cogen-stmt.o eval/eval-compile.o eval/eval-lex.o eval/eval-lex-xml.o eval/eval-parse.o eval/eval-parse-config.o eval/eval-parse-expr.o eval/eval-parse-stmt.o eval/eval-parse-xml.o eval/eval-unicode.o eval/eval-util.o shell/avmshell.o shell/ConsoleOutputStream.o shell/DebugCLI.o shell/DomainClass.o shell/FileClass.o shell/FileInputStream.o shell/ShellCore.o shell/SystemClass.o shell/swf.o shell/../extensions/SamplerScript.o shell/../extensions/Selftest.o shell/../extensions/SelftestInit.o shell/../extensions/SelftestExec.o shell/avmshellUnix.o shell/PosixFile.o shell/PosixPartialPlatform.o -L. -llzma -lzlib -lavmplus -lMMgc -lvmbase   -lzlib -lpthread -m32  
./libavmplus.a(ByteArrayGlue.o): In function `avmplus::ByteArray::LzmaEncoder::Encode()':
ByteArrayGlue.cpp:(.text+0x27f8): undefined reference to `LzmaEncProps_Init'
ByteArrayGlue.cpp:(.text+0x288d): undefined reference to `LzmaDynamicEncode'
./libavmplus.a(ByteArrayGlue.o): In function `avmplus::ByteArray::Uncompress(avmplus::ByteArray::CompressionAlgorithm)':
ByteArrayGlue.cpp:(.text+0x3692): undefined reference to `LzmaUncompress'
./libavmplus.a(ByteArrayGlue.o): In function `avmplus::SzFree(void*, void*)':
ByteArrayGlue.cpp:(.text+0xdeb): undefined reference to `MyFree'
./libavmplus.a(ByteArrayGlue.o): In function `avmplus::SzAlloc(void*, unsigned int)':
ByteArrayGlue.cpp:(.text+0xdfb): undefined reference to `MyAlloc'
collect2: ld returned 1 exit status
make: *** [shell/avmshell] Error 1

Comment 1

[Tamarin Bot]

changeset: 7236:b48bfd3bd34c
user:      Felix S Klock II <[e-mail]>
summary:   Bug 731597: backout lzma support until avmshell build issues are resolved (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/b48bfd3bd34c

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A: fix manifest.mk to get mac working

This version of manifest.mk is essentially the one that msong meant to include.

Unlike the manifest.mk that I hacked up this morning, this version does not attempt to compile the cpu-detection code (CpuArch.c).  It also leaves out BraIA64.c; it is not yet clear to me whether that is better or worse than where we were before.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(the fix for Linux is in a different place: one needs to massage the linker options, namely the AVMSHELL_LDFLAGS, to get things working on Linux, and that in turn will require some configure.py massaging.  I'll take care of it later today/tonight.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B: fix configure.py to get linux working

Need to include -llzma in linker options *after* -lavmplus in the command line, the same way that -lzlib appears after it, so that the linker knows to include the symbols that are referenced by libavmplus.a

I don't know if it is also necessary for -llzma to also appear before -lavmplus, the same way that -lzlib does.  But you definitely need it after, or else the linker fails.  Yay C/C++.

See also:

http://stackoverflow.com/questions/3363398/g-linking-order-dependency-when-linking-c-code-to-c-code

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Both patch A (attachment 601659 [details] [diff] [review]) and patch B (attachment 601797 [details] [diff] [review]) are meant to be applied on top of:

  http://hg.mozilla.org/tamarin-redux/rev/0797d4509e5c

which is where I "finished" checking in LZMA and things started failing.

If you use the repo's subsequent to that one, you'll get my follow-on backout patch (see comment 1), and the patches won't be meaningful in contexts where lzma has been backed out.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 601659 [details] [diff] [review]
patch A: fix manifest.mk to get mac working

For reference, compare against the manifest.mk in shelved CL 1037830

Including msong for feedback, though I suspect he'll implicitly approve; its exactly like his version except that it has 2012 for the date and I've left out the HAVE_VSNPRINTF stuff [*].

[*] See also Bug 506954 for some discussion of the VSNPRINTF stuff, though probably not enough to really grok what's up unless you were there at the time (though hey, Brent clearly was, so maybe its still a worthwhile reference).

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 601797 [details] [diff] [review]
patch B: fix configure.py to get linux working

Get linux building.

This is a tiny bit more invasive (arguably I could have just pasted a concatenation of '$(call EXPAND_LIBNAME,lzma)' into the two definitions of AVMSHELL_LDFLAGS and called it a day).  But I couldn't stomach the asymmetry of that approach.

(On the other hand, its essentially unlikely that a configure lzma-lib option could actually be used, since I believe msong changed the library interface in a couple ways to make it more flexible for hooking into ByteArray.  So that may be an argument against taking this approach.)

Anyway, the important thing is that it should address the issue described in comment 4.

Comment 8

[Tamarin Bot]

changeset: 7238:b92e7af662af
user:      Felix S Klock II <[e-mail]>
summary:   Bug 731597: fix manifest.mk to get at least mac builds working again (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/b92e7af662af

Comment 9

[Tamarin Bot]

changeset: 7239:125d12d2367b
user:      Felix S Klock II <[e-mail]>
summary:   Bug 731597: fix configure.py to get linux builds working again (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/125d12d2367b

Comment 10

[Tamarin Bot]

changeset: 7240:da968b754185
user:      Felix S Klock II <[e-mail]>
summary:   Bug 731597: readd lzma support (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/da968b754185

Comment 11

[Tamarin Bot]

changeset: 7241:9023e2bb2ef3
user:      Felix S Klock II <[e-mail]>
summary:   Bug 731597: merge in fixes to lzma support (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/9023e2bb2ef3

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(realized I should wait for positive patch review before closing ticket; reopening.)

Comment 13

[Brent Baker]

Comment on attachment 601659 [details] [diff] [review]
patch A: fix manifest.mk to get mac working

Review of attachment 601659 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, but mainly satisfied by the fact that the build is working again :)

Comment 14

[Brent Baker]

Comment on attachment 601797 [details] [diff] [review]
patch B: fix configure.py to get linux working

Review of attachment 601797 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, but mainly satisfied by the fact that the build is working again :)

Comment 15

[Michael Song]

Comment on attachment 601659 [details] [diff] [review]
patch A: fix manifest.mk to get mac working

it looks good to me.

Comment 16

[Michael Song]

Comment on attachment 601659 [details] [diff] [review]
patch A: fix manifest.mk to get mac working

Review of attachment 601659 [details] [diff] [review]:
-----------------------------------------------------------------

it looks good.

Comment 17

[Michael Song]

Comment on attachment 601797 [details] [diff] [review]
patch B: fix configure.py to get linux working

Review of attachment 601797 [details] [diff] [review]:
-----------------------------------------------------------------

it look good