|Summary:||fontconfig rebuilds caches when alternating between 32-bit/64-bit processes|
|Product:||fontconfig||Reporter:||Glen Low <glen.low>|
|Component:||fc-cache||Assignee:||Keith Packard <keithp>|
|Status:||RESOLVED FIXED||QA Contact:||Behdad Esfahbod <freedesktop>|
|Priority:||medium||CC:||akira, freedesktop, jeremyhu|
|OS:||Mac OS X (All)|
|i915 platform:||i915 features:|
Test file (testfontconfig.c) and patch to fix (fontconfig.patch).
Patch to fix bug
Compile this to illustrate issue
Description Glen Low 2009-02-18 22:49:52 UTC
SUMMARY When the fontconfig dylib is used from a 32-bit process and then used from a 64-bit process (or vice versa), there is a significant delay as it attempts to rebuild the shared font caches. STEPS TO REPRODUCE 1. Remove any existing user font cache (you may also need to clean the system caches): rm -rf ~/.fontconfig 2. Compile two versions of the enclosed testfontconfig.c as follows: gcc -I/usr/X11/include -L/usr/X11/lib -lfontconfig -o testfontconfig32 -arch i386 testfontconfig.c gcc -I/usr/X11/include -L/usr/X11/lib -lfontconfig -o testfontconfig64 -arch x86_64 testfontconfig.c 3. Run the programs ./testfontconfig32 ./testfontconfig64 ./testfontconfig32 RESULTS Each run causes a significant delay. Inspection of ~/.fontconfig shows the caches have been rewritten on each run. EXPECTED RESULTS The third run should not result in any delay. REGRESSION Running the same architecture twice in a row doesn't result in a delay in-between e.g. ./testfontconfig32 ./testfontconfig32 NOTE The fontconfig as of version 2.6 detects the architecture suffix for cache files at build time and then hard-codes it into the executable. Therefore a Universal Binary build of fontconfig will have the same hard-coded architecture suffix for all architectures, and end up writing to the same set of cache files as well. Alternating between different architectural runs, fontconfig thinks the cache files written by the previous architecture are corrupt and then rewrites them. I've provided a patch (fontconfig.patch) which causes the architecture suffix for cache files to vary according to the currently building architecture. The cache files should then be distinct for the different architectures in a Universal Binary. I've also verified this fix builds and works. This bug is related to https://savannah.nongnu.org/bugs/index.php?23204, however while the freetype2 crash has been fixed as of OS X 10.5.6, the fontconfig cache file bug masked by the crash has not been fixed. I've also submitted to Apple via bug 6602251.
Comment 1 Glen Low 2009-02-18 22:51:19 UTC
Created attachment 23101 [details] [review] Test file (testfontconfig.c) and patch to fix (fontconfig.patch).
Comment 2 Behdad Esfahbod 2009-03-02 02:59:14 UTC
Please attach the test file and the patch as separate, text, files.
Comment 3 Glen Low 2009-03-02 14:47:58 UTC
Created attachment 23455 [details] [review] Patch to fix bug
Comment 4 Glen Low 2009-03-02 14:48:59 UTC
Created attachment 23456 [details] Compile this to illustrate issue
Comment 5 Glen Low 2009-03-02 14:49:49 UTC
(In reply to comment #2) > Please attach the test file and the patch as separate, text, files. > Done.
Comment 6 Behdad Esfahbod 2009-03-06 11:49:06 UTC
I'm not sure how that fixes the problem for you as you still end up with one architecture for the entire fat binary...
Comment 7 Glen Low 2009-03-06 14:44:34 UTC
fontconfig.patch patches the fc-arch.c file so that the fc-arch program produces various #ifdef's for the different architectures. Then the rest of the fontconfig build consumes the output of fc-arch -- when gcc is passed different architectural flags through configure i.e. CFLAGS="-arch ppc -arch ppc64 -arch i386 -arch x86_64", it then transparently compiles the 4 architectures in 4 distinct passes then glues them together. Hope that explains it clear enough. The configure needs to be passed the CFLAGS in order to work, but that is the typical way most packages are built as universal binaries.
Comment 8 Akira TAGOH 2012-03-13 04:33:41 UTC
Does this issue still persist in 2.9.0?
Comment 9 Jeremy Huddleston Sequoia 2012-03-18 21:42:23 UTC
This is not resolved by d1a0fca316ab8d9d61474028da54615e4d9f7540 because it checks values from configure checks which are based only on a single architecture. You can not rely on sizeof() checks or endian checks from configure as they do not apply to all archs. +#if defined(WORDS_BIGENDIAN) && WORDS_BIGENDIAN +# define FC_ARCH_ENDIAN "be" +#else /* !WORDS_BIGENDIAN */ +# define FC_ARCH_ENDIAN "le" +#endif + +#if SIZEOF_VOID_P == 4 +# if ALIGNOF_DOUBLE == 4 +# define FC_ARCH_SIZE_ALIGN "32d4" +# else /* ALIGNOF_DOUBLE != 4 */ +# define FC_ARCH_SIZE_ALIGN "32d8" +# endif +#else /* SIZEOF_VOID_P != 4 */ +# define FC_ARCH_SIZE_ALIGN "64" +#endif + +#ifdef ARCHITECTURE +# define ARCHITECTURE FC_ARCHITECTURE +#else +# define FC_ARCHITECTURE FC_ARCH_ENDIAN FC_ARCH_SIZE_ALIGN +#endif
Comment 10 Jeremy Huddleston Sequoia 2012-03-18 21:48:51 UTC
Also, it looks like additional changes went in such that: /* config.h might override this */ #ifndef FC_ARCHITECTURE # define FC_ARCHITECTURE FC_ARCH_ENDIAN FC_ARCH_SIZE_ALIGN #endif So you determine the architecture by configure, so both i386 and x86_64 slices think they have the same signature.
Comment 11 Jeremy Huddleston Sequoia 2012-03-18 22:40:47 UTC
I'm working on a patch for you...
Comment 12 Jeremy Huddleston Sequoia 2012-03-18 23:29:42 UTC
Created attachment 58651 [details] [review] 0001-fcarch-Restrict-FC_ARCHITECTURE-usage-to-fcarch.h.patch
Comment 13 Jeremy Huddleston Sequoia 2012-03-18 23:30:11 UTC
Created attachment 58652 [details] [review] 0002-fcarch-Check-for-architecture-type-at-runtime-rather.patch
Comment 14 Akira TAGOH 2012-03-19 01:20:59 UTC
(In reply to comment #9) > This is not resolved by d1a0fca316ab8d9d61474028da54615e4d9f7540 because it > checks values from configure checks which are based only on a single > architecture. You can not rely on sizeof() checks or endian checks from > configure as they do not apply to all archs. Since we are using autotools as part of the toolchain, I'd prefer fixing autotools rather than patching something out in fontconfig if its result isn't worth relying. (In reply to comment #10) > So you determine the architecture by configure, so both i386 and x86_64 slices > think they have the same signature. I can see different signature on both i386 and x86_64 on Linux at least though. I'm not familiar with Mac OS X and don't have any box for testing, so can you give me some examples how current code actually behaves on them and how looks like?
Comment 15 Jeremy Huddleston Sequoia 2012-03-19 01:43:18 UTC
(In reply to comment #14) > (In reply to comment #9) > > This is not resolved by d1a0fca316ab8d9d61474028da54615e4d9f7540 because it > > checks values from configure checks which are based only on a single > > architecture. You can not rely on sizeof() checks or endian checks from > > configure as they do not apply to all archs. > > Since we are using autotools as part of the toolchain, I'd prefer fixing > autotools rather than patching something out in fontconfig if its result isn't > worth relying. You can't. This is a defect in autoconf *by design* It sets SIZEOF_VOID_P in config.h and there is no way that a single value will work for all architectures. > (In reply to comment #10) > > So you determine the architecture by configure, so both i386 and x86_64 slices > > think they have the same signature. > > I can see different signature on both i386 and x86_64 on Linux at least though. Linux does not support fat binaries, so that's not a good example. Your linux example would be akin to me building an i386 fcarch and a separate x86_64 fcarch. They'd both work. If you try to build a single fat binary, it won't > I'm not familiar with Mac OS X and don't have any box for testing, so can you > give me some examples how current code actually behaves on them and how looks > like? current code doesn't even build fat because it fails the FC_ASSERT_STATIC: FC_ASSERT_STATIC (SIZEOF_VOID_P == sizeof (intptr_t)); For i386 sizeof(void *) is 4, for x86_64, sizeof(void *) is 8. configure sets SIZEOF_VOID_P to either 8 or 4 depending on if the build machine is 32bit or 64bit, and so the assertion will fail at build time for the other architecture. This is if we're lucky and building for differnet bit levels. If you try to build for i386 and ppc at the same time, you'll get both as either little endian or both as big endian.
Comment 16 Jeremy Huddleston Sequoia 2012-03-19 01:55:30 UTC
(In reply to comment #15) > This is if we're lucky and building for differnet bit levels. If you try to > build for i386 and ppc at the same time, you'll get both as either little > endian or both as big endian. Actually, if you have a new enough autoconf, the endianness issue is worked around by autoconf, but SIZEOF_VOID_P and ALIGNOF_DOUBLE are still just based on a single architecture. It's not safe to make such checks at configure time (which is why I changed them to compile-time checks in the second patch). The changes I've provide do more than just support this particular case. They help to better future-proof the code.
Comment 17 Jeremy Huddleston Sequoia 2012-03-19 02:09:07 UTC
If you want a patch that preserves FC_ARCHITECTURE as a string macro, I can do that instead, but it won't be future proof. That future-proofing is why I went with this approach.
Comment 18 Jeremy Huddleston Sequoia 2012-03-19 02:10:54 UTC
Created attachment 58663 [details] [review] 0002-fcarch-Check-for-architecture-type-at-runtime-rather.patch
Comment 19 Jeremy Huddleston Sequoia 2012-03-19 02:29:39 UTC
Created attachment 58664 [details] [review] 0002-fcarch-Check-for-architecture-type-at-runtime-rather.patch
Comment 20 Jeremy Huddleston Sequoia 2012-03-19 02:47:05 UTC
Created attachment 58665 [details] [review] 0001-fcarch-Check-for-architecture-signature-at-compile-t.patch This one is more hacky and more fragile since we're still relying on the preprocessor for checks and can't use sizeof() or offsetof(), but it has less code change... I prefer the previous series for better flexibility going forward.
Comment 21 Akira TAGOH 2012-03-19 03:00:32 UTC
(In reply to comment #7) > Then the rest of the fontconfig build consumes the output of fc-arch -- when > gcc is passed different architectural flags through configure i.e. > CFLAGS="-arch ppc -arch ppc64 -arch i386 -arch x86_64", it then transparently > compiles the 4 architectures in 4 distinct passes then glues them together. Ah, got it. that clearly explains why you can't rerun configure script with the certain options to update the architecture information. I see why it needs to be addressed at the build time, really.
Comment 22 Behdad Esfahbod 2012-03-20 09:18:47 UTC
Patch looks fine.
Comment 23 Akira TAGOH 2012-03-20 19:32:09 UTC
to keep the backward compatibility (and not to keep the garbage cache files in the cache dir), I prefer the last one though, we could revisit this when the cache version is being bumped, which might be coming in not-so-distant future.
Comment 24 Akira TAGOH 2012-03-20 20:00:22 UTC
Fixed in 8cc44981.
Comment 25 Behdad Esfahbod 2012-03-21 09:50:33 UTC
Thinking about this again, since most of the trick is to fixup autoconf macros, I think we should move that to the bottom of config.h, using AH_BOTTOM. What do people think?
Comment 26 Jeremy Huddleston Sequoia 2012-03-21 13:01:10 UTC
I seem to remember AH_BOTTOM doesn't actually work for this case, but I'm willing to try a patch.
Comment 27 Akira TAGOH 2012-03-21 19:25:07 UTC
Hmm, right. it seems not working properly. even though config.h.in has the correct macros as it's written in configure.in, it's badly replaced when config.h is generated. see: #ifdef __APPLE__ # include <machine/endian.h> # define SIZEOF_VOID_P 8 # define ALIGNOF_DOUBLE 8 # ifdef __LP64__ # define SIZEOF_VOID_P 8 # define ALIGNOF_DOUBLE 8 # else # define SIZEOF_VOID_P 8 # define ALIGNOF_DOUBLE 8 # endif #endif
Comment 28 Behdad Esfahbod 2012-03-22 07:21:32 UTC
(In reply to comment #27) > Hmm, right. it seems not working properly. even though config.h.in has the > correct macros as it's written in configure.in, it's badly replaced when > config.h is generated. see: > > #ifdef __APPLE__ > # include <machine/endian.h> > # define SIZEOF_VOID_P 8 > # define ALIGNOF_DOUBLE 8 > # ifdef __LP64__ > # define SIZEOF_VOID_P 8 > # define ALIGNOF_DOUBLE 8 > # else > # define SIZEOF_VOID_P 8 > # define ALIGNOF_DOUBLE 8 > # endif > #endif Autoconf manual suggests that we should put the custom code into a header and include it in AH_BOTTOM. I feel more confident that way. Cause, as it is right now, those macros are not reliable unless fcarch.h has been included...
Comment 29 Akira TAGOH 2012-03-27 23:13:32 UTC
Okay, proposed fix: http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz20208 I'm not quite sure if it's really likely but this can be easily dropped once it's improved in autoconf.
Comment 30 Jeremy Huddleston Sequoia 2012-03-28 09:20:00 UTC
Yeah, that looks fine. We can then easily add additional "fixups" to that as necessary.
Comment 31 Akira TAGOH 2012-03-28 20:59:36 UTC
Okay, merged into master. thanks!