Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
On Tue, Jul 17, 2018 at 09:39:03AM +0100, Andrew Cooper wrote: > On 17/07/2018 07:57, Jan Beulich wrote: > On 16.07.18 at 18:56, wrote: > >> On 16/07/18 17:46, Jan Beulich wrote: > >>> For a reason that I can't explain, it is only the shim build that fails > >>> for me with an older gcc due to the compiler not recognizing that > >>> apparently uninitialized variables aren't really uninitialized. > >> The only thing that comes to mind is some differences in CFLAGS et al. > >> There is nothing in kconfig which would plausibly impact that code. > > Oh, right - looks like the shim build inherits the tool stack's CFLAGS, > > which specify -O0 in debug builds. Otoh I'm surprised the hypervisor > > builds at all with -O0, so I guess I'll have to look at that in some > > more detail, not the least because xen/Rules.mk uses += to insert > > its own -O. In any event, comparing the two object files clearly > > suggests a difference in optimization level. > > Hmm - interesting. I had intended not to inherit the tools dubious > CFLAGS setting (nothing should use O0, not even for debugging), but how > that I think back, that probably fell through the cracks in the > associated chaos. > > Nevertheless, all of our code should build at any optimisation level. I > know from other attempts that Xen also doesn't build at O3, and we could > do with a way to select the optimisation level, independently of debug > settings. I agree with what you said but unfortunately reworking the build system is rather low on my priority list. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
> From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Wednesday, July 18, 2018 5:39 PM > > >>> On 18.07.18 at 11:36, wrote: > > On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote: > >> For a reason that I can't explain, it is only the shim build that fails > >> for me with an older gcc due to the compiler not recognizing that > >> apparently uninitialized variables aren't really uninitialized. Pull out > >> the assignments used by two of the three case blocks and make them > >> initializers of the variables, as I think I had suggested during review. > >> > >> Signed-off-by: Jan Beulich > > > > Code: > > > > Reviewed-by: Wei Liu > > > > But please update the commit message to say it is caused by compiler > > optimisation level. > > Sure, now that it's clear what the reason is. It now reads: > > Older gcc at -O2 (and perhaps higher) does not recognize that apparently > uninitialized variables aren't really uninitialized. Pull out the > assignments used by two of the three case blocks and make them > initializers of the variables, as I think I had suggested during review. > Acked-by: Kevin Tian ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
On Wed, Jul 18, 2018 at 03:39:20AM -0600, Jan Beulich wrote: > >>> On 18.07.18 at 11:36, wrote: > > On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote: > >> For a reason that I can't explain, it is only the shim build that fails > >> for me with an older gcc due to the compiler not recognizing that > >> apparently uninitialized variables aren't really uninitialized. Pull out > >> the assignments used by two of the three case blocks and make them > >> initializers of the variables, as I think I had suggested during review. > >> > >> Signed-off-by: Jan Beulich > > > > Code: > > > > Reviewed-by: Wei Liu > > > > But please update the commit message to say it is caused by compiler > > optimisation level. > > Sure, now that it's clear what the reason is. It now reads: > > Older gcc at -O2 (and perhaps higher) does not recognize that apparently > uninitialized variables aren't really uninitialized. Pull out the > assignments used by two of the three case blocks and make them > initializers of the variables, as I think I had suggested during review. LGTM. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
>>> On 18.07.18 at 11:36, wrote: > On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote: >> For a reason that I can't explain, it is only the shim build that fails >> for me with an older gcc due to the compiler not recognizing that >> apparently uninitialized variables aren't really uninitialized. Pull out >> the assignments used by two of the three case blocks and make them >> initializers of the variables, as I think I had suggested during review. >> >> Signed-off-by: Jan Beulich > > Code: > > Reviewed-by: Wei Liu > > But please update the commit message to say it is caused by compiler > optimisation level. Sure, now that it's clear what the reason is. It now reads: Older gcc at -O2 (and perhaps higher) does not recognize that apparently uninitialized variables aren't really uninitialized. Pull out the assignments used by two of the three case blocks and make them initializers of the variables, as I think I had suggested during review. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote: > For a reason that I can't explain, it is only the shim build that fails > for me with an older gcc due to the compiler not recognizing that > apparently uninitialized variables aren't really uninitialized. Pull out > the assignments used by two of the three case blocks and make them > initializers of the variables, as I think I had suggested during review. > > Signed-off-by: Jan Beulich Code: Reviewed-by: Wei Liu But please update the commit message to say it is caused by compiler optimisation level. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
>>> On 16.07.18 at 18:56, wrote: > On 16/07/18 17:46, Jan Beulich wrote: >> For a reason that I can't explain, it is only the shim build that fails >> for me with an older gcc due to the compiler not recognizing that >> apparently uninitialized variables aren't really uninitialized. > > The only thing that comes to mind is some differences in CFLAGS et al. > There is nothing in kconfig which would plausibly impact that code. Indeed - I was under the wrong impression that one of my configs on that box had CONFIG_DEBUG=n, but that was wrong. Switching the supposed config back into that mode makes the issue surface in a non-shim build as well. The difference is -O1 vs -O2; the latter allows the compiler to kind of see "deeper" through expressions. So I think the patch should be considered as is, perhaps with the extra question whether to make vmx_add_msr() match the two changed functions, even if the problem doesn't surface there (presumably due to the "return" in the initial switch statement). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
On 17/07/2018 07:57, Jan Beulich wrote: On 16.07.18 at 18:56, wrote: >> On 16/07/18 17:46, Jan Beulich wrote: >>> For a reason that I can't explain, it is only the shim build that fails >>> for me with an older gcc due to the compiler not recognizing that >>> apparently uninitialized variables aren't really uninitialized. >> The only thing that comes to mind is some differences in CFLAGS et al. >> There is nothing in kconfig which would plausibly impact that code. > Oh, right - looks like the shim build inherits the tool stack's CFLAGS, > which specify -O0 in debug builds. Otoh I'm surprised the hypervisor > builds at all with -O0, so I guess I'll have to look at that in some > more detail, not the least because xen/Rules.mk uses += to insert > its own -O. In any event, comparing the two object files clearly > suggests a difference in optimization level. Hmm - interesting. I had intended not to inherit the tools dubious CFLAGS setting (nothing should use O0, not even for debugging), but how that I think back, that probably fell through the cracks in the associated chaos. Nevertheless, all of our code should build at any optimisation level. I know from other attempts that Xen also doesn't build at O3, and we could do with a way to select the optimisation level, independently of debug settings. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
>>> On 16.07.18 at 18:56, wrote: > On 16/07/18 17:46, Jan Beulich wrote: >> For a reason that I can't explain, it is only the shim build that fails >> for me with an older gcc due to the compiler not recognizing that >> apparently uninitialized variables aren't really uninitialized. > > The only thing that comes to mind is some differences in CFLAGS et al. > There is nothing in kconfig which would plausibly impact that code. Oh, right - looks like the shim build inherits the tool stack's CFLAGS, which specify -O0 in debug builds. Otoh I'm surprised the hypervisor builds at all with -O0, so I guess I'll have to look at that in some more detail, not the least because xen/Rules.mk uses += to insert its own -O. In any event, comparing the two object files clearly suggests a difference in optimization level. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] VMX: fix vmx_{find,del}_msr() build
On 16/07/18 17:46, Jan Beulich wrote: > For a reason that I can't explain, it is only the shim build that fails > for me with an older gcc due to the compiler not recognizing that > apparently uninitialized variables aren't really uninitialized. The only thing that comes to mind is some differences in CFLAGS et al. There is nothing in kconfig which would plausibly impact that code. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
