287035 - cairo code for compiling without native int64 types is bitrotted

Status: RESOLVED WONTFIX

(Core Graveyard :: GFX, defect)

Description

[Martin Mokrejs]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050304
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050304

Current cvs gives me on linux:

 gcc -DHAVE_CONFIG_H -I. -I. -I.. -I. -I/scratch/mozilla/dist/include/cairo
-I/usr/include/freetype2 -Wall -Wpointer-arith -Wstrict-prototypes
-Wmissing-prototypes -Wmissing-declarations -Wnested-externs
-fno-strict-aliasing -g3 -O0 -ggdb -MT cairo_hull.lo -MD -MP -MF
.deps/cairo_hull.Tpo -c cairo_hull.c  -fPIC -DPIC -o .libs/cairo_hull.o
cairo_hull.c: In function `_cairo_hull_vertex_compare':
cairo_hull.c:96: error: conversion to non-scalar type requested
cairo_hull.c:97: error: conversion to non-scalar type requested
cairo_hull.c:98: error: conversion to non-scalar type requested
cairo_hull.c:99: error: conversion to non-scalar type requested
cairo_hull.c:100: error: invalid operands to binary <
gmake[3]: *** [cairo_hull.lo] Error 1
gmake[3]: Leaving directory `/scratch/mozilla/gfx/cairo/cairo/src'


Reproducible: Always

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

cool. sounds to me like cairo does not use an intrinsic type for its 64-bit
type, but the structure
(http://lxr.mozilla.org/seamonkey/source/gfx/cairo/cairo/src/cairo-wideint.h#61)

Comment 2

[tor]

What version of gcc are you using, and on what platform (x86, x86-64, etc)?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What that looks like to me is that cairo has ifdefs for when there isn't a
64-bit type, and that those ifdefs aren't used properly in this case.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To test this on any platform, just do:

Index: cairo-wideint.h
...
@@ -57,4 +57,6 @@
 #define I cairo_private

+#undef HAVE_UINT64_T
+#undef HAVE_UINT128_T
 #if !HAVE_UINT64_T

Comment 5

[Martin Mokrejs]

Have resynced to current cvs and reconfigured. I just by chance saw:

configuring in gfx/cairo/cairo
running /bin/sh ./configure  --enable-debug --disable-optimize
--enable-debug-modules=all --enable-debugger-info-modules
--enable-detect-webshell-leaks --enable-svg --enable-svg-renderer-libart
--enable-image-decoders=all --with-qtdir=/usr/qt/3 --enable-application=suite
--disable-freetype2 --enable-jprof --prefix=/scratch/mozilla/dist --disable-ps
--disable-pdf --disable-png --disable-glitz --disable-xcb
--cache-file=../../.././config.cache --srcdir=.
configure: loading cache ../../.././config.cache
checking for a BSD-compatible install... (cached) /bin/install -c
checking whether build environment is sane... yes
checking for gawk... (cached) gawk
checking whether make sets $(MAKE)... (cached) yes
checking whether to enable maintainer-specific portions of Makefiles... no
checking for C compiler default output file name... a.out
checking whether the C compiler works... yes
checking whether we are cross compiling... no
checking for suffix of executables... 
checking for suffix of object files... (cached) o
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ANSI C... (cached) none needed
checking for style of include used by make... GNU
checking dependency style of gcc... (cached) gcc3
checking how to run the C preprocessor... (cached) gcc -E
checking build system type... (cached) i686-pc-linux-gnu
checking host system type... (cached) i686-pc-linux-gnu
checking for a sed that does not truncate output... (cached) 
checking for egrep... (cached) grep -E
checking for ld used by gcc... ./configure: line 3566: s%\\%/%g: No such file or
directory
(cached) /usr/i686-pc-linux-gnu/bin/ld
checking if the linker (/usr/i686-pc-linux-gnu/bin/ld) is GNU ld... (cached) yes
checking for /usr/i686-pc-linux-gnu/bin/ld option to reload object files...
(cached) -r
checking for BSD-compatible nm... (cached) /usr/bin/nm -B
checking whether ln -s works... yes
checking how to recognise dependent libraries... (cached) pass_all
checking for ANSI C header files... (cached) yes

Comment 6

[Martin Mokrejs]

Actually here is the rest of the configure's output, just in case someone would
care about uint64_t and uint128_t. I use Gentoo linux on i686 (intel).

$ gcc --version
gcc (GCC) 3.4.3-20050110 (Gentoo Linux 3.4.3.20050110, ssp-3.4.3.20050110-0,
pie-8.7.7)
[cut]

checking for pkg-config... (cached) /usr/bin/pkg-config
checking for xrender >= 0.6... yes
checking XRENDER_CFLAGS...  
checking XRENDER_LIBS... -lXrender -lX11 -lXext  
checking for Carbon/Carbon.h... (cached) no
checking for some Win32 platform... no
checking for libpixman >= 0.1.4... yes
checking PIXMAN_CFLAGS... -I/scratch/mozilla/dist/include/cairo  
checking PIXMAN_LIBS... -L/scratch/mozilla/dist/lib -lpixman  
checking for fontconfig... yes
checking FONTCONFIG_CFLAGS...  
checking FONTCONFIG_LIBS... -lfontconfig  
checking for freetype-config... (cached) /usr/bin/freetype-config
checking freetype2 libtool version... 9.7.3 - OK
checking for Carbon/Carbon.h... (cached) no
checking for uint64_t... (cached) yes
checking for uint128_t... (cached) no
configure: creating ./config.status
config.status: creating cairo.pc
config.status: creating Makefile
config.status: creating src/Makefile
config.status: creating src/cairo-features.h
config.status: creating test/Makefile
config.status: creating doc/Makefile
config.status: creating doc/public/Makefile
config.status: creating config.h
config.status: config.h is unchanged
config.status: executing depfiles commands

cairo will be compiled with the following surface backends:
  Xlib: yes
  Quartz: no
  XCB: no
  Win32: no
  PostScript: no
  PDF: no
  PNG: no
  glitz: no

and the following font backends:
  FreeType: yes
  Win32: no
  ATSUI: no

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: partial patch

Cairo's code to build without native int64 types is clearly quite bitrotted.

This patch fixes most of the problems, except for the two that require int64
division.  So it doesn't quite compile.  Never mind if it actually would run
correctly.

prlong.h should have some hints on how to write the division macro, but I don't
feel like doing that now, never mind figuring out what testing is appropriate. 
But it is easy to test, since you can just do the undefs I mentioned in my
previous patch.

We'll probably need this fixed to build cairo on all the platforms we support,
though I could be wrong.  I remember people causing some sort of port bustage
when using arithmetic operators on PRInt64.

Comment 8

[Martin Mokrejs]

Attachment: mozilla/config.status

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please don't paste huge gobs of input in the bug.  It's clearly a bug, and we
don't need more info, unless you really think you *should* be using native int64
types on your toolchain, in which case we should probably have two separate bugs.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: partial patch

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The second patch only differs in that it conforms to local style better; it
still lacks int64 division, which is a good bit of work (unless you want to copy
the NSPR code, which actually might be license-compatible...).  The NSPR code is in:

http://lxr.mozilla.org/nspr/find?string=prlong

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> checking for a sed that does not truncate output... (cached) 

> checking for ld used by gcc... ./configure: line 3566: s%\\%/%g: No such file or
> directory

The first line is the cause of the second, most likely.  It seems like a bug in
the libpixman configure that it doesn't give an error when SED is not present.

Comment 13

[tor]

Attachment: compiles, passes "make check"

Passes "make check", but someone who knows this code (keithp?) should
really be the one to verify.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oops, I missed the fact that there already was a divrem function. :-)

Also, you probably don't actually want the #undef lines in the patch.

Comment 15

[tor]

Attachment: remove test #undef lines

Comment 16

[Jason Dorje Short]

Attachment: patch that forces the bug to occur on all systems

This latest patch to fix the bug (178856) looks about right.

This patch (for cairo CVS version) should let you reproduce the bug on a
typical linux machine.	Normally it's impossible to get the problem since
stdint.h is included whether you want it to be or not.	(I think one of the
earlier patches did this as well.)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: attachment 178856 merged to current in-tree cairo (0.5.0, apparently)

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: merged to cairo trunk

Comment 19

[Carl Worth]

-	cairo_fixed_48_16_t a_dist, b_dist;
-	a_dist = ((cairo_fixed_48_16_t) a->slope.dx * a->slope.dx +
-		  (cairo_fixed_48_16_t) a->slope.dy * a->slope.dy);
+	cairo_fixed_48_16_t a_slope_dx, b_slope_dx, a_slope_dy, b_slope_dy,
+			    a_dist, b_dist;
+	a_slope_dx = _cairo_int32_to_int64 (a->slope.dx);
+	a_slope_dy = _cairo_int32_to_int64 (a->slope.dy);
+	b_slope_dx = _cairo_int32_to_int64 (b->slope.dx);
+	b_slope_dy = _cairo_int32_to_int64 (b->slope.dy);
+	a_dist = _cairo_int64_add (_cairo_int64_mul (a_slope_dx, a_slope_dx),
+				   _cairo_int64_mul (a_slope_dy, a_slope_dy));

This piece of the patch (at least) is incorrect, (or it will yield a correct
result, but less efficiently than desired).

The computation should be two 32-bit multiplies yielding 64-bit partial results, 
then one 64-bit addition of those partials. It looks like _cairo_int32x32_64_mul
should do what is desired here.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: use _cairo_int32x32_64_mul more often

This uses _cairo_int32x32_64_mul in a bunch of places where it could be,
including removal of temporary variables in _compare_cairo_edge_by_slope.

Comment 21

[tor]

Comment on attachment 178856 [details] [diff] [review]
remove test #undef lines

Remove old review request.

Comment 22

[Martin Mokrejs]

So is still the cairo code a bit rotten or fixed meanwhile? ;-)

Comment 23

[Gordon P. Hemsley [:GPHemsley]]

This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]