*: `frr-format` with unmodified GCC

Since there's very few locations where the `frr-format` actually prints
false positive warnings, consensus seems to be to just work around the
false positives even if the code is correct.

In fact, there is only one pattern of false positives currently, in
`bfdd/dplane.c` which does `vty_out("%"PRIu64, (uint64_t)be64toh(...))`.
The workaround/fix for this is a replacement `be64toh` whose type is
always `uint64_t` regardless of what OS we're on, making the cast
unnecessary.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>

diff --git a/bfdd/dplane.c b/bfdd/dplane.c
index 4b7f9ba7ace568af64493e1b1b850a8744720ec4..9dee2a558952340410745726f6e837ed52042700 100644 (file)
--- a/bfdd/dplane.c
+++ b/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),
-                          (uint64_t)be64toh(msg->data.echo.bfdd_time));
+                          be64toh(msg->data.echo.dp_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(
-                               msg->data.session_counters.control_input_bytes),
-                       (uint64_t)be64toh(msg->data.session_counters
-                                          .control_input_packets),
-                       (uint64_t)be64toh(msg->data.session_counters
-                                          .control_output_bytes),
-                       (uint64_t)be64toh(msg->data.session_counters
-                                          .control_output_packets),
-                       (uint64_t)be64toh(msg->data.session_counters.echo_input_bytes),
-                       (uint64_t)be64toh(
-                               msg->data.session_counters.echo_input_packets),
-                       (uint64_t)be64toh(
-                               msg->data.session_counters.echo_output_bytes),
-                       (uint64_t)be64toh(msg->data.session_counters
-                                          .echo_output_packets));
+                       be64toh(msg->data.session_counters.control_input_bytes),
+                       be64toh(msg->data.session_counters
+                               .control_input_packets),
+                       be64toh(msg->data.session_counters
+                               .control_output_bytes),
+                       be64toh(msg->data.session_counters
+                               .control_output_packets),
+                       be64toh(msg->data.session_counters.echo_input_bytes),
+                       be64toh(msg->data.session_counters.echo_input_packets),
+                       be64toh(msg->data.session_counters.echo_output_bytes),
+                       be64toh(msg->data.session_counters
+                               .echo_output_packets));
                break;
        }
 }

diff --git a/doc/developer/workflow.rst b/doc/developer/workflow.rst
index 2ce5f5d1c8c9cd7d4ec82af4e415da2eb6f14cad..04a56587ce9421dd469c00aa233742039a9f7599 100644 (file)
--- a/doc/developer/workflow.rst
+++ b/doc/developer/workflow.rst
@@ -1151,6 +1151,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
@@ -1264,6 +1295,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:
 

diff --git a/lib/network.h b/lib/network.h
index 4a9666984fad3ad51c0e6942b6d9ee1ec4ecf5f1..10ed917572dbf5285554b64b69240b1a91c4f1d8 100644 (file)
--- a/lib/network.h
+++ b/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