[Freeciv-Dev] Re: Compaq Tru64 Unix Alpha platform - Building freeciv

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, May 12, 2002 at 08:18:57PM +0200, Davide Pagnin wrote:
>       Hi All!
> 
> This is another mail! 
> (And on a completely different topic!!!)
> 
> I've managed to compile freeciv on a Alpha platform with Tru64 unix 5.1A
> (formerly Digital Unix).
> 
> I've already read that Ben Webb has the same platform (alpha) but
> It was not clear if he used linux or Tru64 Unix.
> 
> Why I want to state this?
> Well, we are in a feature freeze moment, with deep playtesting an
> bugfixing,
> and I hope that patches that can make easier building the game under
> other platforms and unices are appreciated.
> 
> I've spotted some kind of common problem with the actual code in the
> cvs,
> and definitely they depend on the different architecture.
> (I imagine that alla of you know that alpha processor are true 64 bit,
> instead of 32 bit like Intel x86 processors)
> 
> I'll make summary of the most common type of problem:
> 
> 1) when you feed an operand to a function of the print family, with
> a format of %i or %d, gcc expects you feed a 32 bit integer, even 
> with alpha platform, so it is necessary to explicitly cast to integer
> or unsigned integer every pointer that has to be passed to such
> a function. This applies also to particular case, like the passing of
> a precision field.
> 
> Example:
> size_t synlen = strlen(syn)
> my_snprintf(prefix, sizeof(prefix), "%*s", synlen, " ");
> 
> size_t as the POSIX standard ask, is a machine dependant type,
> with has a architecture variable dimension, in particular
> 
> for x86 architecture is defined as:
> typedef unsigned int size_t (thus 32 bit)
> 
> for alpha architecture is defined as:
> typedef unsigned long size_t (thus 64 bit)
> 
> This make the compiler issue a warning every time such a construct
> is encountered.

> There are 2 simple solution that I can see:
> 1) use unsigned int instead of size_t for variables that are
> related to the length of a string
> 2) explicitly cast the size_t variable to unsigned int in the
> printf function
> 
> Any strong opinion on how is the right (TM) way to resolve this issue?

1) is wrong since it propagates the problems which we have with the
printf into the whole code. So if you can post a patch for 2) or the
report the compiler warnings. 

> 2) When you use the XtPointer defined by Xt intrinsics, you definitely
> use a pointer, but in the present coding style of freeciv, many time
> it is necessary to convert such pointers to integers, and this is done
> by casting them to int.
> 
> This make the compiler warn of a conversion from pointer to integer
> of different size, because obviously pointers on alpha are 64 bit,
> and int are 32 bit.
> 
> Example:
> void races_toggles_callback(Widget w, XtPointer client_data,
>                             XtPointer call_data)
> {
>   int index,j,race;
>   ...
>   index = (int)client_data;
> 
> A simple solution (but I thing is not clean), is to convert
> such case to (long) instead of int, this make the trick both on
> 64 and 32 bit platforms. (But clearly doesn't work for 16 bit 
> platforms, like amiga or old 286,386 and the like)
> 
> Another possibility is to have them convert by a type machine
> dependant, like size_t or similar types, with I think is the
> cleanest solution

As Ben already said: this is similar to GTK. There are macros which
does the transformation. Now we have to decide weither there are such
macros in Xt or we have to build our own.

> 3) Again over the XtPointer, this time there is the inverted 
> conversion problem, from an integer to a pointer, there is the
> same mismatch.
> 
> Example:
> int what;
> ...
> XtAddCallback (button, XtNcallback, pillage_callback, (XtPointer)what);
> 
> Ad again I thing that what has to be defined with a machine dependant
> type.

Same as 2).

> 4) Another annoying warning present in many file was the "char
> subscript",
> related to the family of function like isalnum, isspace, toupper,
> tolower,
> isdigit. Alla of this are defined as: isspace(int c) and the c parameter
> has to be convertible to an unsigned char or be the macro EOF.
> 
> In the freeciv code, instead, it happens many time that signed char are
> feed to the funcion, in particular they are part of string, and so they
> are part of an array, this issue the warning, and 

> I think that is more a problem of gcc than of alpha architecture

I agree. From the C standard:

               #include <ctype.h>
               int isalnum(int c);

So if isalnum is a real function c would extended to an int and no
problems arise. So since these functions are implemented as macros (in
a way which give warnings) I would vote for wrappers which are now
real functions (and can be inlined later):

bool fc_isalnum(int c)
{
  return isalnum(c)!=0;
}

> , nontheless I'll ask your
> opinion on these "subscript" problem.
> 
> Example:
> static bool is_ok_opt_name_char(char c)
> {
>   return isalnum(c);
> }
> 
> Besides ignoring this type of warning, the only solution I foresee is 
> an explicit cast to int of the c parameter, because it is not possible
> to have the string defined as arrays of unsigned char. (We may also
> cast to unsigned char, but is is longer and the function definition
> ask for an integer)

> 5) There is a problem also with some asserts that eat a pointer as
> the argument of they evaluation. I remember that some time ago Raimar
> tried to convert a whole bunch of implicit pointer bool evaluation to
> explicit, then all of this was undone, for some good reason.
> Nontheless, I've spotted only 3 or 4 case in which alpha platform
> require to change the actual code, so perhaps this is not a problem
> for other reason. Feeding a pure pointer to an assert make the
> compile warn of the convertion from pointer to integer of different
> size (remember that int, the parameter of assert, in only 32 bit!).
> 
> Example:
> assert(pplayer);
> 
> Solution, make the boolean evaluation explicit: assert(pplayer!=NULL);

Ack.

> 6) The last problema I've found is a very unlike to happen one,
> my problem is related to configure and readline library, the Tru64
> Unix is somewhat non standard (or too standard! who knows?) and 
> when asked to build with the readline library, it doesn't work
> unless you use the termcap or the curses library.
> The configure of freeciv handle this problem quite correctly, but
> there is still a problem, the configure script search in the
> termlib, termcap, curses and ncurses (in this order!) for the
> presence of the initscr function, but when compiling readline need
> the tgetent family of function the the initscr so finding the
> support for initscr doesn't imply that the library support the
> tgetent family of function, this has an insidious side effect,
> because ncurses library do have support to initscr but do not
> have support for tgetent, and beeing the last tried, it is the
> one choose by configure, that fails to build for this reason.
> 
> I'm not a configure hacker, what I can say is that a solution can be
> the change of the order, if this don't hurt for other reason,
> a less stupid way of handling this can be to search for tgetent
> family function instead of initscr (or for both). I'm open
> to every suggestion on this point...

I will leave this for Per.

> I hope that this feature freeze time is a good moment to have the time
> to clean the code even from this warning for odd architecture (not so
> odd in the end, because Itanium is approaching...).

I agree.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  +#if defined(__alpha__) && defined(CONFIG_PCI)
  +       /*
  +        * The meaning of life, the universe, and everything. Plus
  +        * this makes the year come out right.
  +        */
  +       year -= 42;
  +#endif
    -- Patch for 1.3.2 (kernel/time.c) from Marcus Meissner