[bugfix] fix #11590: c compiler warnings silently ignored, giving undefined behavior

changelog.md

@@ -44,6 +44,8 @@
 ## Compiler changes
 
 - Specific warnings can now be turned into errors via `--warningAsError[X]:on|off`.
+- new warning `--warning:backendWarning:on` to enable default backend warnings;
+  the defaults can be customized with additional flags eg: `--passc:-Wwritable-strings`
 - The `define` and `undef` pragmas have been de-deprecated.
 
 ## Tool changes

compiler/extccomp.nim

@@ -99,7 +99,7 @@ compiler nintendoSwitchGCC:
     optSize: " -Os ",
     compilerExe: "aarch64-none-elf-gcc",
     cppCompiler: "aarch64-none-elf-g++",
-    compileTmpl: "-w -MMD -MP -MF $dfile -c $options $include -o $objfile $file",
+    compileTmpl: "-MMD -MP -MF $dfile -c $options $include -o $objfile $file",
     buildGui: " -mwindows",
     buildDll: " -shared",
     buildLib: "aarch64-none-elf-gcc-ar rcs $libfile $objfiles",
@@ -471,8 +471,12 @@ proc addCompileOption*(conf: ConfigRef; option: string) =
 proc addLinkOptionCmd*(conf: ConfigRef; option: string) =
   addOpt(conf.linkOptionsCmd, option)
 
-proc addCompileOptionCmd*(conf: ConfigRef; option: string) =
-  conf.compileOptionsCmd.add(option)
+proc addCompileOptionCmd*(conf: ConfigRef; option: string, isRemove = false) =
+  if isRemove:
+    if (var index = conf.compileOptionsCmd.find(option); index >= 0):
+      conf.compileOptionsCmd.delete(index)
+  else:
+    conf.compileOptionsCmd.add(option)
 
 proc initVars*(conf: ConfigRef) =
   # we need to define the symbol here, because ``CC`` may have never been set!
@@ -543,6 +547,18 @@ proc getOptSize(conf: ConfigRef; c: TSystemCC): string =
   if result == "":
     result = CC[c].optSize    # use default settings from this file
 
+proc flagsToNative(flags: string, c: TSystemCC): string =
+  ## replaces ' -X' by ' /X' for VCC; works ok for some common flags.
+  if c == ccVcc: result = flags.replace(" -", " /")
+  else: result = flags
+
+proc getWarnings(conf: ConfigRef; c: TSystemCC): string =
+  result = getConfigVar(conf, c, ".options.warnings")
+  if not conf.hasWarn(warnBackendWarning):
+    result.add " -w".flagsToNative(c) # disable all warnings
+  else:
+    result.add " -DNIM_UNIGNORE_DEFAULT_BACKEND_WARNINGS".flagsToNative(c)
+
 proc noAbsolutePaths(conf: ConfigRef): bool {.inline.} =
   # We used to check current OS != specified OS, but this makes no sense
   # really: Cross compilation from Linux to Linux for example is entirely
@@ -552,8 +568,14 @@ proc noAbsolutePaths(conf: ConfigRef): bool {.inline.} =
 
 proc cFileSpecificOptions(conf: ConfigRef; nimname, fullNimFile: string): string =
   result = conf.compileOptions
-  addOpt(result, conf.cfileSpecificOptions.getOrDefault(fullNimFile))
 
+  block: # warnings
+    # must occur before `compileOptionsCmd` so user config/cmdline can override
+    let key = nimname & ".warnings"
+    if existsConfigVar(conf, key): addOpt(result, getConfigVar(conf, key))
+    else: addOpt(result, getWarnings(conf, conf.cCompiler))
+
+  addOpt(result, conf.cfileSpecificOptions.getOrDefault(fullNimFile))
   for option in conf.compileOptionsCmd:
     if strutils.find(result, option, 0) < 0:
       addOpt(result, option)
@@ -873,7 +895,8 @@ proc execCmdsInParallel(conf: ConfigRef; cmds: seq[string]; prettyCb: proc (idx:
           cmds[i])
   else:
     tryExceptOSErrorMessage(conf, "invocation of external compiler program failed."):
-      res = execProcesses(cmds, {poStdErrToStdOut, poUsePath, poParentStreams},
+      # keep writing errors/warnings to stderr
+      res = execProcesses(cmds, {poUsePath, poParentStreams},
                             conf.numberOfProcessors, prettyCb, afterRunEvent=runCb)
   if res != 0:
     if conf.numberOfProcessors <= 1:

compiler/lineinfos.nim

@@ -17,6 +17,7 @@ const
 
 type
   TMsgKind* = enum
+    # errors
     errUnknown, errInternal, errIllFormedAstX, errCannotOpenFile,
     errXExpected,
     errGridTableNotImplemented,
@@ -26,6 +27,7 @@ type
     errProveInit,
     errGenerated,
     errUser,
+    # warnings
     warnCannotOpenFile,
     warnOctalEscape, warnXIsNeverRead, warnXmightNotBeenInit,
     warnDeprecated, warnConfigDeprecated,
@@ -42,7 +44,9 @@ type
     warnProveInit, warnProveField, warnProveIndex,
     warnStaticIndexCheck, warnGcUnsafe, warnGcUnsafe2,
     warnUninit, warnGcMem, warnDestructor, warnLockLevel, warnResultShadowed,
-    warnInconsistentSpacing, warnCaseTransition, warnCycleCreated, warnUser,
+    warnInconsistentSpacing, warnCaseTransition, warnCycleCreated,
+    warnBackendWarning, warnUser,
+    # hints
     hintSuccess, hintSuccessX, hintCC,
     hintLineTooLong, hintXDeclaredButNotUsed,
     hintConvToBaseNotNeeded,
@@ -106,6 +110,7 @@ const
     warnInconsistentSpacing: "Number of spaces around '$#' is not consistent",
     warnCaseTransition: "Potential object case transition, instantiate new object instead",
     warnCycleCreated: "$1",
+    warnBackendWarning: "$1",
     warnUser: "$1",
     hintSuccess: "operation successful: $#",
     # keep in sync with `testament.isSuccess`
@@ -155,7 +160,7 @@ const
     "ProveInit", "ProveField", "ProveIndex",
     "IndexCheck", "GcUnsafe", "GcUnsafe2", "Uninit",
     "GcMem", "Destructor", "LockLevel", "ResultShadowed",
-    "Spacing", "CaseTransition", "CycleCreated", "User"]
+    "Spacing", "CaseTransition", "CycleCreated", "BackendWarning", "User"]
 
   HintsToStr* = [
     "Success", "SuccessX", "CC", "LineTooLong",
@@ -189,7 +194,7 @@ proc computeNotesVerbosity(): array[0..3, TNoteKinds] =
   result[3] = {low(TNoteKind)..high(TNoteKind)} - {}
   result[2] = result[3] - {hintStackTrace, warnUninit, hintExtendedContext}
   result[1] = result[2] - {warnProveField, warnProveIndex,
-    warnGcUnsafe, hintPath, hintDependency, hintCodeBegin, hintCodeEnd,
+    warnGcUnsafe, warnBackendWarning, hintPath, hintDependency, hintCodeBegin, hintCodeEnd,
     hintSource, hintGlobalVar, hintGCStats}
   result[0] = result[1] - {hintSuccessX, hintSuccess, hintConf,
     hintProcessing, hintPattern, hintExecuting, hintLinking, hintCC}

config/config.nims

@@ -3,3 +3,31 @@
 when defined(nimHasCppDefine):
   cppDefine "errno"
   cppDefine "unix"
+
+block: # warnings
+  #[
+  ## ignore typical warnings in Nim-generated files
+
+  ## reference:
+  https://clang.llvm.org/docs/DiagnosticsReference.html
+
+  ## useful warnings:
+    -Wunknown-warning-option
+    -Weverything
+
+  ## To pass the test suite, `-Werror` would require the following adjustments:
+  -Werror -Wno-error=parentheses -Wno-error=deprecated-declarations -Wno-error=int-to-void-pointer-cast -Wno-error=ignored-attributes -Wno-error=incompatible-pointer-types -Wno-error=compare-distinct-pointer-types -Wno-error=incompatible-library-redeclaration
+
+  ## suspicious warnings for which we should fix code instead of ignoring warning:
+  -Wno-tautological-constant-out-of-range-compare
+  -Wno-switch-bool: occurs for object variants with bool switch, eg btrees.Node
+  -Wno-incompatible-pointer-types-discards-qualifiers: will prevent 
+    `--passC:-Wwrite-strings` from giving warning for `char*s = "foo"
+    which can indicate a real bug, but cgen also generates a lot of these
+  ]#
+
+  let clangWarningsCommon = "-Wno-logical-op-parentheses -Wno-invalid-noreturn -Wno-tautological-constant-out-of-range-compare -Wno-switch-bool"
+  let clangWarningsC = clangWarningsCommon & " -Wno-incompatible-pointer-types-discards-qualifiers"
+  let clangWarningsCpp = clangWarningsCommon & " -Wno-writable-strings -Wno-invalid-offsetof"
+  switch("clang.options.warnings", clangWarningsC)
+  switch("clang.cpp.options.warnings", clangWarningsCpp)

config/nim.cfg

@@ -170,14 +170,14 @@ path="$lib/pure"
 @if macosx or freebsd:
   cc = clang
   tlsEmulation:on
-  gcc.options.always = "-w"
-  gcc.cpp.options.always = "-w -fpermissive"
+  gcc.options.always = ""
+  gcc.cpp.options.always = "-fpermissive"
 @elif windows:
-  gcc.options.always = "-w -mno-ms-bitfields"
-  gcc.cpp.options.always = "-w -fpermissive -mno-ms-bitfields"
+  gcc.options.always = "-mno-ms-bitfields"
+  gcc.cpp.options.always = "-fpermissive -mno-ms-bitfields"
 @else:
-  gcc.options.always = "-w"
-  gcc.cpp.options.always = "-w -fpermissive"
+  gcc.options.always = ""
+  gcc.cpp.options.always = "-fpermissive"
 @end
 
 # Configuration for Objective-C compiler:
@@ -235,14 +235,14 @@ gcc.cpp.options.debug = "-g3 -Og"
 
 # Configuration for the LLVM GCC compiler:
 llvm_gcc.options.debug = "-g"
-llvm_gcc.options.always = "-w"
+llvm_gcc.options.always = ""
 llvm_gcc.options.speed = "-O2"
 llvm_gcc.options.size = "-Os"
 
 # Configuration for the LLVM CLang compiler:
 clang.options.debug = "-g"
 clang.cpp.options.debug = "-g"
-clang.options.always = "-w"
+
 clang.options.speed = "-O3"
 clang.options.size = "-Os"
 
@@ -282,7 +282,7 @@ vcc.options.size = "/O1"
 vcc.cpp.options.size = "/O1"
 
 # Configuration for the Tiny C Compiler:
-tcc.options.always = "-w"
+tcc.options.always = ""
 
 # Configuration for the Genode toolchain
 @if genode:

lib/nimbase.h

@@ -49,6 +49,7 @@ __AVR__
 
 
 /* ------------ ignore typical warnings in Nim-generated files ------------- */
+#ifndef NIM_UNIGNORE_DEFAULT_BACKEND_WARNINGS
 #if defined(__GNUC__) || defined(__clang__)
 #  pragma GCC diagnostic ignored "-Wpragmas"
 #  pragma GCC diagnostic ignored "-Wwritable-strings"
@@ -73,6 +74,7 @@ __AVR__
 #  pragma warning(disable: 4310 4365 4456 4477 4514 4574 4611 4668 4702 4706)
 #  pragma warning(disable: 4710 4711 4774 4800 4809 4820 4996 4090 4297)
 #endif
+#endif
 /* ------------------------------------------------------------------------- */
 
 #if defined(__GNUC__)

tests/misc/mbackendwarnings.nim

@@ -0,0 +1,7 @@
+## tests cgen warnings, see trunner
+
+proc c_printf(frmt: cstring): cint {.importc: "printf", header: "<stdio.h>", varargs, discardable.}
+
+c_printf("warning_1: %d\n", 1.2) # -Wformat
+c_printf("a: %d, warning_2: %s\n", 1.cint) # -Wformat
+c_printf("a: %d, no_warning: %s\n", 1.cint, "foo".cstring) # no warning

tests/trunner.nim

@@ -7,8 +7,9 @@ discard """
 
 import std/[strformat,os,osproc,strutils]
 
+let mode = if existsEnv("NIM_COMPILE_TO_CPP"): "cpp" else: "c"
+
 proc runCmd(file, options = ""): auto =
-  let mode = if existsEnv("NIM_COMPILE_TO_CPP"): "cpp" else: "c"
   const nim = getCurrentCompilerExe()
   const testsDir = currentSourcePath().parentDir
   let fileabs = testsDir / file.unixToNativePath
@@ -24,18 +25,25 @@ proc testCodegenStaticAssert() =
   doAssert "sizeof(bool) == 2" in output
   doAssert exitCode != 0
 
+proc testBackendWarnings() =
+  when defined clang:
+    let (output, exitCode) = runCmd("misc/mbackendwarnings.nim", "-f --warning:backendWarning:on --stacktrace:off")
+    doAssert r"warning_1" in output, output
+    doAssert r"warning_2" in output, output
+    doAssert r"no_warning" notin output, output  # sanity check
+    doAssert exitCode == 0, output
+
 proc testCTFFI() =
   let (output, exitCode) = runCmd("vm/mevalffi.nim", "--experimental:compiletimeFFI")
   let expected = """
 hello world stderr
 hi stderr
-foo
-foo:100
-foo:101
-foo:102:103
-foo:102:103:104
-foo:0.03:asdf:103:105
-ret={s1:foobar s2:foobar age:25 pi:3.14}
+foo0
+foo1:101
+foo2:102:103
+foo3:102:103:104
+foo4:0.03:asdf:103:105
+foo5:{s1:foobar s2:foobar age:25 pi:3.14}
 """
   doAssert output == expected, output
   doAssert exitCode == 0
@@ -44,3 +52,4 @@ when defined(nimHasLibFFIEnabled):
   testCTFFI()
 else: # don't run twice the same test
   testCodegenStaticAssert()
+  testBackendWarnings()

tests/vm/mevalffi.nim

@@ -24,34 +24,32 @@ proc fun() =
     doAssert c_exp(x2) == c_exp(x)
 
   block: # c_printf
-    c_printf("foo\n")
-    c_printf("foo:%d\n", 100)
-    c_printf("foo:%d\n", 101.cint)
-    c_printf("foo:%d:%d\n", 102.cint, 103.cint)
+    c_printf("foo0\n")
+    c_printf("foo1:%d\n", 101.cint)
+    c_printf("foo2:%d:%d\n", 102.cint, 103.cint)
     let temp = 104.cint
-    c_printf("foo:%d:%d:%d\n", 102.cint, 103.cint, temp)
+    c_printf("foo3:%d:%d:%d\n", 102.cint, 103.cint, temp)
     var temp2 = 105.cint
-    c_printf("foo:%g:%s:%d:%d\n", 0.03, "asdf", 103.cint, temp2)
+    c_printf("foo4:%g:%s:%d:%d\n", 0.03, "asdf", 103.cint, temp2)
 
   block: # c_snprintf, c_malloc, c_free
     let n: uint = 50
     var buffer2: pointer = c_malloc(n)
     var s: cstring = "foobar"
     var age: cint = 25
     discard c_snprintf(buffer2, n, "s1:%s s2:%s age:%d pi:%g", s, s, age, 3.14)
-    c_printf("ret={%s}\n", buffer2)
+    c_printf("foo5:{%s}\n", buffer2)
     c_free(buffer2) # not sure it has an effect
 
-  block: # c_printf bug
-    var a = 123
+  block: # c_printf bug with `var` {.varargs.} params
+    var a = 123.cint
     var a2 = a.addr
-    #[
-    bug: different behavior between CT RT in this case:
-    at CT, shows foo2:a=123
-    at RT, shows foo2:a=<address as int>
-    ]#
-    if false:
-      c_printf("foo2:a=%d\n", a2)
+    when false:
+      # BUG: CT FFI currently does not handle this edge case correctly,
+      # passing `a2` as `cint` instead of as `ptr cint` for a {.varargs.} param:
+      # at CT, shows foo6:a2=0x7b
+      # at RT, shows foo2:a2=<address>
+      c_printf("foo6:a2=%p\n", a2)
 
 
 static: