Bug 30277

Shouldn't bugs like these be resolved in mingw instead?

To my naive eyes it seems way preferrable to fix it once instead of sprinkling a workaround to every project that needs ffs().

MinGW is a minimal implementation that only exposes functions from the Windows APIs, so they wont add it.  As stated in bug #30968, there is already a workaround for VC, and there is a patch, so not sure if this should be marked a duplicate of that.

For the record, it does not seem it is going to be added:

http://old.nabble.com/undefined-reference-to-%60ffs%27-when-using--O0-tt21607092.html#a21612094

*** Bug 30968 has been marked as a duplicate of this bug. ***

MingW provides a minimal environment for compiling with gcc and linking against msvcrt. As ffs() is a POSIX function it is out of scope for MingW and won't be added.

I think the best solution is to allow MingW to use the cairo version of ffs() that is already used when compiling with VC. This would be better for debugging that using the builtin and of course gcc will still use the builtin with -O2.

Here's an untested patch to implement this. Please test.

diff --git a/src/cairo-compiler-private.h b/src/cairo-compiler-private.h
index e25ee1f..4ce3339 100644
--- a/src/cairo-compiler-private.h
+++ b/src/cairo-compiler-private.h
@@ -212,7 +212,9 @@
 #ifdef _MSC_VER
 #undef inline
 #define inline __inline
+#endif
 
+#if defined(_MSC_VER) || defined(__MINGW32__)
 /* Add a definition of ffs */
 #include <intrin.h>
 #pragma intrinsic(_BitScanForward)

(In reply to comment #5)

> Here's an untested patch to implement this. Please test.

This patch doesn't compile because the existing implementation of ffs in cairo is specific to MSVC (the #pragma is an MSVC-ism). The patch in bug 30968 provides the equivalent GCC-ism.

I think the last comment on that bug about avoiding the definition for Cygwin isn't necessary because Cygwin won't define __WIN32__ because it provides a Posix emulation layer instead of presenting the Windows API. I'm also not too familiar with Cygwin however so I'm not 100% sure about that but I did find some mention in the Cygwin documentation that it doesn't define _WIN32 for that reason. I'm not sure if we should be using that macro instead or it might make sense to just check for __MINGW32__ anyway.

The "#if defined(__MINGW32__) && !defined(HAVE_FFS)" with the patch from bug 30968 is probably the best option. Could you attach a complete tested patch.

Inclusion of this special case should not be based on __WIN32__ as this case is compiler specific. This special case should not be enabled for cygwin as cygwin provides a posix emulation environment which includes the ffs function. In fact even though the win32 backend can be compiled and run within cygwin, the version of cairo included with cygwin has the win32 backend disabled. The cygwin maintainers clearly don't like win32 stuff mixed in with their posix emulation.

Created attachment 40203 [details] [review]
cairo-mingw-ffs.patch

Added tested patch.  At least here with gcc-4.5.1, -O2 does not make this patch redundant.

(In reply to comment #8)
> Created an attachment (id=40203) [details]
> cairo-mingw-ffs.patch

Martin, I thought the idea with the defined(HAVE_FFS) check would be that someone would add a check for ffs() in configure.ac? Where would it be defined otherwise? I would personally argue that that's not necessary as it's unlikely that MinGW would ever add an ffs function and even if they do the #define would still work.

(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=40203) [details] [details]
> > cairo-mingw-ffs.patch
> 
> Martin, I thought the idea with the defined(HAVE_FFS) check would be that
> someone would add a check for ffs() in configure.ac? Where would it be defined
> otherwise? I would personally argue that that's not necessary as it's unlikely
> that MinGW would ever add an ffs function and even if they do the #define would
> still work.

Ah, yes, you know what they say about assumption ... Though with it in config.h, it was tested, never verified.  I can stick in a AC_CHECK_FUNCS, but yeah, it seems unlikely that it will be implemented.

Do you need an updated patch, or will you just update attached?

Anyhow, give credit to google or one of the original reporters, because that is how I initially got the workaround as well, just wanted to let you know it works and not keep waiting.

An alternative fix (that removes the usage of ffs on win32) has been pushed to master:

commit f46ba56d5b8c54be5f0379aca204c0ce05d0f58a
Author: Andrea Canciani <ranma42@gmail.com>
Date:   Sat Jun 25 15:35:48 2011 +0200

    Do not open-code freed-pool
    
    Reuse the freed-pool system to reduce allocation pressure of context
    creation/destruction.
    
    As a side effect, this removes the use of ffs() on Win32, cleaning up
    some MSVC-specific code and fixing a mingw-related build issue.
    
    Fixes https://bugs.freedesktop.org/show_bug.cgi?id=30277