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

[timotheecour]

• fix c compiler warnings silently ignored, giving wrong results/undefined behavior #11590
  other compilers besides clang can be fixed in followup PR's

After this PR, compiling the example from #11590 gives:

/tmp/nim/tmp.d02/t04377.nim.c:149:8: warning: implicit declaration of function 'mach_absolute_time' is invalid in C99 [-Wimplicit-function-declaration]
        T2_ = mach_absolute_time();
              ^

instead of silently accepting and giving undefined behavior without any warning

as a side benefit, since warnings (and errors) are now printed to stderr (instead of capturing sdout + stderr as it was before PR), clang will honor colors (since it's a tty) so we get colored clang errors/warnings.

Note: whether to treat errors as warnings

in a previous version of this PR I was passing -Werror + a minimal whitelist of warnings to ignore so that it would pass the test suite. This would be a more breaking change so should be done in subsequent PR if at all.
These were:

-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

note that after this PR, recompiling nim only generates 1 warning: -Wparentheses (but test running test suite may generate more), so we don't generate a ton of noise, but the benefit is we do now show warnings which could indicate a bug (as was case in #11590)

note:

appveyor failures seem unrelated, see https://ci.appveyor.com/project/Araq/nim/builds/25543607/job/lfumr9dh3qogtjpm/tests

[Araq]

I don't see the need, just compile with nim cpp, who knows if C's super broken type checking can be fixed by changing the warning settings.

[timotheecour]

nim cpp is just the same, before this PR there's no way to get warnings, even if user tweaks his config/nim.cfg or even if he passes --clang.options.always:-Weverything on cmd line:

# main.nim
include system/io
var s = new int
c_fprintf(stderr, "addr:%d\n", s.addr) # bug!
c_fprintf(stderr, "addr2:%s\n", s.addr) # bug!

nim cpp -r --clang.options.always:-Weverything main.nim
# no C++ warnings shown, just gives buggy results and undefined behavior

after this PR, C++ shows you the warning, so the bug can be fixed:

/Users/timothee/git_clone/nim/timn/build/nimcache/t0513.nim.cpp:331:43: warning: format specifies type 'int' but the argument has type 'NI **' (aka 'long long **') [-Wformat]
        int T1_ = fprintf(stderr, "addr:%d\012", &s_A9aLlIGg2b5079bdubLTCoyA);
                                        ~~       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/timothee/git_clone/nim/timn/build/nimcache/t0513.nim.cpp:333:44: warning: format specifies type 'char *' but the argument has type 'NI **' (aka 'long long **') [-Wformat]
        int T2_ = fprintf(stderr, "addr2:%s\012", &s_A9aLlIGg2b5079bdubLTCoyA);

(I also had to uncomment pragma GCC diagnostic ignored "-Wformat" in nimbase.h ; but note that uncommenting it before this PR would have no effect since warnings wouldn't be shown; as a side note some of the suppressed warnings in nimbase.h look suspicious)

Note that after this PR, recompiling nim itself generates a single warning:

/Users/timothee/.cache/nim/nim_r/sem.nim.cpp:15862:37: warning: & has lower precedence than ==; == will be evaluated first [-Wparentheses]
                                T116_ = (((*(*proto).typ).flags & ~ (*(*s).typ).flags ==0)&&((*(*proto).typ).flags != (*(*s).typ).flags));
                                                                ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/timothee/.cache/nim/nim_r/sem.nim.cpp:15862:37: note: place parentheses around the '==' expression to silence this warning
                                T116_ = (((*(*proto).typ).flags & ~ (*(*s).typ).flags ==0)&&((*(*proto).typ).flags != (*(*s).typ).flags));

not sure whether it's a harmless one, but the extra noise is minimal.
If needed for this PR, I can undo the change in config/nim.cfg so warnings won't be shown by default but at least user would be able to show them by passing --clang.options.always:-Wall or whatever they choose

[timotheecour]

ping. It turns out the above mentioned warning (warning: & has lower precedence than ==) was actually a very useful one, thanks to it I found and fixed a bug, see #13290
Can we please merge this?

[Araq]

No, I don't like warnings. I agree it does detect bugs, but why do Nim users need to run Nim in the "find C codegen bugs" mode all the time? Makes no sense to me.

[timotheecour]

because:

• it finds bugs in user code too, not just C codegen bugs; see example below; it's not theoretical, this helped me find bugs in my own code
• there's no opt-in way before this PR (there's no way to show warnings if compilation succeeds)
• it shouldn't show spurious warnings; nim would now compile without warnings under this PR now that fix bug discovered by #11591: proc <*[T](x, y: set[T]) was wrong #13290 was merged

include system/io
var s = new int
c_fprintf(stderr, "addr:%d\n", s.addr) # bug!
c_fprintf(stderr, "addr2:%s\n", s.addr) # bug!
c_fprintf(stderr, "addr2:%p\n", s.addr) # ok
c_fprintf(stderr, "addr2:%llu\n", cast[int](s.addr)) # ok

before PR: it prints garbage for 1st 2 lines (or even undefined behavior) and there's no way to show the warnings.
after PR: it shows warning: format specifies type 'int' but the argument has type 'NI **' (aka 'long long **') [-Wformat] etc

note

if for whatever reason it turns out those warnings become unbearable for some projects, I can send a follow-up PR that would make those warnings opt-in (or opt-out, alternatively), but right now there's no indication this is the case

[Araq]

Alright. But before merging I'll check it produces no warnings on my machines either.

[timotheecour]

did it end up producing any warning on your machines? I'm happy to add a flag to control whether to turn these off if needed.
This issue (of warnings being ignored and there being no way to unignore) was brought up again in https://forum.nim-lang.org/t/5901

[timotheecour]

PTAL; as alternative to --passc:-w, I've added in last commit a platform independent way to suppress backend compiler warnings if it's ever needed:

--warning:BackendWarning:off

so that --warnings:off would suppress these as well

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

[Araq]

This needs to be done differently, not with more flags and options and overrides and "let's replace -- with / for Visual C++".