Date: Sat, 18 Apr 2020 15:31:21 +0200 From: Polytropon <freebsd@edvax.de> To: =?UTF-8?B?UGF3ZcWC?= Jasiak <pawel@jasiak.xyz> Cc: freebsd-questions@freebsd.org Subject: Re: Arguments format Message-ID: <20200418153121.c4419587.freebsd@edvax.de> In-Reply-To: <20200418120700.GA47913@gmail.com> References: <20200417160556.GA44862@gmail.com> <20200417184725.b49109b7.freebsd@edvax.de> <20200418120700.GA47913@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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, ...
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20200418153121.c4419587.freebsd>