Bug 2335

Summary:	icbltone.c compile warnings on x86_64 - possibly real bug.
Product:	cairo	Reporter:	ellson
Component:	general	Assignee:	Carl Worth <cworth>
Status:	RESOLVED FIXED	QA Contact:	cairo-bugs mailing list <cairo-bugs>
Severity:	normal
Priority:	high	CC:	jwatt
Version:	0.9.3
Hardware:	x86 (IA32)
OS:	Linux (All)
Whiteboard:
i915 platform:		i915 features:
Attachments:	patch to pixman.h suggested by Keith

Description ellson 2005-01-19 16:55:05 UTC

I get many warnings like this:

icbltone.c:461: warning: left shift count >= width of type

I tracked it down to a problem in Mask24Pos(x,r) when BITMAP_BIT_ORDER != MSBFirst

#if BITMAP_BIT_ORDER == MSBFirst
#define Mask24Pos(x,r) ((x)*24-(r))
#else
#define Mask24Pos(x,r) ((x)*24-((r) ? 24 - (r) : 0))
#endif

When x=2 r=0 this produces a value of 48, which in  Mask24(x,r) results
in 0xffffff << 48 which is 72bits, i.e larger than uint64.

I suspect this is the cause of the mashed PNG images reported on the cairo
development list a couple of days ago.

I haven't been able to identify a patch, because I still don't fully understand
what this is doing.  I'd spend more effort on it but I understand you have a 
major rewrite about to land in CVS and so perhaps you've fixed it already?

Comment 1 Carl Worth 2005-01-19 18:23:06 UTC

The "rewrite" is just moving some newer code over from the X server, but
only for the trapezoid rasterization, (most of what is currently in ictrap.c
will disappear). It won't affect the function you are looking at.

I don't know offhand the details of whats going on in this function, so some
investigation would be useful.

Comment 2 ellson 2005-01-21 17:59:24 UTC

Right.  The problem still exists with the latest updates.

Do you happen to know what "BITMAP_BIT_ORDER != MSBFirst" means?

Does it really imply bit-order reversal, or just byte order?

Do you happen to have a description of bit/byte ordering on x86_64
as compared with i386 ?

Comment 3 Keith Packard 2005-01-21 18:46:08 UTC

The 64-bit code in fb (pixman) is probably broken; it hasn't been used in quite
some time as PCI (and AGP) is 32-bits wide, so doing things 64-bits at a time is
a net loss.  To quickly fix this, I suggest just using 32-bit datatypes by
setting IC_SHIFT to 5 for all machines.

Comment 4 ellson 2005-01-21 19:03:22 UTC

Thanks, that fixes both the compile warnings (this bug) and the distortion
of fonts from cairo-demo/png/text.c reported to the cairo list a few days ago.

patch follows

Comment 5 ellson 2005-01-21 19:05:15 UTC

Created attachment 1733 [details] [review]
patch to pixman.h suggested by Keith

Comment 6 Carl Worth 2005-01-26 09:11:25 UTC

I've committed a fix for this, but have not tested on any 64-bit platforms.

Please test and re-open the report if necessary.

Comment 7 ellson 2005-01-26 09:42:33 UTC

Works for me on x86_64.

Do you happen to know, or can you suggest a test to find out what this does
to in-memory bitmaps?     I'm hoping it doesn't double their size?

Comment 8 Carl Worth 2005-08-22 17:14:37 UTC

Move bugs against "cvs" version to "0.9.3" so we can remove the "cvs" version.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.