Merge pull request #9684 from opensourcerouting/printfrr-false-positive

*: `frr-format` with unmodified GCC

bfdd/dplane.c

@@ -157,8 +157,8 @@ static void bfd_dplane_debug_message(const struct bfddp_message *msg)
 	case ECHO_REPLY:
 	case ECHO_REQUEST:
 		zlog_debug("  [dp_time=%" PRIu64 " bfdd_time=%" PRIu64 "]",
-			   (uint64_t)be64toh(msg->data.echo.dp_time),
+			   be64toh(msg->data.echo.dp_time),
-			   (uint64_t)be64toh(msg->data.echo.bfdd_time));
+			   be64toh(msg->data.echo.bfdd_time));
 		break;
 
 	case DP_ADD_SESSION:
@@ -245,21 +245,18 @@ static void bfd_dplane_debug_message(const struct bfddp_message *msg)
 			" packets), "
 			"out %" PRIu64 " bytes (%" PRIu64 " packets)}]",
 			ntohl(msg->data.session_counters.lid),
-			(uint64_t)be64toh(
+			be64toh(msg->data.session_counters.control_input_bytes),
-				msg->data.session_counters.control_input_bytes),
+			be64toh(msg->data.session_counters
-			(uint64_t)be64toh(msg->data.session_counters
+				.control_input_packets),
-					   .control_input_packets),
+			be64toh(msg->data.session_counters
-			(uint64_t)be64toh(msg->data.session_counters
+				.control_output_bytes),
-					   .control_output_bytes),
+			be64toh(msg->data.session_counters
-			(uint64_t)be64toh(msg->data.session_counters
+				.control_output_packets),
-					   .control_output_packets),
+			be64toh(msg->data.session_counters.echo_input_bytes),
-			(uint64_t)be64toh(msg->data.session_counters.echo_input_bytes),
+			be64toh(msg->data.session_counters.echo_input_packets),
-			(uint64_t)be64toh(
+			be64toh(msg->data.session_counters.echo_output_bytes),
-				msg->data.session_counters.echo_input_packets),
+			be64toh(msg->data.session_counters
-			(uint64_t)be64toh(
+				.echo_output_packets));
-				msg->data.session_counters.echo_output_bytes),
-			(uint64_t)be64toh(msg->data.session_counters
-					   .echo_output_packets));
 		break;
 	}
 }

doc/developer/workflow.rst

@@ -1161,6 +1161,37 @@ but are no longer actively maintained. MemorySanitizer is not available in GCC.
    The different Sanitizers are mostly incompatible with each other.  Please
    refer to GCC/LLVM documentation for details.
 
+frr-format plugin
+   This is a GCC plugin provided with FRR that does extended type checks for
+   ``%pFX``-style printfrr extensions.  To use this plugin,
+
+   1. install GCC plugin development files, e.g.::
+
+         apt-get install gcc-10-plugin-dev
+
+   2. **before** running ``configure``, compile the plugin with::
+
+         make -C tools/gcc-plugins CXX=g++-10
+
+   (Edit the GCC version to what you're using, it should work for GCC 9 or
+   newer.)
+
+   After this, the plugin should be automatically picked up by ``configure``.
+   The plugin does not change very frequently, so you can keep it around across
+   work on different FRR branches.  After a ``git clean -x``, the ``make`` line
+   will need to be run again.  You can also add ``--with-frr-format`` to the
+   ``configure`` line to make sure the plugin is used, otherwise if something
+   is not set up correctly it might be silently ignored.
+
+   .. warning::
+
+      Do **not** enable this plugin for package/release builds.  It is intended
+      for developer/debug builds only.  Since it modifies the compiler, it may
+      cause silent corruption of the executable files.
+
+      Using the plugin also changes the string for ``PRI[udx]64`` from the
+      system value to ``%L[udx]`` (normally ``%ll[udx]`` or ``%l[udx]``.)
+
 Additionally, the FRR codebase is regularly scanned with Coverity.
 Unfortunately Coverity does not have the ability to handle scanning pull
 requests, but after code is merged it will send an email notifying project
@@ -1274,6 +1305,24 @@ may not be obvious in how to fix.  Here are some notes on specific warnings:
   (and varargs calling convention.)  This is a notable difference to C++, where
   the ``void`` is optional and an empty parameter list means no parameters.
 
+* ``"strict match required"`` from the frr-format plugin:  check if you are
+  using a cast in a printf parameter list.  The frr-format plugin cannot
+  access correct full type information for casts like
+  ``printfrr(..., (uint64_t)something, ...)`` and will print incorrect
+  warnings particularly if ``uint64_t``, ``size_t`` or ``ptrdiff_t`` are
+  involved.  The problem is *not* triggered with a variable or function return
+  value of the exact same type (without a cast).
+
+  Since these cases are very rare, community consensus is to just work around
+  the warning even though the code might be correct.  If you are running into
+  this, your options are:
+
+  1. try to avoid the cast altogether, maybe using a different printf format
+     specifier (e.g. ``%lu`` instead of ``%zu`` or ``PRIu64``).
+  2. fix the type(s) of the function/variable/struct member being printed
+  3. create a temporary variable with the value and print that without a cast
+     (this is the last resort and was not necessary anywhere so far.)
+
 
 .. _documentation:
 

lib/network.h

@@ -22,6 +22,13 @@
 #ifndef _ZEBRA_NETWORK_H
 #define _ZEBRA_NETWORK_H
 
+#ifdef HAVE_SYS_ENDIAN_H
+#include <sys/endian.h>
+#endif
+#ifdef HAVE_ENDIAN_H
+#include <endian.h>
+#endif
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -45,6 +52,35 @@ extern int set_cloexec(int fd);
 extern float htonf(float);
 extern float ntohf(float);
 
+/* force type for be64toh/htobe64 to be uint64_t, *without* a direct cast
+ *
+ * this is a workaround for false-positive printfrr warnings from FRR's
+ * frr-format GCC plugin that would be triggered from
+ * { printfrr("%"PRIu64, (uint64_t)be64toh(...)); }
+ *
+ * the key element here is that "(uint64_t)expr" causes the warning, while
+ * "({ uint64_t x = expr; x; })" does not.  (The cast is the trigger, a
+ * variable of the same type works correctly.)
+ */
+
+/* zap system definitions... */
+#ifdef be64toh
+#undef be64toh
+#endif
+#ifdef htobe64
+#undef htobe64
+#endif
+
+#if BYTE_ORDER == LITTLE_ENDIAN
+#define be64toh(x)	({ uint64_t r = __builtin_bswap64(x); r; })
+#define htobe64(x)	({ uint64_t r = __builtin_bswap64(x); r; })
+#elif BYTE_ORDER == BIG_ENDIAN
+#define be64toh(x)	({ uint64_t r = (x); r; })
+#define htobe64(x)	({ uint64_t r = (x); r; })
+#else
+#error nobody expects the endianish inquisition. check OS endian.h headers.
+#endif
+
 /**
  * Helper function that returns a random long value. The main purpose of
  * this function is to hide a `random()` call that gets flagged by coverity