In-kernel build error: `error: macro "__assign_str" passed 2 arguments, but takes just 1`

[robn]

System information

Type	Version/Name
Distribution Name	N/A
Distribution Version	N/A
Kernel Version	Linux 6.10+
Architecture	all?
OpenZFS Version	all

Describe the problem you're observing

Build fails against kernel 6.10+ (in-kernel and "GPL hack" builds):

In file included from ./include/trace/trace_events.h:419,
                 from ./include/trace/define_trace.h:102,
                 from /home/robn/code/zfs/include/os/linux/zfs/sys/trace_dbgmsg.h:80,
                 from /home/robn/code/zfs/module/os/linux/zfs/trace.c:44:
/home/robn/code/zfs/include/os/linux/zfs/sys/trace_dbgmsg.h:65:1: error: macro "__assign_str" passed 2 arguments, but takes just 1
   65 | );
      | ^~

__assign_str in 6.10 dropped second argument in torvalds/linux@2c92ca849fcc.

Most of the (probable) fix is robn/zfs@ee94ce9c8, but it needs configure support. I messed with it last night, but it didn't look like a simple check, because of the whole tracepoint sequencing thing. Probably lifting the guts of the DECLARE_EVENT_CLASS check and parameterising it to include __assign_str() is the "right" way to do it. I have no particular time or interest for this; feel free to grab the patch and finish it, and ask me if you like an assist.

[Harry-Chen]

Can you confirm whether it requires GPL license hack in META to reproduce to problem, or just copying files into kernel? In the former case, I think it's not that valuable to craft any fix to it.

[robn]

@Harry-Chen I have only tried myself with the META hack, but the vibe I got was that it would affect in-kernel builds as well.

Personally, I don't care about either modes and don't think it should be possible to do (for reasons that are way outside the scope of this), thus wanting to remove HAVE_DECLARE_EVENT_CLASS from #16434. But that has to stay, and if something is gonna stay, it should work properly. And also, I'm a bit sick of hearing about it, so here we are.

[Pinghigh]

In #16390 I simply removed the second parameter, it works for me and no error was caused (Although I accidentally corrected some places).
Is that a reliable idea?

[robn]

@Pinghigh not enough, because the second arg is required in kernels before 6.10. Thus the need for the configure check to decide which one to use.

[tonyhutter]

@robn this configure check works for me (note it looks for 1ARG not 2ARGS like in your version):

diff --git a/config/kernel-assign_str.m4 b/config/kernel-assign_str.m4
new file mode 100644
index 000000000..96159fae7
--- /dev/null
+++ b/config/kernel-assign_str.m4
@@ -0,0 +1,62 @@
+dnl #
+dnl # 6.10 kernel, check number of args of __assign_str() for trace:
+dnl
+dnl # 6.10+:           one arg
+dnl # 6.9 and older:   two args
+dnl #
+dnl # More specifically, this will test to see if __assign_str() takes one
+dnl # arg.  If __assign_str() takes two args, or is not defined, then
+dnl # HAVE_1ARG_ASSIGN_STR will not be set.
+dnl #
+AC_DEFUN([ZFS_AC_KERNEL_1ARG_ASSIGN_STR], [
+       AC_MSG_CHECKING([whether __assign_str() has one arg])
+       ZFS_LINUX_TRY_COMPILE_HEADER([
+               #include <linux/module.h>
+               MODULE_LICENSE("$ZFS_META_LICENSE");
+
+               #define CREATE_TRACE_POINTS
+               #include "conftest.h"
+       ],[
+               trace_zfs_autoconf_event_one("1");
+               trace_zfs_autoconf_event_two("2");
+       ],[
+               AC_MSG_RESULT(yes)
+               AC_DEFINE(HAVE_1ARG_ASSIGN_STR, 1,
+                   [__assign_str() has one arg])
+       ],[
+               AC_MSG_RESULT(no)
+       ],[
+               #if !defined(_CONFTEST_H) || defined(TRACE_HEADER_MULTI_READ)
+               #define _CONFTEST_H
+
+               #undef  TRACE_SYSTEM
+               #define TRACE_SYSTEM zfs
+               #include <linux/tracepoint.h>
+
+               DECLARE_EVENT_CLASS(zfs_autoconf_event_class,
+                       TP_PROTO(char *string),
+                       TP_ARGS(string),
+                       TP_STRUCT__entry(
+                               __string(str, string)
+                       ),
+                       TP_fast_assign(
+                               __assign_str(str);
+                       ),
+                       TP_printk("str = %s", __get_str(str))
+               );
+
+               #define DEFINE_AUTOCONF_EVENT(name) \
+               DEFINE_EVENT(zfs_autoconf_event_class, name, \
+                       TP_PROTO(char * str), \
+                       TP_ARGS(str))
+               DEFINE_AUTOCONF_EVENT(zfs_autoconf_event_one);
+               DEFINE_AUTOCONF_EVENT(zfs_autoconf_event_two);
+
+               #endif /* _CONFTEST_H */
+
+               #undef  TRACE_INCLUDE_PATH
+               #define TRACE_INCLUDE_PATH .
+               #define TRACE_INCLUDE_FILE conftest
+               #include <trace/define_trace.h>
+       ])
+])
diff --git a/config/kernel.m4 b/config/kernel.m4
index 4d471358d..c3d0dcbed 100644
--- a/config/kernel.m4
+++ b/config/kernel.m4
@@ -328,6 +328,7 @@ AC_DEFUN([ZFS_AC_KERNEL_TEST_RESULT], [
        ZFS_AC_KERNEL_SYNC_BDEV
        ZFS_AC_KERNEL_MM_PAGE_SIZE
        ZFS_AC_KERNEL_MM_PAGE_MAPPING
+       ZFS_AC_KERNEL_1ARG_ASSIGN_STR
        case "$host_cpu" in
                powerpc*)
                        ZFS_AC_KERNEL_CPU_HAS_FEATURE

I couldn't get it to work with robn@ee94ce9c8 though:

In file included from ./include/trace/define_trace.h:103,
                 from /home/hutter/zfs/include/os/linux/zfs/sys/trace_dbuf.h:159,
                 from /home/hutter/zfs/module/os/linux/zfs/trace.c:45:
/home/hutter/zfs/include/os/linux/zfs/sys/trace_dbuf.h: In function ‘perf_trace_zfs_dbuf_class’:
/home/hutter/zfs/include/os/linux/zfs/sys/trace_dbuf.h:81:17: warning: ‘snprintf’ argument 4 may overlap destination object ‘entry’ [-Wrestrict]
   81 |                 snprintf(__get_str(msg), TRACE_DBUF_MSG_MAX,            \
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   82 |                     DBUF_TP_PRINTK_FMT, DBUF_TP_PRINTK_ARGS);           \
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/trace/perf.h:51:11: note: in definition of macro ‘DECLARE_EVENT_CLASS’
   51 |         { assign; }                                                     \
      |           ^~~~~~
/home/hutter/zfs/include/os/linux/zfs/sys/trace_dbuf.h:112:9: note: in expansion of macro ‘TP_fast_assign’
  112 |         TP_fast_assign(DBUF_TP_FAST_ASSIGN),
      |         ^~~~~~~~~~~~~~
/home/hutter/zfs/include/os/linux/zfs/sys/trace_dbuf.h:112:24: note: in expansion of macro ‘DBUF_TP_FAST_ASSIGN’
  112 |         TP_fast_assign(DBUF_TP_FAST_ASSIGN),
      |                        ^~~~~~~~~~~~~~~~~~~
./include/linux/fortify-string.h:114:33: warning: ‘__builtin_memcpy’ reading 511 bytes from a region of size 7 [-Wstringop-overread]
  114 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^

Hopefully someone with more tracepoint experience can wade though these errors.

[Harry-Chen]

@tonyhutter Not looking through the code, just a guess, size 7 comes from "(null)", which is used to replace "NULL" by the new 1-arg version of __assign_str.

As for the patch, do we really need a tracepoint_compact.h to replace all tracepoint.h? There are only two occurrences of __assign_str, maybe replacing them is enough?

[Harry-Chen]

@wmmur Kernel versions are not typically used as evidence of whether some feature exists, because distros will backport commits in their own vendored kernels. It is necessary to perform actually compiling to test.

But of course, it we can use more concise code snippets for testing availability, it would be good.

[snajpa]

FWIW I don't think tracing works with ZFS until there's some kind of change in the direction of license-compatibility (I've just verified against current zfs master + debian/sid 6.9.12 kernel):

root@snajpadev:~/zfs (ozfs-master)# grep HAVE_DECLARE_EVENT_CLASS zfs_config.h
/* #undef HAVE_DECLARE_EVENT_CLASS */
[...changing branches and reconfiguring for patched 6.10.5 kernel where license_is_gpl_compatible(CDDL) = true...]
root@snajpadev:~/zfs (vpsadminos-master-next)# grep HAVE_DECLARE_EVENT_CLASS zfs_config.h
#define HAVE_DECLARE_EVENT_CLASS 1

I got it to compile by doing this: vpsfreecz@f5045c2 - it seems to work like it should:

root@snajpadev:/sys/kernel/tracing# cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 50/50   #P:64
#
#                                _-----=> irqs-off/BH-disabled
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| / _-=> migrate-disable
#                              |||| /     delay
#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
            ntpd-12807   [034] .....  3629.076421: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 542 level 0 blkid 0 offset 0 size 512 state 0 holds 0 }
            ntpd-12807   [034] .....  3629.076432: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 542 level 0 blkid 0 offset 0 size 512 state 3 holds 1 }
            ntpd-12807   [034] .....  3629.076476: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 542 level 0 blkid 0 offset 0 size 512 state 4 holds 2 }
            ntpd-12807   [034] .....  3629.076493: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 542 level 0 blkid 0 offset 0 size 512 state 0 holds 0 }
            ntpd-12807   [034] .....  3629.076493: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 542 level 0 blkid 0 offset 0 size 512 state 5 holds 0 }
 systemd-journal-10908   [027] .....  3629.466908: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 330017 level 0 blkid 0 offset 0 size 512 state 0 holds 0 }
 systemd-journal-10908   [027] .....  3629.466922: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 330017 level 0 blkid 0 offset 0 size 512 state 3 holds 1 }
 systemd-journal-10908   [027] .....  3629.466968: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 330017 level 0 blkid 0 offset 0 size 512 state 4 holds 2 }
 systemd-journal-10908   [027] .....  3629.466997: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 330017 level 0 blkid 0 offset 0 size 512 state 0 holds 0 }
 systemd-journal-10908   [027] .....  3629.466997: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 330017 level 0 blkid 0 offset 0 size 512 state 5 holds 0 }
            ntpd-12812   [036] .....  3634.076509: zfs_dbuf__state_change: dbuf { spa "tank" objset 158 object 542 level 0 blkid 0 offset 0 size 512 state 0 holds 0 }

Now the question is how to integrate it so it works with older kernels (and doesn't look stupid, that part seems the hardest for me :D)

[snajpa]

now I've got these two commits: vpsfreecz@e83f060 (with @tonyhutter as the author) and vpsfreecz@d59ddaf
... it seems to compile down to 5.10 (tried 6.9, 6.8, 5.10)

[snajpa]

Was going to try 4.18, which is supposed to be supported, but that doesn't compile with more recent toolchain. Latest 4.19 + above patches + META change + tracing do seem to work together. So I should probably do a PR from this?