Enable extra warnings for mpy-cross and unix port #6510

esmil · 2020-10-03T11:12:52Z

Here are the changes needed to enable -Wextra -Wno-unused-parameter for mpy-cross and the unix port.

Even if we don't want to enable these by default I think a lot the changes make sense regardless, so I've ordered the commits roughly by how much sense I think they make without actually enabling the warnings and added the warnings last. This way it should be possible to merge only up until a certain point.

Many of these changes are actually from my GD32VF103 port #5449 which unfortunately hasn't had much attention, so my hope is that by splitting the unrelated changes out in separate pull requests the port will be smaller and easier to merge.

py/emitinlinethumb.c

jimmo

Thanks @esmil -- this looks like a really worthwhile change.

py/emitinlinethumb.c

extmod/moductypes.c

py/lexer.c

stinos · 2020-10-19T07:20:05Z

one of the commits mentions 'On x86 chars are signed,' which leaves the reader wondering whether that's not the case for others, and if so whether it's a good idea to make changes which only apply to x86.
next one is titled 'add missing defval initializers', maybe reword the not-so-common 'defval'? And have the commit message explain why this is needed (code compiled and worked fine without it?)

esmil · 2020-10-19T08:04:29Z

That is indeed the case. On ARM, RISC-V and many other architectures chars are unsigned and you need signed char if you want it otherwise. But on x86 this is the other way around: chars are signed and you need unsigned char if you want them unsigned.
Given the fact that most of us work on x86 systems and would therefore use/test the unix port and mpy cross on x86 I definitely think it's worthwhile to fix this warning on x86. On other architectures this cast doesn't do any harm.

I'll try to reword the other commit message a bit.

stinos · 2020-10-19T18:20:30Z

py/mpconfig.h

@@ -1643,6 +1643,13 @@ typedef double mp_float_t;
 #endif
 #endif

+// Explicitly annotate switch case fall throughs
+#if defined(__GNUC__) && __GNUC__ >= 7


How about enabling -Wimplicit-fallthrough for clang as well? See http://clang.llvm.org/docs/AttributeReference.html#fallthrough

Yeah, I tried that in the first three attempts, but it turns out clang only does -Wimplicit-fallthrough for C++

Aha ok, wasn't clear to me.

No, me neither

esmil · 2020-10-19T18:33:06Z

@stinos Ok, I think I've addressed all comments so far. Please have a look again.

stinos · 2020-10-19T18:37:52Z

py/emitinlinethumb.c

@@ -108,7 +108,7 @@ STATIC mp_uint_t emit_inline_thumb_count_params(emit_inline_asm_t *emit, mp_uint
            return 0;
        }
        const char *p = qstr_str(MP_PARSE_NODE_LEAF_ARG(pn_params[i]));
-        if (!(strlen(p) == 2 && p[0] == 'r' && p[1] == '0' + i)) {
+        if (!(strlen(p) == 2 && p[0] == 'r' && (mp_uint_t)p[1] == '0' + i)) {


Just a question: C99 guarantees that '0' + i will be of type mp_uint?

Yes, as far as I understand when you have signed + unsigned then the signed value is promoted to unsigned, and then they're widened to the widest integer, which in this case is mp_uint_t (which should be wider than char).

Ugh, it's more complicated than that. First both sides are promoted to int if they're narrower or unsigned int if int can't represent all the values. Then if the widest operand is a signed type T and T is able to hold all values of the other type, the other type is promoted to T. Otherwise both sides are promoted to the unsigned type as wide as the widest of the two operands.

So here '0' is automatically converted to 'int', and then (assuming mp_uint_t is `unsigned int') both sides are promoted to 'unsigned int'. So the explanation is more complicated, but the result is the same ;)

stinos · 2020-10-19T18:40:43Z

Looks good to me. Maybe prefix the commit with signed char comparison fix with py, extmod:, that's the style usually used for other commits as well. Or if there are more then 2 or 3 components/directories involved just all: or ports: or something suitable. Have a look at the git log to get an idea.

extmod/re1.5/compilecode.c

dpgeorge · 2020-10-22T04:25:40Z

extmod/machine_i2c.c

-        { MP_QSTR_scl, MP_ARG_REQUIRED | MP_ARG_OBJ },
-        { MP_QSTR_sda, MP_ARG_REQUIRED | MP_ARG_OBJ },
+        { MP_QSTR_scl, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
+        { MP_QSTR_sda, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },


Since these are required args they don't need a default value. But I can see why the compiler complains, and it won't hurt to have defaults here (shouldn't add anything to code size). So this is OK!

Agreed. I think this pretty much reflects what is in the commit message. 👍

Yes indeed! I didn't read the commit message...

dpgeorge · 2020-10-22T04:26:50Z

lib/libm/erf_lgamma.c

@@ -23,6 +23,7 @@
 *
 */

+#include "py/mpconfig.h"


Hmm.. this is not great, the libm library shouldn't depend on this header. Is there another way around this? Maybe pass through a definition of MP_FALLTHROUGH in the makefile?

libm isn't included in in the scope of this PR (?) so this file could just as well be left untouched? Or as another alternative: excluded from being compiled with this particular warning enabled. Just thinking that it would be not too concistent and possibly confusing to have nearly all MP_XXX complier-specifics in mpconfig.h but not this one.

If possible my preference would be to not touch this file. But yet another alternative would be to define a separate fallthrough macro in libm.h, eg LIBM_FALLTHROUGH.

Agree. I guess I must have misunderstood @stinos request to replace all fallthrough comments without extra info with the macro.

Indded sorry if that wasn't clear, I meant 'all the ones you changed so far' :)

py/objexcept.c

The function scope_find_or_add_id used to take a scope_kind_t enum and save it in an uint8_t. Saving an enum in a uint8_t is fine, but everywhere this function is called it is not actually given a scope_kind_t but an anonymous enum instead. Let's give this enum a name and use that as the argument type. This doesn't change the generated code, but is a C type mismatch that unfortunately doesn't show up unless you enable -Wenum-conversion.

mp_emergency_exception_buf_size is signed, so let's make sure we compare it as such.

On x86 chars are signed, but we're comparing a char to '0' + unsigned int, which is promoted to an unsigned int. Let's promote the char to unsigned before doing the comparison to avoid weird corner cases.

When compiling with -Wextra which includes -Wmissing-field-initializers GCC will warn that the defval field of mp_arg_val_t is not initialized. This is just a warning as it is defined to be zero initialized, but since it is a union it makes sense to be explicit about which member we're going to use, so add the explicit initializers and get rid of the warning.

Like Clang, GCC warns about this file, but only with -Woverride-init which is enabled by -Wextra. Disable the warnings for this file just like we do for Clang to make -Wextra happy.

Newer GCC versions are able to warn about switch cases that fall through. This is usually a sign of a forgotten break statement, but in the few cases where a fall through is intended we annotate it with this macro to avoid the warning.

esmil · 2020-10-22T10:58:13Z

@dpgeorge Nice catch on the tabs. I would have thought uncrustify could catch this. I think I've addressed all your comments now.

dpgeorge · 2020-10-22T11:01:24Z

I would have thought uncrustify could catch this.

It doesn't touch all .c code... in particular none of the 3rd-party libraries like re1.5 (which actually have a better home in the lib/ directory).

dpgeorge · 2020-10-22T11:57:33Z

py/gc.c

@@ -298,7 +298,8 @@ STATIC void gc_sweep(void) {
                #if MICROPY_PY_GC_COLLECT_RETVAL
                MP_STATE_MEM(gc_collected)++;
                #endif
-            // fall through to free the head
+                // fall through to free the head
+                MP_FALLTHROUGH


A nice bonus: uncrustify now indents the comment to the correct level because it sees MP_FALLTHROUGH as a statement within this case.

dpgeorge · 2020-10-22T12:00:12Z

Thanks for making the commits distinct and writing good commit messages. It was merged as-is, without rebase.

Also helps with #6473 where I needed to disable some compiler warnings, which are now fixed here.

jimmo · 2020-11-25T12:43:08Z

@esmil Thanks for doing this - these extra warnings keep saving me (e.g. case fall-though tonight). :D

esmil mentioned this pull request Oct 3, 2020

Add GD32VF103 port #5449

Open

dlech reviewed Oct 3, 2020

View reviewed changes

py/emitinlinethumb.c Outdated Show resolved Hide resolved

jimmo reviewed Oct 19, 2020

View reviewed changes

py/emitinlinethumb.c Outdated Show resolved Hide resolved

stinos reviewed Oct 19, 2020

View reviewed changes

extmod/moductypes.c Outdated Show resolved Hide resolved

stinos reviewed Oct 19, 2020

View reviewed changes

py/lexer.c Outdated Show resolved Hide resolved

esmil force-pushed the wextra branch 5 times, most recently from 803a0bb to 6d9ecb7 Compare October 19, 2020 18:12

stinos reviewed Oct 19, 2020

View reviewed changes

esmil force-pushed the wextra branch from 6d9ecb7 to e832622 Compare October 19, 2020 18:47

esmil requested a review from jimmo October 20, 2020 13:18

dpgeorge reviewed Oct 22, 2020

View reviewed changes

esmil added 9 commits October 22, 2020 11:40

py/objexcept: Compare mp_emergency_exception_buf_size signed.

6d3aa16

mp_emergency_exception_buf_size is signed, so let's make sure we compare it as such.

py: Use unsigned comparison of chars.

fdd6fa3

On x86 chars are signed, but we're comparing a char to '0' + unsigned int, which is promoted to an unsigned int. Let's promote the char to unsigned before doing the comparison to avoid weird corner cases.

py/vmentrytable: Ignore GCC -Woverride-init.

f1f6ef7

Like Clang, GCC warns about this file, but only with -Woverride-init which is enabled by -Wextra. Disable the warnings for this file just like we do for Clang to make -Wextra happy.

extmod: Disable -Wmissing-field-initializers for lfs2.

dde3db2

mpy-cross: Enable more warnings.

bef4127

unix: Enable more warnings.

05f9568

esmil force-pushed the wextra branch from e832622 to 05f9568 Compare October 22, 2020 10:56

dpgeorge merged commit 05f9568 into micropython:master Oct 22, 2020

dpgeorge reviewed Oct 22, 2020

View reviewed changes

esmil deleted the wextra branch October 22, 2020 12:17

jimmo mentioned this pull request Oct 29, 2020

extmod/btstack/btstack.mk: Add -Wimplicit-fallthrough=0. #6578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable extra warnings for mpy-cross and unix port #6510

Enable extra warnings for mpy-cross and unix port #6510

esmil commented Oct 3, 2020 •

edited

Loading

jimmo left a comment

stinos commented Oct 19, 2020 •

edited

Loading

esmil commented Oct 19, 2020

stinos Oct 19, 2020

esmil Oct 19, 2020

stinos Oct 19, 2020

esmil Oct 19, 2020

esmil commented Oct 19, 2020

stinos Oct 19, 2020

esmil Oct 19, 2020

esmil Oct 19, 2020

stinos commented Oct 19, 2020

dpgeorge Oct 22, 2020

esmil Oct 22, 2020

dpgeorge Oct 22, 2020

dpgeorge Oct 22, 2020

stinos Oct 22, 2020

dpgeorge Oct 22, 2020

esmil Oct 22, 2020

stinos Oct 22, 2020

esmil commented Oct 22, 2020

dpgeorge commented Oct 22, 2020

dpgeorge Oct 22, 2020

dpgeorge commented Oct 22, 2020

jimmo commented Nov 25, 2020

Enable extra warnings for mpy-cross and unix port #6510

Enable extra warnings for mpy-cross and unix port #6510

Conversation

esmil commented Oct 3, 2020 • edited Loading

jimmo left a comment

Choose a reason for hiding this comment

stinos commented Oct 19, 2020 • edited Loading

esmil commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esmil commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinos commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esmil commented Oct 22, 2020

dpgeorge commented Oct 22, 2020

Choose a reason for hiding this comment

dpgeorge commented Oct 22, 2020

jimmo commented Nov 25, 2020

esmil commented Oct 3, 2020 •

edited

Loading

stinos commented Oct 19, 2020 •

edited

Loading