Hidden warning during compilation #1728

lestofante · 2013-12-09T22:59:34Z

in brach 1.5.x avr-gcc is called passing argument -w, that hide all warning.
This is very bad as wanings help to find error in the code, especially if it is written by someone new to coding and the magic of pointers.

please, at least give a flag in preferencies to remove the evil -w flag.

thank you

matthijskooijman · 2013-12-10T08:42:24Z

Agreed, this has annoyed me for a while too. It seems that the core is not warning-free, so perhaps that's why the flag was introduced, but then at the least it should only be applied to the core and not the sketch and libraries. Even better, we should make the core warning free and compile everything without -w.

lestofante · 2013-12-10T10:40:36Z

we (italian community) are working in this direction, warning-free for
current-arduino AND current-released avr-gcc (3.4.3). I'll pull-request
when the work will be over AND tested

(warning: italian language ahead)
http://forum.arduino.cc/index.php?topic=203097.0

2013/12/10 Matthijs Kooijman notifications@github.com

Agreed, this has annoyed me for a while too. It seems that the core is not
warning-free, so perhaps that's why the flag was introduced, but then at
the least it should only be applied to the core and not the sketch and
libraries. Even better, we should make the core warning free and compile
everything without -w.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1728#issuecomment-30208005
.

Previously, pointer casting was used, but this resulted in strict-aliasing warnings: IPAddress.h: In member function ‘IPAddress::operator uint32_t() const’: IPAddress.h:46:61: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] operator uint32_t() const { return *((uint32_t*)_address); }; ^ IPAddress.h: In member function ‘bool IPAddress::operator==(const IPAddress&) const’: IPAddress.h:47:81: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); }; ^ IPAddress.h:47:114: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); }; Converting between unrelated types like this is commonly done using a union, which do not break the strict-aliasing rules. Using that union, inside IPAddress there is now an attribute _address.bytes for the raw byte arra, or _address.dword for the uint32_t version. Since we now have easy access to the uint32_t version, this also removes two memcpy invocations that can just become assignments. This patch does not change the generated code in any way, the compiler already optimized away the memcpy calls and the previous casts mean exactly the same. This is a different implementation of a part of arduino#1399 and it helps toward fixing arduino#1728.

In C++, true and false are language keywords, so there is no need to define them as macros. Including stdbool.h in C++ effectively changes nothing. In C, true, false and also the bool type are not available, but including stdbool.h will make them available. Using stdbool.h means that we get true, false and the bool type in whatever way the compiler thinks is best, which seems like a good idea to me. This also fixes the following compiler warnings if a .c file includes both stdbool.h and Arduino.h: warning: "true" redefined [enabled by default] #define true 0x1 warning: "false" redefined [enabled by default] #define false 0x0 This fixes arduino#1570 and helps toward fixing arduino#1728. This only changed the AVR core, the SAM core already doesn't define true and false (but doesn't include stdbool.h either).

peekNextDigit() returns an int, so it can return -1 in addition to all 256 possible bytes. By putting the result in a signe char, all bytes over 128 will be interpreted as "no bytes available". Furthermore, it seems that on SAM "char" is unsigned by default, causing the "if (c < 0)" line a bit further down to always be false. Using an int is more appropriate. A different fix for this issue was suggested in arduino#1399. This fix helps towards arduino#1728.

matthijskooijman · 2014-04-29T08:50:02Z

Now that #1877 is merged, most of the warnings in the core are fixed (except for some USB-related warnings), which brings us a step closer to allowing warnings to be enabled again. There still are plenty of warnings in the core libraries, I expect though.

As to enabling warnings, on the mailing list it was discussed to make this optional, and probably have a few levels of warnings. By defaults, either no warnings, or only selected warnings in the user sketch are enabled. Advanced users can increase the warning level to enable all warnings and show them for libraries and the core as well.

https://groups.google.com/a/arduino.cc/d/msg/developers/5iaId6nPzqA/ZLCPph6w0kgJ

If we take an opt-in approach to warnings for the core and libraries, we could even start to implement this now, even when not all warnings in the core and core libraries have been fixed yet.

mikaelpatel · 2014-04-29T14:29:50Z

I cannot stress this point enough. It is really important to put quality
first if Arduino is to be able to scale. Quality starts with simple
things such as;

Highest possible level of compiler warnings. Let the compiler help,
and remove all warnings - always!
Document at least the external interface to classes, and use a tool
(e.g. doxygen) to generate documentation
Adopt a coding standard
Add a test framework/harness
Structure device drivers (aka Southbound Interfaces). Have an
extension strategy

Without higher quality, more structure and easier library/driver
integration the newer Arduino boards will be a very expensive experience
for many of us. The SPI discussion is a very good example of this fact.

You have my vote for adding -Wall and -Wextra to the compiler options,

Mikael

On 04/29/2014 10:50 AM, Matthijs Kooijman wrote:

Now that #1877 #1877 is
merged, most of the warnings in the core are fixed (except for some
USB-related warnings), which brings us a step closer to allowing
warnings to be enabled again. There still are plenty of warnings in
the core libraries, I expect though.

As to enabling warnings, on the mailing list it was discussed to make
this optional, and probably have a few levels of warnings. By
defaults, either no warnings, or only selected warnings in the user
sketch are enabled. Advanced users can increase the warning level to
enable all warnings and show them for libraries and the core as well.

https://groups.google.com/a/arduino.cc/d/msg/developers/5iaId6nPzqA/ZLCPph6w0kgJ

If we take an opt-in approach to warnings for the core and libraries,
we could even start to implement this now, even when not all warnings
in the core and core libraries have been fixed yet.

—
Reply to this email directly or view it on GitHub
#1728 (comment).

tomfelker · 2015-03-16T05:34:04Z

I get that the Arduino libraries are not yet warning free, and you don't want to show errors to users. But the -w means I can't even turn them on for myself with #pragma diagnostic. This is incredibly frustrating.

It's been over a year. Perfect is the enemy of good - nobody seems to be implementing optional warnings. Just remove -w, so the warnings show up in verbose mode, and so that I can put

#pragma GCC diagnostic error "-Wall"

in my own code.

mikaelpatel · 2015-03-16T07:40:06Z

There is no excuse for poor software quality. The compiler warnings should be set at the highest possible level so that all potential problems are exposed. And then they should be corrected.

I cannot stress the importance of this.

Please introduce -Wall and -Wextra and then do the work. Suppressing and ignoring warnings is a very bad and unprofessional practice.

ffissore · 2015-03-16T07:55:09Z

@tomfelker and @mikaelpatel your concerns are clear and understandable. However we must keep in mind that Arduino is also and especially a learning tool. A warning caused by the core or some library and not by a sketch may scare a student.
Enabling warnings when verbose output is on may be an acceptable proposal. I'm going to write to the teachers mailing list

mikaelpatel · 2015-03-16T09:10:59Z

@ffissore
I fully agree with not having warnings originating from the source code for the Arduino core in any released/delivered code and that might confuse students. The idea is to have the highest possible warning level and then remove all warnings before release. The warning level should be maintained to help students write better code and get as much help as possible from the compiler.

I do understand that this initially is a lot of work to clean up all the warnings that are in libraries right now but after that phase the overall code quality will be so much better. I also believe that this will help students.

tomfelker · 2015-03-16T09:37:08Z

I completely understand that we don't want to have to explain why a particular type-punned pointer breaks strict anti-aliasing rules, or whatever, to newbies. I am not saying warnings should be visible by default. I just want, personally, to be able to see warnings in my own code, without having to locally build the Arduino IDE or install hacky wrappers around gcc (as StackOverflow suggests). And that's currently technically impossible because of -w. I think if you take that out, even if the output is still hidden, then it should be possible for me to just add pragmas to make warnings be errors in my own code, and set verbose mode on locally.

ffissore · 2015-03-16T09:45:26Z

The output is hidden if you keep verbose compilation output unchecked. Take a look at file > preferences. In order to enable all warnings you need neither to build the IDE nor to use hacky wrappers. Just edit file platform.txt and remove -w from compiler.c.flags and compiler.cpp.flags

mikaelpatel · 2015-03-16T09:50:26Z

@tomfelker
Here is a link to the platform.txt that I am using in Cosa; https://github.com/mikaelpatel/Cosa/blob/master/platform.txt

The latest addition to the options is -mcall-prologues which turns out to reduce foot print further.

Cheers!

tomfelker · 2015-03-16T10:16:24Z

Thanks, that's very helpful! Removing all references to -w in my platform.txt allowed my pragmas to work.

And with verbose mode off in the preferences, as per default, it actually has no user-visible student-confusing effect. So I'd be in favor of it being the default, but at least knowing where to set it is good enough for now.

ffissore · 2015-04-10T13:34:08Z

I've added a new preference "show all compiler warnings" that turns warnings on
Available as nightly builds in the next few hours

* upstream/master: (239 commits) Fix for issue arduino#292 Windows: build_pull_request needed to be upgraded as well Update revisions.txt Windows: JRE is chosen at build time via WINDOWS_BUNDLED_JVM property Update Tone.cpp Update revisions.txt Block discovery threads until packages is not null, otherwise boards discovered during startup will miss model name Also SerialDiscovery was affected by bug found at 40535df. Fixes arduino#2892 NetworkDiscovery was silently failing because packages werenìt ready yet. Fixes arduino#2837 Better preference for setting warnings level. See arduino@61592d7#commitcomment-10668365 SAM boards stop compiling due to way of handling params with spaces on different OSs. Fixed Update Tone.cpp Restored error messages. Got rid of MessageSyphon as ther were losing some error messages. Fixes arduino#2737 Windows: added listComPorts test case New preference: enable all compiler warnings, off by default. Fixes arduino#1728 and arduino#2415. Also affects arduino#2634 and arduino#2207 build.xml: spreading failonerror on all exec tasks, it's better to crash early Lib/Board Manager CRC check is now case insensitive. Fixes arduino#2953 License fix to audio library License fix Library Manager: better error message ...

lmihalkovic · 2015-12-10T23:55:50Z

@mikaelpatel

There is no excuse for poor software quality.

Except if you can hide it... Like in the case of the IDE itself. If the hardware/firmware had been as bad, this would never have taken off. Dont get me wrong, the achievement is great.. the quality is not.

Previously, pointer casting was used, but this resulted in strict-aliasing warnings: IPAddress.h: In member function ‘IPAddress::operator uint32_t() const’: IPAddress.h:46:61: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] operator uint32_t() const { return *((uint32_t*)_address); }; ^ IPAddress.h: In member function ‘bool IPAddress::operator==(const IPAddress&) const’: IPAddress.h:47:81: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); }; ^ IPAddress.h:47:114: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); }; Converting between unrelated types like this is commonly done using a union, which do not break the strict-aliasing rules. Using that union, inside IPAddress there is now an attribute _address.bytes for the raw byte arra, or _address.dword for the uint32_t version. Since we now have easy access to the uint32_t version, this also removes two memcpy invocations that can just become assignments. This patch does not change the generated code in any way, the compiler already optimized away the memcpy calls and the previous casts mean exactly the same. This is a different implementation of a part of arduino#1399 and it helps toward fixing arduino#1728.

peekNextDigit() returns an int, so it can return -1 in addition to all 256 possible bytes. By putting the result in a signe char, all bytes over 128 will be interpreted as "no bytes available". Furthermore, it seems that on SAM "char" is unsigned by default, causing the "if (c < 0)" line a bit further down to always be false. Using an int is more appropriate. A different fix for this issue was suggested in arduino#1399. This fix helps towards arduino#1728.

This comment was marked as off-topic.

Sign in to view

matthijskooijman mentioned this issue Feb 4, 2014

silently ignore bad #include bug #1846

Closed

matthijskooijman mentioned this issue Feb 18, 2014

IDE buries warnings where users can easily miss them #1215

Closed

This was referenced Feb 22, 2014

Missing header file is not reported at compile (or verify) time #1508

Closed

IDE does not report missing #include files any more [imported] #1057

Closed

ffissore added Uncategorized and removed Uncategorized labels Feb 27, 2014

matthijskooijman referenced this issue Apr 29, 2014

Enabling link-time optimization. See #660

b34c973

This was referenced Nov 3, 2014

Erratic INT Roll Over #2207

Closed

FYI #error and #warning on Nightly build 1.5.8 #2415

Closed

matthijskooijman mentioned this issue Feb 12, 2015

functions no longer return what subfunctions return to them #2634

Closed

matthijskooijman mentioned this issue Feb 19, 2015

Added multi address slave #2403

Open

ffissore self-assigned this Mar 16, 2015

ffissore closed this as completed in 61592d7 Apr 10, 2015

mikaelpatel mentioned this issue Nov 27, 2015

Enable "Compiler warnings" preference by default arduino/arduino-ide#1630

Open

3 tasks

ffissore modified the milestone: Release 1.6.7 Dec 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hidden warning during compilation #1728

Hidden warning during compilation #1728

lestofante commented Dec 9, 2013

matthijskooijman commented Dec 10, 2013

lestofante commented Dec 10, 2013

This comment was marked as off-topic.

matthijskooijman commented Apr 29, 2014

mikaelpatel commented Apr 29, 2014

tomfelker commented Mar 16, 2015

mikaelpatel commented Mar 16, 2015

ffissore commented Mar 16, 2015

mikaelpatel commented Mar 16, 2015

tomfelker commented Mar 16, 2015

ffissore commented Mar 16, 2015

mikaelpatel commented Mar 16, 2015

tomfelker commented Mar 16, 2015

ffissore commented Apr 10, 2015

lmihalkovic commented Dec 10, 2015

Hidden warning during compilation #1728

Hidden warning during compilation #1728

Comments

lestofante commented Dec 9, 2013

matthijskooijman commented Dec 10, 2013

lestofante commented Dec 10, 2013

This comment was marked as off-topic.

matthijskooijman commented Apr 29, 2014

mikaelpatel commented Apr 29, 2014

tomfelker commented Mar 16, 2015

mikaelpatel commented Mar 16, 2015

ffissore commented Mar 16, 2015

mikaelpatel commented Mar 16, 2015

tomfelker commented Mar 16, 2015

ffissore commented Mar 16, 2015

mikaelpatel commented Mar 16, 2015

tomfelker commented Mar 16, 2015

ffissore commented Apr 10, 2015

lmihalkovic commented Dec 10, 2015