Hi! The xen-cast.diff adds the -Wcast-qual flag. The xen-cast-xen.diff makes the hypervisor itself buildable with the new flag - at least on x86_32 and x86_64. The xen-cast-tools.diff makes the tools build with the new flag. Generally, the places with the __UNCONST() hack should be considered to be reworked. These places may hide bugs. Christoph _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 21/12/06 10:05, "Christoph Egger" <Christoph.Egger@amd.com> wrote:> The xen-cast.diff adds the -Wcast-qual flag.We don''t enable this flag precisely because we end up needing to scatter const all over the place like confetti. I''m not convinced that use of const improves code quality. I fear we''d end up with ''const foo * const bar'' all over the place -- ugly and confusing to most programmers (who frequently mess up const placement, in my experience). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thursday 21 December 2006 11:41, Keir Fraser wrote:> On 21/12/06 10:05, "Christoph Egger" <Christoph.Egger@amd.com> wrote: > > The xen-cast.diff adds the -Wcast-qual flag. > > We don''t enable this flag precisely because we end up needing to scatter > const all over the place like confetti.That doesn''t imply that this is bad. Compilers may generate better code.> I''m not convinced that use of const improves code quality.In my experience, exactly this will happen.> I fear we''d end up with ''const foo * const bar'' all > over the place -- ugly and confusing to most programmers (who frequently > mess up const placement, in my experience).Exactly these programmers will stop doing this, when the compiler tells them. With -Wcast-qual you find code that does things like this: int foo(const char *str) { char *s = str; int i; .... s[i] = ''\0''; } Modifying read-only memory is IMO never a good idea, in particular, when it is shared with something else. And there are indeed places in the xen code, where this happens. (Example: Look how canonicalize() is used in tools/xenstored/*, Look at copy_to_user_hvm() in xen/arch/x86/hvm/platform.c ) Please have a closer look at the patches. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Keir Fraser <keir@xensource.com> 21.12.06 11:41 >>> >On 21/12/06 10:05, "Christoph Egger" <Christoph.Egger@amd.com> wrote: > >> The xen-cast.diff adds the -Wcast-qual flag. > >We don''t enable this flag precisely because we end up needing to scatter >const all over the place like confetti. I''m not convinced that use of const >improves code quality. I fear we''d end up with ''const foo * const bar'' all >over the place -- ugly and confusing to most programmers (who frequently >mess up const placement, in my experience).While I''d really like to see all Xen code become const-correct (because I do believe this helps with code quality, like does marking read only data read only in the page tables), I don''t think adding the warning here is appropriate - after all, in C this is one of the purposes of adding explicit casts (although I agree it''s questionable to use casts for this purpose). And no, I don''t think that many people screwing up const placement is a proper argument for not using const where possible. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thursday 21 December 2006 13:51, Jan Beulich wrote:> >>> Keir Fraser <keir@xensource.com> 21.12.06 11:41 >>> > > > >On 21/12/06 10:05, "Christoph Egger" <Christoph.Egger@amd.com> wrote: > >> The xen-cast.diff adds the -Wcast-qual flag. > > > >We don''t enable this flag precisely because we end up needing to scatter > >const all over the place like confetti. I''m not convinced that use of > > const improves code quality. I fear we''d end up with ''const foo * const > > bar'' all over the place -- ugly and confusing to most programmers (who > > frequently mess up const placement, in my experience). > > While I''d really like to see all Xen code become const-correct (because I > do believe this helps with code quality, like does marking read only data > read only in the page tables), I don''t think adding the warning here is > appropriateIMO, it is. It is one step towards to do more type-safe coding.> - after all, in C this is one of the purposes of adding > explicit casts (although I agree it''s questionable to use casts for this > purpose). And no, I don''t think that many people screwing up const > placement is a proper argument for not using const where possible.Full ack. Christoph _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 21/12/06 12:51, "Jan Beulich" <jbeulich@novell.com> wrote:> While I''d really like to see all Xen code become const-correct (because I do > believe this helps with code quality, like does marking read only data read > only in the page tables), I don''t think adding the warning here is > appropriate - after all, in C this is one of the purposes of adding explicit > casts (although I agree it''s questionable to use casts for this purpose). > And no, I don''t think that many people screwing up const placement is a > proper argument for not using const where possible.Well, I''m inclined to take the clean-up parts of the patches but leave out __UNCONST() and -Wcast-qual. I really dislike the compiler thinking it knows better than an explicit cast, and casting via unsigned long is just a kludge. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Keir Fraser <keir@xensource.com> 21.12.06 14:28 >>> >On 21/12/06 12:51, "Jan Beulich" <jbeulich@novell.com> wrote: > >> While I''d really like to see all Xen code become const-correct (because I do >> believe this helps with code quality, like does marking read only data read >> only in the page tables), I don''t think adding the warning here is >> appropriate - after all, in C this is one of the purposes of adding explicit >> casts (although I agree it''s questionable to use casts for this purpose). >> And no, I don''t think that many people screwing up const placement is a >> proper argument for not using const where possible. > >Well, I''m inclined to take the clean-up parts of the patches but leave out >__UNCONST() and -Wcast-qual. I really dislike the compiler thinking it knows >better than an explicit cast, and casting via unsigned long is just a >kludge.This I completely agree with. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thursday 21 December 2006 15:18, Jan Beulich wrote:> >>> Keir Fraser <keir@xensource.com> 21.12.06 14:28 >>> > > > >On 21/12/06 12:51, "Jan Beulich" <jbeulich@novell.com> wrote: > >> While I''d really like to see all Xen code become const-correct (because > >> I do believe this helps with code quality, like does marking read only > >> data read only in the page tables), I don''t think adding the warning > >> here is appropriate - after all, in C this is one of the purposes of > >> adding explicit casts (although I agree it''s questionable to use casts > >> for this purpose). And no, I don''t think that many people screwing up > >> const placement is a proper argument for not using const where possible. > > > >Well, I''m inclined to take the clean-up parts of the patches but leave out > >__UNCONST() and -Wcast-qual. I really dislike the compiler thinking it > > knows better than an explicit cast, and casting via unsigned long is just > > a kludge. > > This I completely agree with.Me, too. I mentioned in my first mail, these parts with __UNCONST() must be reworked to get rid of it. There was no obvious solution for me at least. If someone knows a way, I am for it. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel