View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000635LDMud 3.3Compilation, Installationpublic2009-05-13 14:172009-05-28 07:46
Reporterzesstra Assigned Tozesstra  
PrioritynormalSeveritymajorReproducibilityN/A
Status resolvedResolutionfixed 
Platformx86_64OSMacOS XOS Version10.5.x
Product Version3.3.718 
Target Version3.3.719Fixed in Version3.3.719 
Summary0000635: Use -fwrapv by default if available and the compiler is gcc
DescriptionThe overflow of signed integers is undefined behaviour according to the C standard. While usually a wrap to -INT_MAX occurs and it is a fairly common assumption among C programmers (and often used), modern gcc (and maybe other compilers) may generate code with the assumption, that such a wrap-around does not happen. Unfortunately we have some (yet mostly unidentified) pieces of code which assumes the wrapping behaviour. This will silently behave different than intended and may cause who-knows-what problems.

Until we have had time to analyze our code with respect to this problem, I suggest to add the compiler switch -fwrapv to CFLAGS by default to enforce wrapping in case of signed integers. This will not help users of other compilers, but reduces the number of affected people at least.

The downside is: enabling this option disables some optimizations, most prominently concerning loops. ("When a loop uses a signed integer index, the compiler can do much better when it doesn’t have to consider the possibility that the index will overflow." from http://www.airs.com/blog/archives/120)
But actually, I don't see alternatives until we checked our code. Users could remove the -fwrapv if they are willing to take the risk.
TagsNo tags attached.
Attached Files
0003-Added-check-for-the-support-of-fwrapv.patch (1,261 bytes)    
From 13085ba07d93192fb34bc661d65901bd360d02b8 Mon Sep 17 00:00:00 2001
From: zesstra <zesstra@zesstra.de>
Date: Wed, 13 May 2009 23:31:47 +0200
Subject: [PATCH 3/3] Added check for the support of -fwrapv.

To enable a defined wrap-around behaviour for signed integers (currently
needed), we add -fwrapv to CFLAGS if supported by the compiler.

Signed-off-by: zesstra <zesstra@zesstra.de>
---
 src/autoconf/configure.in |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/src/autoconf/configure.in b/src/autoconf/configure.in
index e244240..a07100d 100644
--- a/src/autoconf/configure.in
+++ b/src/autoconf/configure.in
@@ -556,6 +556,20 @@ if test "${ac_cv_prog_cc_stdc}" = no; then
   AC_MSG_ERROR(You need an ANSI-C89 or ISO-C (C99) compiler! sorry..)
 fi
 
+# --- check for -fwrapv support ---
+savecflags="$CFLAGS"
+CFLAGS="${CFLAGS} -fwrapv"
+AC_MSG_CHECKING([check whether compiler supports -fwrapv...])
+AC_COMPILE_IFELSE(
+    [AC_LANG_PROGRAM(
+      [[]],
+      [[]])],
+    [AC_MSG_RESULT([ yes])]
+    EXTRA_CFLAGS="$EXTRA_CFLAGS -fwrapv",
+    [AC_MSG_RESULT([ no])]
+    CFLAGS="$saveflags"
+    )
+
 # does the compile have an inline keyword?
 AC_C_INLINE
 if test "x$ac_cv_c_inline" != "xno"; then
-- 
1.6.1
0003-Added-check-for-the-support-of-fwrapv.patch (1,261 bytes)   

Relationships

related to 0000642 new LDMud 3.5 Check code which assumes a defined overflow behaviour of signed integers 

Activities

zesstra

2009-05-13 14:34

administrator   ~0001104

Ah, BTW: does anybody know the simplest way to detect in autoconf if a compiler supports -fwrapv? ;-)
An alternative may be -fno-strict-overflow, but that is supported only by newer gcc. BTW: I don't exactly get the difference between the two until now.

zesstra

2009-05-13 15:39

administrator   ~0001105

I added an experimental patch for checking the support of -fwrapv in in the attached patch. Maybe we want to do some benchmarks for checking the impact, although I don't have a reasonable benchmark suite for LDMud. Does anybody have? But... Speed is less important than obscure bugs, crashes and exploits...

fufu

2009-05-14 07:11

manager   ~0001106

-fstrict-overflow affects both integer arithmetic (unless -fwrapv is set) and pointer arithmetic. (An instance of the latter is simplifying p + x > p to x > 0 if p is a pointer.)

-fwrapv makes integer overflows fully defined, even if -fstrict-overflow is set.

I think -fwrapv is what we want. The patch looks fine to me, except for this extra "check":

checking check whether compiler supports -fwrapv...... yes

zesstra

2009-05-18 14:30

administrator   ~0001114

Our configure checks for several compiler switches. I had the idea to use the same autoconf macro for all of them and re-wrote my first patch to include the following macro. It adds supported switches for all environment variables which are specified in a list. Unfortunately, I am not sure about the portability of the shell code:
dnl Check if the compiler in use supports the given argument as command-line
dnl switch and adds it to CFLAGS and EXTRA_CFLAGS if supported.
AC_DEFUN([LD_CHECK_CC_SWITCH], [
    ld_cc_switch_savecflags="$CFLAGS"
    CFLAGS="${CFLAGS} $1"
    AC_MSG_CHECKING([whether compiler supports $1])
    AC_COMPILE_IFELSE(
      [AC_LANG_PROGRAM(
        [[]],
        [[]])],
      [AC_MSG_RESULT([yes])]
      CFLAGS="$ld_cc_switch_savecflags"
      for v in $2; do
          todo="$v=\"`echo ${!v}` $1\"";
          eval "$todo";
      done,
      [AC_MSG_RESULT([no])]
      CFLAGS="$ld_cc_switch_savecflags"
    )
    ]
)

Could anybody comment? The other possibility would be to add a macro for every environment variable where compiler switches are added...

zesstra

2009-05-25 08:09

administrator   ~0001153

I will take care of including this switch into our configure. Additionally, I guess, this is a candidate for being back-ported to 3.2?

I am still unsure how to consolidate all compiler option checks into one autoconf macro, because I guess, the code in the for loop is not portable enough... On the other hand, we might postpone it... ;-)

zesstra

2009-05-28 07:46

administrator   ~0001171

-fwrapv was added to the default compiler flags in r2602, if supported by the compiler.

Issue History

Date Modified Username Field Change
2009-05-13 14:17 zesstra New Issue
2009-05-13 14:34 zesstra Note Added: 0001104
2009-05-13 15:33 zesstra File Added: 0001-Remove-changing-CFLAGS-before-float-branch-incompati.patch
2009-05-13 15:33 zesstra File Added: 0003-Added-check-for-the-support-of-fwrapv.patch
2009-05-13 15:39 zesstra Note Added: 0001105
2009-05-13 15:40 zesstra File Deleted: 0001-Remove-changing-CFLAGS-before-float-branch-incompati.patch
2009-05-14 07:11 fufu Note Added: 0001106
2009-05-18 14:30 zesstra Note Added: 0001114
2009-05-25 07:53 zesstra Project LDMud => LDMud 3.3
2009-05-25 08:09 zesstra Note Added: 0001153
2009-05-25 08:09 zesstra Assigned To => zesstra
2009-05-25 08:09 zesstra Status new => assigned
2009-05-26 08:01 zesstra Relationship added related to 0000642
2009-05-28 07:46 zesstra Note Added: 0001171
2009-05-28 07:46 zesstra Status assigned => resolved
2009-05-28 07:46 zesstra Fixed in Version => 3.3.719
2009-05-28 07:46 zesstra Resolution open => fixed