VM should build with -Wall, -Wextra, -Werror

[rmacnak]

Warnings typically mean one's program is relying on Undefined Behavior, and should be taken seriously to avoid bugs that are hard to track down. A C program that fails to compile with warnings-as-errors can't be considered production quality.

[nicolas-cellier-aka-nice]

+1 Compiler warnings are a useful tool that we should not ignore. They enable finding bugs. But there are too many false positive that we must eliminate. So we have to:

• either eliminate all warning as suggested
• or track regressions on the generated warnings (as generated in the LOGF files) as I suggested once (comment on https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/34) It's not as easy as a simple diff, because line number change etc... For example there is https://wiki.jenkins-ci.org/display/JENKINS/Warnings+Plugin.

The first alternative requires serious work on code generation. In both alternatives, the first step is to use -Wall -Wextra to at least give a chance to inspect/track those warnings.

[eliotmiranda]

While I agree with the sentiment, Slang will require significant work to avoid most unused variable warnings, and the source code will have to be carefully rewritten (or Slang made much more intelligent) to avoid all unused variable warnings. For example, in the Cogit, there are methods that have #if NewspeakVM sections in them that refer to variables only used in the NewspeakVM configuration (e.g. methods that scan inline caches that look for Newspeak-style send caches only in Newspeak VMs).

Perhaps an acceptable intermediate step is to compile with all warnings as errors except unused variables.

[clementbera]

What is the status of this @nicolas-cellier-aka-nice @eliotmiranda? I remember there was some work in that direction in the previous years?

[nicolas-cellier-aka-nice]

Just change the WARNINGS variable in build*/common/Makefile.flags, and inspect the generated LOGF, you'll see that we still have plenty of warnings. Due to inlining, the same warnings are duplicated many times, but still... Some are benign like a label was not used... Some more suspicious. Removing them means more work on C Code Generator.

I like to work with Xcode or VisualStudio or whatever IDE for navigating in generated code and for understanding the level of danger related to these warnings, unfortunately we don't maintain such Xcode/MSVC projects, nor are we able to generate them with cmake, we don't use cmake...

Concerning undefined behavior, we have removed some UB yet, but not sure if the work is finished or not. There are more warnings than UB (false alarms), and not all UB raise warnings (or it would be impossible to write the most simple signed arithmetic expression, every overflow condition is UB, and potentially, anything can overflow, unless sub-expressions can be bounded).

For UB, I would strongly advise to turn on the runtime checks of clang, at least in debug mode.