clang llvm 2.9 compiler widelands crash

I expect more problems, especially in the Lua part (though jari's cool fixups might have solved all issues already). We should look into this, I feel CLANG is the future of C compiling.

I initially thought this had been posted in the forum, so my comment is probably a bit more off-topic than it could have been.

Anyways, I recently discovered that clang* has a static code analyzer built-in, which we might want to take advantage of. For those unfamiliar with static code analyzers, they will basically go through the source code and look for error patterns. This allows developers to notice and fix issues before they become serious bugs. I haven't tried it on widelands myself, but I think someone should at least take a look to see whether it reports any issues which should be fixed.

Some links I've been reading lately for more background information:
http://grep.be/blog/en/computer/code/static_analysis_with_clang (example of issue found by static code analysis)
http://altdevblogaday.com/2011/12/24/static-code-analysis/ (Carmack's thoughts on static code analysis. One highlight I especially liked was "This seems to imply that if you have a large enough codebase, any class of error that is syntactically legal probably exists there.")

*It also looks like Visual Studio includes one, so we might want to look at that for Windows as well.

Running static code analysis is a good idea, as are other forms of more testing. However, two things must be kept in mind:

1. Those things slow down builds, so I am against adding them to the default cmake run.

2. If they are not added to the default build, then in the long run they will almost never be done.

As a consequence, the proper way to implement this is as an automated build process on a server somewhere, that sends sufficiently annoying failure messages when somebody introduces a regression. It should probably also automatically send a summary report every two or four weeks, to remind everybody of long-standing problems.

Nicolai: I agree on both points.

It reminded me of the PPA with daily builds we have for Ubuntu [1], which I guess works as a continuous integration system building each revision. Since I don't think we should interfere with (read break :p) the existing one (which anyways builds using gcc), I guess we should attempt to set up an alternative one running clang/llvm with static code analysis. I have no idea how complicated this would be though.

The problem is of course whether anyone will look at these reports or not. Depending on the amount of issues, I think this might require some initial work in order to eliminate existing issues to keep new ones from drowning in the noise. We might want to consider creating a general bug report for errors found by the static analyser, attach new logs every couple of weeks (and comment notable changes).

[1]https://launchpad.net/~widelands-dev/+archive/widelands-daily

As for whether anyone will look at the reports: That's definitely an important issue.

I've seen the PPA, and I've seen that it occasionally generates some automatic e-mails. The problem with those e-mails is that they're impossible to understand without some knowledge of how the Launchpad PPA system works in the back-end. I don't have that knowledge, and so far I've just ignored those e-mail reports because of that. So to me, the take away of that is that those reports should be sufficiently self-explanatory to anyone who is able to contribute in the first place, otherwise they end up being useless and ignored.

Still, I think with a little bit of thought, it should be possible to set up a system that generates useful reports.

Just as a side note regarding static code analysis: we already have integrated a static code checker (CppCheck), and as nha writes, it slows down the build, so it is not active on default but can be activated with a specific cmake parameter (-DWL_EXTRAWARNINGS=on) along with a few altered compiler warning flags. But, also as nha writes, it is being forgotten/ignored long time...

By the way, the cppcheck people often change their command line parameters so the current settings done by widelands are not working with the current version of cppcheck. We could however enable *all* warnings, which creates lots of noise.

Regarding the topic making widelands clang compatible: +1 as it is most probably the future. However, I don't know if clang 2.9 is worth the effort? I'm not deep enough into that topic, but clang 3.0 is completely different (if I can believe what people write about it), so to be future safe, concentrating on clang 3.0 should be enough, I guess.

Nicolai: Take this with a pinch of salt, but I think the PPA process is something like this:
- pull in and install dependencies
- pull in latest WL source code
- compile WL
- clean up and remove packages
which naturally adds a couple of unrelated things to the build log.

Jens: ah thanks, I wasn't aware that it activated cppCheck. (And you also explained why it doesn't seem to do anything currently. )
I am not sure if all warnings should be enable by default, I would at least wait a while until the amount of noise goes down. (See bug 825957 for GCC-warnings, and bug 913369 for clang warnings.) I suspect some of the issues trigger warnings would also trigger in static code analysis so fixing warnings might reduce noise here also.

And now, a bit on-topic for once:
I am not sure what happens when wares reach a building, because a builder arrives first when I try. (Didn't that cause a problem for you, David?) Anyways, this is what happens when a builder reaches a constructions site when compiled with clang 3.0:
Program received signal SIGSEGV, Segmentation fault.
0x8b08ec83 in ?? ()
(gdb) bt
#0 0x8b08ec83 in ?? ()
#1 0xb7b6451c in ?? () from /usr/lib/libgcc_s.so.1
#2 0xb7b64a28 in _Unwind_Resume () from /usr/lib/libgcc_s.so.1
#3 0x0865f996 in Widelands::Worker::buildingwork_update (this=0xbfb0ef0, game=..., state=...) at /opt/widelands/src/logic/worker.cc:1727
#4 0x08614233 in Widelands::Bob::do_act (this=0xbfb0ef0, game=...) at /opt/widelands/src/logic/bob.cc:226
#5 0x086140d8 in Widelands::Bob::act (this=0xbfb0ef0, game=..., data=18) at /opt/widelands/src/logic/bob.cc:210
#6 0x086847b2 in Widelands::Cmd_Act::execute (this=0x10673900, game=...) at /opt/widelands/src/logic/instances.cc:107
#7 0x086fc909 in Widelands::Cmd_Queue::run_queue (this=0xbfffd0b4, interval=1230, game_time_var=@0xbfffcf58) at /opt/widelands/src/logic/cmd_queue.cc:133
#8 0x086b8781 in Widelands::Game::think (this=0xbfffcf28) at /opt/widelands/src/logic/game.cc:628
#9 0x0889a07d in Interactive_Base::think (this=0x9377e48) at /opt/widelands/src/wui/interactive_base.cc:333
#10 0x0886aa45 in Interactive_Player::think (this=0x9377e48) at /opt/widelands/src/wui/interactive_player.cc:256
#11 0x087d7652 in UI::Panel::do_think (this=0x9377e48) at /opt/widelands/src/ui_basic/panel.cc:570
#12 0x087d6f9f in UI::Panel::run (this=0x9377e48) at /opt/widelands/src/ui_basic/panel.cc:174
#13 0x086b67f4 in Widelands::Game::run (this=0xbfffcf28, loader_ui=0xbfffbd08, start_game_type=Widelands::Game::NewSPScenario) at /opt/widelands/src/logic/game.cc:582
#14 0x086b5808 in Widelands::Game::run_splayer_scenario_direct (this=0xbfffcf28, mapname=0x8fbe374 "campaigns/t01.wmf") at /opt/widelands/src/logic/game.cc:272
#15 0x084b137c in WLApplication::campaign_game (this=0x8cde3b8) at /opt/widelands/src/wlapplication.cc:2112
#16 0x084af196 in WLApplication::mainmenu_singleplayer (this=0x8cde3b8) at /opt/widelands/src/wlapplication.cc:1596
#17 0x084a9a4c in WLApplication::mainmenu (this=0x8cde3b8) at /opt/widelands/src/wlapplication.cc:1510
#18 0x084a8c02 in WLApplication::run (this=0x8cde3b8) at /opt/widelands/src/wlapplication.cc:441
#19 0x084a3d04 in main ...

How critical is this? I vote for postponing to build 18.

What's the unit of measurement? You can use clang to build gcc, if you need to...

You are a true philosopher, David.

Fwiw, FreeBSD has plans to use llvm rather than gcc for version 10. While the overlap between FreeBSD users and WL players might be small, it is packaged there...

I also stumbled across someone rebuilding all of Debian's archive using clang [1]. At the moment, this seems to be a "for fun/because I can" project though.

So I think this should be one of the highlights for build 18. We have the stacktraces, so someone should hopefully be able to look into it with a debugger and see where the difference lies...

[1] http://sylvestre.ledru.info/blog/sylvestre/2012/02/29/rebuild_of_the_debian_archive_with_clang

Since Revision 6319 cmake issues a warning if the compiler is identified as Clang.

The warning text is:

WARNING!!! You seem to compile widelands using the clang/llvm compiler infrastructure. Please note that we do not yet support compiling with clang. You are very likely to run into problems. We are working on it, though. In the mean time, please use GCC infrastructure to compile widelands. You can use the CMake parameter -DCMAKE_CXX_COMPILER=g++ to force GCC if you have it installed. Thank you for your understanding.

Fixed (hopefully!).

https://code.launchpad.net/~peter.waller/widelands/fix-clang-crashes/+merge/103991

Removed the clang warning as of bzr6380 as we don't see any major issues with clang anymore.

Released in build-18 rc1.