#231 Should compile cleanly with warnings enabled.

Mike Sharov

I've tried compiling the source (current CVS version)
with gcc 4.0.0 and all warnings enabled with:

-Werror -Wall -W -Winline -Wpointer-arith
-Wno-cast-align -Wcast-qual -Wwrite-strings -Wshadow
-Wredundant-decls -Wconversion -Wsign-promo -Wsynth

Machine-specific flags were:
-Os -pipe -march=athlon-mp -mfpmath=sse
On a dual-proc Athlon MP, Linux 2.6.12

Thousands of warnings result due to problems in the
following areas:
1) const correctness. This is the most unpleasant one
to deal with; there are char all over the place that
are pointing to const strings. Functions take char

when they really only need const char*.
2) Unused function arguments. In old time C code it was
ok to leave the parameter in if it is not used. In C++,
the names for unused parameters should be omitted (or
commented out, as in the patch).
3) Uninitialized variables. At several points undefined
behaviour would result from some parameter values. This
is a serious problem and may lead to hard-to-find
crashes. On the other hand, some instances of this are
in what might be critical loops, so the changes should
be reviewed by whoever knows how they really work and
determine the actual impact of the changes.
4) Variable shadowing. There are many instances of
class members shadowed by parameters or locals with the
same name. The patch renames the parameters, but a
better solution would be to better name the member
variables; the common solution is to use m_ Hungarian
prefix on them.
5) Incomplete enum switches. C++ will warn if the
switch contains some enum values, but not all of them.
The patch fixes these by adding default: cases, which
should be reviewed for possible performance impact.
6) Anonymous structs. File-scoped variables should not
use anonymous types; the compiler thinks they may be
exported and warns that external users will not know
the type.
7) Unused static functions. These should be commented
out to avoid a warning.
8) Mixed-type comparisons. Signed-unsigned mismatches
can result in strange behaviour if the wrong conversion
is implicitly performed.

There was also some strange code in sdlmain.cpp, which
I think is a bug. On line 274 there is ~(CAN_8,CAN_16),
although it probably ought to be ~(CAN_8|CAN_16). Yes,
there was a warning about it.

The patch to fix all these things is attached. I've
tested it under X with Sid Meyer's Colonization. (on
the console SDL sets an unusable refresh rate and locks
up my LCD, although I can see the shell come up; this
is a configuration problem I haven't figured out yet)


  • Mike Sharov
    Mike Sharov

    Patch to enable clean compilation under gcc 4.0.0

  • Peter Veenstra
    Peter Veenstra

    Logged In: YES

    Must have been quite a work,
    At first glance it looks allright. Although I would have to
    talk with some other devs to see wether those changes in the
    cpu cores when it comes to ea handling are correct.

  • Mike Sharov
    Mike Sharov

    Logged In: YES

    I'm attaching an updated full diff to include two more
    missed default: cases in ints/int10_pal.cpp. All switches
    with enum values will need those.

    Regarding your question about EA handling; I assume you mean
    the change where I removed two calls to GetEArd in
    prefix_0f.h. GetEArd is defined as Bit32u*
    eard=lookupRMEAregd[rm]; in modrm.h and is a no-op if eard
    is not used. This is not necessarily true of GetEAa, which
    is a function call with an unused return value. If GetEAa
    doesn't actually do anything except return a value, then
    instead of replacing the call with the _NR variant (which
    simply omits the return value declaration), it can be simply
    removed. I didn't, since it looked like possible obscure
    magic to me.

  • Mike Sharov
    Mike Sharov

    Updated diff as of 7/19

  • Peter Veenstra
    Peter Veenstra

    Logged In: YES

    Sorry for getting back so late.
    But now that 0.65 is out we have time for this.

    c2woody and I will work together on getting (most of) this
    patch in the cvs.

    Some parts don't really fit in anymore and there are some
    things that MS virtual C wants changed (attr is a
    "protected" keyword I recall)

  • Mike Sharov
    Mike Sharov

    Logged In: YES

    That's great! I'll pull down the latest cvs and help. The
    problem is that I now have an Athlon 64, where dosbox has
    quite a few other problems, but I'll see what I can do.

  • Mike Sharov
    Mike Sharov

    Logged In: YES

    The attached update0409 allows clean compile with
    --enable-debug on my Athlon 64. I have not yet tackled the
    various "statement no effect" warnings in the cpu core that
    appear with debug off. That's pretty complicated, requiring
    looking at each function and figuring out if it has any side

    The changes are as described before with two additional issues:

    There were some definitions added to config.h.in, which is
    not in CVS because you apparently expect people to autogen
    after checkout. I have moved the definitions to dosbox.h.

    Also, there is a POSIX standard header stdint.h which
    defines fixed-size types. It is present on all compilers
    I've seen, so I have replaced the #ifdef guessing with the
    standard int8_t, int16_t, etc.

    x86_64 is picky about printf format strings; you can't print
    long values with %d - it must be %ld. So the patch contains
    all those tedious changes to all the logging format strings
    which print Bitu values (which by your #ifdef logic I take
    to be a ulong? ulong is supposed to always be equal in size
    to pointer. You can also use size_t and ssize_t, in which
    case the format strings would need to be %zu and %zd
    respectively) To facilitate finding these I have added the
    printf gcc attribute to all the relevant functions.

    There was some really strange new code that I was surprized
    compiled for you at all. render_scalars.cpp defines
    two-dimensional arrays as one-dimensional arrays. That
    should be invalid code under any compiler. Was I supposed to
    compile that? Or was that file included due to some
    configuration fluke on my checkout?

    I am unable to continue at present because SourceForge
    anonymous cvs has stopped working for some reason; I keep
    getting end of file from the server. I'll try again in a few
    days. It's strange, since developer access to my own
    projects is working fine. Anyway, take a look at the patch
    in the meantime. Those format string changes touch a lot of
    files and it might be a good idea to check them in separately.

  • Mike Sharov
    Mike Sharov

  • Peter Veenstra
    Peter Veenstra

    Logged In: YES

    What is wrong with having to run autogen.sh after checking
    out ? Without it you have no configure either.
    Some definitions could be moved to dosbox.h though

    The stdint.h: Are you certain that MSVC.net has this as well ?

    I'm not very happy with the fact that the Bitu reference in
    E_Exit must be replaced by a %ud. I will talk talk about
    with a few of the other devs.

    Bitu is intended as a for the platform optimized counter type.

    The 1 vs 2 dimensional arrays. Well arrays are always
    special. 2 dimensional arrays are in memory offcourse 1
    dimensional as well and I think a compiler treats them that
    way as well. (aren't a[x] always converted to *(a+x) )
    The warnings for that should be easy to fix though. a few
    brackets should do magic.

    As You might have noticed I commited parts of the patch.
    Sometimes a bit changed to correct for errors.
    (for example the warnings about the compare always being
    true in if (ret<0) E_Exit("dosbox machine close in
    pagefault) were to be fixed by modifying the type of ret. (a
    cpu core can return 0 on succes. -1 on exit and +X to
    indicate a callback should be called.)

  • Mike Sharov
    Mike Sharov

    Logged In: YES

    What is wrong with having to run autogen.sh after checking

    autogen.sh generates config.h.in, which is not present in
    cvs. Because the type definitions, like Bitu and Bits were
    added to config.h.in, they are not available and not created
    in a pure cvs checkout. You can either move the definitions
    to some file you store in cvs, as I have done in the patch,
    or simply check in config.h.in, which I assumed you do not
    want to do so you'll be able to keep using autogen.

    The stdint.h: Are you certain that MSVC.net has this as well ?

    Not having MSVC.net I am unable to say. It is a standard C99
    header though, and should be available everywhere. Borland
    C++ Builder 6 has it. In any case, it should be used if
    available. Using the standard is a good thing, IMO. autoconf
    can check for its existence, and you can provide a
    substitute if it isn't present.

    I'm not very happy with the fact that the Bitu reference
    in E_Exit must be replaced by a %ud.


    Bitu is intended as a for the platform optimized counter type.

    May I ask what benchmarks you have used to determine that a
    long is unsuitable for this role? I'll be happy to run them.
    On any 32bit machine long and int are the same thing, so
    only 64bit platforms care. Using 64bit values on a 64bit
    platforms ought to be the fastest possible operation. There
    are memory alignment issues; 8-grain access would be faster
    than 4-grain. Then there are all those extra registers; if
    you use a 32bit value, you're stuck with eax, ebx, ecx, and
    edx, while 64bit values can be placed into any of the 16
    general purpose registers on the Athlon 64. This alone
    should provide you with enormous speed benefits.

    The 1 vs 2 dimensional arrays. Well arrays are always
    special. 2 dimensional arrays are in memory offcourse 1
    dimensional as well and I think a compiler treats
    them that way as well.

    That is true, but you can not declare them that way. gcc can
    not parse it, and I'd be curious to know what compiler can.

  • Mike Sharov
    Mike Sharov

    Logged In: YES
    Originator: YES

    Is anyone still interested in this bug? I can redo my changes for the latest sources, if you need that.