compiling with gcc 7.1.0: there are some notes and warnings

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

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:

Yes, this isn't critical. This report was FYI about an exotic compiler.

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.

index 918fb79..592a9ed 100644
@@ -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:

index 592a9ed..c1587cd 100644
@@ -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:

@@ -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:

index 6bebb7d..7123d83 100755
@@ -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);

index 2239440..8337ee8 100644
@@ -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;

index eca1406..c0a02a8 100644
@@ -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.

---
 functions.c | 2 +-
 functions.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 2239440..8337ee8 100644
@@ -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;

index eca1406..c0a02a8 100644
@@ -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);

2.13.0

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.

---
 Makefile | 5 +++++
 1 file changed, 5 insertions(+)

index 6bebb7d..29b7e2f 100755
@@ -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

2.13.0

Am Dienstag, den 13.06.2017, 10:48 +0200 schrieb Simon Wunderlich:

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-Wimplicit-fallthrough_003d

---
 tp_meter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 918fb79..bd7fdb4 100644
@@ -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;

2.13.0

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-Wimplicit-fallthrough_003d

---
 tp_meter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 918fb79..eb67ba1 100644
@@ -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;

2.13.0

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:
[...]

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

Sry, I meant "non-newline"/"non-return" chars.

Kind regards,
Sven

Am Dienstag, den 13.06.2017, 13:53 +0200 schrieb Sven Eckelmann:

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