64-unclean code in eval.c

[Ben Wing]

Aidan Kehoe wrote:
>  Ar an ceathrú lá déag de mí Meitheamh, scríobh Ben Wing: 
>
>  > i'm staring at this code and i don't see what's wrong even on 64-bit 
>  > platforms.  i'm pretty sure i wrote this code.  it is used for saving 
>  > and restoring internal C variables of type `int' and it tries to put it 
>  > into a Lisp int but it's careful to use a opaque pointer in the rare 
>  > instance where the value won't fit into a Lisp int.  the code is careful 
>  > with its casting so i don't see what the problem is.
>  > 
>  > if the problem is this line:
>
> [It is.]
>
>  >       val = (int) get_opaque_ptr (lval);
>  > 
>  > 
>  > then something is too smart for its own good.
>
> Yes, the compiler is. But hey, it’s just software. 
>
>  > i would not recommend Aidan's patch because i think it's too obscure.  
>  > instead, put in a comment to the effect that we know the value stored in 
>  > the void * is a crypto-int since we put an int into it in the first 
>  > place, and i'd use more clever casts to get rid of the problem.  for 
>  > example, cast to (long) and then to (int).
>
> On AMD64, every C int (32 value bits) will fit into a Lisp int (63 value
> bits). So no value other than a Lisp int need ever be saved, and no value
> other than a Lisp int need be restored. I believe long is also 32 bits
> there; maybe a cast to long long and then a cast to int would work.  
>   

You could cast to intptr_t, which (see lisp.h) is an integral type of 
the same size as a void pointer, then cast again.  Or you could also 
cast to EMACS_INT, which is guaranteed to be large enough to fit a 
pointer, then cast again.

In either case, put in a comment explaining what's going on, using text 
from my and/or your recent messages.  This is certainly the *most* 
important thing to do -- obscure code like this is easily be 
misunderstood and consequently fucked up, sometimes in subtle ways that 
bite in really hard-to-trace ways.
>  > btw how can this be an error under C++?  when i used to do devwork i 
>  > always ran a C++ build as part of my tests, and unless something changed 
>  > recently, this code has been around for long before then.
>
> The change is the availablility of the AMD64 platform and its
> pointer-bigger-than-int approach. If XEmacs had been running and regularly
> compiled under DOS we probably wouldn’t be seeing these errors. 
>
>  > and if it is indeed a problem under C++, use this:
>  > 
>  >       val = reinterpret_cast<int> get_opaque_ptr (lval);
>  > 
>  > 
>  > that's the C++ way of saying "abandon hope all ye who enter here" :)
>
> Good to know.
take a look at Stroustrup's C++ book, third edition.  it seems that the 
old C-style casts are allowed (maybe only some of the time?), but 
deprecated.  they get replaced by static_cast<>, dynamic_cast<>, 
const_cast<>, and reinterpret_cast<>.  the first two are for converting 
between related types, depending on whether the cast can be done by the 
compiler or needs run-time info -- a dynamic cast is a downcast or 
cross-cast, and returns NULL upon failure.  const_cost is for casting 
away a const qualifier, and reinterpret_cast is for casting between 
pointers and integers or similar casts between unrelated types where you 
just stick the bit patterns from one directly into the other.

ben