Date: Wed, 13 Jan 2016 13:57:12 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> Cc: Steven Hartland <steven@multiplay.co.uk>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r293724 - in head/sys/boot: arm64/libarm64 common efi/boot1 efi/fdt efi/include efi/include/arm64 efi/libefi efi/loader efi/loader/arch/amd64 efi/loader/arch/arm efi/loader/arch/arm64 i... Message-ID: <20160113124535.X982@besplex.bde.org> In-Reply-To: <1452648737.46848.50.camel@freebsd.org> References: <201601120217.u0C2HdBC089684@repo.freebsd.org> <1452645668.46848.34.camel@freebsd.org> <56959DA7.9050206@freebsd.org> <1452646442.46848.37.camel@freebsd.org> <5695A5C4.9000409@multiplay.co.uk> <1452648737.46848.50.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 12 Jan 2016, Ian Lepore wrote:
> On Wed, 2016-01-13 at 01:17 +0000, Steven Hartland wrote:
>>
>> On 13/01/2016 00:54, Ian Lepore wrote:
>>> On Wed, 2016-01-13 at 00:43 +0000, Steven Hartland wrote:
>>> ...
>>>> Passes a full tinderbox so I assume your forcing gcc for some
>>>> reason?
>>> For several reasons. The fact that gcc isn't the default compiler
>>> doesn't mean that it's okay for code to not compile with gcc; it's
>>> still a supported compiler for arm.
I usually force gcc.
>> Not disagreeing with that, was just curious that's all ;-)
>>
>> The warnings you list seem to be detail, typical gcc, specifically:
>>
>> sys/boot/efi/fdt/../include/efiapi.h:535: warning: function
>> declaration isn't a prototype
>>
>> I'm guessing its being picky and wants EFI_RESERVED_SERVICE to have
>> void in there due to no params.
It is not broken, so it is warning about an unprototyped function as
requested by -Wstrict-prototypes.
>> Does the following help:
>>
>> Index: sys/boot/efi/fdt/Makefile
>> ===================================================================
>> --- sys/boot/efi/fdt/Makefile (revision 293796)
>> +++ sys/boot/efi/fdt/Makefile (working copy)
>> @@ -7,6 +7,8 @@
>> LIB= efi_fdt
>> INTERNALLIB=
>> WARNS?= 6
>> +CWARNFLAGS.gcc+= -Wno-strict-prototypes
>> +CWARNFLAGS.gcc+= -Wno-redundant-decls
>>
>> SRCS= efi_fdt.c
>>
This just breaks the warning.
>> @@ -34,4 +36,6 @@ CLEANFILES+= machine
>>
>> .include <bsd.lib.mk>
>>
>> +CFLAGS+= ${CWARNFLAGS.${COMPILER_TYPE}}
>> +
>> beforedepend ${OBJS}: machine
>>
>> Could you detail detail how you're switching to gcc so I an run a
>> full pass on that too?
Sometimes I use CC=gccNN, where gccNN is somwhere in $PATH. Sometimes
it is a script to select an arch-dependent gcc. This is unlikely to
work for makeworld but it works for kernels.
> Yep, but then I had to do this because ef->off is 64 bits even on 32
> bit arches, so I got a pointer/int size mismatch warning...
gcc detects this error too.
> Index: common/load_elf.c
> ===================================================================
> --- common/load_elf.c (revision 293796)
> +++ common/load_elf.c (working copy)
> @@ -886,7 +886,7 @@ __elfN(parse_modmetadata)(struct preloaded_file *f
> error = __elfN(reloc_ptr)(fp, ef, v, &md, sizeof(md));
> if (error == EOPNOTSUPP) {
> md.md_cval += ef->off;
> - md.md_data = (void *)((uintptr_t)md.md_data + ef->off);
> + md.md_data = (void *)(uintptr_t)((uintptr_t)md.md_data +
> ef->off);
> } else if (error != 0)
> return (error);
> #endif
>
>
> That is just some special kind of ugly. Fixing warnings is supposed to
> lead to better code, but all this casting isn't better, it's just an
> unreadable mess. Man I miss the days when C was just a really powerful
> macro assembler. :)
This is made extra-ugly by misformatting it. Fixing warnings
unfortunately usually leads to worse code, using extra code to break
the warning.
Here the detected bug is the bogus type for ef->off. Values >=
UINT32_MAX in it cannot work in expressions like this. This was only
detected accidentally.
md_data is a very confusing variable name. It is used nearby in 4 structs
band has a different type in each. Sometimes it is uint32_t or
uint64_t; sometimes it is void * and sometimes it is char *. Here it
is void *.
Some of the structs are:
- mod_medadata (md is this); type void *
- mod_metadata64; type uint64_t
- mod_metadata32; type uint32_t
- file_metadata; type char []
The prefix is supposed to be unique in context. One is better than none.
'off' in ef has none.
The void * type is inconvenient to work with. The nearby md_cval has
the same bugs, except it isn't in file_metadata and its type is
const char * so you can add an integer to it without casting. The
previous round of fixes was to fix a warning about using the gnu
extension of adding an integer to a void * without a cast.
> + md.md_data = (void *)(uintptr_t)((uintptr_t)md.md_data +
> ef->off);
I don't see any better quick fix than changing 'off' to vm_offset_t
or maybe signed vm_offset_t. It is in a local struct so this seems
to be possible. Changing the void * instance of md_data to an integer
is harder.
md_cval should also be an integer.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160113124535.X982>