Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing semicolon in trace_dbuf.h #16281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dberlin
Copy link
Contributor

@dberlin dberlin commented Jun 18, 2024

Motivation and Context

Missing semicolon in trace_dbuf causes compilation errors on newer kernels.

Description

On fedora 40, on the 6.9.4 kernel (in updates-testing), assign_str expands to a "do { } while(0)" loop. Without this semicolon, the while(0) is unterminated, causing a cascade of unhelpful errors (admittedly, it does identify the missing semicolon in that jumble, just not in a helpful way). With this semicolon, it compiles fine. It also compiles fine on 6.8.11 (the previous kernel). I have not tested earlier kernels than that, but at worst it should add a pointless semicolon.

Also, all other instances in the source tree are already terminated with semicolons:

include/os/linux/zfs/sys/trace_dbuf.h
64:                     __assign_str(os_spa,                            \
			spa_name(DB_DNODE(db)->dn_objset->os_spa));	\
67:                     __assign_str(os_spa, "NULL");                   \
83:             __assign_str(os_spa, "NULL")                            \

include/os/linux/zfs/sys/trace_dbgmsg.h
62:         __assign_str(msg, msg);

(This change is fixing is line 83 above)

How Has This Been Tested?

Compilation of 6.8.11 kernel module, compilation of 6.9.4 kernel module on fedora 40

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@dberlin
Copy link
Contributor Author

dberlin commented Jun 18, 2024

Because i was lazy and did it from the github UI, it didn't wrap the commit message properly.
I'll fix it in a sec.

On fedora 40, on the 6.9.4 kernel (in updates-testing), assign_str
expands to a "do {<stuff> } while(0)" loop.  Without this semicolon,
the while(0) is unterminated, causing a cascade of useless errors.
With this semicolon, it compiles fine.  It also compiles fine on 6.8.11
(the previous kernel).  I have not tested earlier kernels than that, but
at worst it should add a pointless semicolon.

All other instances in the source tree are already terminated with
semicolons.

Signed-off-by: Daniel Berlin <[email protected]>
@jcadduono
Copy link

jcadduono commented Jun 23, 2024

while this does allow for compilation, it also exposes a lot of stringop-overread warnings for me (reading 511 bytes from a region of size 7)
i'm assuming these can be safely ignored, but maybe worth looking into?

compiled on vanilla kernel.org Linux 6.9.6, in tree, using ./configure --enable-linux-builtin and ./copy-builtin

In file included from ./include/trace/define_trace.h:103,
                 from ./include/zfs/os/linux/zfs/sys/trace_dbuf.h:158,
                 from fs/zfs/os/linux/zfs/trace.c:45:
./include/zfs/os/linux/zfs/sys/trace_dbuf.h: In function ‘perf_trace_zfs_dbuf_class’:
./include/linux/fortify-string.h:110:33: warning: ‘__builtin_memcpy’ reading 511 bytes from a region of size 7 [-Wstringop-overread]
  110 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^
./include/trace/perf.h:51:11: note: in definition of macro ‘DECLARE_EVENT_CLASS’
   51 |         { assign; }                                                     \
      |           ^~~~~~
./include/zfs/os/linux/zfs/sys/trace_dbuf.h:111:9: note: in expansion of macro ‘TP_fast_assign’
  111 |         TP_fast_assign(DBUF_TP_FAST_ASSIGN),
      |         ^~~~~~~~~~~~~~
./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
  633 |         __underlying_##op(p, q, __fortify_size);                        \
      |         ^~~~~~~~~~~~~
./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
  678 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
      |                          ^~~~~~~~~~~~~~~~~~~~
./include/trace/stages/stage6_event_callback.h:40:17: note: in expansion of macro ‘memcpy’
   40 |                 memcpy(__str__, __data_offsets.dst##_ptr_ ? :           \
      |                 ^~~~~~
./include/zfs/os/linux/zfs/sys/trace_dbuf.h:64:25: note: in expansion of macro ‘__assign_str’
   64 |                         __assign_str(os_spa,                            \
      |                         ^~~~~~~~~~~~
./include/zfs/os/linux/zfs/sys/trace_dbuf.h:111:24: note: in expansion of macro ‘DBUF_TP_FAST_ASSIGN’
  111 |         TP_fast_assign(DBUF_TP_FAST_ASSIGN),
      |                        ^~~~~~~~~~~~~~~~~~~

@dberlin
Copy link
Contributor Author

dberlin commented Jun 23, 2024

while this does allow for compilation, it also exposes a lot of stringop-overread warnings for me (reading 511 bytes from a region of size 7) i'm assuming these can be safely ignored, but maybe worth looking into?

I assume this is also in 6.9.x?

In file included from ./include/trace/define_trace.h:103,
                 from ./include/zfs/os/linux/zfs/sys/trace_dbuf.h:158,
                 from fs/zfs/os/linux/zfs/trace.c:45:
./include/zfs/os/linux/zfs/sys/trace_dbuf.h: In function ‘perf_trace_zfs_dbuf_class’:
./include/linux/fortify-string.h:110:33: warning: ‘__builtin_memcpy’ reading 511 bytes from a region of size 7 [-Wstringop-overread]
  110 | #define __underlying_memcpy     __builtin_memcpy
      |                                 ^
./include/trace/perf.h:51:11: note: in definition of macro ‘DECLARE_EVENT_CLASS’
   51 |         { assign; }                                                     \
      |           ^~~~~~
./include/zfs/os/linux/zfs/sys/trace_dbuf.h:111:9: note: in expansion of macro ‘TP_fast_assign’
  111 |         TP_fast_assign(DBUF_TP_FAST_ASSIGN),
      |         ^~~~~~~~~~~~~~
./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
  633 |         __underlying_##op(p, q, __fortify_size);                        \
      |         ^~~~~~~~~~~~~
./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
  678 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
      |                          ^~~~~~~~~~~~~~~~~~~~
./include/trace/stages/stage6_event_callback.h:40:17: note: in expansion of macro ‘memcpy’
   40 |                 memcpy(__str__, __data_offsets.dst##_ptr_ ? :           \
      |                 ^~~~~~
./include/zfs/os/linux/zfs/sys/trace_dbuf.h:64:25: note: in expansion of macro ‘__assign_str’
   64 |                         __assign_str(os_spa,                            \
      |                         ^~~~~~~~~~~~
./include/zfs/os/linux/zfs/sys/trace_dbuf.h:111:24: note: in expansion of macro ‘DBUF_TP_FAST_ASSIGN’
  111 |         TP_fast_assign(DBUF_TP_FAST_ASSIGN),
      |                        ^~~~~~~~~~~~~~~~~~~

Yeah, I would assume this is a flexible array member or something that isn't annotated right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants