Paths

Table of Contentst

Fix build of `stand/` with base gcc
ClosedPublic
Actions

Authored by dim on May 30 2018, 8:23 PM.

Details

Reviewers

imp
kevans
emaste
rgrimes

Group Reviewers

bhyve

Commits

rS334572: MFC r334432:
rS334432: Fix build of stand with base gcc
rS334416: Note the need for a new kernel / userland for devinfo to work

Summary

I tried building world with base gcc, and one of the parts that fails is
stand/, with several different types of errors.

The first type of error is:

cc1: warnings being treated as errors
/usr/src/stand/i386/gptboot/gptboot.c: In function 'main':
/usr/src/stand/i386/gptboot/gptboot.c:258: warning: declaration of 'autoboot' shadows a global declaration
/usr/src/stand/common/bootstrap.h:64: warning: shadowed declaration is here

This is because the autoboot() function is now declared in
bootstrap.h, and several boot loaders use autoboot also as a local
variable. To minimize churn, I propose to make the autoboot()
function static for stand/common/boot.c, since there are no external
callers. Alternatively, it can be renamed to autoboot_actual(), or
something like that.

The second error is:

cc1: warnings being treated as errors
/usr/src/stand/userboot/userboot/main.c: In function 'extract_currdev':
/usr/src/stand/userboot/userboot/main.c:170: warning: dereferencing type-punned pointer will break strict-aliasing rules

gcc is right here, it *is* dereferencing it, so let's just insert a
plain memcpy instead.

The third type of error is:

/usr/bin/gcc  -O2 -pipe   -I/usr/src/stand/i386/btx/lib -nostdinc -I/usr/obj/usr/src/amd64.amd64/stand/libsa32 -I/usr/src/stand/libsa -D_STANDALONE -I/usr/src/sys -Ddouble=jagged-little-pill -Dfloat=floaty-mcfloatface -DLOADER_GELI_SUPPORT -I/usr/src/stand/geli -DLOADER_DISK_SUPPORT -m32 -mcpu=i386 -ffreestanding -mno-mmx -mno-sse  -msoft-float -march=i386 -I. -DBOOTPROG=\"gptboot\"  -O1  -DGPT  -DUFS1_AND_UFS2  -DSIOPRT=0x3f8  -DSIOFMT=0x3  -DSIOSPD=9600  -I/usr/src/stand/common  -I/usr/src/stand/i386/common  -I/usr/src/stand/i386/boot2  -Wall -Waggregate-return -Wbad-function-cast -Wno-cast-align  -Wmissing-declarations -Wmissing-prototypes -Wnested-externs  -Wpointer-arith -Wshadow -Wstrict-prototypes -Wwrite-strings  -Winline -Wno-pointer-sign -g -std=gnu99 -Wsystem-headers -Werror -Wno-pointer-sign   -mpreferred-stack-boundary=2 --param max-inline-insns-single=100  -c /usr/src/stand/i386/gptboot/gptboot.c -o gptboot.o
`-mcpu=' is deprecated. Use `-mtune=' or '-march=' instead.
cc1: warnings being treated as errors
/usr/src/sys/geom/eli/g_eli.h: In function 'geli_taste':
/usr/src/sys/geom/eli/g_eli.h:339: warning: inlining failed in call to 'eli_metadata_decode_v0': --param max-inline-insns-single limit reached
/usr/src/sys/geom/eli/g_eli.h:400: warning: called from here
/usr/src/sys/geom/eli/g_eli.h:365: warning: inlining failed in call to 'eli_metadata_decode_v1v2v3v4v5v6v7': --param max-inline-insns-single limit reached
/usr/src/sys/geom/eli/g_eli.h:409: warning: called from here
/usr/src/sys/geom/eli/g_eli.h:339: warning: inlining failed in call to 'eli_metadata_decode_v0': --param max-inline-insns-single limit reached
/usr/src/sys/geom/eli/g_eli.h:400: warning: called from here
/usr/src/sys/geom/eli/g_eli.h:365: warning: inlining failed in call to 'eli_metadata_decode_v1v2v3v4v5v6v7': --param max-inline-insns-single limit reached
/usr/src/sys/geom/eli/g_eli.h:409: warning: called from here
*** Error code 1

Stop.
make[2]: stopped in /usr/src/stand/i386/gptboot
*** Error code 1

This is gcc warning us that it failed to inline, but I fail to see the
value of this warning. Do we really care, and is it absolutely critical
that this function is inlined? If so, it is better to mark the function
as __forceinline, if old gcc supports that. But I'd like to propose
getting rid of -Winline instead, there is no useful information in
these warnings.

The fourth type of error is:

cc1: warnings being treated as errors
/usr/src/stand/libsa/cd9660read.c: In function 'cd9660_lookup':
/usr/src/stand/libsa/cd9660read.c:176: warning: 'len' may be used uninitialized in this function
/usr/src/stand/libsa/cd9660read.c:176: note: 'len' was declared here

Here base gcc's compile-time analysis is too dumb to realize that len
can never be used, but let's initialize it to zero just in case.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16929
Build 16806: arc lint + arc unit

Event Timeline

dim created this revision.May 30 2018, 8:23 PM

Herald added a reviewer: rgrimes. · View Herald TranscriptMay 30 2018, 8:23 PM

Harbormaster completed remote builds in B16929: Diff 43172.May 30 2018, 8:23 PM

imp added inline comments.May 30 2018, 8:30 PM

stand/userboot/userboot/main.c
170	this feels wrong. I thought we'd fixed all the type punning, so there may be a less unsafe way to do that.

imp added inline comments.May 30 2018, 8:31 PM

stand/userboot/userboot/main.c
170	dev = zdev.dd;

imp added inline comments.May 30 2018, 8:36 PM

stand/libsa/cd9660read.c
176	len is initialized below, so this is a false positive.

dim added inline comments.May 30 2018, 8:42 PM

stand/userboot/userboot/main.c
170	That doesn't work: /usr/src/stand/userboot/userboot/main.c: In function 'extract_currdev': /usr/src/stand/userboot/userboot/main.c:170: error: incompatible types in assignment `dev` is a `struct disk_devdesc`, while `zdev.dd` is a `struct devdesc`. It looks like the construction was meant to force copying incompatible devdesc structs, assuming that they approximately have the same memory layout. `struct disk_devdesc` has: struct disk_devdesc { struct devdesc dd; int d_slice; int d_partition; uint64_t d_offset; }; while `struct zfs_devdesc` has: struct zfs_devdesc { struct devdesc dd; uint64_t pool_guid; uint64_t root_guid; }; Ignoring any padding, the structs will match in size, though it is probably better to add a __packed attribute to be 100% sure. Or manually copy the individual fields.

dim added inline comments.May 30 2018, 8:43 PM

stand/libsa/cd9660read.c
176	Yeah, I remarked in the review that it is due to base gcc's limited compile time analysis. Initializing it is the cheapest way to get rid of the warning. Otherwise, we'll have to add -Wno-uninitialized to the compile flags for gcc <= 4.2.

imp added inline comments.May 30 2018, 8:43 PM

stand/userboot/userboot/main.c
170	Then this copying is wrong. Full stop. We need to find the right way to fix it.

imp added inline comments.May 30 2018, 9:33 PM

stand/userboot/userboot/main.c
170	https://reviews.freebsd.org/D15629 has this fix

rgrimes mentioned this in D15629: Pass a struct devdesc to the format commands. Use proper type rather than doing weird type-punning that happened to work because the size was right :(.May 30 2018, 9:43 PM

rgrimes added a reviewer: bhyve.

emaste added inline comments.May 31 2018, 12:48 AM

stand/libsa/cd9660read.c
176	I'd be fine with `-Wno-uninitialized` or turn off werror for old gcc or initializing it, but if we do keep this way please add a comment like /* Unnecessary initialization to appease GCC 4.2.1 */

This revision was not accepted when it landed; it landed in state Needs Review.May 31 2018, 2:58 AM

Closed by commit rS334416: Note the need for a new kernel / userland for devinfo to work (authored by imp). · Explain Why

This revision was automatically updated to reflect the committed changes.

wrong review

I'm sorry, but it looks like my change replaced this review...

Re-uploading, with a few changes:

Removed the userboot changes, since rS334412 is a better fix
Use -Wno-unitialized with gcc 4.2.1 for cd9660read.c
Use -march=i386 instead of -mcpu=i386 in defs.mk. The gcc documentation states that -mcpu is a deprecated synonym for -mtune, but -mtune does not prevent instructions "higher" than i386, it merely tunes for them.

Harbormaster completed remote builds in B16946: Diff 43199.May 31 2018, 1:14 PM

In D15628#330215, @dim wrote:

Use -march=i386 instead of -mcpu=i386 in defs.mk. The gcc documentation states that -mcpu is a deprecated synonym for -mtune, but -mtune does not prevent instructions "higher" than i386, it merely tunes for them.

The current floor is i486. Maybe we should consider using that instead, but I'm unsure what effects that will have on code size.

These look good. Sorry for my screw up on the review number.

stand/defs.mk
94 ↗	(On Diff #43199)	This looks good, though I might consider i486. The -mcpu=i386 was cargo-culted through many generations of Makefile, and misinterpreted along the way to mean 32-bit code (as distinct from 64-bit code). I think this is more right having now read the docs.