Bug #8569

problem with inline functions in abd.h

Added by Andriy Gapon 2 months ago. Updated about 2 months ago.

Status:ClosedStart date:2017-08-09
Priority:NormalDue date:
Assignee:-% Done:

100%

Category:zfs - Zettabyte File System
Target version:-
Difficulty:Bite-size Tags:needs-triage

Description

C [C99] has peculiar rules for inline functions that are different from the C++ rules.
Unlike C++ where inline is "fire and forget", in C a programmer must pay attention to the function's storage class / visibility.
The main problem is with the case where a compiler decides to not inline a call to the function declared as inline.
Some relevant links:
- http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15831.html
- http://www.drdobbs.com/the-new-c-inline-functions/184401540
The summary is that either the inline functions should be declared 'static inline' or one of the compilation units (.c files) must provide a callable externally visible function definition.
In the former case, the compiler would automatically create a local non-inlined function instance in every compilation unit where it's needed.
In the latter case the single external definition is used to satisfy any non-inlined calls in all compilation units.

As things stand right now, we can get an undefined reference error under certain combinations of compilers and compiler options.
For example, this is what I get on FreeBSD when compiling with clang 4.0.0 and -O1:

In function `abd_free':
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/abd.c:385: undefined reference to `abd_is_linear'

So, there are two alternatives. Either to qualify each inline function in abd.h with static storage class.
Or to add declarations like the following to abd.c:

extern inline boolean_t abd_is_linear(abd_t *abd);

Both work. I am not sure which one would be more consistent with the illumos development rules.

History

#1 Updated by Andriy Gapon 2 months ago

I see that the illumos code in general has a great deal of static inline functions in header files.
The ZFS code has a few instances of the naked inline in a header file and a matching extern inline in a C file (e.g. zap_f_phys, zap_m_phys, dmu_buf_init_user).

#2 Updated by Gordon Ross 2 months ago

Your 2nd way (put "extern inline ..." in abd.c) has the advantage of predictability about where the externally visible symbol will be instantiated. I find that preferable.

#3 Updated by Andriy Gapon 2 months ago

Submitted a pull request with the change: https://github.com/openzfs/openzfs/pull/444

#4 Updated by Electric Monk about 2 months ago

  • % Done changed from 0 to 100
  • Status changed from New to Closed

git commit 37e84ab74e939caf52150fc3352081786ecc0c29

commit  37e84ab74e939caf52150fc3352081786ecc0c29
Author: Andriy Gapon <avg@FreeBSD.org>
Date:   2017-08-31T05:29:47.000Z

    8569 problem with inline functions in abd.h
    Reviewed by: Matt Ahrens <mahrens@delphix.com>
    Approved by: Robert Mustacchi <rm@joyent.com>

Also available in: Atom