Bug 89339

Summary: util/cairo-script: compilation errors on AIX (no member named '__jmpbuf')
Product: cairo Reporter: Michael Haubenwallner <michael.haubenwallner>
Component: generalAssignee: Chris Wilson <chris>
Status: RESOLVED FIXED QA Contact: cairo-bugs mailing list <cairo-bugs>
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: proposed patch for proper include order

Description Michael Haubenwallner 2015-02-26 16:06:21 UTC
Using gcc on AIX I do get these compiler error:

/.../cairo-9999/util/cairo-script/cairo-script-scanner.c: In function '_buffer_grow':
/.../cairo-9999/util/cairo-script/cairo-script-scanner.c:200: error: 'csi_scanner_t' has no member named '__jmpbuf'
/.../cairo-9999/util/cairo-script/cairo-script-scanner.c:206: error: 'csi_scanner_t' has no member named '__jmpbuf'

Problem here is that
*) cairo-script-scanner.c   does #include "cairo-script-private.h"
*) "cairo-script-private.h" does #include "config.h"
                                 #include <setjmp.h>
                                 typedef struct _csi_scanner csi_scanner_t;
                                 struct _csi_scanner { jmp_buf jmpbuf; ...}
*) cairo-script-scanner.c   does #include <math.h>
*) <math.h>                 does #include <stdlib.h>
*) <stdlib.h>               does #include <sys/wait.h>
*) <sys/wait.h>             does #include <sys/signal.h>
*) <sys/signal.h>           does #include <sys/context.h>
*) <sys/context.h>          does #define jmpbuf __jmpbuf
*) cairo-script-scanner.c   does use: _csi_scanner.jmpbuf
             which is now defined to: _csi_scanner.__jmpbuf
           but was declared above as: _csi_scanner.jmpbuf
Comment 1 Michael Haubenwallner 2015-02-26 16:15:13 UTC
Created attachment 113847 [details] [review]
proposed patch for proper include order

This proposed patch does:

for cairo-script-private.h:
  *) remove include "config.h"

for cairo-script-*.c:
  1) include "config.h" for the ABI-specific macros, before anything else
  2) include system headers
  3) include local headers
Comment 2 Bryce Harrington 2015-03-05 22:45:53 UTC
In studying bug 89354, and googling around for best practices in order of header inclusion, there seems to be a rough concensus[1] that you should include in this order:

Given foo.c:
1.  "config.h"
2.  "foo.h"
3.  headers from the same component
4.  headers from other components
5.  system headers

The rationale here is to avoid hidden dependencies between headers.

For comparison, Google's coding style guide[2] swaps #3 and #5, arguing that this ensures that build breaks show up first for people working on the files in question, not for people in other projects.

Poking around Cairo .c files, the first form seems to be the more common convention.  I think in the interest of consistency we should stick with that form.

But looking at the original problem, maybe header shuffling is the wrong way to solve this.  The actual underlying problem here is a namespace conflict on jmpbuf.  The _csi_scanner structure is not part of any public API, so how about just renaming the member to jump_buffer?

[1] http://stackoverflow.com/questions/2762568/c-c-include-file-order-best-practices

[2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Order_of_Includes
Comment 3 Michael Haubenwallner 2015-03-06 08:59:13 UTC
(In reply to Bryce Harrington from comment #2)
> In studying bug 89354, and googling around for best practices in order of
> header inclusion, there seems to be a rough concensus[1] that you should
> include in this order:

As long as "config.h" is included first, I don't really care which practice a particular project does prefer.

> But looking at the original problem, maybe header shuffling is the wrong way
> to solve this.  The actual underlying problem here is a namespace conflict
> on jmpbuf.  The _csi_scanner structure is not part of any public API, so how
> about just renaming the member to jump_buffer?

Renaming is fine with me too.
Comment 4 Bryce Harrington 2015-06-18 23:03:51 UTC
commit b4a922f62d34973ea89495b40ce8bc6378110b9e
Author:     Bryce Harrington <bryce@osg.samsung.com>
AuthorDate: Tue Jun 16 16:42:56 2015 -0700
Commit:     Bryce Harrington <bryce@osg.samsung.com>
CommitDate: Thu Jun 18 12:38:59 2015 -0700

    cairo-script: Rename struct member to avoid name collision on AIX

    On AIX, the token jmpbuf is a pre-processor macro.
    cairo-script-scanner.c includes a private struct with a member named
    jmpbuf which gets renamed to __jmpbuf when AIX's sys/context.h has been
    included.

    While judicious ordering of includes might kludge around this problem
    (by causing all references to .jmpbuf to become .__jmpbuf), it's better
    to simply select a new name for the struct member that won't suffer the
    collision.

    Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=89339

    Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
    Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

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.