Hi all,
it's nothing to worry about but since 2011 I always noticed a clean compile run of your code. Now with gcc 7.1.0 there are some warnings and notes that might interest you:
~ LANG=C make CC bat-hosts.o CC debugfs.o CC debug.o CC functions.o CC genl.o CC hash.o CC icmp_helper.o CC interface.o CC ioctl.o CC main.o CC netlink.o CC ping.o CC sys.o In file included from sys.c:37:0: sys.c: In function 'handle_ra_setting': sys.h:33:25: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 185 [-Wformat-truncation=] #define SYS_IFACE_PATH "/sys/class/net" ^ sys.h:38:30: note: in expansion of macro 'SYS_IFACE_PATH' #define SYS_ROUTING_ALGO_FMT SYS_IFACE_PATH"/%s/mesh/routing_algo" ^~~~~~~~~~~~~~ sys.c:480:38: note: in expansion of macro 'SYS_ROUTING_ALGO_FMT' snprintf(path_buff, PATH_BUFF_LEN, SYS_ROUTING_ALGO_FMT, iface_dir->d_name); ^~~~~~~~~~~~~~~~~~~~ sys.h:38:46: note: format string is defined here #define SYS_ROUTING_ALGO_FMT SYS_IFACE_PATH"/%s/mesh/routing_algo" ^~ sys.c:480:3: note: 'snprintf' output between 34 and 289 bytes into a destination of size 200 snprintf(path_buff, PATH_BUFF_LEN, SYS_ROUTING_ALGO_FMT, iface_dir->d_name); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ CC tcpdump.o CC tp_meter.o tp_meter.c: In function 'tp_meter': tp_meter.c:502:3: warning: this statement may fall through [-Wimplicit-fallthrough=] printf("CANCEL received: test aborted\n"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tp_meter.c:504:2: note: here case BATADV_TP_REASON_COMPLETE: ^~~~ CC traceroute.o CC translate.o LD batctl
# batctl -v batctl 2017.1-1-g3069ca8 [batman-adv: 2017.1-4-g2149d80d]
It's really nothing bad at all. gcc 7.1.0 is somehow a little bit capricious but with more suggestions to make cleaner code.
best regards
Philipp
On Monday, May 29, 2017 7:09:09 PM CEST Philipp Psurek wrote:
Hi all,
it's nothing to worry about but since 2011 I always noticed a clean compile run of your code. Now with gcc 7.1.0 there are some warnings and notes that might interest you:
~ LANG=C make CC bat-hosts.o CC debugfs.o CC debug.o CC functions.o CC genl.o CC hash.o CC icmp_helper.o CC interface.o CC ioctl.o CC main.o CC netlink.o CC ping.o CC sys.o In file included from sys.c:37:0: sys.c: In function 'handle_ra_setting': sys.h:33:25: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 185 [-Wformat-truncation=] #define SYS_IFACE_PATH "/sys/class/net" ^ sys.h:38:30: note: in expansion of macro 'SYS_IFACE_PATH' #define SYS_ROUTING_ALGO_FMT SYS_IFACE_PATH"/%s/mesh/routing_algo" ^~~~~~~~~~~~~~ sys.c:480:38: note: in expansion of macro 'SYS_ROUTING_ALGO_FMT' snprintf(path_buff, PATH_BUFF_LEN, SYS_ROUTING_ALGO_FMT, iface_dir->d_name); ^~~~~~~~~~~~~~~~~~~~ sys.h:38:46: note: format string is defined here #define SYS_ROUTING_ALGO_FMT SYS_IFACE_PATH"/%s/mesh/routing_algo" ^~ sys.c:480:3: note: 'snprintf' output between 34 and 289 bytes into a destination of size 200 snprintf(path_buff, PATH_BUFF_LEN, SYS_ROUTING_ALGO_FMT, iface_dir->d_name); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ CC tcpdump.o CC tp_meter.o tp_meter.c: In function 'tp_meter': tp_meter.c:502:3: warning: this statement may fall through [-Wimplicit-fallthrough=] printf("CANCEL received: test aborted\n"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tp_meter.c:504:2: note: here case BATADV_TP_REASON_COMPLETE: ^~~~ CC traceroute.o CC translate.o LD batctl
# batctl -v batctl 2017.1-1-g3069ca8 [batman-adv: 2017.1-4-g2149d80d]
It's really nothing bad at all. gcc 7.1.0 is somehow a little bit capricious but with more suggestions to make cleaner code.
Hi Philipp,
thanks for showing this. I think none of that is really critical, and the fall through is actually intended.
Do you want to propose a patch for these things?
Cheers, Simon
Hi Simon
Am Donnerstag, den 01.06.2017, 10:34 +0200 schrieb Simon Wunderlich:
I think none of that is really critical, and the fall through is actually intended.
Yes, this isn't critical. This report was FYI about an exotic compiler.
Do you want to propose a patch for these things?
Give me some time to understand the code and I look what I can do in my spare time. This might take some time.
best regards
Philipp
__attribute__ ((fallthrough)) tells the compiler to not complain about an intended fallthrough.
diff --git a/tp_meter.c b/tp_meter.c index 918fb79..592a9ed 100644 --- a/tp_meter.c +++ b/tp_meter.c @@ -501,6 +501,7 @@ int tp_meter(char *mesh_iface, int argc, char **argv) case BATADV_TP_REASON_CANCEL: printf("CANCEL received: test aborted\n"); /* fall through and print the partial result */ + __attribute__ ((fallthrough)); case BATADV_TP_REASON_COMPLETE: if (result.test_time > 0) { throughput = result.total_bytes * 1000;
Well, now it's a little bit tricky to suppress compiler warnings as anything below 7.1.0 dislike “__attribute__ ((fallthrough));” with a warning. One could tell the 7.1.0 with “-Wimplicit-fallthrough=2” to suppress the warning because there is a “falls?[ \t-]*thr(ough|u)” comment, but other compiles drop an unrecognised command line option error.
This one is not possible:
diff --git a/tp_meter.c b/tp_meter.c index 592a9ed..c1587cd 100644 --- a/tp_meter.c +++ b/tp_meter.c @@ -501,7 +501,9 @@ int tp_meter(char *mesh_iface, int argc, char **argv) case BATADV_TP_REASON_CANCEL: printf("CANCEL received: test aborted\n"); /* fall through and print the partial result */ + #if (__STDC_VERSION__ == 201112L) + __attribute__ ((fallthrough)); + #endif case BATADV_TP_REASON_COMPLETE: if (result.test_time > 0) { throughput = result.total_bytes * 1000;
because batctl is compiled with “-std=gnu99” which is predefined in the makefile. And I don't know if you like such code.
What actually works is this hack in the makefile:
--- a/Makefile +++ b/Makefile @@ -101,6 +101,11 @@ MKDIR ?= mkdir -p COMPILE.c = $(Q_CC)$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c LINK.o = $(Q_LD)$(CC) $(CFLAGS) $(LDFLAGS) $(TARGET_ARCH) +# Check for GCC >=7 +ifeq ($(shell $(CC) -x c -E -P - <<< __STDC_VERSION__),201112L) +CFLAGS += -Wimplicit-fallthrough=2 +endif + # standard install paths PREFIX = /usr/local SBINDIR = $(PREFIX)/sbin
I suggest to modify the makefile with this patch and make comments on intended fallthroughs like you already did.
OK, sorry for the many posts ... there was some trouble recognizing the used compiler with the last patch. This now really do the work:
diff --git a/Makefile b/Makefile index 6bebb7d..7123d83 100755 --- a/Makefile +++ b/Makefile @@ -101,6 +101,11 @@ MKDIR ?= mkdir -p COMPILE.c = $(Q_CC)$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c LINK.o = $(Q_LD)$(CC) $(CFLAGS) $(LDFLAGS) $(TARGET_ARCH)
+# Check for GCC >=7 +ifeq ($(shell $(CC) -x c++ --std=c++17 -E -P - <<< __cplusplus),201703L) + CFLAGS += -Wimplicit-fallthrough=2 +endif + # standard install paths PREFIX = /usr/local SBINDIR = $(PREFIX)/sbin
'snprintf' output in sys.c can be between 34 and 289 bytes and should not write into a destination of size 200
snprintf(path_buff, PATH_BUFF_LEN, SYS_ROUTING_ALGO_FMT, iface_dir->d_name);
diff --git a/functions.c b/functions.c index 2239440..8337ee8 100644 --- a/functions.c +++ b/functions.c @@ -59,7 +59,7 @@ #include "debugfs.h" #include "netlink.h"
-#define PATH_BUFF_LEN 200 +#define PATH_BUFF_LEN 289
static struct timespec start_time; static char *host_name; diff --git a/functions.h b/functions.h index eca1406..c0a02a8 100644 --- a/functions.h +++ b/functions.h @@ -31,7 +31,7 @@ #define ETH_STR_LEN 17 #define BATMAN_ADV_TAG "batman-adv:"
-#define PATH_BUFF_LEN 200 +#define PATH_BUFF_LEN 289
/* return time delta from start to end in milliseconds */ void start_timer(void);
Hi Simon,
I assume my patches were not formatted according to the rules and this is the reason why there has been no comment on them. I hope the patches are now more satisfying.
best regards
Philipp
The output of
snprintf(path_buff, PATH_BUFF_LEN, SYS_ROUTING_ALGO_FMT, iface_dir->d_name)
in sys.c can be between 34 and 289 bytes and should not write into a destination of size 200.
Signed-off-by: Philipp Psurek philipp.psurek@gmail.com --- functions.c | 2 +- functions.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/functions.c b/functions.c index 2239440..8337ee8 100644 --- a/functions.c +++ b/functions.c @@ -59,7 +59,7 @@ #include "debugfs.h" #include "netlink.h"
-#define PATH_BUFF_LEN 200 +#define PATH_BUFF_LEN 289
static struct timespec start_time; static char *host_name; diff --git a/functions.h b/functions.h index eca1406..c0a02a8 100644 --- a/functions.h +++ b/functions.h @@ -31,7 +31,7 @@ #define ETH_STR_LEN 17 #define BATMAN_ADV_TAG "batman-adv:"
-#define PATH_BUFF_LEN 200 +#define PATH_BUFF_LEN 289
/* return time delta from start to end in milliseconds */ void start_timer(void);
On Tuesday, June 13, 2017 10:25:59 AM CEST Philipp Psurek wrote:
The output of
snprintf(path_buff, PATH_BUFF_LEN, SYS_ROUTING_ALGO_FMT, iface_dir->d_name)
in sys.c can be between 34 and 289 bytes and should not write into a destination of size 200.
Signed-off-by: Philipp Psurek philipp.psurek@gmail.com
I've picked this one, but increased the size to 400 to be a bit more future proof. :)
Thanks, Simon
GCC 7.1.0 complains about an intended fallthrough. “__attribute__ ((fallthrough))” in this part of code would suppress this warning. Because older GCC compiler don’t understand this statement attribute and because there is already a comment in the source containing “falls?[ \t-]*thr(ough|u)” we can suppress the warning with the “-Wimplicit-fallthrough=2” warning option. Unintended fallthroughs without a comment will trigger this warning.
Older GCC compiler don’t understand this warning option and exits with error. There has to be a compiler recognition after that only GCC =>7.1.0 receives this option.
Signed-off-by: Philipp Psurek philipp.psurek@gmail.com --- Makefile | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Makefile b/Makefile index 6bebb7d..29b7e2f 100755 --- a/Makefile +++ b/Makefile @@ -101,6 +101,11 @@ MKDIR ?= mkdir -p COMPILE.c = $(Q_CC)$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c LINK.o = $(Q_LD)$(CC) $(CFLAGS) $(LDFLAGS) $(TARGET_ARCH)
+# Check for GCC >=7 +ifeq ($(shell $(CC) -x c++ --std=c++17 -E -P - <<< __cplusplus),201703L) + CFLAGS += -Wimplicit-fallthrough=2 +endif + # standard install paths PREFIX = /usr/local SBINDIR = $(PREFIX)/sbin
On Tuesday, June 13, 2017 10:26:00 AM CEST Philipp Psurek wrote:
GCC 7.1.0 complains about an intended fallthrough. “__attribute__ ((fallthrough))” in this part of code would suppress this warning. Because older GCC compiler don’t understand this statement attribute and because there is already a comment in the source containing “falls?[ \t-]*thr(ough|u)” we can suppress the warning with the “-Wimplicit-fallthrough=2” warning option. Unintended fallthroughs without a comment will trigger this warning.
Older GCC compiler don’t understand this warning option and exits with error. There has to be a compiler recognition after that only GCC =>7.1.0 receives this option.
Signed-off-by: Philipp Psurek philipp.psurek@gmail.com
Makefile | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Makefile b/Makefile index 6bebb7d..29b7e2f 100755 --- a/Makefile +++ b/Makefile @@ -101,6 +101,11 @@ MKDIR ?= mkdir -p COMPILE.c = $(Q_CC)$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c LINK.o = $(Q_LD)$(CC) $(CFLAGS) $(LDFLAGS) $(TARGET_ARCH)
+# Check for GCC >=7 +ifeq ($(shell $(CC) -x c++ --std=c++17 -E -P - <<< __cplusplus),201703L)
CFLAGS += -Wimplicit-fallthrough=2+endif
Hmm, I don't like this one too much, since it looks cryptic and we will have to add up new versions, which I'd like to avoid.
Did you try to change the wording? I've googled a bit and it seems a comment containing the wording "fallthrough" will avoid it. Maybe we should just change "fall through" to "fallthrough"?
Thanks, Simon
Am Dienstag, den 13.06.2017, 10:48 +0200 schrieb Simon Wunderlich:
Hmm, I don't like this one too much, since it looks cryptic and we will have to add up new versions, which I'd like to avoid.
Did you try to change the wording? I've googled a bit and it seems a comment containing the wording "fallthrough" will avoid it. Maybe we should just change "fall through" to "fallthrough"?
I also read about the wording but for some reason I do not want to change the comment; because the many changes I tried were not recognized without a compiler warning option in conjunction with the complete comment. The now provided patch is recognized but the comment looks a little bit messed up. It works if you don't mind the formatting of the code.
Thanks for the opportunity to contribute something to B.A.T.M.A.N.-Advanced.
Best regards,
Philipp
GCC 7.1.0 complains about an intended fallthrough. “__attribute__ ((fallthrough))” in this part of code would suppress this warning. Because older GCC compiler don’t understand this statement attribute and because there is already a comment in the source containing “falls?[ \t-]*thr(ough|u)” we can suppress the warning with the “-Wimplicit-fallthrough=2” warning option. Unintended fallthroughs without a comment would trigger this warning again.
To avoid compiler recognition in the Makefile a simply change of the comment is sufficient to suppress the warning. For some reason only stand alone comments mentioned in [1] are recognized so the comment has to be split up into two parts.
[1] https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Warning-Options.html#index-Wimp...
Signed-off-by: Philipp Psurek philipp.psurek@gmail.com --- tp_meter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tp_meter.c b/tp_meter.c index 918fb79..bd7fdb4 100644 --- a/tp_meter.c +++ b/tp_meter.c @@ -500,7 +500,7 @@ int tp_meter(char *mesh_iface, int argc, char **argv) break; case BATADV_TP_REASON_CANCEL: printf("CANCEL received: test aborted\n"); - /* fall through and print the partial result */ + /* fallthrough *//* and print the partial result */ case BATADV_TP_REASON_COMPLETE: if (result.test_time > 0) { throughput = result.total_bytes * 1000;
Sorry for the many posts but this one works also; the comment has to be definitely split up into two parts if you do not want to append warning options to the compiler.
GCC 7.1.0 complains about an intended fallthrough. “__attribute__ ((fallthrough))” in this part of code would suppress this warning. Because older GCC compiler don’t understand this statement attribute and because there is already a comment in the source containing “falls?[ \t-]*thr(ough|u)” we can suppress the warning with the “-Wimplicit-fallthrough=2” warning option. Unintended fallthroughs without a comment would trigger this warning again.
To avoid compiler recognition in the Makefile a simply change of the comment is sufficient to suppress the warning. For some reason only stand alone comments mentioned in [1] are recognized so the comment has to be split up into two parts.
[1] https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Warning-Options.html#index-Wimp...
Signed-off-by: Philipp Psurek philipp.psurek@gmail.com --- tp_meter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tp_meter.c b/tp_meter.c index 918fb79..eb67ba1 100644 --- a/tp_meter.c +++ b/tp_meter.c @@ -500,7 +500,7 @@ int tp_meter(char *mesh_iface, int argc, char **argv) break; case BATADV_TP_REASON_CANCEL: printf("CANCEL received: test aborted\n"); - /* fall through and print the partial result */ + /* fall through *//* and print the partial result */ case BATADV_TP_REASON_COMPLETE: if (result.test_time > 0) { throughput = result.total_bytes * 1000;
On Tuesday, June 13, 2017 1:08:24 PM CEST Philipp Psurek wrote:
GCC 7.1.0 complains about an intended fallthrough. “__attribute__ ((fallthrough))” in this part of code would suppress this warning. Because older GCC compiler don’t understand this statement attribute and because there is already a comment in the source containing “falls?[ \t-]*thr(ough|u)” we can suppress the warning with the “-Wimplicit-fallthrough=2” warning option. Unintended fallthroughs without a comment would trigger this warning again.
To avoid compiler recognition in the Makefile a simply change of the comment is sufficient to suppress the warning. For some reason only stand alone comments mentioned in [1] are recognized so the comment has to be split up into two parts.
[1] https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Warning-Options.html#index-Wim plicit-fallthrough_003d
Signed-off-by: Philipp Psurek philipp.psurek@gmail.com
Thank you for your persistence and testing!
I've adopted your patch, but put the second part of the comment into the following case to avoid the ugliness. I didn't test with gcc 7.1 but hope that should work. Please check here:
https://git.open-mesh.org/batctl.git/blobdiff/ 620226bf8cff30e6dd966c8fe922b2d4cddf843b.. 50ee3c45feeda6d8c04ee127097badf99f78a26e:/tp_meter.c
Thank you! Simon
On Dienstag, 13. Juni 2017 13:46:04 CEST Simon Wunderlich wrote: [...]
I've adopted your patch, but put the second part of the comment into the following case to avoid the ugliness. I didn't test with gcc 7.1 but hope that should work. Please check here:
https://git.open-mesh.org/batctl.git/blobdiff/ 620226bf8cff30e6dd966c8fe922b2d4cddf843b.. 50ee3c45feeda6d8c04ee127097badf99f78a26e:/tp_meter.c
The other option would have been to change it to:
/* fall through - print the partial result */
At least the documentation allows non-escape chars after the "fall through" marker when a "-" was added between them. I've removed the "and" in this example because it looked weird.
Kind regards, Sven
On Dienstag, 13. Juni 2017 13:53:01 CEST Sven Eckelmann wrote:
At least the documentation allows non-escape chars after the "fall through"
Sry, I meant "non-newline"/"non-return" chars.
Kind regards, Sven
Am Dienstag, den 13.06.2017, 13:53 +0200 schrieb Sven Eckelmann:
The other option would have been to change it to: /* fall through - print the partial result */
Thank you Sven for pointing this out. This one works perfectly! Regex understanding 4TW ;-)
Thank you Simon for restyling the patch.
Batctl compiles now without any warnings with GCC 7.1.0. I’m surprised how far it has come: the compiler inspects comments now …
Thank you all for B.A.T.M.A.N.-Advanced
Best regards,
Philipp
Hi Philipp,
sorry for the late reply - I was out at battlemesh last week, so I didn't had time to check them. But yes, the format was lacking indeed, thanks for fixing that. :)
I'll check the patches and reply to them directly.
Thanks, Simon
On Tuesday, June 13, 2017 10:25:58 AM CEST Philipp Psurek wrote:
Hi Simon,
I assume my patches were not formatted according to the rules and this is the reason why there has been no comment on them. I hope the patches are now more satisfying.
best regards
Philipp
b.a.t.m.a.n@lists.open-mesh.org
-
Philipp Psurek -
Simon Wunderlich -
Sven Eckelmann