Arguments format

[Polytropon]

On Sat, 18 Apr 2020 14:07:00 +0200, Paweł Jasiak wrote:
> Thanks for your response!
> 
> On 17/04/20, Polytropon wrote:
> > On Fri, 17 Apr 2020 18:05:56 +0200, Paweł Jasiak wrote:
> > > 1. In sys/mips/mips/autoconf.c we have functions
> > > 
> > > static void configure_first(dummy)
> > > static void configure(dummy)
> > > static void configure_final(dummy)
> > > 
> > > and we are not using argument. We are having those functions also in
> > > ricv, arm, arm64, powerpc and x86 and in non of them we are using dummy,
> > > so maybe we can just remove it? Or if it is necessary why we don't mark
> > > it as __unused like in other functions?
> > 
> > I haven't checked any further, but I could imagine that
> > is has to do with the requirement of those functions
> > being able - at least in their declaration - to accept
> > a parameter; the type void * is a "somewhat universal"
> > type. Note that the functions are being mentioned in
> > macros, such as
> > 
> > 	SYSINIT(configure1, SI_SUB_CONFIGURE, SI_ORDER_FIRST, configure_first, NULL);
> > 
> > in /usr/src/sys/powerpc/powerpc/autoconf.c which might
> > be the reason why there has to be a dummy parameter...
> > 
> > Okay, further investigation. ;-)
> > 
> > According to "man 9 SYSINIT", the definition is
> > 
> >      SYSINIT(uniquifier, enum sysinit_sub_id subsystem,
> >          enum sysinit_elem_order order, sysinit_cfunc_t func,
> >          const void *ident);
> > 
> > and the type sysinit_cfunc_t is defined as
> > 
> > 	typedef void (*sysinit_cfunc_t)(const void *);
> > 
> > in /usr/src/sys/sys/kernel.h, so this is the reaon why
> > the configure_first(), configure(), and configure_final()
> > functions have to be "compatible".
> 
> Thanks, I didn't really pay attention to SYSINIT but still don't 
> understand why we don't mark the arguments as __unused.

If I remember correctly, __unused is an attribute primarily
intended as a hint to the compiler that says "the variable
probably will not be used", so the compiler won't issue the
corresponding warning message. So __unused is optional here,
what matters is that sysinit_cfunc_t in SYSINIT requires the
argument list of (void *). However, if the dummy argument is
not to be used, adding __unused would probably be a good
idea.



> > > 2. Above functions have strange definition for arguments.
> > > 
> > >     static void
> > >     configure(dummy)
> > >         void *dummy;
> > >     {
> > >         ...
> > >     }
> > > 
> > > Why we are not using
> > > 
> > >     static void
> > >     configure(void *dummy)
> > >     {
> > >         ...
> > >     }
> > > 
> > > like in other places?
> > 
> > That is not a strange format, it's an older dialect of C,
> > usually called "K&R C", where the definition of a function
> > typically is:
> > 
> > 	return-type function-name(arg1, arg2, arg3, ...)
> > 		type arg1;
> > 		type arg2;
> > 		type arg3;
> > 		...
> > 	{
> > 		function-body
> > 	}
> > 
> > A convention also is to put the function's return type on
> > an individual line, so the function's name always starts
> > at column 1.
> > 
> > See "man 9 style" for details.
> > 
> > Still, this style is not being followed consistently:
> > 
> > % grep "^configure_first" `find /usr/src/sys -name autoconf.c`
> > /usr/src/sys/x86/x86/autoconf.c:configure_first(void *dummy)
> > /usr/src/sys/arm64/arm64/autoconf.c:configure_first(void *dummy)
> > /usr/src/sys/arm/arm/autoconf.c:configure_first(void *dummy)
> > /usr/src/sys/riscv/riscv/autoconf.c:configure_first(void *dummy)
> > /usr/src/sys/sparc64/sparc64/autoconf.c:configure_first(void *dummy)
> > /usr/src/sys/powerpc/powerpc/autoconf.c:configure_first(void *dummy)
> > /usr/src/sys/mips/mips/autoconf.c:configure_first(dummy)
> > 
> > Some use "K&R C" style, others use "ANSI C" style.
> 
> Thanks, I know both styles and I was worried about mixing them.

Excellent. :-)

The primary reason probably is the age of certain code. Not
all code present in FreeBSD conforms to "man 9 style".



> > > 3. In sys/mips/mips/octeon_cop2.c we are having
> > > 
> > >     struct octeon_cop2_state *
> > >     octeon_cop2_alloc_ctx()
> > >     {
> > >         ...
> > >     }
> > > but it's declaration in sys/mips/include/octeon_cop2.h is
> > > 
> > >     struct octeon_cop2_state* octeon_cop2_alloc_ctx(void);
> > > 
> > > Question is if we should change octeon_cop2_alloc_ctx() into
> > > octeon_cop2_alloc_ctx(void)?
> > 
> > There is a difference between () and (void) which _might_ be
> > intended; however, prototype and declaration should in fact
> > have the same signature. If the argument is (), the function
> > will accept any parameters, including none ("any parameters
> > list"); if it's (void), the function will refuse to accept
> > any parameters ("emtpy parameter list"), which is explicit
> > for "it doesn't use any parameters".
> 
> I know the difference again ;) 
> 
> % grep -nr "octeon_cop2_alloc_ctx"
> sys/mips/mips/vm_machdep.c:164:             td2->td_md.md_cop2 = octeon_cop2_alloc_ctx();
> sys/mips/mips/vm_machdep.c:169:             td2->td_md.md_ucop2 = octeon_cop2_alloc_ctx();
> sys/mips/mips/octeon_cop2.c:53:octeon_cop2_alloc_ctx()
> sys/mips/mips/trap.c:942:                   td->td_md.md_cop2 = octeon_cop2_alloc_ctx();
> sys/mips/mips/trap.c:995:                           td->td_md.md_ucop2 = octeon_cop2_alloc_ctx();
> sys/mips/include/octeon_cop2.h:208:struct octeon_cop2_state* octeon_cop2_alloc_ctx(void);
> 
> 
> I believe that all uses of the octeon_cop2_alloc_ctx function so I still
> don't understand why we have different signatures.

According to "man 9 style", argument names can be omitted in
the header files, so declaration and definition might "look
different", but are the same to the compiler and linker:

	int foo(int a, void *b, char c, struct blah d)
	{
		...
	}

	int bar(void)
	{
		...
	}

The "simplified prototype" in the header file becomes:

	int foo(int, void *, char, struct blah);
	int bar(void);

Only the types remain.

That would be consistent; the example you provided is
inconsistent; /usr/src/sys/mips/mips/octeon_cop2.c contains
the definition as follows:

	struct octeon_cop2_state *
	octeon_cop2_alloc_ctx()
	{
	        return uma_zalloc(ctxzone, M_NOWAIT);
	}

The header file /usr/src/sys/mips/include/octeon_cop2.h
defines:

	struct octeon_cop2_state* octeon_cop2_alloc_ctx(void);

As I wrote about the meaning of () vs. (void), this might be
an occassion to suggest that probably (void) should be used
in both places. You could file a bug report for this. Also
note that according to "man 9 style", the * belongs to the
name:

	struct octeon_cop2_state *octeon_cop2_alloc_ctx(void);

And similarly:

	struct octeon_cop2_state *
	octeon_cop2_alloc_ctx(void)
	{
	        return uma_zalloc(ctxzone, M_NOWAIT);
	}

I cannot imagine why two different signatures exist, but
you could ask the developers if there _is_ a reason to do so.
As I mentioned, it's not actually wrong, as () "anything"
allows (void) "nothing", but still it should match. And
the function is always called with _no_ arguments, so (void)
should be the correct thing to choose here.



-- 
Polytropon
Magdeburg, Germany
Happy FreeBSD user since 4.0
Andra moi ennepe, Mousa, ...