Skip site navigation (1)Skip section navigation (2)
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>