8:30 p.m.
I've wanted to fix the alias analysis problems in our code base for a
few years now. Today, while waiting for a Linux kernel compile to
finish, I took at look at one of them. It is caused by this
definition in console-impl.h:
#ifdef ERROR_CHECK_TYPES
DECLARE_INLINE_HEADER (
struct console *
error_check_console_type (struct console *con, Lisp_Object sym)
)
{
assert (EQ (CONSOLE_TYPE (con), sym));
return con;
}
# define CONSOLE_TYPE_DATA(con, type) \
(*(struct type##_console **) \
&(error_check_console_type (con, Q##type))->console_data)
#else
# define CONSOLE_TYPE_DATA(con, type) \
(*(struct type##_console **) &((con)->console_data))
#endif
That way, CONSOLE_TYPE_DATA(foo, bar) is an lvalue. However, it
breaks compilations with alias analysis turned on. The way to fix
this is to give up on having a single polymorphic macro that is both
an rvalue and an lvalue. I'll admit it's cute, but with today's
compilers, it is also dangerous. The rvalue version should look like
this:
#ifdef ERROR_CHECK_TYPES
DECLARE_INLINE_HEADER (
struct console *
error_check_console_type (struct console *con, Lisp_Object sym)
)
{
assert (EQ (CONSOLE_TYPE (con), sym));
return con;
}
# define CONSOLE_TYPE_DATA(con, type) \
((struct type##_console *) \
(error_check_console_type (con, Q##type))->console_data)
#else
# define CONSOLE_TYPE_DATA(con, type) \
((struct type##_console *) ((con)->console_data))
#endif
The lvalue version is easy, since console_data is a void *, so
anything can be assigned to it. I'd probably write the
non-error-checking version like this:
#define CONSOLE_TYPE_DATA_LVALUE(con) ((con)->console_data)
or, more likely, just dispense with the stupid macro and assign to
con->console_data directly. What do you other developers think?
--
Jerry James
http://www.jamezone.org/
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
8:52 p.m.
On Fri, May 15, 2009 at 1:30 PM, Jerry James <james(a)xemacs.org> wrote:
The lvalue version is easy, since console_data is a void *, so
anything can be assigned to it. I'd probably write the
non-error-checking version like this:
#define CONSOLE_TYPE_DATA_LVALUE(con) ((con)->console_data)
or, more likely, just dispense with the stupid macro and assign to
con->console_data directly. What do you other developers think?
So here's a concrete proposal. Please give me your reaction to this
patch. It looks like we have several other places in the codebase
that need a similar treatment for us to compile safely with alias
analysis turned on:
Note that we aren't losing anything with error-checking on, because
each affected function has other error-checking calls using the
original macro.
diff -r c064d7197712 src/console-impl.h
--- a/src/console-impl.h Sat Apr 18 03:24:48 2009 +0900
+++ b/src/console-impl.h Fri May 15 13:49:44 2009 -0600
＠＠ -469,11 +469,11 ＠＠
return con;
}
# define CONSOLE_TYPE_DATA(con, type) \
- (*(struct type##_console **) \
- &(error_check_console_type (con, Q##type))->console_data)
+ ((struct type##_console *) \
+ (error_check_console_type (con, Q##type))->console_data)
#else
# define CONSOLE_TYPE_DATA(con, type) \
- (*(struct type##_console **) &((con)->console_data))
+ ((struct type##_console *) ((con)->console_data))
#endif
#define CHECK_CONSOLE_TYPE(x, type) do { \
diff -r c064d7197712 src/console-stream.c
--- a/src/console-stream.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/console-stream.c Fri May 15 13:49:44 2009 -0600
＠＠ -72,12 +72,12 ＠＠
struct stream_console *stream_con;
#ifdef NEW_GC
- if (CONSOLE_STREAM_DATA (con) == NULL)
- CONSOLE_STREAM_DATA (con) = alloc_lrecord_type (struct stream_console,
- &lrecord_stream_console);
+ if (con->console_data == NULL)
+ con->console_data = alloc_lrecord_type (struct stream_console,
+ &lrecord_stream_console);
#else /* not NEW_GC */
- if (CONSOLE_STREAM_DATA (con) == NULL)
- CONSOLE_STREAM_DATA (con) = xnew_and_zero (struct stream_console);
+ if (con->console_data == NULL)
+ con->console_data = xnew_and_zero (struct stream_console);
#endif /* not NEW_GC */
stream_con = CONSOLE_STREAM_DATA (con);
＠＠ -140,7 +140,7 ＠＠
#ifndef NEW_GC
xfree (stream_con, struct stream_console *);
#endif /* not NEW_GC */
- CONSOLE_STREAM_DATA (con) = NULL;
+ con->console_data = NULL;
}
}
diff -r c064d7197712 src/console-tty.c
--- a/src/console-tty.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/console-tty.c Fri May 15 13:49:44 2009 -0600
＠＠ -77,10 +77,10 ＠＠
{
/* zero out all slots except the lisp ones ... */
#ifdef NEW_GC
- CONSOLE_TTY_DATA (con) = alloc_lrecord_type (struct tty_console,
- &lrecord_tty_console);
+ con->console_data = alloc_lrecord_type (struct tty_console,
+ &lrecord_tty_console);
#else /* not NEW_GC */
- CONSOLE_TTY_DATA (con) = xnew_and_zero (struct tty_console);
+ con->console_data = xnew_and_zero (struct tty_console);
#endif /* not NEW_GC */
CONSOLE_TTY_DATA (con)->terminal_type = Qnil;
CONSOLE_TTY_DATA (con)->instream = Qnil;
＠＠ -222,7 +222,7 ＠＠
#ifndef NEW_GC
xfree (tty_con, struct tty_console *);
#endif /* not NEW_GC */
- CONSOLE_TTY_DATA (con) = NULL;
+ con->console_data = NULL;
}
}
--
Jerry James
http://www.jamezone.org/
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
9:26 p.m.
There's nothing quite like having a nice conversation with yourself, is there?
On Fri, May 15, 2009 at 1:52 PM, Jerry James <james(a)xemacs.org> wrote:
So here's a concrete proposal. Please give me your reaction to
this
patch. It looks like we have several other places in the codebase
that need a similar treatment for us to compile safely with alias
analysis turned on:
I had to compile another kernel. :-)
This patch eliminates ALL gcc warnings about breaches of alias
analysis ettiquette the way I have my XEmacs configured. Those on
other platforms or using different configuration options may encounter
other places that need to be fixed. It may uglify the code a bit
(although I don't really think that it does; in fact, I generally
detest the macro soup I have to swim in when I go through our code).
diff -r c064d7197712 src/console-impl.h
--- a/src/console-impl.h Sat Apr 18 03:24:48 2009 +0900
+++ b/src/console-impl.h Fri May 15 14:20:28 2009 -0600
＠＠ -469,11 +469,11 ＠＠
return con;
}
# define CONSOLE_TYPE_DATA(con, type) \
- (*(struct type##_console **) \
- &(error_check_console_type (con, Q##type))->console_data)
+ ((struct type##_console *) \
+ (error_check_console_type (con, Q##type))->console_data)
#else
# define CONSOLE_TYPE_DATA(con, type) \
- (*(struct type##_console **) &((con)->console_data))
+ ((struct type##_console *) ((con)->console_data))
#endif
#define CHECK_CONSOLE_TYPE(x, type) do { \
diff -r c064d7197712 src/console-stream.c
--- a/src/console-stream.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/console-stream.c Fri May 15 14:20:28 2009 -0600
＠＠ -72,12 +72,12 ＠＠
struct stream_console *stream_con;
#ifdef NEW_GC
- if (CONSOLE_STREAM_DATA (con) == NULL)
- CONSOLE_STREAM_DATA (con) = alloc_lrecord_type (struct stream_console,
- &lrecord_stream_console);
+ if (con->console_data == NULL)
+ con->console_data = alloc_lrecord_type (struct stream_console,
+ &lrecord_stream_console);
#else /* not NEW_GC */
- if (CONSOLE_STREAM_DATA (con) == NULL)
- CONSOLE_STREAM_DATA (con) = xnew_and_zero (struct stream_console);
+ if (con->console_data == NULL)
+ con->console_data = xnew_and_zero (struct stream_console);
#endif /* not NEW_GC */
stream_con = CONSOLE_STREAM_DATA (con);
＠＠ -140,7 +140,7 ＠＠
#ifndef NEW_GC
xfree (stream_con, struct stream_console *);
#endif /* not NEW_GC */
- CONSOLE_STREAM_DATA (con) = NULL;
+ con->console_data = NULL;
}
}
diff -r c064d7197712 src/console-tty.c
--- a/src/console-tty.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/console-tty.c Fri May 15 14:20:28 2009 -0600
＠＠ -77,10 +77,10 ＠＠
{
/* zero out all slots except the lisp ones ... */
#ifdef NEW_GC
- CONSOLE_TTY_DATA (con) = alloc_lrecord_type (struct tty_console,
- &lrecord_tty_console);
+ con->console_data = alloc_lrecord_type (struct tty_console,
+ &lrecord_tty_console);
#else /* not NEW_GC */
- CONSOLE_TTY_DATA (con) = xnew_and_zero (struct tty_console);
+ con->console_data = xnew_and_zero (struct tty_console);
#endif /* not NEW_GC */
CONSOLE_TTY_DATA (con)->terminal_type = Qnil;
CONSOLE_TTY_DATA (con)->instream = Qnil;
＠＠ -222,7 +222,7 ＠＠
#ifndef NEW_GC
xfree (tty_con, struct tty_console *);
#endif /* not NEW_GC */
- CONSOLE_TTY_DATA (con) = NULL;
+ con->console_data = NULL;
}
}
diff -r c064d7197712 src/faces.c
--- a/src/faces.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/faces.c Fri May 15 14:20:28 2009 -0600
＠＠ -1544,8 +1544,8 ＠＠
}
cachel->display_table = Qunbound;
cachel->background_pixmap = Qunbound;
- FACE_CACHEL_FONT_SPECIFIED (cachel)->size = sizeof(cachel->font_specified);
- FACE_CACHEL_FONT_UPDATED (cachel)->size = sizeof(cachel->font_updated);
+ cachel->font_specified.size = sizeof(cachel->font_specified);
+ cachel->font_updated.size = sizeof(cachel->font_updated);
}
/* Retrieve the index to a cachel for window W that corresponds to
＠＠ -1628,7 +1628,7 ＠＠
struct face_cachel *cachel = Dynarr_atp (w->face_cachels, elt);
cachel->updated = 0;
- memset(FACE_CACHEL_FONT_UPDATED(cachel)->bits, 0,
+ memset(&cachel->font_updated.bits, 0,
BIT_VECTOR_LONG_STORAGE (NUM_LEADING_BYTES));
}
}
diff -r c064d7197712 src/glyphs-x.c
--- a/src/glyphs-x.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/glyphs-x.c Fri May 15 14:20:28 2009 -0600
＠＠ -408,7 +408,7 ＠＠
/* We can release the callbacks again. */
ungcpro_popup_callbacks (IMAGE_INSTANCE_X_WIDGET_LWID (p));
- IMAGE_INSTANCE_X_WIDGET_ID (p) = 0;
+ IMAGE_INSTANCE_SUBWINDOW_ID (p) = 0;
IMAGE_INSTANCE_X_CLIPWIDGET (p) = 0;
}
}
diff -r c064d7197712 src/glyphs-x.h
--- a/src/glyphs-x.h Sat Apr 18 03:24:48 2009 +0900
+++ b/src/glyphs-x.h Fri May 15 14:20:28 2009 -0600
＠＠ -124,9 +124,9 ＠＠
#define IMAGE_INSTANCE_X_CLIPWIDGET(i) \
(X_SUBWINDOW_INSTANCE_DATA (i)->data.wid.clip_window)
#define IMAGE_INSTANCE_X_SUBWINDOW_ID(i) \
- (* (Window *) & IMAGE_INSTANCE_SUBWINDOW_ID (i))
+ ((Window) IMAGE_INSTANCE_SUBWINDOW_ID (i))
#define IMAGE_INSTANCE_X_WIDGET_ID(i) \
- (* (Widget *) & IMAGE_INSTANCE_SUBWINDOW_ID (i))
+ ((Widget) IMAGE_INSTANCE_SUBWINDOW_ID (i))
#define XIMAGE_INSTANCE_X_SUBWINDOW_PARENT(i) \
IMAGE_INSTANCE_X_SUBWINDOW_PARENT (XIMAGE_INSTANCE (i))
--
Jerry "Let's reimplement XEmacs in Ruby" James
http://www.jamezone.org/
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
11:20 p.m.
>>>> Jerry James <james(a)xemacs.org> writes:
Jerry> This patch eliminates ALL gcc warnings about breaches of alias
Jerry> analysis ettiquette the way I have my XEmacs configured.
What configuration option is needed to get these warnings? e.g. How do
you have your XEmacs configured?
Yours
--
%% Mats
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
9:26 p.m.
On Fri, May 15, 2009 at 4:20 PM, Mats Lidell <matsl(a)xemacs.org> wrote:
What configuration option is needed to get these warnings? e.g. How
do
you have your XEmacs configured?
With a sufficiently recent GCC, supplying -fstrict-aliasing
-Wstrict-aliasing does the trick. Note that -O2 implies
-fstrict-aliasing. I'm not sure how recent "sufficiently recent" is.
I know that GCC 4.0 improved on the GCC 3.x compilers quite a bit for
this kind of analysis, but I also know that GCC 4.1 and 4.2 both
contained further improvements. I'm currently using Fedora 10, which
ships with GCC 4.3.2. In 2 weeks, I'll be running Fedora 11, which
ships with GCC 4.4.0.
--
Jerry James
http://www.jamezone.org/
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
9:06 a.m.
Jerry James writes:
I had to compile another kernel. :-)
Stop bragging! :-)
This patch [...] may uglify the code a bit (although I don't
really
think that it does; in fact, I generally detest the macro soup I
have to swim in when I go through our code).
I wouldn't go so far as say I detest it. Having accessors is kind of
Lisp-y, although I think that the prevalence of allcaps identifiers is
ugly, and it's certainly a barrier to entry to new programmers.
I wonder, given your statement that we always get error-checking in an
error-checking build anyway, if it might be a good idea to take the
next step, and do away with the maybe-error-checking macros entirely,
and use an explicit check as we do in Lisp primitives.
Especially since that doesn't actually seem to be true, eg in this
hunk:
diff -r c064d7197712 src/faces.c
--- a/src/faces.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/faces.c Fri May 15 14:20:28 2009 -0600
＠＠ -1544,8 +1544,8 ＠＠
}
cachel->display_table = Qunbound;
cachel->background_pixmap = Qunbound;
- FACE_CACHEL_FONT_SPECIFIED (cachel)->size = sizeof(cachel->font_specified);
- FACE_CACHEL_FONT_UPDATED (cachel)->size = sizeof(cachel->font_updated);
+ cachel->font_specified.size = sizeof(cachel->font_specified);
+ cachel->font_updated.size = sizeof(cachel->font_updated);
}
I assume that the maybe-error-checking isn't called elsewhere, so
where's the error-checking now?
So I suggest the following form:
diff -r c064d7197712 src/faces.c
--- a/src/faces.c Sat Apr 18 03:24:48 2009 +0900
+++ b/src/faces.c Fri May 15 14:20:28 2009 -0600
＠＠ -1544,8 +1544,10 ＠＠
}
cachel->display_table = Qunbound;
cachel->background_pixmap = Qunbound;
- FACE_CACHEL_FONT_SPECIFIED (cachel)->size = sizeof(cachel->font_specified);
- FACE_CACHEL_FONT_UPDATED (cachel)->size = sizeof(cachel->font_updated);
+ CHECK_FACE_CACHEL_FONT_SPECIFIED (cachel);
+ CHECK_FACE_CACHEL_FONT_UPDATED (cachel);
+ cachel->font_specified.size = sizeof(cachel->font_specified);
+ cachel->font_updated.size = sizeof(cachel->font_updated);
}
I'm not sure that's the right name, maybe the checks should be
something like
+ CHECK_FACE_CACHEL_FONT (cachel->font_specified);
+ CHECK_FACE_CACHEL_FONT (cachel->font_updated);
or even just
+ CHECK_FACE_CACHEL (cachel);
and there are yet other possibilities depending on just what checks
are done, and how they tend to associate.
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
9:29 p.m.
On Sat, May 16, 2009 at 2:06 AM, Stephen J. Turnbull <stephen(a)xemacs.org> wrote:
I wouldn't go so far as say I detest it. Having accessors is
kind of
Lisp-y, although I think that the prevalence of allcaps identifiers is
ugly, and it's certainly a barrier to entry to new programmers.
Sure. The problem I have is macros that expand to macros that expand
to macros that ... I have no idea how complex of an expression I'm
going to get when all the expanding is done. Maybe there will be 15
pointer dereferences on top of solving a partial differential
equation. Maybe it will expand to a simple structure reference. I
don't know.
I wonder, given your statement that we always get error-checking in
an
error-checking build anyway, if it might be a good idea to take the
next step, and do away with the maybe-error-checking macros entirely,
and use an explicit check as we do in Lisp primitives.
Especially since that doesn't actually seem to be true, eg in this
hunk:
Oops. You're right. I'll work on that next week.
So I suggest the following form:
> diff -r c064d7197712 src/faces.c
> --- a/src/faces.c Sat Apr 18 03:24:48 2009 +0900
> +++ b/src/faces.c Fri May 15 14:20:28 2009 -0600
> ＠＠ -1544,8 +1544,10 ＠＠
> }
> cachel->display_table = Qunbound;
> cachel->background_pixmap = Qunbound;
> - FACE_CACHEL_FONT_SPECIFIED (cachel)->size =
sizeof(cachel->font_specified);
> - FACE_CACHEL_FONT_UPDATED (cachel)->size = sizeof(cachel->font_updated);
> + CHECK_FACE_CACHEL_FONT_SPECIFIED (cachel);
> + CHECK_FACE_CACHEL_FONT_UPDATED (cachel);
> + cachel->font_specified.size = sizeof(cachel->font_specified);
> + cachel->font_updated.size = sizeof(cachel->font_updated);
> }
I'm not sure that's the right name, maybe the checks should be
something like
> + CHECK_FACE_CACHEL_FONT (cachel->font_specified);
> + CHECK_FACE_CACHEL_FONT (cachel->font_updated);
or even just
> + CHECK_FACE_CACHEL (cachel);
and there are yet other possibilities depending on just what checks
are done, and how they tend to associate.
Good suggestions. Are you comfortable with the general direction I'm
taking here, though? I'd like to make our codebase alias analysis
safe.
--
Jerry James
http://www.jamezone.org/
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
4:16 p.m.
Jerry James writes:
Good suggestions. Are you comfortable with the general direction
I'm
taking here, though? I'd like to make our codebase alias analysis
safe.
I think it is definitely preferable to use the default optimizations
of the compiler where possible (except that I'm of the school that
says "I'll see your '-O6' and raise you 10" when I'm holding
"-Os"
:-). GCC has always been more reliable when you let it do what it
wants to do.
The macros though were Ben and Martin's creations. Last I talked to
Martin, it seemed very unlikely that he would return to active duty on
21.5. He had his private ambitions to rewrite in C++, and he favored
21.4 anyway. I don't know what Ben's intentions are, but I know he's
alive and has unmerged workspaces, so maybe we should ask him.
I personally have no objection to removing what you consider
unnecessary macros though, as long is something is done to maintain
error-checking.
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
1:36 p.m.
Ar an séú lá déag de mí Bealtaine, scríobh Jerry James:
On Sat, May 16, 2009 at 2:06 AM, Stephen J. Turnbull
<stephen(a)xemacs.org> wrote:
> I wouldn't go so far as say I detest it. Having accessors is kind of
> Lisp-y, although I think that the prevalence of allcaps identifiers is
> ugly, and it's certainly a barrier to entry to new programmers.
Sure. The problem I have is macros that expand to macros that expand
to macros that ... I have no idea how complex of an expression I'm
going to get when all the expanding is done. Maybe there will be 15
pointer dereferences on top of solving a partial differential
equation. Maybe it will expand to a simple structure reference. I
don't know.
It would be helpful to have makefile targets that call CC with -E as well as
the normal CFLAGS; I know you’re well capable of doing this yourself, but
once I got in the habit of doing that when I was curious about something,
the macros upon macros didn’t bother me remotely as much, and I think
getting other folk in the habit of it would make their lives easier too.
--
¿Dónde estará ahora mi sobrino Yoghurtu Nghe, que tuvo que huir
precipitadamente de la aldea por culpa de la escasez de rinocerontes?
_______________________________________________
XEmacs-Beta mailing list
XEmacs-Beta(a)xemacs.org
http://calypso.tux.org/cgi-bin/mailman/listinfo/xemacs-beta
5668
days inactive
5671
days old
8 comments
4 participants
participants (4)
-
Aidan Kehoe -
Jerry James -
Stephen J. Turnbull -
The XEmacs Package Smoketest