Date: Thu, 20 Jul 2017 13:33:24 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Ryan Libby <rlibby@gmail.com> Cc: Bruce Evans <brde@optusnet.com.au>, Ryan Libby <rlibby@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r321284 - in head/sys: amd64/include sys Message-ID: <20170720103323.GG1935@kib.kiev.ua> In-Reply-To: <CAHgpiFxW7JzurYeYuN5WaN0Z%2BjcpPLSjtHL34iCgmdJUz7bSyg@mail.gmail.com> References: <201707200647.v6K6l7Hq076554@repo.freebsd.org> <20170720172157.W1152@besplex.bde.org> <CAHgpiFxW7JzurYeYuN5WaN0Z%2BjcpPLSjtHL34iCgmdJUz7bSyg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jul 20, 2017 at 02:08:30AM -0700, Ryan Libby wrote:
> On Thu, Jul 20, 2017 at 1:01 AM, Bruce Evans <brde@optusnet.com.au> wrote:
> > On Thu, 20 Jul 2017, Ryan Libby wrote:
> >> Modified: head/sys/amd64/include/efi.h
> >>
> >> ==============================================================================
> >> --- head/sys/amd64/include/efi.h Thu Jul 20 05:43:48 2017
> >> (r321283)
> >> +++ head/sys/amd64/include/efi.h Thu Jul 20 06:47:06 2017
> >> (r321284)
> >> @@ -36,8 +36,14 @@
> >> * XXX: from gcc 6.2 manual:
> >> * Note, the ms_abi attribute for Microsoft Windows 64-bit targets
> >> * currently requires the -maccumulate-outgoing-args option.
> >> + *
> >> + * Avoid EFIABI_ATTR declarations for compilers that don't support it.
> >> + * GCC support began in version 4.4.
> >> */
> >> +#if defined(__clang__) || defined(__GNUC__) && \
> >> + (__GNUC__ > 4 || __GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> >> #define EFIABI_ATTR __attribute__((ms_abi))
> >
> >
> > This is still broken. ms_abi is in the application namespace. Thus
> > my hack of defining it to nothing is valid, and so is defining it to
> > 'syntax error', but the latter shows the brokenness of EFIABI_ATTR.
>
> To be clear, you're referring to how ms_abi is spelled here without
> underscores? I can prepare a follow-up to spell it as
> __attribute__((__ms_abi__))
Spelling it __ms_abi__ is fine but perhaphs unnecessary because it is
used only under _KERNEL.
I do not object against changing the spelling, of course.
>
> >> +#endif
> >>
> >> #ifdef _KERNEL
> >> struct uuid;
> >>
> >> Modified: head/sys/sys/efi.h
> >>
> >> ==============================================================================
> >> --- head/sys/sys/efi.h Thu Jul 20 05:43:48 2017 (r321283)
> >> +++ head/sys/sys/efi.h Thu Jul 20 06:47:06 2017 (r321284)
> >> @@ -122,6 +122,9 @@ struct efi_tblhdr {
> >> uint32_t __res;
> >> };
> >>
> >> +#ifdef _KERNEL
> >> +
> >> +#ifdef EFIABI_ATTR
> >> struct efi_rt {
> >> struct efi_tblhdr rt_hdr;
> >> efi_status (*rt_gettime)(struct efi_tm *, struct efi_tmcap *)
> >> @@ -144,6 +147,7 @@ struct efi_rt {
> >> efi_status (*rt_reset)(enum efi_reset, efi_status, u_long,
> >> efi_char *) EFIABI_ATTR;
> >
> >
> > This was more broken when it was outside of _KERNEL.
> >
> >> };
> >> +#endif
> >>
> >> struct efi_systbl {
> >> struct efi_tblhdr st_hdr;
> >> @@ -163,7 +167,6 @@ struct efi_systbl {
> >> uint64_t st_cfgtbl;
> >> };
> >>
> >> -#ifdef _KERNEL
> >> extern vm_paddr_t efi_systbl_phys;
> >> #endif /* _KERNEL */
> >
> >
> > This bug is not very common. There seem to be no instances of it in
> > <sys> (only sys/cdefs.h uses __attribute__(()), and it seems to use
> > underscores for all the attributes). Grepping sys/include/*.h for
> > attribute shows the following bugs:
> >
> > X amd64/include/efi.h:#define EFIABI_ATTR __attribute__((ms_abi))
> > X i386/include/efi.h:#define EFIABI_ATTR /* __attribute__((ms_abi)) */ /* clang fails with this */
> > X ofed/include/rdma/ib_user_mad.h:typedef unsigned long __attribute__((aligned(4))) packed_ulong;
> > X ofed/include/rdma/ib_smi.h:} __attribute__ ((packed));
> > X ofed/include/rdma/ib_mad.h:} __attribute__ ((packed));
> > X ofed/include/rdma/ib_mad.h:} __attribute__ ((packed));
> >
> > The commented-out ms_abi was only a style bug. Now it is a larger style
> > bug -- it is different and worse than amd64.
>
> I'm not sure what to do about i386 there (again beyond fixing up the
> spelling in the comment). Maybe the unsupported architectures should
> just not be declaring EFIABI_ATTR at all? (Thoughts, kib?)
I think i386 should be treated exactly same as amd64, i.e. EFIABI_ATTR
should be not defined if gcc < 4.4. Or I do not understand the scope
of the question.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170720103323.GG1935>