Date: Sun, 19 Jun 2011 21:26:41 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: TAKAHASHI Yoshihiro <nyan@FreeBSD.org> Cc: benl@FreeBSD.org, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r223262 - in head: cddl/contrib/opensolaris/lib/libdtrace/common contrib/binutils/bfd contrib/binutils/gas contrib/binutils/gas/config contrib/binutils/ld contrib/binutils/opcodes contr... Message-ID: <20110619193320.Y917@besplex.bde.org> In-Reply-To: <20110619.114704.59640143160110864.nyan@FreeBSD.org> References: <201106181356.p5IDuXhW044171@svn.freebsd.org> <20110619.114704.59640143160110864.nyan@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 19 Jun 2011, TAKAHASHI Yoshihiro wrote:
> In article <201106181356.p5IDuXhW044171@svn.freebsd.org>
> Ben Laurie <benl@freebsd.org> writes:
>>
>> Log:
>> Fix clang warnings.
>>
>> Modified: head/sys/sys/diskpc98.h
>> ==============================================================================
>> --- head/sys/sys/diskpc98.h Sat Jun 18 13:54:36 2011 (r223261)
>> +++ head/sys/sys/diskpc98.h Sat Jun 18 13:56:33 2011 (r223262)
>> @@ -36,8 +36,11 @@
>> #include <sys/ioccom.h>
>>
>> #define DOSBBSECTOR 0 /* DOS boot block relative sector number */
>> +#undef DOSPARTOFF
>> #define DOSPARTOFF 0
>> +#undef DOSPARTSIZE
>> #define DOSPARTSIZE 32
>> +#undef NDOSPART
>> #define NDOSPART 16
>> #define DOSMAGICOFFSET 510
>> #define DOSMAGIC 0xAA55
>> @@ -52,6 +55,7 @@
>>
>> #define DOSMID_386BSD (PC98_MID_386BSD | PC98_MID_BOOTABLE)
>> #define DOSSID_386BSD (PC98_SID_386BSD | PC98_SID_ACTIVE)
>> +#undef DOSPTYP_386BSD
>> #define DOSPTYP_386BSD (DOSSID_386BSD << 8 | DOSMID_386BSD)
>>
>> struct pc98_partition {
>>
>
> I wonder why this is needed, and why only for diskpc98.h, not
> diskmbr.h.
This is needed to break the warning even more. There are are enormous
number of layers of brokenness:
o These header files unfortunately define some ioctls (just 1), so
kdump/mkioctls generates an include of both in ioctl.c. This causes
conflicts. The conflicts should cause errors (though the conflicting
definitions aren't actually used in ioctl.c), but they only generates
warnings.
o If you compile ioctl.c standalone to test this, then it compiles with
no warnings with both gcc and clang, since -Wno-system-headers is the
default for gcc and apparently for clang too. However, for world builds
there is magic that results in <sys> being
$(realpath /usr/obj)/src/tmp/usr/include/sys instead of just
/usr/include/sys. I'm not sure if clang only is confused by this so
that -Wno-system-headers doesn't break the warnings for clang only,
or if the warnings have always been happening and they are just more
obvious with clang colorizing them.
o The build system knows about -Wno-system-headers breaking warnings, so
it puts -Wsystem-headers in CFLAGS to turn this off. But it only does
this at WARNS >= 1, and kdump still uses WARNS = 0.
o Poor structuring of the disk headers. I've always thought that
diskmbr.h shouldn't exist. It doesn't describe "the" disk mbr
structure, but only the "i386" one. It should have been named
something like diski386.h. The current problem shows that these
files should have been purely MD so that they can be kept separate.
Their names should have been something like <machine/diskmbr.h>.
Or for simplicity, keep all of their definitions with ifdefs in
<sys/disklabel.h> like they used to be as recently as FreeBSD-4.
disklabel.h still has some ugly unsorted MD ifdefs (just 2, for
LABELSECTOR and LABELOFFSET). Of course, definitions for MBRs
should never have been in disklabel.h. For simplicity with fewer
hacks, put all the MBR definitions with ifdefs in <sys/diskmbr.h>.
The current problem shows that many ifdefs are needed with the
current structuring anyway. We only escape having to ifdef everything
in both of the files (or complications in mkioctls) because some names
are different (e.g., struct pc98_partition instead of struct
dos_partition, and more importantly, DIOCSPC98 instead of DIOCSMBR --
the latter allows both ioctls to be defined in ioctl.c, though only
the pc98 is normally used on pc98. BTW, can pc98 handle i386-formatted
disks? MacOS supports i386-mbr formatted USB drives better than
WinXP does -- WinXP can only mount FAT32 partitions in MBR slot 1,
though it can recognize them in any slot. Such support is easiest
if all the mbr ioctls are available in all arches. But I now think
that depending on any disk ioctl in an application like fdisk or
newfs is a bug. Such applications should be able to work on any
"direct addressable" file (including regular files and foreign
disks). Some Linux disk applications now work better on FreeBSD than
FreeBSD ones do, since they don't depend so much on ioctls :-().
This is only "needed" for diskpc98.h because 'm' is less than 'p', so
diskmbr.h is always included before diskpc98.h. I thought at first
that the order of the includes could not be depended on, because the
find(1) in mkioctls produced output in (unsorted) tree traversal order.
But the find is actually on "*" in each directory in the include path,
so the shell will sort the includes alphabetically within each directory.
I now remember that this is intentional, to get the same breakage as
an application which complies with style(9) will get if there are any
bogus ordering requirements for directories that are not accidentally
satisified by keeping the includes sorted. See the /* XXX obnoxious
prerequisites. */ section in mkioctls. This intentionally includes
a few headers out of order, to work around cases where the normal
order doesn't work. One of these headers is disklabel.h, which
might not be needed there any more now that it has been cleaned up.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110619193320.Y917>