Date: Thu, 20 Jul 2017 02:08:30 -0700 From: Ryan Libby <rlibby@gmail.com> To: Bruce Evans <brde@optusnet.com.au>, kib@freebsd.org Cc: 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: <CAHgpiFxW7JzurYeYuN5WaN0Z%2BjcpPLSjtHL34iCgmdJUz7bSyg@mail.gmail.com> In-Reply-To: <20170720172157.W1152@besplex.bde.org> References: <201707200647.v6K6l7Hq076554@repo.freebsd.org> <20170720172157.W1152@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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__))
>> +#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?)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHgpiFxW7JzurYeYuN5WaN0Z%2BjcpPLSjtHL34iCgmdJUz7bSyg>