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...

[Bruce Evans]

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