Differential D11636
efirt: restrict visibility of EFIABI_ATTR-declared functions Authored by rlibby on Jul 18 2017, 8:10 AM. Tags None Referenced Files
Details
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. I realize that the old in-tree gcc is not a high priority but there is some value in keeping it functioning and this is one of the last couple thing blocking amd64/gcc4.2 buildkernel.
Diff Detail
Event TimelineComment Actions I would strongly prefer to have gcc version check in amd64/include/efi.h. The arm64 architecture currently lacks real-time clock, which must be implemented as GetTime() rt service and this is overdue. I do not see much need to support old clang. Comment Actions IMHO, This is completely bogus. We do not support and cannot support building anything using EFI with the old gcc compiler. Better to have a check to see if it is the old gcc compiler and #error, or like we do for the loader build, exclude those modules when built with old gcc. There will be other architectures where this code is used, like arm64, once the VM stuff is done for them. Do not commit this code. What's the use case that this is even included with gcc 4.2.1? Comment Actions This isn't to build efirt with gcc 4.2, it's to NOT build it! The EFIABI_ATTR decls currently are visible even when not compiling efi code, which breaks the gcc 4.2 build even when specifying WITHOUT_MODULES=efirt. E.g. cc [...] /usr/src/freebsd/sys/amd64/amd64/machdep.c cc1: warnings being treated as errors In file included from /usr/src/freebsd/sys/amd64/amd64/machdep.c:65: /usr/src/freebsd/sys/sys/efi.h:146: warning: 'ms_abi' attribute directive ignored [-Wattributes] And also some userland code (don't recall exactly what, some ioctl code). So either the decls need to be hidden from in-tree gcc somehow, or it will no longer work for amd64 buildkernel or buildworld. Comment Actions The proper solution then is what both kib and I are saying: add the proper ifdef so it isn't included when compiling with the old compiler. The declarations belong here as they aren't machine specific. The old compiler is going away, and it makes no sense to move something out of the way when we'll just move it back for arm64 EFIRT support later. Just ifdef it out for gcc < 4.4. We'll be dropping support for that in 12 most likely and it's a lot easier to delete a couple of lines than to move it around. Comment Actions Oh, yeah, I'm happy to do that. I got the wrong impression from the previous comment I guess. I'll update this shortly with the compiler version detection. Comment Actions kib/imp feedback: detect old compiler version rather than move decls. I checked that this compiles with gcc 4.2 WITHOUT_MODULES=efirt, and with clang 4.0 (plain, including efirt). I am still testing gcc > 4.4. I guess clang also defines __GNUC__ as the clang build failed without the clang check. I don't have a way to check __INTEL_COMPILER, I am just following sys/sys/cdefs.h on that one. Comment Actions Fair enough. Dollars to donuts says that the intel compiler support is busted, but this won't change that.
Comment Actions kib feedback: guard by arch and kernel Checked buildkernel -DKERNFAST compilation, will check more thoroughly later. Comment Actions kib feedback: can implement check with just the one macro I like your suggestion just to use EFIABI_ATTR. I struggled to make the conditional macro definition not super ugly. I can go back to defining NO_EFIABI_ATTR and then using that as an intermediate (guard with #ifndef NO_EFIABI_ATTR) (or something else?) if you prefer. Checked clang 4.0 buildkernel, others still in progress. Comment Actions Modulo the small tweaks I suggested inline, I am fine with this version.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||