Date: Thu, 20 Jul 2017 18:01:14 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ryan Libby <rlibby@freebsd.org> Cc: 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: <20170720172157.W1152@besplex.bde.org> In-Reply-To: <201707200647.v6K6l7Hq076554@repo.freebsd.org> References: <201707200647.v6K6l7Hq076554@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 20 Jul 2017, Ryan Libby wrote:
> Log:
> efi: restrict visibility of EFIABI_ATTR-declared functions
>
> In-tree gcc (4.2) doesn't understand __attribute__((ms_abi))
> (EFIABI_ATTR). Avoid declaring functions with that attribute when the
> compiler is detected to be gcc < 4.4.
Thanks. I used to work around this using -Dms_abi=.
> 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.
> +#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.
All uses of the packed attribute are bugs since it changes the ABI for
compilers that don't use it. ofed has much larger bugs for it:
- it hard-codes __attribute__ instead of using __packed from cdefs.h
- it mis-hard-codes __attribute__ using the application namespace
- it uses gnu style (space before parentheses). Perhaps ofed is supposed
to use linux style instead of gnu style, but even linux (2.6.10 include
directory) uses no spaces after __attribute__ more than half the time
(it has a measly 420 instances with spaces and 538 without. FreeBSD
has a whole 30 instances of __attribute__ in cdefs.h and most of the
instances not in there are bugs).
Grepping sys/*/* for attribute shows the following bugs:
X bsm/audit.h:typedef u_int64_t au_asflgs_t __attribute__ ((aligned (8)));
X libkern/explicit_bzero.c:__attribute__((weak)) void __explicit_bzero_hook(void *, size_t);
X libkern/explicit_bzero.c:__attribute__((weak)) void
X net/ieee8023ad_lacp.c: __attribute__((__format__(__printf__, 2, 3)));
X net/netmap.h: uint8_t __attribute__((__aligned__(NM_CACHE_ALIGN))) sem[128];
X net/netmap_user.h: static void *__xxzt[] __attribute__ ((unused)) =
X netinet/sctp.h:#define SCTP_PACKED __attribute__((packed))
X netinet/sctp_header.h:#define SCTP_PACKED __attribute__((packed))
X netinet/sctp_input.c:__attribute__((noinline))
X netinet/sctp_input.c:__attribute__((noinline))
X netinet/sctp_os_bsd.h:#define SCTP_UNUSED __attribute__((unused))
X opencrypto/gfmult.h:#define __aligned(x) __attribute__((__aligned__(x)))
X xen/blkif.h: uint64_t __attribute__((__aligned__(8))) id;
X xen/blkif.h: uint64_t __attribute__((__aligned__(8))) id;
X xen/xen_intr.h: __attribute__((format(printf, 2, 3)));
Almost all can be fixed by using the FreeBSD wrapper in cdefs.h. I think
ms_abi is MD so it doesn't belong there, but almost everything else does.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170720172157.W1152>