I don't know, Walt!

[nabijaczleweli]

Motivation and Context

You've been acting pretty sus lately!

Description

Almost like there's an impostor among us.

Well, there isn't any-more. See individual commit messages. Or, well, do so tomorrow because I need to write them.

Also there's a -Wunused-parameters thing for kFreeBSD module at the top. It'll probably be gone because the headers are, I'm assuming, as irredeemable as Linux's.

How Has This Been Tested?

Builds. Crucially, I can finally inline my configure CFLAGS I've been nursing for two(?!!) years into the autohell rules.

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:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes. – none apply
• I have run the ZFS Test Suite with this change applied. – CI take my hand
• All commit messages are properly formatted and contain Signed-off-by.

[nabijaczleweli]

The "mark arguments used" commits are broken into two sections: the first one is the first commit, which replaces the sussy baka markings (all in module/), the second is the rest of the commits, which fix CI errors as I got them.
@behlendorf thoughts on distributing them (i.e. should I (a) break them up like the latter are or (b) squish them into one big blob)?

[behlendorf]

Once you have everything building cleanly can you squash all the related fixes together. In particular, I'm mainly looking at the "mark arguments used" commits for the user libraries. The rest looks pretty reasonable to me.

[alejandro-colomar]

Why cast to (uintptr_t)? You're not doing bitwise operations on pointers; that seems a bit weird. Why not (void *), if you need the cast at all?

Moreover, POSIX (and I guess ISO C does the same) only guarantees the following:

       The following type designates an unsigned  integer  type  with  the
       property  that  any  valid pointer to void can be converted to this
       type, then converted back to a pointer to void, and the result will
       compare equal to the original pointer: uintptr_t

So you really can't even transform uintptr_t -> void * -> uintptr_t. You're only allowed to do void * -> uintptr_t -> void *. Going from uintptr_t to function pointer is UB, although I expect it to work, but who knows GCC 20 :)

What's the warning you're trying to silence?

[nabijaczleweli]

func is void (*)(void *), pthread_create() takes void *(*)(void *), this produces an invalid cast warning (and rightfully so):

In file included from kernel.c:27:
kernel.c: In function ‘zk_thread_create’:
kernel.c:121:6: warning: cast between incompatible function types from ‘void (*)(void *)’ to ‘void * (*)(void *)’ [-Wcast-function-type]
  121 |      (void *(*)(void *))func, arg));
      |      ^
../../lib/libspl/include/assert.h:104:37: note: in definition of macro ‘VERIFY0’
  104 |  const uint64_t __left = (uint64_t)(LEFT);   \
      |                                     ^~~~

What I did think about doing was allocating a (func, arg) bundle and pushing it through a helper, but this would screw with the (seemingly) precise stack size measurements, so.

I mean, casting it like this is obviously technically UB anyway because of the incompatible return types, but going through an uintptr_t (or, indeed, a pointer) is worse? But then save for a union or screwing with the stack size stuff I can't realistically come up with a good way of making the compiler go away here.

Updated to use a union.

[alejandro-colomar]

Hmm. Here there be dragons!

• Converting between function pointers is permitted by the standard (you may not use it to call, but you can store). See C2x::6.3.2.3/8:

A pointer to a function of one type may be converted to a pointer to a function of another type and
back again; the result shall compare equal to the original pointer. If a converted pointer is used to
call a function whose type is not compatible with the referenced type, the behavior is undefined.

• The call is being done in a different translation unit, and therefore, the compiler can't know if you will invoke UB or not, so it has to assume you're not invoking UB, and so it needs to follow the ABI.
• If the ABI defines both function pointers to be compatible (I assume they are, or you'd've noticed before), then, I think (not 100% sure, dragons are flying around) it's defined behavior ($DEITY bless translation unit boundaries to workaround UB).

So I'd say you're not invoking UB with the function pointer cast, but you need a cast. I'd either use -Wno-cast-function-type (since the compiler is invalidly blaming you) or cast to (void *), if you need to keep that warning. Choose your poison carefully.

[nabijaczleweli]

Good find – I rolled back to the straight cast and added -Wno-cast-function-type to this file, pointing at this thread. It should™ be fine.

[alejandro-colomar]

BTW, there's another thing you could do to not disable the warning from the compiler: use noreturn.

As weird as it may seem, noreturn int foo(void); is valid C (I asked a question to corfirm it, though).

So you may use the prototype that pthread_create(3) expects, mark it as noreturn, and then the compiler should not warn about it.

Example:

alx@ady1:~/src/test$ cat noreturn.c
#include <stdnoreturn.h>
#include <pthread.h>

noreturn void *foo(void *x);
int bar(pthread_t *restrict th, void *arg);

noreturn void *
foo(void *x)
{
	pthread_exit(x);
}

int
bar(pthread_t *restrict th, void *arg)
{
	return pthread_create(th, NULL, &foo, arg);
}
alx@ady1:~/src/test$ cc -Wall -Wextra -Werror -S noreturn.c 
alx@ady1:~/src/test$ clang -Wall -Wextra -Weverything -Werror -S noreturn.c 

But you'd need to modify a few functions for that.

I used that trick in some embedded systems where main() needs to be int main(void), but it's an infinite loop and never returns, so I declared it as noreturn int main(void) :)

Using noreturn in this case has the advantage that the behavior is 100% defined (as long as the Standard continues allowing this dirty hack), since you don't depend on the ABI having compatible function calls for a return of void and void *, but has the readability problems that one may ask WTF means noreturn void*. Again, choose your poison carefully :p

[nabijaczleweli]

@behlendorf squished all the userspace argsused, and fixed all the problems Alejandro found (and the rest that I did when rereading) – if CI gives it the all-clear then it should be g2g

[alejandro-colomar]

@behlendorf squished all the userspace argsused, and fixed all the problems Alejandro found (and the rest that I did when rereading) – if CI gives it the all-clear then it should be g2g

Consider this, which you may have not read since I replied when it was already marked as resolved:

#13110 (comment)

[nabijaczleweli]

I get mail for all comments, worry not.

[alejandro-colomar]

Is it guaranteed that get_objset_type(ds, &ds_type) won't fail below? Otherwise, you might want to check the return code.

[nabijaczleweli]

It's void-casted there (and used elsewhere) so I think so, yeah.

[alejandro-colomar]

You can cast -1 directly. Don't need to subtract from 0.

And you can use a slightly better version which accepts a variable:

#define is_unsigned(x)  (((__typeof__(x)) -1) > 0)

So you can use c directly:

if (((IS_UNSIGNED(c) || c >= 0x00) && c <= 0x1f) || (c > 0x7f && c <= 0xffff))

[nabijaczleweli]

I can also not to that because it doesn't work. And neither does any other suppression method, as noted in the original thread #13110 (comment)

[alejandro-colomar]

Much cleaner, yep :p

[alejandro-colomar]

Well, I'd say

Reviewed-by: Alejandro Colomar <[email protected]>

:-)