cairo code for compiling without native int64 types is bitrotted

RESOLVED WONTFIX

Status

Core Graveyard
GFX
RESOLVED WONTFIX
14 years ago
4 years ago

People

(Reporter: Martin Mokrejs, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

14 years ago
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
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

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

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.
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

Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

14 years ago
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
(Reporter)

Comment 6

14 years ago
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
Created attachment 178631 [details] [diff] [review]
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.
(Reporter)

Comment 8

14 years ago
Created attachment 178632 [details]
mozilla/config.status
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.
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
Summary: cairo_hull.c:96: error: conversion to non-scalar type requested → cairo code for compiling without native int64 types is bitrotted
(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

14 years ago
Created attachment 178845 [details] [diff] [review]
compiles, passes "make check"

Passes "make check", but someone who knows this code (keithp?) should
really be the one to verify.
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

14 years ago
Created attachment 178856 [details] [diff] [review]
remove test #undef lines
Attachment #178845 - Attachment is obsolete: true

Updated

14 years ago
Attachment #178856 - Flags: review?(keithp)

Comment 16

14 years ago
Created attachment 180019 [details] [diff] [review]
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.)
Created attachment 192475 [details] [diff] [review]
attachment 178856 [details] [diff] [review] merged to current in-tree cairo (0.5.0, apparently)
Attachment #178856 - Attachment is obsolete: true
Created attachment 192481 [details] [diff] [review]
merged to cairo trunk
Attachment #192475 - Attachment is obsolete: true

Comment 19

13 years ago
-	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.
Created attachment 192485 [details] [diff] [review]
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.
Attachment #192481 - Attachment is obsolete: true

Comment 21

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

Remove old review request.
Attachment #178856 - Flags: review?(keithp)
Product: Core → Core Graveyard
(Reporter)

Comment 22

9 years ago
So is still the cairo code a bit rotten or fixed meanwhile? ;-)
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]
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.