Support for supporting WERROR=1?

[pabigot]

Contiki's build environment nominally supports using WERROR=1 to turn on gcc -Werror checking. I find this feature helps reduce the number of bugs I introduce, but Contiki's source has several "errors" in core files that prevent its use.

Most of these are variables that are declared and either unused or set but never referenced. Often the reference is in code that is present but normally excluded from compilation (e.g., debug blocks).

Some are uses of functions without a prototype in scope. Others are violations of the edges of the C standard. A few appear to be wrong code that happens to work in most cases.

Is there support for patches to fix these issues? A current set of such changes can be viewed at https://github.com/pabigot/contikix/tree/features/werror. I don't want to spend the time refactoring and submitting a pull request if it's clear the approach would be rejected.

BTW: I am aware that -Wno-error=unused-variable could be used to reduce the number of changes. I don't support that approach, so if review prefers it somebody else can take over from here by cherry-picking any other changes that are judged worthwhile.

[karlp]

:+1: There's hundreds more if you have a compiler with strict-alias warnings turned on, every little bit helps!

[pabigot]

Most platforms use -fno-strict-aliasing since Contiki's use of cast pointer-to-struct is unsafe but fixing it would be too invasive (and possibly not even possible without introducing memory copies, though having solved this in other systems I'm unconvinced).

[karlp]

http://blog.regehr.org/archives/959

TL;DR: use memcpy, let the compilers do the work. (cue objections that the compilers for the 8051/6502 will never ever be good enough ;)

[oliverschmidt]

Hi,

Contiki's build environment nominally supports using WERROR=1 to turn on gcc -Werror checking.

Shortly after the Contiki-2.x CVS module was introduced in SF this option was discussed and when I remember correctly after some work a single warning was left. It was had to be removed so the option was not actually set - and soon after that the warnings started to spread all over the place again.

Is there support for patches to fix these issues?

Given the feedback here so far and the fact that the "old core team" was close to enforcing a warning-free build in "former times" I'd say it's save to presume: In general yes.

A current set of such changes can be viewed at https://github.com/pabigot/contikix/tree/features/werror.

I've looked into some commits and unfortunately the ratio of those I'm having a hard time to agree to is rather high. Two examples:

https://github.com/pabigot/contikix/commit/83342b3a1a6a8b5d3847055f05a7e09804d2c37e I consider it rather risky to make assumptions on how C library functions are actually declared in header files. In general system header files are a little more than just convenience over putting all necessary declarations into the source files. If your approach would be generally a good idea a lot of people (i.e. significant parts of auto-conf) would have been pretty stupid by investing into finding ways to include the appropriate headers.

https://github.com/pabigot/contikix/commit/bac28b7972d8e82c1ff5ba6dc60b8664be94207b Your change comment implies that you have fully understood the situation. So why don't you fix it by having the variable in question be only defined when necessary instead of that ugly workaround? Looking at the resulting code in a few years without your change comment at hand - and unfortunately without any source comment whatsoever from your side will give a hard time to understand what's going on.

I don't support that approach, so if review prefers it somebody else can take over from here by cherry-picking any other changes that are judged worthwhile.

The attitude you show with statements like this - and that you show in other topics - is not supported by me personally. I'm not willing to invest any of my time in discussions on pull request from someone with this attitude.

I don't want to spend the time refactoring and submitting a pull request if it's clear the approach would be rejected.

If I were the only merger for this repo the answer would be pretty simple ;-))

Oliver

[pabigot]

pabigot/contikix@83342b3 : I entirely agree. I don't see how to solve the issue cleanly, though, since Contiki doesn't support autoconf and the strcasecmp implementation it provides is never referenced. I considered hard-coding the prototype to what research suggested was its common form to be the lesser evil compared to disabling the warning and assuming that all functions without a prototype in scope were magically supplied in a compatible manner.

pabigot/contikix@bac28b7 : In that case, yes, it's easy to isolate the declaration and include it in the existing conditional block; most others are separated from their use and would add more conditionals to the code. Cleaning up this case would probably be part of any review I did before submitting a pull request, or would be a comment easy to accommodate. At this stage, I'm still just trying to eliminate gratuitous warnings with as little effort as possible so I can focus on what's important to me. If you review some other commits, pabigot/contikix@f62849e4d1 among others, you'll see there are some things that are almost certainly problems for which additional discussion enlightened by an experienced reviewer might have led to an improvement in Contiki's robustness.

As far as attitude there's not much I can say other than Contiki is an open source project so you'll get contributions from people who are willing to make them. My willingness is not unlimited: specifically, I won't spend my personal time solving a problem in a way I feel is inappropriate, which includes disabling checks for issues that have easy fixes, and I wanted to be up front about that. It's parallel to your unwillingness to accept changes you feel are inappropriate, a stance I accept since as a merger you share responsibility for the overall quality of Contiki.

As you are the only merger who has commented on any non-trivial non-architectural issue I've raised, and your feedback has overall shown an absence of encouragement, support, or cooperation toward a common goal of an improved Contiki, it's unlikely I'll continue my attempts to contribute.

Be patient: I'll probably be gone soon.

[malvira]

On Wed, Jun 19, 2013 at 5:32 PM, Peter A. Bigot notifications@github.comwrote:

As you are the only merger who has commented on any non-trivial non-architectural issue I've raised, and your feedback has overall shown an absence of encouragement, support, or cooperation toward a common goal of an improved Contiki, it's unlikely I'll continue my attempts to contribute.

We are listening.

There is also a detailed conversation going on about the radio-API which is taking people's attention.

In the past, Contiki was too loose with the merging so I wouldn't expect things to be rapidly merged without conversation (as we are trying to not fall into the old traps which caused damage).

-Mar.

[adamdunkels]

A warning-free build would be exceptionally well-received. In the past, one issue that was always holding back was lack of someone with enough insight into C compiler specifics to be able to maneuver the trickier details of C. We'd always stumble across some change that would compile nicely with (say) avr-gcc or cc65 but cause a warning in (say) mspgcc and nobody had enough guts to be able to correctly deal with it.

[pabigot]

@malvira @adamdunkels Thanks; I appreciate your more welcoming reaction.

A set of minimally-invasive patches to produce a warning-free build on all supported Contiki toolchains is well within my abilities, and "dealing correctly" with problems while balancing the needs of an existing community is fundamental to my development philosophy, as demonstrated by mspgcc. I too think a clean build is worth the effort if there's a reasonable chance that the work would be accepted.

Although the process and impact would not be nearly as traumatic as the coding style compliance questions I raised elsewhere, it's clear that pain would be involved. I have some familiarity with the history and goals of Contiki, but not enough to unilaterally produce a ready-to-merge solution. It's not possible to select the "best" of a range of alternative approaches without understanding community expectations.

If there are core Contiki developers who can find time to work with me to understand and meet those expectations I'd be happy to continue with this effort.

[oliverschmidt]

It's not possible to select the "best" of a range of alternative approaches without understanding community expectations.

sounds much more ready to compromise than

I don't support that approach, so if review prefers it somebody else can take over from here by cherry-picking any other changes that are judged worthwhile.

:-)

Be patient: I'll probably be gone soon.

Don't be silly. If that would be my goal I'd just ignore you instead of investing time into looking at your code and providing detailed feedback. I'm not exactly bored, you see?

Oliver

[pabigot]

I'm going to keep up with this as a side-line, with scope limited to producing a warning-free build with GNU-based toolchains when run on the existing regression framework. I've got the standard Travis toolchains installed locally along with cc65 and sdcc-r8721 so should be able to verify what I do doesn't break anybody else that uses those systems, but at this time have no plans to fix warnings generated only by non-GNU tools.

I'd like to go over the plan so whatever I ask you to review is more likely to pass. Sorry this gets long, but time spent now saves time later if these changes are to go in.

• When I encounter a problem I create a commit specific to that problem, even if there are multiple problems in a file. This keeps things clear in my mind, and permits later aggregation into squashed commits where appropriate.

  Do you want the commits to remain separate, or to be squashed by general category? My preference is squashing in categories discussed below.

  At least during the review phase if there's any question in my mind that a warning reveals a potential bug my plan is to keep that commit separate so the issue can be discussed during the reviews. An example would be a return code that is checked in one code path and not checked in another as in pabigot/contikix@edc78bd8a483ba690b78c022ed4e324116194643 .

So far I've identified the following aggregatable changes, along with how I propose to fix them. Please comment on any reservations as to categories and the default fix.

• Local variables declared but never set or used. These would be removed.
• Local variables declared and set but not used in any code path. These would be removed.

• Local variables declared and set but only used in diagnostic code paths. Two alternatives: (a) add (void)var; statements to shut the compiler up, or (b) place both the declaration and the set within conditional compilation directives with the same control as the diagnostic.

  My choice would be (a) for every case where (b) would require introducing new preprocessor directives.

• Local variables declared and set but only used in a code path that depends on a configuration parameter (e.g. UIP_IPV6). Same options as above; probably same approach too, except with more of a tendency to use (b) in borderline cases.

• There are a couple cases where context suggests an unreferenced variable may be inspected through a debugger (it is declared static within the function). Two alternatives: (a) remove the variable as in pabigot/contikix@6b1fb05f1021dee5b938faf51763027e3408de87, or (b) mark the declaration volatile to ensure it isn't eliminated, as well as adding a void statement to quiet the compiler as in pabigot/contikix@eefa8fb4bb37d807222deb9c275b01d555aaf8a5.

  My choice would be (a).

• File scope variables declared but only referenced in diagnostics or code paths that depend on a configuration parameter will be enclosed in preprocessor directives matching whatever condition enables the use.

• printf(2) format statements where the specifier does not match the value, e.g. "%u" when the value is a long. Several alternatives: (a) "up-cast" the specifier or the value to long so they match; (b) "down-cast" the value to match "%u" if context suggests it would not exceed the platform's int representation (e.g. short-duration intervals); (c) introduce CONTIKI_PRclock_time defines parallel to PRuint16 from inttypes.h.

  My choice would be (a) on the argument that if the application is generating formatted text any space or time penalty incurred by the runtime upcast is irrelevant.

• Improper use of pointers where the underlying type changes, e.g. char* versus unsigned char * versus uint8_t * versus signed char *.

  Solution here would be to add explicit casts to the destination type, except where the destination type is arguably incorrect (e.g. using unqualified char where either uint8_t or unsigned char is more appropriate).

  FWIW, my tendency would be to use uint8_t instead of unsigned char in almost every instance where the data is not clearly text. I believe this is consistent with current coding style; please let me know if I'm mistaken.

There may be others as time goes on, and the existing patches have not yet been updated to match the proposal above.

Comments?

[oliverschmidt]

Do you want the commits to remain separate, or to be squashed by general category?

I personally didn't have issues so far with "too many commits" but certainly had with "too many things put together".

Local variables declared and set but only used in diagnostic code paths. Two alternatives: (a) add (void)var; statements to shut the compiler up, or (b) place both the declaration and the set within conditional compilation directives with the same control as the diagnostic. My choice would be (a) for every case where (b) would require introducing new preprocessor directives.

ACK.

There are a couple cases where context suggests an unreferenced variable may be inspected through a debugger (it is declared static within the function). Two alternatives: (a) remove the variable as in pabigot/contikix@6b1fb05, or (b) mark the declaration volatile to ensure it isn't eliminated, as well as adding a void statement to quiet the compiler as in pabigot/contikix@eefa8fb. My choice would be (a).

ACK.

printf(2) format statements where the specifier does not match the value, e.g. "%u" when the value is a long. Several alternatives: (a) "up-cast" the specifier or the value to long so they match; (b) "down-cast" the value to match "%u" if context suggests it would not exceed the platform's int representation (e.g. short-duration intervals); (c) introduce CONTIKI_PRclock_time defines parallel to PRuint16 from inttypes.h. My choice would be (a) on the argument that if the application is generating formatted text any space or time penalty incurred by the runtime upcast is irrelevant.

ACK.

FWIW, my tendency would be to use uint8_t instead of unsigned char in almost every instance where the data is not clearly text. I believe this is consistent with current coding style; please let me know if I'm mistaken.

ACK.

I've got the standard Travis toolchains installed locally along with cc65 and sdcc-r8721 so should be able to verify what I do doesn't break anybody else that uses those systems, but at this time have no plans to fix warnings generated only by non-GNU tools.

I appreciate that you're going to think out of the box of the GNU world. There are not few not even knowing what constructs are GCC-proprietary until they fail on other compilers.

Regarding cc65 I presume you went for cc65 2.13.3 from cc65.org. The Contiki Makefile for the relevant targets (apple2enh, atari, c64 and c128) are however by now incompatible with this old, not-maintained-anymore release. Rather they require the HEAD of oliverschmidt.github.io/cc65. Please note that the target atarixl is currently highly experimental and therefore does not need to be considered by you in any way.

You didn't mention MS VC++ in your list above. It is the primary tool to build the target win32: https://github.com/contiki-os/contiki/blob/master/platform/win32/README-VC.md.

Oliver

[pabigot]

I personally didn't have issues so far with "too many commits" but certainly had with "too many things put together".

I didn't get the "too many things put together" concern from your previous comments; can you expand on that? Is your preference is to keep these commits separate? Anybody feel strongly the other way? There are currently 30 commits and there will be more; my thought was when they could be aggregated the commit message would provide the general description in detail that would be tedious repeated in a dozen single-change commits.

Regarding cc65 I presume you went for cc65 2.13.3 from cc65.org.

In fact no, I'm using your cc65 at master as of early this morning. Research looking for a git repository showed the old site was no longer maintained and your repo seems to be the replacement. I did use one of the atari platforms to confirm the install worked for building a file, but certainly haven't done anything more with it and at most plan to run the compile regression test to make sure nothing broke.

You didn't mention MS VC++ in your list above.

No, because I don't normally work in the Windows environment unless a client requires it, and don't have VC++. Similarly, I don't have or intend to test IAR Embedded Workbench. To the degree those platforms are important (probably pretty high) I'd hope we could find somebody who is familiar with those tools to try the branch before it gets merged.

[oliverschmidt]

I didn't get the "too many things put together" concern from your previous comments; can you expand on that? Is your preference is to keep these commits separate? Anybody feel strongly the other way? There are currently 30 commits and there will be more; my thought was when they could be aggregated the commit message would provide the general description in detail that would be tedious repeated in a dozen single-change commits.

I agree in general. If you get that "category thingy" right then it's of course preferable for everybody. However I've been involved in more than one Contiki pull request asking to be separated into several branches. And I personally won't complain to have to deal with a higher number - that's all I wanted to say.

In fact no, I'm using your cc65 at master as of early this morning. Research looking for a git repository showed the old site was no longer maintained and your repo seems to be the replacement.

Okay, so you did your homework ;-) This transition isn't that visible so far...

I did use one of the atari platforms to confirm the install worked for building a file, but certainly haven't done anything more with it and at most plan to run the compile regression test to make sure nothing broke.

Yep. I certainly don't expect anything more than checking for build errors on a single program, i.e. examples/webbrowser.

To the degree those platforms are important (probably pretty high) I'd hope we could find somebody who is familiar with those tools to try the branch before it gets merged.

Regarding MS VC++ I will do. BTW: The importance for MS VC++ comes from being able to use the MS VS debugger for Contiki.

Oliver

[simonduq]

Contiki is now warning-free (on most platforms) with https://github.com/contiki-os/contiki/pull/1293 and https://github.com/contiki-os/contiki/pull/1364, closing this issue!