Type-error in C code

Type-error in C code

[Stefan Monnier]

The following code installed by the recent atom-change has a type-error:

static void
set_wm_state (Lisp_Object frame, int add, Atom atom, Atom value)
{
  struct x_display_info *dpyinfo = FRAME_X_DISPLAY_INFO (XFRAME (frame));

  x_send_client_event (frame, make_number (0), frame,
                       dpyinfo->Xatom_net_wm_state,
                       make_number (32),
                       /* 1 = add, 0 = remove */
                       Fcons
                       (make_number (add ? 1 : 0),
                        Fcons
                        (atom,
                         value != 0 ? value : Qnil)));
}

The error is to put an "Atom" into a cons cell: those can only hold
Lisp_Objects.  The usual compilation flags won't catch the error because
both types are actually some kind of integer, but if you
compile --enable-use-lisp-union-type, the C compiler will
dutyfully burp.


        Stefan

Re: Type-error in C code

[Julien Danjou]

On Fri, Nov 12 2010, Stefan Monnier wrote:

> static void
> set_wm_state (Lisp_Object frame, int add, Atom atom, Atom value)
> {
>   struct x_display_info *dpyinfo = FRAME_X_DISPLAY_INFO (XFRAME (frame));
>
>   x_send_client_event (frame, make_number (0), frame,
>                        dpyinfo->Xatom_net_wm_state,
>                        make_number (32),
>                        /* 1 = add, 0 = remove */
>                        Fcons
>                        (make_number (add ? 1 : 0),
>                         Fcons
>                         (atom,
>                          value != 0 ? value : Qnil)));
> }
>
> The error is to put an "Atom" into a cons cell: those can only hold
> Lisp_Objects.  The usual compilation flags won't catch the error because
> both types are actually some kind of integer, but if you
> compile --enable-use-lisp-union-type, the C compiler will
> dutyfully burp.

Good catch Stefan. I should use this flag to compile my code now.

The fix should be easy I think, you just need to replace atom with
make_number (atom).

I can provide a patch is that handier.

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Jan Djärv]

An Atom is 32 bits.  make_number can't cover that range when long is 32 bits, 
so it is an unsafe thing to do.  Best is to revert and go back to using a string.

	Jan D.


Julien Danjou skrev 2010-11-12 15.21:
> On Fri, Nov 12 2010, Stefan Monnier wrote:
>
>> static void
>> set_wm_state (Lisp_Object frame, int add, Atom atom, Atom value)
>> {
>>    struct x_display_info *dpyinfo = FRAME_X_DISPLAY_INFO (XFRAME (frame));
>>
>>    x_send_client_event (frame, make_number (0), frame,
>>                         dpyinfo->Xatom_net_wm_state,
>>                         make_number (32),
>>                         /* 1 = add, 0 = remove */
>>                         Fcons
>>                         (make_number (add ? 1 : 0),
>>                          Fcons
>>                          (atom,
>>                           value != 0 ? value : Qnil)));
>> }
>>
>> The error is to put an "Atom" into a cons cell: those can only hold
>> Lisp_Objects.  The usual compilation flags won't catch the error because
>> both types are actually some kind of integer, but if you
>> compile --enable-use-lisp-union-type, the C compiler will
>> dutyfully burp.
>
> Good catch Stefan. I should use this flag to compile my code now.
>
> The fix should be easy I think, you just need to replace atom with
> make_number (atom).
>
> I can provide a patch is that handier.
>

Re: Type-error in C code

[Julien Danjou]

On Fri, Nov 12 2010, Jan Djärv wrote:

> An Atom is 32 bits.  make_number can't cover that range when long is 32
> bits, so it is an unsafe thing to do.  Best is to revert and go back to
> using a string.

There's no really no way to make a 32 bits number in elisp?
I mean, I trust you, I am not familiar with elisp's internal, but that
sounds weird.

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Julien Danjou]

On Fri, Nov 12 2010, Julien Danjou wrote:

> There's no really no way to make a 32 bits number in elisp?
> I mean, I trust you, I am not familiar with elisp's internal, but that
> sounds weird.

Ok, reading the Lisp_Object definition explains why. I've learned
something today. :)

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Julien Danjou]

On Fri, Nov 12 2010, Julien Danjou wrote:

> Ok, reading the Lisp_Object definition explains why. I've learned
> something today. :)

I've replying again to myself, but I'm still discovering the internal.
Sorry.

It seems we can store 32 bits unsigned using XSETFASTINT.

Reading x_fill_property_data, I see:

--8<---------------cut here---------------start------------->8---
  for (iter = data; CONSP (iter); iter = XCDR (iter))
    {
      Lisp_Object o = XCAR (iter);

      if (INTEGERP (o))
        val = (long) XFASTINT (o);
--8<---------------cut here---------------end--------------->8---

So IIUC, we could use XSETFASTINT to store the atom. Am I wrong?

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Eli Zaretskii]

> From: Julien Danjou <julien@danjou.info>
> Date: Fri, 12 Nov 2010 16:32:10 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> It seems we can store 32 bits unsigned using XSETFASTINT.

No.

>   for (iter = data; CONSP (iter); iter = XCDR (iter))
>     {
>       Lisp_Object o = XCAR (iter);
> 
>       if (INTEGERP (o))
>         val = (long) XFASTINT (o);
> --8<---------------cut here---------------end--------------->8---
> 
> So IIUC, we could use XSETFASTINT to store the atom. Am I wrong?

Yes, you are wrong.  You should only use XSETFASTINT if you know the
value will not overflow a Lisp integer.

In a nutshell, nothing can work around the basic limitation that only
integer values smaller or equal to most-positive-fixnum can be put
into a Lisp integer.

Re: Type-error in C code

[Eli Zaretskii]

> From: Julien Danjou <julien@danjou.info>
> Date: Fri, 12 Nov 2010 16:22:07 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> There's no really no way to make a 32 bits number in elisp?

Not as an integer, but you could use a float if an integer isn't large
enough.

Re: Type-error in C code

[Julien Danjou]

On Fri, Nov 12 2010, Eli Zaretskii wrote:

Hi Eli,

> Yes, you are wrong.  You should only use XSETFASTINT if you know the
> value will not overflow a Lisp integer.
> 
> In a nutshell, nothing can work around the basic limitation that only
> integer values smaller or equal to most-positive-fixnum can be put
> into a Lisp integer.

Ok, I did not know that. So XSETFASTINT is really to be fast, not to be
bigger. Thanks for the explanation.

OTOH, x_fill_property_data deals with floats too, so using a float here
sounds like a good solution, as you suggested.

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[John Yates]

On Fri, Nov 12, 2010 at 10:58 AM, Julien Danjou <julien@danjou.info> wrote:

> OTOH, x_fill_property_data deals with floats too, so using a float here
> sounds like a good solution, as you suggested.

Alternatively you could check the atom's value and so long as it is in
range you could generate an elisp integer, falling back to a float
only when the atom is too large.

/john
-- 
John Yates
257 Nashoba Rd
Concord, MA 01742
978 371-4923

Re: Type-error in C code

[Eli Zaretskii]

> Date: Fri, 12 Nov 2010 11:06:26 -0500
> From: John Yates <john@yates-sheets.org>
> 
> On Fri, Nov 12, 2010 at 10:58 AM, Julien Danjou <julien@danjou.info> wrote:
> 
> > OTOH, x_fill_property_data deals with floats too, so using a float here
> > sounds like a good solution, as you suggested.
> 
> Alternatively you could check the atom's value and so long as it is in
> range you could generate an elisp integer, falling back to a float
> only when the atom is too large.

That's what I meant, actually (we already do such things in other
places in Emacs sources).

Re: Type-error in C code

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 12 Nov 2010 11:06:26 -0500
>> From: John Yates <john@yates-sheets.org>
>> 
>> On Fri, Nov 12, 2010 at 10:58 AM, Julien Danjou <julien@danjou.info> wrote:
>> 
>> > OTOH, x_fill_property_data deals with floats too, so using a float here
>> > sounds like a good solution, as you suggested.
>> 
>> Alternatively you could check the atom's value and so long as it is in
>> range you could generate an elisp integer, falling back to a float
>> only when the atom is too large.
>
> That's what I meant, actually (we already do such things in other
> places in Emacs sources).

See make_fixnum_or_float.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Type-error in C code

[Jan D.]

Julien Danjou skrev 2010-11-12 16:22:
> On Fri, Nov 12 2010, Jan Djärv wrote:
>
>> An Atom is 32 bits.  make_number can't cover that range when long is 32
>> bits, so it is an unsafe thing to do.  Best is to revert and go back to
>> using a string.
>
> There's no really no way to make a 32 bits number in elisp?
> I mean, I trust you, I am not familiar with elisp's internal, but that
> sounds weird.
>

Either use a 64-bit OS or use a cons with 16 bits each.  I'm not sure if 
the send_client_message functions support that, you have to check (I'm 
not near the source right now).

	Jan D.

Re: Type-error in C code

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> The error is to put an "Atom" into a cons cell: those can only hold
> Lisp_Objects.  The usual compilation flags won't catch the error because
> both types are actually some kind of integer, but if you
> compile --enable-use-lisp-union-type, the C compiler will
> dutyfully burp.

I think we should remove the union Lisp_Object, and instead define a
struct Lisp_Object { EMACS_INT i; } (reusing the macros of the non-union
type), and make that the default Lisp_Object at least during
development.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Type-error in C code

[Miles Bader]

John Yates <john@yates-sheets.org> writes:
> Alternatively you could check the atom's value and so long as it is in
> range you could generate an elisp integer, falling back to a float
> only when the atom is too large.

... and presumably on 64-bit systems (increasingly the norm), a 32-bit
quantity would _always_ fit.

-miles

-- 
Mayonnaise, n. One of the sauces that serve the French in place of a state
religion.

Re: Type-error in C code

[Stefan Monnier]

>> The error is to put an "Atom" into a cons cell: those can only hold
>> Lisp_Objects.  The usual compilation flags won't catch the error because
>> both types are actually some kind of integer, but if you
>> compile --enable-use-lisp-union-type, the C compiler will
>> dutyfully burp.

> I think we should remove the union Lisp_Object, and instead define a
> struct Lisp_Object { EMACS_INT i; } (reusing the macros of the non-union
> type), and make that the default Lisp_Object at least during
> development.

Not sure what we be the benefit.


        Stefan

Re: Type-error in C code

[Thien-Thi Nguyen]

() Jan Djärv <jan.h.d@swipnet.se>
() Fri, 12 Nov 2010 16:12:17 +0100

   An Atom is 32 bits.

... with the top 3 bits guaranteed to be zero
(X Windows System Protocol, section 3. Common Types).

In this Emacs, ‘most-positive-fixnum’ is 536870911,
i.e., #x1fffffff, i.e., 29 1s.

Coincidence?

Re: Type-error in C code

[Jan Djärv]

Thien-Thi Nguyen skrev 2010-11-12 22.15:
> () Jan Djärv<jan.h.d@swipnet.se>
> () Fri, 12 Nov 2010 16:12:17 +0100
>
>     An Atom is 32 bits.
>
> ... with the top 3 bits guaranteed to be zero
> (X Windows System Protocol, section 3. Common Types).
>
> In this Emacs, ‘most-positive-fixnum’ is 536870911,
> i.e., #x1fffffff, i.e., 29 1s.
>
> Coincidence?

Interesting.  Maybe they used those bits as a type indictor at one point. 
Looking at the X source now, I can't find any reson for this.

However, I fixed the code to use make_fixnum_or_float anyway.  In the future 
somebody may come along and wonder why a 32-bit number is put in a Lisp integer.

	Jan D.

Re: Type-error in C code

[Thien-Thi Nguyen]

() Jan Djärv <jan.h.d@swipnet.se>
() Sat, 13 Nov 2010 20:19:05 +0100

   Interesting.  Maybe they used those bits as a type indictor at
   one point.  Looking at the X source now, I can't find any reson
   for this.

IIRC, X was originally conceived and implemented on a Lisp system.
The term "Atom", and its 29 bits max-ness are direct consequences.

   However, I fixed the code to use make_fixnum_or_float anyway.
   In the future somebody may come along and wonder why a 32-bit
   number is put in a Lisp integer.

Not if they understand that an Atom is NOT a 32-bit number.
Perhaps if you revert the change and explain the limits of
the type in a nice comment, future programmers will not be
inclined to wonder when they look (only) at the source.

I think routing through ‘make_fixnum_or_float’ is suboptimal.

Re: Type-error in C code

[Julien Danjou]

On Fri, Nov 12 2010, Andreas Schwab wrote:

> I think we should remove the union Lisp_Object, and instead define a
> struct Lisp_Object { EMACS_INT i; } (reusing the macros of the non-union
> type), and make that the default Lisp_Object at least during
> development.

I can't think of a good reason not to do that. Is there one?

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Johan Bockgård]

Jan Djärv <jan.h.d@swipnet.se> writes:

> However, I fixed the code to use make_fixnum_or_float anyway.

> +                         value != 0
> +                         ? make_fixnum_or_float (value) : Qnil)));

This part should be

    ... ? Fcons (..., Qnil) : Qnil

Re: Type-error in C code

[Jan Djärv]

Thien-Thi Nguyen skrev 2010-11-13 20.52:
> () Jan Djärv<jan.h.d@swipnet.se>
> () Sat, 13 Nov 2010 20:19:05 +0100
>     However, I fixed the code to use make_fixnum_or_float anyway.
>     In the future somebody may come along and wonder why a 32-bit
>     number is put in a Lisp integer.
>
> Not if they understand that an Atom is NOT a 32-bit number.
> Perhaps if you revert the change and explain the limits of
> the type in a nice comment, future programmers will not be
> inclined to wonder when they look (only) at the source.
>
> I think routing through ‘make_fixnum_or_float’ is suboptimal.

This code is only used when changing fullscreen or sticky frame parameter.
And that does not happen often.  An extra check that ensures the value is 
below max fixnum is too small to be worth to optimize away.  If atoms are 29 
bits, they will be fixnum anyway.

	Jan D.

Re: Type-error in C code

[Jan Djärv]

Johan Bockgård skrev 2010-11-13 21.38:
> Jan Djärv<jan.h.d@swipnet.se>  writes:
>
>> However, I fixed the code to use make_fixnum_or_float anyway.
>
>> +                         value != 0
>> +                         ? make_fixnum_or_float (value) : Qnil)));
>
> This part should be
>
>      ... ? Fcons (..., Qnil) : Qnil

Indeed, thanks.

	Jan D.

Re: Type-error in C code

[Stefan Monnier]

>> I think we should remove the union Lisp_Object, and instead define a
>> struct Lisp_Object { EMACS_INT i; } (reusing the macros of the non-union
>> type), and make that the default Lisp_Object at least during
>> development.
> I can't think of a good reason not to do that. Is there one?

Data of type "struct ..." is often treated differently by compilers than
data of type int/long/... so the resulting code is likely to be
less efficient.


        Stefan

Re: Type-error in C code

[Julien Danjou]

On Fri, Nov 12 2010, Stefan Monnier wrote:

> Not sure what we be the benefit.

Avoid programming mistakes?

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> The error is to put an "Atom" into a cons cell: those can only hold
>>> Lisp_Objects.  The usual compilation flags won't catch the error because
>>> both types are actually some kind of integer, but if you
>>> compile --enable-use-lisp-union-type, the C compiler will
>>> dutyfully burp.
>
>> I think we should remove the union Lisp_Object, and instead define a
>> struct Lisp_Object { EMACS_INT i; } (reusing the macros of the non-union
>> type), and make that the default Lisp_Object at least during
>> development.
>
> Not sure what we be the benefit.

Let the compiler burp.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Type-error in C code

[Stefan Monnier]

>> Not sure what we be the benefit.
> Let the compiler burp.

We already have that with use-union-lisp-type.


        Stefan

Re: Type-error in C code

[Julien Danjou]

On Tue, Nov 16 2010, Stefan Monnier wrote:

> We already have that with use-union-lisp-type.

And I think it should not be an option. It should be the default.

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Stephen J. Turnbull]

Julien Danjou writes:
 > On Tue, Nov 16 2010, Stefan Monnier wrote:
 > 
 > > We already have that with use-union-lisp-type.
 > 
 > And I think it should not be an option. It should be the default.

Historically, GCC has been buggy with union type in XEmacs, at least.
It had problems (which regressed at least once after being fixed) with
structures inside of unions.  Microsoft compilers have also had
trouble with it.  That was ages ago == GCC 3.3 or so ... we think.  So
maybe it's not a relevant consideration now.

It also may make things slower because it restricts the compiler from
doing some code transformations that turn out (according to Martin
Buchholz, anyway) entire safe in the context of Lisp object
manipulations.  Also, in XEmacs it conflicts with the optimizations
that require no variable aliasing (there are a few places where casts
are used to fit Lisp objects into XPointers and the like), which in
some microbenchmarks of the XEmacs bytecode interpreter gave a small
speedup.  It's not just that warning; the code is actually wrong.
Martin and Ben Wing claimed that GCC was being too aggressive, but the
GCC developers replied reasonably enough that the code they used was
technically permitted by the standard, and fixing union-type for
XEmacs didn't justify the effort needed to add a special case.

Finally, note that a default *is* an option.  Do you want to force
union type for all builds, or do you want to make it the default and
leave the option to the user to use the (possibly faster) bit-flicking
implementation?

Re: Type-error in C code

[Julien Danjou]

On Tue, Nov 16 2010, Stephen J. Turnbull wrote:

> Finally, note that a default *is* an option.  Do you want to force
> union type for all builds, or do you want to make it the default and
> leave the option to the user to use the (possibly faster) bit-flicking
> implementation?

Since I don't buy your arguments[1], I think it should NOT be an option,
but the implementation itself.

OTOH, I only talked about using a struct including the integer, not
necessarily the whole union type used by use-union-lisp-type.

We already use that kind of type in XCB[2] without any problem.

[1]  Which were probably true a long time ago, but you will need to prove
     that it does not compile anywhere and is slow to me in 2010. :)

[2]  http://xcb.freedesktop.org

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Not sure what we be the benefit.
>> Let the compiler burp.
>
> We already have that with use-union-lisp-type.

It lacks support for USE_2_TAGS_FOR_INTS.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Type-error in C code

[Stephen J. Turnbull]

Julien Danjou writes:

 > Since I don't buy your arguments[1],

I'm not selling them, simply mentioning them for those who aren't
familiar with the history of the option (of course, I'm assuming the
history in Emacs is similar to that of XEmacs, but I think that's
reasonable).

 > I think it should NOT be an option, but the implementation itself.
 > 
 > OTOH, I only talked about using a struct including the integer, not
 > necessarily the whole union type used by use-union-lisp-type.

Well, please be careful, then; the message you replied to (and quoted)
specifically mentioned use-union-lisp-type.  Just because you
specifically described the trivial "int in a struct" type in an
earlier message doesn't mean that you wouldn't advocate making union
Lisp type the default, or perhaps only, implementation of Lisp
objects.  That was certainly the way I interpreted it.

Re: Type-error in C code

[Stefan Monnier]

>> We already have that with use-union-lisp-type.
> And I think it should not be an option. It should be the default.

Same problem as the structs type: it's not optimized as well as the
int/long... types.



        Stefan

Re: Type-error in C code

[Julien Danjou]

On Tue, Nov 16 2010, Stefan Monnier wrote:

> Same problem as the structs type: it's not optimized as well as the
> int/long... types.

May I ask any references about that?
Not that I don't trust you, but I'd like to know why.

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

Re: Type-error in C code

[Stefan Monnier]

>> Same problem as the structs type: it's not optimized as well as the
>> int/long... types.
> May I ask any references about that?
> Not that I don't trust you, but I'd like to know why.

IIRC calling conventions usually dictate that non-scalar types get
passed via copies on the stack rather than in registers.

Also of course, in general structs can't be handled in the same way as
int/long/... by the compiler, so in order to handle single-slot structs
efficiently, the compiler needs to special case these.  I'd expect
a mature C compiler to go through the trouble to try and special-case
them, but that's no guarantee that it's going to be as efficient as just
scalar types.


        Stefan