3.55 MB, patch
|Details | Diff | Splinter Review|
1.96 KB, patch
|Details | Diff | Splinter Review|
2.40 KB, patch
|Details | Diff | Splinter Review|
Created attachment 680397 [details] 3.0.11 changelog Mozilla is currently running on a snapshot of libffi from 08/2010. Version 3.0.11 was released in 04/2012. I don't know who would own this at this point, but I figured this should probably be on file anyway. Attached is the changelog from Mozilla's snapshot to the release of 3.0.11.
I'm the effective owner of js-ctypes at this point, but I don't see any pressing reason to upgrade. It works fine for the limited ways in which we use it, so the risk-reward ratio of upgrading doesn't seem that great.
Though it's not urgent for Mozilla, will we extract some fix for libffi from the latest source code? Such as https://github.com/atgreen/libffi/commit/ee18766b169811426c14b011fbb46d81e344f926 Fix for a crasher due to misaligned stack on x86-32. I don't know whether the crash will happen in Fx.
I have a feeling we probably would have seen the crashes, but it might be worthwhile to do at some point.
On the bright side, it appears that most of the in-tree patches we have are no longer needed. 01-bug-670719 - upstreamed 02-bug-682180 - upstreamed 03-bug-712594 - upstreamed 04-bug-756740 - still needed (upstream?) 05-bug-644136 - upstreamed (with some changes) 06-bug-778414 - still needed (upstream?) glandium/gaston, did we ever try to upstream the patches from bug 756740 and bug 778414, respectively? And of course, none of the above means there won't be all new patches in need of making :)
(In reply to Ryan VanderMeulen [:RyanVM][UTC-4] from comment #5) > On the bright side, it appears that most of the in-tree patches we have are > no longer needed. > > 01-bug-670719 - upstreamed > 02-bug-682180 - upstreamed > 03-bug-712594 - upstreamed > 04-bug-756740 - still needed (upstream?) > 05-bug-644136 - upstreamed (with some changes) > 06-bug-778414 - still needed (upstream?) > > glandium/gaston, did we ever try to upstream the patches from bug 756740 and > bug 778414, respectively? For the latter, i've just reported it in https://github.com/atgreen/libffi/issues/42
See Also: → https://github.com/atgreen/libffi/issues/42
I've got a set of patches for libffi 3.0.13 that seems to work reasonably well. However, I'm hitting linking errors on Windows. https://tbpl.mozilla.org/?tree=Try&rev=2fd73302a413&showall=1 https://tbpl.mozilla.org/php/getParsedLog.php?id=26851193&tree=Try Any ideas, Mike? AFAICT, those symbols aren't new to this release, so I don't know why MSVC doesn't like them suddenly.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #7) > I've got a set of patches for libffi 3.0.13 that seems to work reasonably > well. However, I'm hitting linking errors on Windows. > https://tbpl.mozilla.org/?tree=Try&rev=2fd73302a413&showall=1 > https://tbpl.mozilla.org/php/getParsedLog.php?id=26851193&tree=Try > > Any ideas, Mike? AFAICT, those symbols aren't new to this release, so I > don't know why MSVC doesn't like them suddenly. Maybe the similar value such as ffi_type_pointer is defined as __declspec(dllexport)
This is the relevant diff between what we have in the tree and 3.0.13. +/* Need minimal decorations for DLLs to works on Windows. */ +/* GCC has autoimport and autoexport. Rely on Libtool to */ +/* help MSVC export from a DLL, but always declare data */ +/* to be imported for MSVC clients. This costs an extra */ +/* indirection for MSVC clients using the static version */ +/* of the library, but don't worry about that. Besides, */ +/* as a workaround, they can define FFI_BUILDING if they */ +/* *know* they are going to link with the static library. */ +#if defined _MSC_VER && !defined FFI_BUILDING +#define FFI_EXTERN extern __declspec(dllimport) +#else +#define FFI_EXTERN extern +#endif + /* These are defined in types.c */ -extern ffi_type ffi_type_void; -extern ffi_type ffi_type_uint8; -extern ffi_type ffi_type_sint8; -extern ffi_type ffi_type_uint16; -extern ffi_type ffi_type_sint16; -extern ffi_type ffi_type_uint32; -extern ffi_type ffi_type_sint32; -extern ffi_type ffi_type_uint64; -extern ffi_type ffi_type_sint64; -extern ffi_type ffi_type_float; -extern ffi_type ffi_type_double; -extern ffi_type ffi_type_pointer; +FFI_EXTERN ffi_type ffi_type_void; +FFI_EXTERN ffi_type ffi_type_uint8; +FFI_EXTERN ffi_type ffi_type_sint8; +FFI_EXTERN ffi_type ffi_type_uint16; +FFI_EXTERN ffi_type ffi_type_sint16; +FFI_EXTERN ffi_type ffi_type_uint32; +FFI_EXTERN ffi_type ffi_type_sint32; +FFI_EXTERN ffi_type ffi_type_uint64; +FFI_EXTERN ffi_type ffi_type_sint64; +FFI_EXTERN ffi_type ffi_type_float; +FFI_EXTERN ffi_type ffi_type_double; +FFI_EXTERN ffi_type ffi_type_pointer; In other words, ffi kind of hardcodes being a shared library. Which is not the case for us when using the in-tree copy. As stated in the comment, we just need to define FFI_BUILDING when including the ffi header. Although that's a bit broad, you can add the following to js/src/Makefile.in and that should work: ifndef MOZ_NATIVE_FFI DEFINES += -DFFI_BUILDING endif
Well, it builds now. Unfortunately, Windows still hates it. https://tbpl.mozilla.org/?tree=Try&rev=79942e93f540 https://tbpl.mozilla.org/php/getParsedLog.php?id=26872490&tree=Try 08:01:06 WARNING - PROCESS-CRASH | C:\slave\test\build\tests\xpcshell\tests\toolkit\components\ctypes\tests\unit\test_jsctypes.js | application crashed [@ 0x1ba0048] 08:01:06 INFO - Crash dump filename: c:\docume~1\cltbld~1.t-x\locals~1\temp\tmpao1gnj\11633183-7f06-4dff-a7fe-1662b3106de8.dmp 08:01:06 INFO - Operating system: Windows NT 08:01:06 INFO - 5.1.2600 Service Pack 3 08:01:06 INFO - CPU: x86 08:01:06 INFO - GenuineIntel family 6 model 30 stepping 5 08:01:06 INFO - 8 CPUs 08:01:06 INFO - Crash reason: EXCEPTION_ILLEGAL_INSTRUCTION 08:01:06 INFO - Crash address: 0x1ba0048 08:01:06 INFO - Thread 0 (crashed) 08:01:06 INFO - 0 0x1ba0048 08:01:06 INFO - eip = 0x01ba0048 esp = 0x0012f380 ebp = 0x02b90200 ebx = 0x02dd7480 08:01:06 INFO - esi = 0x060bccc4 edi = 0x78ab07b5 eax = 0xffffffe3 ecx = 0x0012f344 08:01:06 INFO - edx = 0x01da5000 efl = 0x00010246 08:01:06 INFO - Found by: given as instruction pointer in context 08:01:06 INFO - 1 mozjs.dll!ffi_call + 0xa0 08:01:06 INFO - eip = 0x0065d8b1 esp = 0x0012f38c ebp = 0x02b90200 08:01:06 INFO - Found by: stack scanning 08:01:06 INFO - 2 mozjs.dll + 0x24d30f 08:01:06 INFO - eip = 0x0065d310 esp = 0x0012f390 ebp = 0x02b90200 08:01:06 INFO - Found by: stack scanning 08:01:06 INFO - 3 jsctypes-test.dll + 0x1747 08:01:06 INFO - eip = 0x02b81748 esp = 0x0012f3a8 ebp = 0x02b90200 08:01:06 INFO - Found by: stack scanning 08:01:06 INFO - 4 mozjs.dll!js::ctypes::AutoValue::SizeToType(JSContext *,JSObject *) [CTypes.cpp:79942e93f540 : 5275 + 0x8] 08:01:06 INFO - eip = 0x0064db21 esp = 0x0012f3bc ebp = 0x02b90200 08:01:06 INFO - Found by: stack scanning 08:01:06 INFO - 5 mozjs.dll!js::ctypes::FunctionType::Call [CTypes.cpp:79942e93f540 : 5827 + 0x1d] 08:01:06 INFO - eip = 0x00658c24 esp = 0x0012f3d8 ebp = 0x0012f4e4 08:01:06 INFO - Found by: stack scanning 08:01:06 INFO - 6 mozjs.dll!js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) [Interpreter.cpp:79942e93f540 : 482 + 0x29] 08:01:06 INFO - eip = 0x00453810 esp = 0x0012f4ec ebp = 0x0012f668 08:01:06 INFO - Found by: call frame info 08:01:06 INFO - 7 mozjs.dll!js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value *,JS::MutableHandle<JS::Value>) [Interpreter.cpp:79942e93f540 : 539 + 0xc] 08:01:06 INFO - eip = 0x00453aa3 esp = 0x0012f670 ebp = 0x0012f708 08:01:06 INFO - Found by: call frame info 08:01:06 INFO - 8 mozjs.dll!js::DirectProxyHandler::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [jsproxy.cpp:79942e93f540 : 445 + 0x22] 08:01:06 INFO - eip = 0x00534583 esp = 0x0012f710 ebp = 0x0012f734 08:01:06 INFO - Found by: call frame info
AArch64 (64-bit ARM) needs update as well. Port was added in https://github.com/atgreen/libffi/commit/58e8b66f70cef2e3c9b0e5a707b45d634cbbf5d9 commit. https://github.com/atgreen/libffi/commit/f64e4a865557e440774436b4c2b2fd7374290e97 has testsuite update.
(In reply to Marcin Juszkiewicz from comment #11) > AArch64 (64-bit ARM) needs update as well. Any idea when they're planning an official release with it included? Would be nice if some of the issues/PRs would be resolved too. /me notes that the issue filed in comment 6 is still open.
No idea about their release plans. Little Endian PowerPC guys requested new release in December but there was no reply.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12) > (In reply to Marcin Juszkiewicz from comment #11) > > AArch64 (64-bit ARM) needs update as well. > > Any idea when they're planning an official release with it included? Would > be nice if some of the issues/PRs would be resolved too. /me notes that the > issue filed in comment 6 is still open. Yeah, upstream is not really responsive on github..
Is there a chance for update anyway? I now have to keep 55KB patch just for libffi update.
I'm happy to run your patch through Try if you want. I never made any progress after comment 10, FWIW. And obviously we can't land anything unless it works on all platforms :)
I rebased my WIP patches and ran them through Try again. Everything looks good, except the Windows crashes persist :(. I need someone who can better understand that crash stack to help point in the right direction of what's going on here. https://tbpl.mozilla.org/php/getParsedLog.php?id=35019532&tree=Try
Here's the Try push, which will be available to look at for the next 30 days. https://tbpl.mozilla.org/?tree=Try&rev=e37790ec0d48 Another crash stack too. Comment 17 looks basically the same as comment 10. https://tbpl.mozilla.org/php/getParsedLog.php?id=35019240&tree=Try
Just for kicks, I updated to libffi tip from the upstream Git repo. I see a win32 alignment fix in the post-3.0.13 changelog, so why not. https://tbpl.mozilla.org/?tree=Try&rev=4c71f78e7af1
Cool, libffi tip does appear to have fixed the Windows ctypes test crashes we were seeing with 3.0.13 :) However, we've still got the WinXP/Win8 (but *NOT* Win7 ?!?!) debug test failures. Interestingly, we also hit a similar-looking failure on one Win8 opt Cpp unit test run (in TestStartupCache) that went away on subsequent retriggers. AFAICT, the tests are crashing/hanging on startup. Unfortunately, we're not getting a usable stack trace when they do :(
The error codes from the logs convert to hex code 0xC0000135, which is STATUS_DLL_NOT_FOUND. I'm wondering if this is related to the linking issues we were hitting in comment 9.
When I try to run the build in question, I get "The program can't start because MSVCR100D.dll is missing from your computer. Try reinstalling the program to fix this problem."
I took a look at the try build logs. with 61a0549, msvcc.sh is invoked with -DFFI_BUILDING and -DFFI_DEBUG. with 8bad679, mvscc.sh is invoked with -fexceptions instead of the two above. translated in cl calls: with 61a0549, cl is given -DFFI_BUILDING, -RTC1, and not with 8bad679. Was your last try push with 8bad679 with --enable-debug disabled for ffi?
ah, i forgot the main difference: with 61a0549, cl is given -MDd, with 8bad679, -MD
> Was your last try push with 8bad679 with --enable-debug disabled for ffi? It was. So it would seem that --enable-debug is not working as expected...
Created attachment 8383795 [details] [diff] [review] Relevant parts of 61a0549->8bad679 diff Here's what appear to be the relevant changes between revisions 61a0549 and 8bad679.
Of course, the current code is also very different from rev 8bad679. On the Try push from comment 19, this is the output: sh.exe ./libtool --tag=CC --mode=compile c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/msvcc.sh -DHAVE_CONFIG_H -I. -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi -I. -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/include -Iinclude -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/src -O3 -warn all -c -o src/x86/ffi.lo c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/src/x86/ffi.c
The Windows linker issues were resolved upstream. The fix to re-add code that had been accidentally removed. Try confirms that Windows debug is fixed now :) https://tbpl.mozilla.org/?tree=Try&rev=debd3b591edb I'm in touch with Anthony (the upstream maintainer) and will try to get some of the remaining in-tree patches upstreamed and then try to get this landed soon after.
Created attachment 8384586 [details] config.status from broken build More fun, I attempted to build this locally and ran into mozmake-specific bustage. 1:26.96 config.status: linking c:/mozbuild/src/mozilla-central/js/src/ctypes/libffi/src/x86/ffitarget.h to include/ffitarget.h 1:27.03 config.status: executing buildir commands 1:27.03 config.status: skipping top_srcdir/Makefile - not created 1:27.06 config.status: executing depfiles commands 1:29.03 config.status: executing libtool commands 1:29.30 config.status: executing include commands 1:29.33 config.status: executing src commands 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- a 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- c 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- c 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- c 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- c 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- J 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- 6 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- 4 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- 1:29.41 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- y 1:29.42 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- y 1:29.42 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- a 1:29.42 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- 1:29.42 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- A 1:29.42 C:\mozbuild\msys\bin\mozmake.EXE: invalid option -- N 1:29.42 Usage: mozmake.EXE [options] [target] ... 1:29.42 Options: 1:29.42 -b, -m Ignored for compatibility. 1:29.42 -B, --always-make Unconditionally make all targets. 1:29.42 -C DIRECTORY, --directory=DIRECTORY 1:29.42 Change to DIRECTORY before doing anything. 1:29.42 -d Print lots of debugging information. 1:29.42 --debug[=FLAGS] Print various types of debugging information. 1:29.42 -e, --environment-overrides 1:29.42 Environment variables override makefiles. 1:29.42 --eval=STRING Evaluate STRING as a makefile statement. 1:29.42 -f FILE, --file=FILE, --makefile=FILE 1:29.42 Read FILE as a makefile. 1:29.42 -h, --help Print this message and exit. 1:29.42 -i, --ignore-errors Ignore errors from recipes. 1:29.42 -I DIRECTORY, --include-dir=DIRECTORY 1:29.42 Search DIRECTORY for included makefiles. 1:29.42 -j [N], --jobs[=N] Allow N jobs at once; infinite jobs with no arg. 1:29.42 -k, --keep-going Keep going when some targets can't be made. 1:29.42 -l [N], --load-average[=N], --max-load[=N] 1:29.42 Don't start multiple jobs unless load is below N. 1:29.42 -L, --check-symlink-times Use the latest mtime between symlinks and target. 1:29.42 -n, --just-print, --dry-run, --recon 1:29.42 Don't actually run any recipe; just print them. 1:29.42 -o FILE, --old-file=FILE, --assume-old=FILE 1:29.42 Consider FILE to be very old and don't remake it. 1:29.42 -O[TYPE], --output-sync[=TYPE] 1:29.42 Synchronize output of parallel jobs by TYPE. 1:29.43 -p, --print-data-base Print make's internal database. 1:29.43 -q, --question Run no recipe; exit status says if up to date. 1:29.43 -r, --no-builtin-rules Disable the built-in implicit rules. 1:29.43 -R, --no-builtin-variables Disable the built-in variable settings. 1:29.43 -s, --silent, --quiet Don't echo recipes. 1:29.43 -S, --no-keep-going, --stop 1:29.43 Turns off -k. 1:29.43 -t, --touch Touch targets instead of remaking them. 1:29.43 --trace Print tracing information. 1:29.43 -v, --version Print the version number of make and exit. 1:29.43 -w, --print-directory Print the current directory. 1:29.43 --no-print-directory Turn off -w, even if it was turned on implicitly. 1:29.43 -W FILE, --what-if=FILE, --new-file=FILE, --assume-new=FILE 1:29.43 Consider FILE to be infinitely new. 1:29.43 --warn-undefined-variables Warn when an undefined variable is referenced. 1:29.43 1:29.43 This program built for Windows32 1:29.43 Report bugs to <email@example.com> 1:29.43 Makefile:716: recipe for target 'all' failed 1:29.43 mozmake.EXE: *** [all] Error 2 Attached is the config.status from the failed run.
Attachment #8383795 - Attachment is obsolete: true
Try run of version 3.1rc1 (build system defaults). https://tbpl.mozilla.org/?tree=Try&rev=95c3cd1b2bee And Windows w/ mozmake: https://tbpl.mozilla.org/?tree=Try&rev=dc3c81358978
Created attachment 8421469 [details] [diff] [review] Update to libffi 3.1 libffi 3.1 is out. This is just the straight-up replacement of the in-tree sources with the upstream. There will be more patches coming for the in-tree modifications and build system changes needed to actually get it to build (not as bad as you'd think, I swear!). Try run: https://tbpl.mozilla.org/?tree=Try&rev=a92f6cb9ad10 And FWIW, I've been running with various upstream git revs in my local builds for months now with no ill effects, so I'm feeling good about this :)
Created attachment 8421480 [details] [diff] [review] Modifications to upstream libffi for the Mozilla build These days, we only need a single one-liner to get libffi to build \m/. However, this does come with one big caveat - this removes some past hackarounds needed for pymake compatibility. This is generally well and good these days since pymake is deprecated on trunk in favor of mozmake/gmake 4.0 anyway (and mozmake is included with MozillaBuild 1.9+). However, AFAIK, pymake is still required to build comm-central, so this patch would break Thunderbird and Seamonkey if it lands as-is. I really hoped that jcranmer's big build system refactoring would land before libffi 3.1 was released, but that hasn't happened. So we can either sit on this patch until it does or I can re-add the pymake hackarounds. Thoughts? :)
Attachment #8421480 - Attachment is patch: true
Created attachment 8421481 [details] [diff] [review] Mozilla build system changes for libffi 3.1 A couple changes are needed on the Mozilla side of things for it to build.
Attachment #8421481 - Flags: review?(mh+mozilla)
My plan at this point is to first figure out who should officially do the review on the upstream update (I thought I heard the other day that jorendorff owns CTypes these days?). And I need a decision on the question in comment 33 about whether to maintain pymake compatibility or not. Once those are taken care of, this will land as a folded patch as there's really no sense in landing them separately anyway.
Considering how quickly things are moving on c-c land, i'd rather work around pymake issues for now.
Attachment #8421480 - Flags: review?(mh+mozilla) → review+
Attachment #8421481 - Flags: review?(mh+mozilla) → review+
Attachment #8421469 - Flags: review?(jorendorff)
Comment on attachment 8421469 [details] [diff] [review] Update to libffi 3.1 Review of attachment 8421469 [details] [diff] [review]: ----------------------------------------------------------------- OK.
Attachment #8421469 - Flags: review?(jorendorff) → review+
BOOM GOES THE DYNAMITE! https://hg.mozilla.org/integration/mozilla-inbound/rev/4c012df41e1b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.