Re: netbsd32_{,u}int64 in sys/types.h for compat/sys/siginfo.h

[Rin Okuyama <rokuyama.rk%gmail.com@localhost>]

On 2019/11/18 16:52, Kamil Rytarowski wrote:
> On 18.11.2019 05:38, Rin Okuyama wrote:
> > Thank you for your comment!
> >
> > On 2019/11/17 22:42, Kamil Rytarowski wrote:
> > > Please check it also with picotrace/truss:
> > >
> > > http://pkgsrc.se/devel/picotrace
> >
> > netbsd32_signal.c needed to catch up with kern_sig.c so that syscall
> > information is provided with SIGTRAP TRAP_SCE/TRAP_SCX. I committed
> > the fix and picotrace/truss works fine now on COMPAT_NETBSD32.
>
> Thanks! I have submitted a mail how to further improve it.

Thank you for your advice! I committed it!

> > On 2019/11/17 22:42, Kamil Rytarowski wrote:
> > > On 17.11.2019 04:34, Rin Okuyama wrote:
> > > > Hi,
> > > >
> > > > In order to distangle circular dependency between
> > > > <sys/compat/sys/siginfo.h> v.s. <machine/netbsd32_machdep.h>,
> > > > I propose
> > > >
> > > > (1) Move NETBSD32_INT64_ALIGN from <machine/netbsd32_machdep.h> to
> > > > <machine/types.h>
> > > >
> > > > (2) Move netbsd32_{,u}int64 from <sys/compat/netbsd32/netbsd32.h> to
> > > > <sys/types.h>
> > > >
> > > > See attached patch for example on amd64.
> > >
> > > What do you think about duplicating the defines of netbsd32_uint64
> > > inside sys/compat/sys/siginfo.h + adding a comment about keeping it in
> > > sync with netbsd32.h?
> > >
> > > I think that avoiding spaghetti dependencies is a benefit.
> > >
> > > We already duplicated there _ptrace_state, removing circular
> > > dependencies between sys/ptrace.h and sys/siginfo.h.
> >
> > I don't think this is a good idea in this case. If we want to have
> > duplicate define of netbsd32_uint64, and to avoid an "#ifdef __x86_64__"
> > mess in <compat/sys/siginfo.h>, we need to move NETBSD32_INT64_ALIGN to
> > <machine/somewhere.h> other than <machine/netbsd32_machdep.h>. If so,
> > why not <machine/types.h>?
>
> No need to move anything out of machine/netbsd32_machdep.h. It's
> sufficient to define netbsd32_int64 with a custom non-conflicting name
> or protect it with #ifdef before typedefing/defining twice.
>
> > Also, in my proposal, spaghetti dependencies are avoided in the end;
> > everyone depends on <sys/types.h>, and not on each other.
>
> However I have no strong opinions here. I would personally avoid
> compat32 definitions in sys/types.h.
>
> Compat code tends to need hacks, so it is sensible imho to restrict them
> to compat headers (I am aware that it's not always followed).

I agree with you on that compat codes should not be put into sys headers
as far as possible. However, I still do not understand what you mean.

(1) NETBSD32_INT64_ALIGN == __attribute__((__aligned__(4))) is in
    <machine/netbsd32_machdep.h>, and

(2) <machine/netbsd32_machdep.h> needs <compat/sys/siginfo.h>

Therefore, we cannot use NETBSD32_INT64_ALIGN in <compat/sys/siginfo.h>.
Of course, we can typedef _netbsd32_uint64 in <compat/sys/siginfo.h> like:

typedef uint64_t _netbsd32_uint64
#ifdef __x86_64__
    __attribute__((__aligned__(4)))
#endif
    ;

Do you think it is OK? I guess that I miss some elementary things...
Sorry in advance ;-).

Thanks,
rin