Re: const*ify cfattach
On Fri, Feb 14, 2020 at 06:08:31PM +0100, Martin Pieuchot wrote: > On 13/02/20(Thu) 17:23, Visa Hankala wrote: > > On Thu, Feb 13, 2020 at 06:07:01PM +0100, Martin Pieuchot wrote: > > > On 13/02/20(Thu) 16:53, Visa Hankala wrote: > > > [...] > > > > I wonder if this constification should also be reflected in the output > > > > of config(8). > > > > > > What do you mean? > > > > The program generates file ioconf.c that contains lines like these: > > > > extern struct cfattach nsphy_ca; > > extern struct cfattach nsphyter_ca; > > extern struct cfattach inphy_ca; > > extern struct cfattach iophy_ca; > > extern struct cfattach rlphy_ca; > > extern struct cfattach lxtphy_ca; > > extern struct cfattach ukphy_ca; > > extern struct cfattach brgphy_ca; > > extern struct cfattach rgephy_ca; > > extern struct cfattach ciphy_ca; > > extern struct cfattach scsibus_ca; > > extern struct cfattach cd_ca; > > extern struct cfattach sd_ca; > > > > These are non-const even though the actual struct instances are > > read-only. Later in the same file, the struct cfdata array references > > these structs through non-const pointers. This has worked so far, but > > the situation is not ideal because the compiler makes wrong assumptions. > > Below is a full diff that: > - add the "const" prefix to cfattach in config(8) > - add it, when required, to kern/subr_autoconf.c and sys/device.h > - drop the keyword in dev/rd.c Please commit your initial const struct cfattach diff without the rd.c part first. You have my OK for that. The change should be fine because there already are many existing instances of the use of const with struct cfattach. The config(8) change should be reviewed separately. Given that the cf_attach pointer of struct cfdata gets the const qualifier, couldn't rd_ca now become const as well? To find any inconsistencies of autoconf structs quickly, it would be nice if config(8) put the extern declarations in a header file, say "ioconf.h". Then this header could be included in driver files, maybe as part of under #ifdef _KERNEL. However, this would create a point that easily triggers a rebuild of a large portion of the kernel. Hence it might be easiest to allow looseness and rely on discipline. (:
Re: const*ify cfattach
Visa Hankala wrote: > On Thu, Feb 13, 2020 at 12:00:35PM +0100, Martin Pieuchot wrote: > > These structures are only used by autoconf(9) and don't need to be > > modified. Some subsystems already define most of them as 'const'. > > Diff below turn all the remaining one as such. > > > > Only a single driver, de(4), needed a modification apart from adding > > the const: removing a forward definition fixed it ;) > > > > Built for all archs, I tested i386, amd64 and sparc64. > > > > Ok? > > No, RAMDISK build fails. I continue to be very dissapointed when developers don't do complete snapshot builds as test.
Re: const*ify cfattach
On Thu, Feb 13, 2020 at 06:07:01PM +0100, Martin Pieuchot wrote: > On 13/02/20(Thu) 16:53, Visa Hankala wrote: > > On Thu, Feb 13, 2020 at 12:00:35PM +0100, Martin Pieuchot wrote: > > > These structures are only used by autoconf(9) and don't need to be > > > modified. Some subsystems already define most of them as 'const'. > > > Diff below turn all the remaining one as such. > > > > > > Only a single driver, de(4), needed a modification apart from adding > > > the const: removing a forward definition fixed it ;) > > > > > > Built for all archs, I tested i386, amd64 and sparc64. > > > > > > Ok? > > > > No, RAMDISK build fails. > > > > /usr/src/sys/dev/rd.c:87:15: error: assigning to 'struct cfattach *' from > > 'const struct cfattach *' discards qualifiers > > [-Werror,-Wincompatible-pointer-types-discards-qualifiers] > > cf.cf_attach = &rd_ca; > > ^ ~~ > > Removing const from rd(4) fixed it here in my tree. Thanks! > > > I wonder if this constification should also be reflected in the output > > of config(8). > > What do you mean? The program generates file ioconf.c that contains lines like these: extern struct cfattach nsphy_ca; extern struct cfattach nsphyter_ca; extern struct cfattach inphy_ca; extern struct cfattach iophy_ca; extern struct cfattach rlphy_ca; extern struct cfattach lxtphy_ca; extern struct cfattach ukphy_ca; extern struct cfattach brgphy_ca; extern struct cfattach rgephy_ca; extern struct cfattach ciphy_ca; extern struct cfattach scsibus_ca; extern struct cfattach cd_ca; extern struct cfattach sd_ca; These are non-const even though the actual struct instances are read-only. Later in the same file, the struct cfdata array references these structs through non-const pointers. This has worked so far, but the situation is not ideal because the compiler makes wrong assumptions.
Re: const*ify cfattach
On 13/02/20(Thu) 16:53, Visa Hankala wrote: > On Thu, Feb 13, 2020 at 12:00:35PM +0100, Martin Pieuchot wrote: > > These structures are only used by autoconf(9) and don't need to be > > modified. Some subsystems already define most of them as 'const'. > > Diff below turn all the remaining one as such. > > > > Only a single driver, de(4), needed a modification apart from adding > > the const: removing a forward definition fixed it ;) > > > > Built for all archs, I tested i386, amd64 and sparc64. > > > > Ok? > > No, RAMDISK build fails. > > /usr/src/sys/dev/rd.c:87:15: error: assigning to 'struct cfattach *' from > 'const struct cfattach *' discards qualifiers > [-Werror,-Wincompatible-pointer-types-discards-qualifiers] > cf.cf_attach = &rd_ca; > ^ ~~ Removing const from rd(4) fixed it here in my tree. Thanks! > I wonder if this constification should also be reflected in the output > of config(8). What do you mean?
Re: const*ify cfattach
On Thu, Feb 13, 2020 at 12:00:35PM +0100, Martin Pieuchot wrote: > These structures are only used by autoconf(9) and don't need to be > modified. Some subsystems already define most of them as 'const'. > Diff below turn all the remaining one as such. > > Only a single driver, de(4), needed a modification apart from adding > the const: removing a forward definition fixed it ;) > > Built for all archs, I tested i386, amd64 and sparc64. > > Ok? No, RAMDISK build fails. /usr/src/sys/dev/rd.c:87:15: error: assigning to 'struct cfattach *' from 'const struct cfattach *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] cf.cf_attach = &rd_ca; ^ ~~ I wonder if this constification should also be reflected in the output of config(8).
