tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings #27401

pull martinus wants to merge 1 commits into bitcoin:master from martinus:2023-04-no-tracepoint-warnings changing 1 files +22 −13

martinus commented at 5:28 AM on April 3, 2023: contributor
Fixes #26916 by disabling the warning -Wgnu-zero-variadic-macro-arguments when clang is used as the compiler.

Also see the comments
- Proposed changes in the bug #26916 (comment)
- Proposed changes when moving to a variadic maro: #26593 (review)
DrahtBot commented at 5:28 AM on April 3, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK hebasto, fanquake

Concept ACK 0xB10C, ajtowns

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
hebasto commented at 9:02 AM on April 4, 2023: member

Maybe add a check to configure.ac whether ignoring of -Wgnu-zero-variadic-macro-arguments is really required?

Type	Reviewers
ACK	hebasto, fanquake
Concept ACK	0xB10C, ajtowns

tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings

Fixes #26916 by disabling the warning `-Wgnu-zero-variadic-macro-arguments` when clang is used as the compiler.

5197660e94

martinus force-pushed on Apr 4, 2023
martinus commented at 6:24 PM on April 4, 2023: contributor

Maybe add a check to configure.ac whether ignoring of -Wgnu-zero-variadic-macro-arguments is #26916 (comment) required?

Sounds good, but I have no idea how to do this with configure.ac... But it's also possible to do this in code level, I've now added a __cplusplus < 202002L so disabling the warning only happens when compiling below c++20
0xB10C commented at 8:22 AM on April 11, 2023: contributor

Concept ACK and thank you for looking into this!

I would include this in #26593 (e.g. with #26593 (review)) too. If #26916 is blocking something (e.g. switching to a newer clang) I think merging this PR here first should be preferred as #26593 is a bigger change that might take a bit longer.

hebasto commented at 3:09 PM on April 13, 2023: member

Maybe add a check to configure.ac whether ignoring of -Wgnu-zero-variadic-macro-arguments is #26916 (comment) required?

Sounds good, but I have no idea how to do this with configure.ac...

Something like in this branch:

--- a/configure.ac
+++ b/configure.ac
@@ -470,6 +470,19 @@ if test "$CXXFLAGS_overridden" = "no"; then
   if test "$suppress_external_warnings" != "yes" ; then
     AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy], [NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"], [], [$CXXFLAG_WERROR])
   fi
+
+  AX_CHECK_COMPILE_FLAG([-Wgnu-zero-variadic-macro-arguments],
+    [
+      TEMP_CXXFLAGS="$CXXFLAGS"
+      CXXFLAGS="-Wgnu-zero-variadic-macro-arguments $CXXFLAG_WERROR $CXXFLAGS"
+      AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
+          #define VARIADIC_MACRO(a, ...) f(__VA_ARGS__)
+          void f() {}
+          int main() { VARIADIC_MACRO(42); }
+        ]])], [],
+        [NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-gnu-zero-variadic-macro-arguments"])
+      CXXFLAGS="$TEMP_CXXFLAGS"
+    ], [], [$CXXFLAG_WERROR])
 fi
 
 dnl Don't allow extended (non-ASCII) symbols in identifiers. This is easier for code review.

hebasto approved
hebasto commented at 1:51 PM on June 14, 2023: member

ACK 5197660e947435e510ef3ef72be8be8dee3ffa41, I've reconsidered my comment and I think the current localized approach is optimal.
ajtowns commented at 9:10 AM on July 1, 2023: contributor

utACK

Manually setting that flag by overriding CXXFLAGS disables a whole bunch of debugging warnings, so handling it directly seems very helpful.

in src/util/trace.h:13 in 5197660e94

  21 | -#define TRACE9(context, event, a, b, c, d, e, f, g, h, i) DTRACE_PROBE9(context, event, a, b, c, d, e, f, g, h, i)
  22 | -#define TRACE10(context, event, a, b, c, d, e, f, g, h, i, j) DTRACE_PROBE10(context, event, a, b, c, d, e, f, g, h, i, j)
  23 | -#define TRACE11(context, event, a, b, c, d, e, f, g, h, i, j, k) DTRACE_PROBE11(context, event, a, b, c, d, e, f, g, h, i, j, k)
  24 | -#define TRACE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) DTRACE_PROBE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l)
  25 | +// Disabling this warning can be removed once we switch to C++20
  26 | +#if defined(__clang__) && __cplusplus < 202002L

fanquake commented at 12:38 PM on August 7, 2023:

Generally not a huge fan of (in code) compiler and c++ version specific work arounds, but I think this is use-specific enough, with a clear (future) time for removal.

fanquake approved
fanquake commented at 1:50 PM on August 7, 2023: member

ACK 5197660e947435e510ef3ef72be8be8dee3ffa41 - checked that this fixes the warnings under Clang.
fanquake merged this on Aug 7, 2023
fanquake closed this on Aug 7, 2023
martinus deleted the branch on Aug 7, 2023
sidhujag referenced this in commit 36f919d626 on Aug 9, 2023
bitcoin locked this on Aug 6, 2024

Contributors

Linked (view graph)

#23363 Discussion: Upgrading to C++20 #26593 tracing: Only prepare tracepoint arguments when actually tracing #26916 tracepoints: gnu-zero-variadic-macro-arguments warnings

tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings #27401

Reviews

Conflicts