Bug 93281

It sure looks like the compiler is wrong in casting the int to the enum, rather than the enum to an int.

However this is wrong anyway, as the resulting if will always be false. I think the value has to be added to the enum declaration. Or change the value to something less than 256.

typedef enum _cairo_surface_type {
    CAIRO_SURFACE_TYPE_IMAGE,
    CAIRO_SURFACE_TYPE_PDF,
    CAIRO_SURFACE_TYPE_PS,
    CAIRO_SURFACE_TYPE_XLIB,
    CAIRO_SURFACE_TYPE_XCB,
    CAIRO_SURFACE_TYPE_GLITZ,
    CAIRO_SURFACE_TYPE_QUARTZ,
    CAIRO_SURFACE_TYPE_WIN32,
    CAIRO_SURFACE_TYPE_BEOS,
    CAIRO_SURFACE_TYPE_DIRECTFB,
    CAIRO_SURFACE_TYPE_SVG,
    CAIRO_SURFACE_TYPE_OS2,
    CAIRO_SURFACE_TYPE_WIN32_PRINTING,
    CAIRO_SURFACE_TYPE_QUARTZ_IMAGE,
    CAIRO_SURFACE_TYPE_SCRIPT,
    CAIRO_SURFACE_TYPE_QT,
    CAIRO_SURFACE_TYPE_RECORDING,
    CAIRO_SURFACE_TYPE_VG,
    CAIRO_SURFACE_TYPE_GL,
    CAIRO_SURFACE_TYPE_DRM,
    CAIRO_SURFACE_TYPE_TEE,
    CAIRO_SURFACE_TYPE_XML,
    CAIRO_SURFACE_TYPE_SKIA,
    CAIRO_SURFACE_TYPE_SUBSURFACE,
    CAIRO_SURFACE_TYPE_COGL
} cairo_surface_type_t;

(In reply to Zunfeng Chen from comment #0)
> static inline cairo_bool_t
> _cairo_surface_is_snapshot (cairo_surface_t *surface)
> {
>     return surface->backend->type ==
> (cairo_surface_type_t)CAIRO_INTERNAL_SURFACE_TYPE_SNAPSHOT;
> }
> 
> On some platforms or compilers, size of enumuration types may be refered to
> the max value of them, to reduce the memory usage. Like armcc of RVDS/MDK.
> 
> The detail on the armcc:
> 
> All the enum values of cairo_surface_type_t are small than 255, so the
> variable defined by cairo_surface_type_t only has 1 bytes on the memory
> stack, but CAIRO_INTERNAL_SURFACE_TYPE_SNAPSHOT = 0x1000, so it has two
> bytes.
> 
> The code snippet from cairo-surface-snapshort-inline.h (above), comparation
> between cairo_surface_type_t and cairo_internal_surface_type_t,
> CAIRO_INTERNAL_SURFACE_TYPE_SNAPSHOT is casted to cairo_surface_type_t, the
> value may be wrong. Because not every compiler treates enumuration type as
> int.
> 
> Because of this, image surface crashed on arm cpu which is not running any
> OS.
> 
> There are too many code snippets cast a enumuration type to another one,
> this may be not a good idea. ARMCC of RVDS/MDK gave me too many warning
> about the issue.
> 
> I think there are not many persons run cairo on the bear metal without OS,
> but it it indeed a problem.
> 
> The fixed code snippet that I've done is below:
> 
> static inline cairo_bool_t
> _cairo_surface_is_snapshot (cairo_surface_t *surface)
> {
>     return (int)surface->backend->type ==
> (int)CAIRO_INTERNAL_SURFACE_TYPE_SNAPSHOT;
> }

Hi, Bill. This may be not wrong about compiler. There are details in "ARM Compiler toolchain Compiler Reference"

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491f/Babjddhe.html

NOTE:
Implementing enum in this way can reduce data size. The command-line option --enum_is_int forces the underlying type of enum to at least as wide as int.

This means sizeof(enum) isn't always equal to sizeof(int).

These two enum types are defined below, the author may want to use cairo_internal_surface_type privately inside the source code of cairo, but the values of cairo_internal_surface_type are always assign to the variables of cairo_surface_type_t. It means sizeof(cairo_surface_type_t) should be as great as sizeof(cairo_internal_surface_type), at least >= sizeof(short).

Or we can specify CAIRO_SURFACE_TYPE_IMAGE = 0x0000, this way maybe can avoid the optimization of compiler. Another way to avoid this problem is to use the option --enum_is_int of armcc, but this requires to recompile all libraries used(Some libraries used we have no source codes). Anyway, we should not make the source code of cairo depended on compiler behaviors. Specifying CAIRO_SURFACE_TYPE_IMAGE = 0x0000 may be a good choice?
------------------------------------------------------------------------
typedef enum _cairo_surface_type {
    CAIRO_SURFACE_TYPE_IMAGE,
    CAIRO_SURFACE_TYPE_PDF,
    CAIRO_SURFACE_TYPE_PS,
    CAIRO_SURFACE_TYPE_XLIB,
    CAIRO_SURFACE_TYPE_XCB,
    CAIRO_SURFACE_TYPE_GLITZ,
    CAIRO_SURFACE_TYPE_QUARTZ,
    CAIRO_SURFACE_TYPE_WIN32,
    CAIRO_SURFACE_TYPE_BEOS,
    CAIRO_SURFACE_TYPE_DIRECTFB,
    CAIRO_SURFACE_TYPE_SVG,
    CAIRO_SURFACE_TYPE_OS2,
    CAIRO_SURFACE_TYPE_WIN32_PRINTING,
    CAIRO_SURFACE_TYPE_QUARTZ_IMAGE,
    CAIRO_SURFACE_TYPE_SCRIPT,
    CAIRO_SURFACE_TYPE_QT,
    CAIRO_SURFACE_TYPE_RECORDING,
    CAIRO_SURFACE_TYPE_VG,
    CAIRO_SURFACE_TYPE_GL,
    CAIRO_SURFACE_TYPE_DRM,
    CAIRO_SURFACE_TYPE_TEE,
    CAIRO_SURFACE_TYPE_XML,
    CAIRO_SURFACE_TYPE_SKIA,
    CAIRO_SURFACE_TYPE_SUBSURFACE,
    CAIRO_SURFACE_TYPE_COGL
} cairo_surface_type_t;

typedef enum _cairo_internal_surface_type {
    CAIRO_INTERNAL_SURFACE_TYPE_SNAPSHOT = 0x1000,
    CAIRO_INTERNAL_SURFACE_TYPE_PAGINATED,
    CAIRO_INTERNAL_SURFACE_TYPE_ANALYSIS,
    CAIRO_INTERNAL_SURFACE_TYPE_OBSERVER,
    CAIRO_INTERNAL_SURFACE_TYPE_TEST_FALLBACK,
    CAIRO_INTERNAL_SURFACE_TYPE_TEST_PAGINATED,
    CAIRO_INTERNAL_SURFACE_TYPE_TEST_WRAPPING,
    CAIRO_INTERNAL_SURFACE_TYPE_NULL,
    CAIRO_INTERNAL_SURFACE_TYPE_TYPE3_GLYPH
} cairo_internal_surface_type_t;
-----------------------------------------------------------------------------

Setting something like CAIRO_SURFACE_TYPE_IMAGE = 0x0000 to force the type looks kind of kludgy to me.

Offhand I think Bill's right that the tested value should be added to the enum.  In general casting enums from one to another seems crude; there's no way to be sure that one day someone adds FOO as item 42 in one enum definition and someone else adds BAR as 42 in the other, which mean completely different and incompatible things?

If there's just a handful of enum casts, I'd say just fix them to work without needing casts.  If this is used extensively throughout the codebase, this probably needs further thought.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/215.