Message ID | 1457403820-2949-2-git-send-email-cardoe@cardoe.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: > The build should not use -O0 as that results in miscompilations. There This needs some (concrete) references. Is that a known issue in gcc? If so can you reference the bug number? > have been a few instances on the ML where users were told to switch > from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The > preferred route should be to use -Og if its available, otherwise use > -O1 which is the default. This change undoes the change from -O1 to -O0 gcc manual says -O0 is the default. Not that I disagree with this patch in general, but the commit message seems a bit misleading. > in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. > And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for more input. > Signed-off-by: Doug Goldstein <cardoe@cardoe.com> > --- > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > --- > tools/Rules.mk | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/Rules.mk b/tools/Rules.mk > index 9ef0b47..ae6b01f 100644 > --- a/tools/Rules.mk > +++ b/tools/Rules.mk > @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) > > ifeq ($(debug),y) > # Disable optimizations and enable debugging information for macros > -CFLAGS += -O0 -g3 > +$(call cc-option-add,CFLAGS,CC,-Og) > +CFLAGS += -g3 > # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. > PY_CFLAGS += $(PY_NOOPT_CFLAGS) > endif > -- > 2.4.10 >
On 3/8/16 9:38 AM, Wei Liu wrote: > On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >> The build should not use -O0 as that results in miscompilations. There > > This needs some (concrete) references. Is that a known issue in gcc? If > so can you reference the bug number? So its not really a bug in GCC but just the complete lack of optimizations in play. inlines aren't inlined. dead code elimination isn't run so things are much bigger. structures aren't padded the same way. This came about from reading reports on the -devel and -user's ML that were solved by building Xen with debug=n. I was also striving to reduce the duplication of CFLAGS that are passed on the command line of builds. > >> have been a few instances on the ML where users were told to switch >> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >> preferred route should be to use -Og if its available, otherwise use >> -O1 which is the default. This change undoes the change from -O1 to -O0 > > gcc manual says -O0 is the default. I wasn't clear about where the 'the default' came from. That's the default in the Xen tree (see: config/StdGNU.mk for example but every platform has -O1 set). > > Not that I disagree with this patch in general, but the commit message > seems a bit misleading. I can rewrite it. I'd also be willing to change the patch to prefer -Og if its available and use -O0 if its not. > >> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. >> > > And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for > more input. -O1 can optimize things out when you look at them with gdb but -Og is suppose to do the right thing. > >> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> >> --- >> CC: Ian Jackson <ian.jackson@eu.citrix.com> >> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> CC: Wei Liu <wei.liu2@citrix.com> >> --- >> tools/Rules.mk | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tools/Rules.mk b/tools/Rules.mk >> index 9ef0b47..ae6b01f 100644 >> --- a/tools/Rules.mk >> +++ b/tools/Rules.mk >> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) >> >> ifeq ($(debug),y) >> # Disable optimizations and enable debugging information for macros >> -CFLAGS += -O0 -g3 >> +$(call cc-option-add,CFLAGS,CC,-Og) >> +CFLAGS += -g3 >> # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. >> PY_CFLAGS += $(PY_NOOPT_CFLAGS) >> endif >> -- >> 2.4.10 >>
On Tue, Mar 08, 2016 at 10:34:42AM -0600, Doug Goldstein wrote: > On 3/8/16 9:38 AM, Wei Liu wrote: > > On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: > >> The build should not use -O0 as that results in miscompilations. There > > > > This needs some (concrete) references. Is that a known issue in gcc? If > > so can you reference the bug number? > > So its not really a bug in GCC but just the complete lack of > optimizations in play. inlines aren't inlined. dead code elimination > isn't run so things are much bigger. structures aren't padded the same way. > Urgh... > This came about from reading reports on the -devel and -user's ML that > were solved by building Xen with debug=n. I was also striving to reduce > the duplication of CFLAGS that are passed on the command line of builds. > I agree this is a good idea. > > > >> have been a few instances on the ML where users were told to switch > >> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The > >> preferred route should be to use -Og if its available, otherwise use > >> -O1 which is the default. This change undoes the change from -O1 to -O0 > > > > gcc manual says -O0 is the default. > > I wasn't clear about where the 'the default' came from. That's the > default in the Xen tree (see: config/StdGNU.mk for example but every > platform has -O1 set). > OK. I thought you're talking about something in the manual. > > > > Not that I disagree with this patch in general, but the commit message > > seems a bit misleading. > > I can rewrite it. I'd also be willing to change the patch to prefer -Og > if its available and use -O0 if its not. > No need to do it now because ... > > > >> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. > >> > > > > And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for > > more input. > > -O1 can optimize things out when you look at them with gdb but -Og is > suppose to do the right thing. > .. I don't know much about gcc so I would like to wait for Ian to give some input. Wei. > > > >> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> > >> --- > >> CC: Ian Jackson <ian.jackson@eu.citrix.com> > >> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> CC: Wei Liu <wei.liu2@citrix.com> > >> --- > >> tools/Rules.mk | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/tools/Rules.mk b/tools/Rules.mk > >> index 9ef0b47..ae6b01f 100644 > >> --- a/tools/Rules.mk > >> +++ b/tools/Rules.mk > >> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) > >> > >> ifeq ($(debug),y) > >> # Disable optimizations and enable debugging information for macros > >> -CFLAGS += -O0 -g3 > >> +$(call cc-option-add,CFLAGS,CC,-Og) > >> +CFLAGS += -g3 > >> # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. > >> PY_CFLAGS += $(PY_NOOPT_CFLAGS) > >> endif > >> -- > >> 2.4.10 > >> > > > -- > Doug Goldstein >
On 3/8/16 10:50 AM, Wei Liu wrote: > On Tue, Mar 08, 2016 at 10:34:42AM -0600, Doug Goldstein wrote: >> On 3/8/16 9:38 AM, Wei Liu wrote: >>> On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >>>> The build should not use -O0 as that results in miscompilations. There >>> >>> This needs some (concrete) references. Is that a known issue in gcc? If >>> so can you reference the bug number? >> >> So its not really a bug in GCC but just the complete lack of >> optimizations in play. inlines aren't inlined. dead code elimination >> isn't run so things are much bigger. structures aren't padded the same way. >> > > Urgh... > >> This came about from reading reports on the -devel and -user's ML that >> were solved by building Xen with debug=n. I was also striving to reduce >> the duplication of CFLAGS that are passed on the command line of builds. >> > > I agree this is a good idea. > >>> >>>> have been a few instances on the ML where users were told to switch >>>> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >>>> preferred route should be to use -Og if its available, otherwise use >>>> -O1 which is the default. This change undoes the change from -O1 to -O0 >>> >>> gcc manual says -O0 is the default. >> >> I wasn't clear about where the 'the default' came from. That's the >> default in the Xen tree (see: config/StdGNU.mk for example but every >> platform has -O1 set). >> > > OK. I thought you're talking about something in the manual. > >>> >>> Not that I disagree with this patch in general, but the commit message >>> seems a bit misleading. >> >> I can rewrite it. I'd also be willing to change the patch to prefer -Og >> if its available and use -O0 if its not. >> > > No need to do it now because ... > >>> >>>> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. >>>> >>> >>> And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for >>> more input. >> >> -O1 can optimize things out when you look at them with gdb but -Og is >> suppose to do the right thing. >> > > .. I don't know much about gcc so I would like to wait for Ian to give > some input. > > Wei. > >>> >>>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> >>>> --- >>>> CC: Ian Jackson <ian.jackson@eu.citrix.com> >>>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>> CC: Wei Liu <wei.liu2@citrix.com> >>>> --- >>>> tools/Rules.mk | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/Rules.mk b/tools/Rules.mk >>>> index 9ef0b47..ae6b01f 100644 >>>> --- a/tools/Rules.mk >>>> +++ b/tools/Rules.mk >>>> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) >>>> >>>> ifeq ($(debug),y) >>>> # Disable optimizations and enable debugging information for macros >>>> -CFLAGS += -O0 -g3 >>>> +$(call cc-option-add,CFLAGS,CC,-Og) >>>> +CFLAGS += -g3 >>>> # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. >>>> PY_CFLAGS += $(PY_NOOPT_CFLAGS) >>>> endif >>>> -- >>>> 2.4.10 >>>> >> >> >> -- >> Doug Goldstein >> > > > ping?
On 3/16/16 2:14 PM, Doug Goldstein wrote: > On 3/8/16 10:50 AM, Wei Liu wrote: >> On Tue, Mar 08, 2016 at 10:34:42AM -0600, Doug Goldstein wrote: >>> On 3/8/16 9:38 AM, Wei Liu wrote: >>>> On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >>>>> The build should not use -O0 as that results in miscompilations. There >>>> >>>> This needs some (concrete) references. Is that a known issue in gcc? If >>>> so can you reference the bug number? >>> >>> So its not really a bug in GCC but just the complete lack of >>> optimizations in play. inlines aren't inlined. dead code elimination >>> isn't run so things are much bigger. structures aren't padded the same way. >>> >> >> Urgh... >> >>> This came about from reading reports on the -devel and -user's ML that >>> were solved by building Xen with debug=n. I was also striving to reduce >>> the duplication of CFLAGS that are passed on the command line of builds. >>> >> >> I agree this is a good idea. >> >>>> >>>>> have been a few instances on the ML where users were told to switch >>>>> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >>>>> preferred route should be to use -Og if its available, otherwise use >>>>> -O1 which is the default. This change undoes the change from -O1 to -O0 >>>> >>>> gcc manual says -O0 is the default. >>> >>> I wasn't clear about where the 'the default' came from. That's the >>> default in the Xen tree (see: config/StdGNU.mk for example but every >>> platform has -O1 set). >>> >> >> OK. I thought you're talking about something in the manual. >> >>>> >>>> Not that I disagree with this patch in general, but the commit message >>>> seems a bit misleading. >>> >>> I can rewrite it. I'd also be willing to change the patch to prefer -Og >>> if its available and use -O0 if its not. >>> >> >> No need to do it now because ... >> >>>> >>>>> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. >>>>> >>>> >>>> And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for >>>> more input. >>> >>> -O1 can optimize things out when you look at them with gdb but -Og is >>> suppose to do the right thing. >>> >> >> .. I don't know much about gcc so I would like to wait for Ian to give >> some input. >> >> Wei. >> >>>> >>>>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> >>>>> --- >>>>> CC: Ian Jackson <ian.jackson@eu.citrix.com> >>>>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>>> CC: Wei Liu <wei.liu2@citrix.com> >>>>> --- >>>>> tools/Rules.mk | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/tools/Rules.mk b/tools/Rules.mk >>>>> index 9ef0b47..ae6b01f 100644 >>>>> --- a/tools/Rules.mk >>>>> +++ b/tools/Rules.mk >>>>> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) >>>>> >>>>> ifeq ($(debug),y) >>>>> # Disable optimizations and enable debugging information for macros >>>>> -CFLAGS += -O0 -g3 >>>>> +$(call cc-option-add,CFLAGS,CC,-Og) >>>>> +CFLAGS += -g3 >>>>> # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. >>>>> PY_CFLAGS += $(PY_NOOPT_CFLAGS) >>>>> endif >>>>> -- >>>>> 2.4.10 >>>>> >>> >>> >>> -- >>> Doug Goldstein >>> >> >> >> > > ping? > > ping the ping?
On Mon, Mar 28, 2016 at 4:01 PM, Doug Goldstein <cardoe@cardoe.com> wrote: > On 3/16/16 2:14 PM, Doug Goldstein wrote: >> On 3/8/16 10:50 AM, Wei Liu wrote: >>> On Tue, Mar 08, 2016 at 10:34:42AM -0600, Doug Goldstein wrote: >>>> On 3/8/16 9:38 AM, Wei Liu wrote: >>>>> On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >>>>>> The build should not use -O0 as that results in miscompilations. There >>>>> >>>>> This needs some (concrete) references. Is that a known issue in gcc? If >>>>> so can you reference the bug number? >>>> >>>> So its not really a bug in GCC but just the complete lack of >>>> optimizations in play. inlines aren't inlined. dead code elimination >>>> isn't run so things are much bigger. structures aren't padded the same way. >>>> >>> >>> Urgh... >>> >>>> This came about from reading reports on the -devel and -user's ML that >>>> were solved by building Xen with debug=n. I was also striving to reduce >>>> the duplication of CFLAGS that are passed on the command line of builds. >>>> >>> >>> I agree this is a good idea. >>> >>>>> >>>>>> have been a few instances on the ML where users were told to switch >>>>>> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >>>>>> preferred route should be to use -Og if its available, otherwise use >>>>>> -O1 which is the default. This change undoes the change from -O1 to -O0 >>>>> >>>>> gcc manual says -O0 is the default. >>>> >>>> I wasn't clear about where the 'the default' came from. That's the >>>> default in the Xen tree (see: config/StdGNU.mk for example but every >>>> platform has -O1 set). >>>> >>> >>> OK. I thought you're talking about something in the manual. >>> >>>>> >>>>> Not that I disagree with this patch in general, but the commit message >>>>> seems a bit misleading. >>>> >>>> I can rewrite it. I'd also be willing to change the patch to prefer -Og >>>> if its available and use -O0 if its not. >>>> >>> >>> No need to do it now because ... >>> >>>>> >>>>>> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. >>>>>> >>>>> >>>>> And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for >>>>> more input. >>>> >>>> -O1 can optimize things out when you look at them with gdb but -Og is >>>> suppose to do the right thing. >>>> >>> >>> .. I don't know much about gcc so I would like to wait for Ian to give >>> some input. >>> >>> Wei. >>> >>>>> >>>>>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> >>>>>> --- >>>>>> CC: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>>>> CC: Wei Liu <wei.liu2@citrix.com> >>>>>> --- >>>>>> tools/Rules.mk | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/tools/Rules.mk b/tools/Rules.mk >>>>>> index 9ef0b47..ae6b01f 100644 >>>>>> --- a/tools/Rules.mk >>>>>> +++ b/tools/Rules.mk >>>>>> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) >>>>>> >>>>>> ifeq ($(debug),y) >>>>>> # Disable optimizations and enable debugging information for macros >>>>>> -CFLAGS += -O0 -g3 >>>>>> +$(call cc-option-add,CFLAGS,CC,-Og) >>>>>> +CFLAGS += -g3 >>>>>> # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. >>>>>> PY_CFLAGS += $(PY_NOOPT_CFLAGS) >>>>>> endif >>>>>> -- >>>>>> 2.4.10 >>>>>> >>>> >>>> >>>> -- >>>> Doug Goldstein >>>> >>> >>> >>> >> >> ping? >> >> > > ping the ping? So just reading through the history -- I'm a bit confused why, if -Og is supposed to "do the right thing", why you didn't add that in this patch? In any case, having debug=y *functioning* should take priority over having gdb working, so I'm inclined to say that we should take a patch like this. -George
On 3/29/16 6:44 AM, George Dunlap wrote: > On Mon, Mar 28, 2016 at 4:01 PM, Doug Goldstein <cardoe@cardoe.com> wrote: >> On 3/16/16 2:14 PM, Doug Goldstein wrote: >>> On 3/8/16 10:50 AM, Wei Liu wrote: >>>> On Tue, Mar 08, 2016 at 10:34:42AM -0600, Doug Goldstein wrote: >>>>> On 3/8/16 9:38 AM, Wei Liu wrote: >>>>>> On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >>>>>>> The build should not use -O0 as that results in miscompilations. There >>>>>> >>>>>> This needs some (concrete) references. Is that a known issue in gcc? If >>>>>> so can you reference the bug number? >>>>> >>>>> So its not really a bug in GCC but just the complete lack of >>>>> optimizations in play. inlines aren't inlined. dead code elimination >>>>> isn't run so things are much bigger. structures aren't padded the same way. >>>>> >>>> >>>> Urgh... >>>> >>>>> This came about from reading reports on the -devel and -user's ML that >>>>> were solved by building Xen with debug=n. I was also striving to reduce >>>>> the duplication of CFLAGS that are passed on the command line of builds. >>>>> >>>> >>>> I agree this is a good idea. >>>> >>>>>> >>>>>>> have been a few instances on the ML where users were told to switch >>>>>>> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >>>>>>> preferred route should be to use -Og if its available, otherwise use >>>>>>> -O1 which is the default. This change undoes the change from -O1 to -O0 >>>>>> >>>>>> gcc manual says -O0 is the default. >>>>> >>>>> I wasn't clear about where the 'the default' came from. That's the >>>>> default in the Xen tree (see: config/StdGNU.mk for example but every >>>>> platform has -O1 set). >>>>> >>>> >>>> OK. I thought you're talking about something in the manual. >>>> >>>>>> >>>>>> Not that I disagree with this patch in general, but the commit message >>>>>> seems a bit misleading. >>>>> >>>>> I can rewrite it. I'd also be willing to change the patch to prefer -Og >>>>> if its available and use -O0 if its not. >>>>> >>>> >>>> No need to do it now because ... >>>> >>>>>> >>>>>>> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. >>>>>>> >>>>>> >>>>>> And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for >>>>>> more input. >>>>> >>>>> -O1 can optimize things out when you look at them with gdb but -Og is >>>>> suppose to do the right thing. >>>>> >>>> >>>> .. I don't know much about gcc so I would like to wait for Ian to give >>>> some input. >>>> >>>> Wei. >>>> >>>>>> >>>>>>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> >>>>>>> --- >>>>>>> CC: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>>>>> CC: Wei Liu <wei.liu2@citrix.com> >>>>>>> --- >>>>>>> tools/Rules.mk | 3 ++- >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/tools/Rules.mk b/tools/Rules.mk >>>>>>> index 9ef0b47..ae6b01f 100644 >>>>>>> --- a/tools/Rules.mk >>>>>>> +++ b/tools/Rules.mk >>>>>>> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) >>>>>>> >>>>>>> ifeq ($(debug),y) >>>>>>> # Disable optimizations and enable debugging information for macros >>>>>>> -CFLAGS += -O0 -g3 >>>>>>> +$(call cc-option-add,CFLAGS,CC,-Og) >>>>>>> +CFLAGS += -g3 >>>>>>> # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. >>>>>>> PY_CFLAGS += $(PY_NOOPT_CFLAGS) >>>>>>> endif >>>>>>> -- >>>>>>> 2.4.10 >>>>>>> >>>>> >>>>> >>>>> -- >>>>> Doug Goldstein >>>>> >>>> >>>> >>>> >>> >>> ping? >>> >>> >> >> ping the ping? > > So just reading through the history -- I'm a bit confused why, if -Og > is supposed to "do the right thing", why you didn't add that in this > patch? I did. +$(call cc-option-add,CFLAGS,CC,-Og) That tests to see if the version of GCC you're using supports that flag and if it does adds it. Otherwise it does nothing. Per the README we support down to GCC 4.1.2 and -Og was added in 4.7 I believe. For versions of GCC older than 4.7 this uses the debug=y default of -O1 instead of using -O0 which is known to break in some cases. > > In any case, having debug=y *functioning* should take priority over > having gdb working, so I'm inclined to say that we should take a patch > like this. Exactly why I'd like to see this land. > > -George >
On Tue, Mar 29, 2016 at 6:21 PM, Doug Goldstein <cardoe@cardoe.com> wrote: > On 3/29/16 6:44 AM, George Dunlap wrote: >> On Mon, Mar 28, 2016 at 4:01 PM, Doug Goldstein <cardoe@cardoe.com> wrote: >>> On 3/16/16 2:14 PM, Doug Goldstein wrote: >>>> On 3/8/16 10:50 AM, Wei Liu wrote: >>>>> On Tue, Mar 08, 2016 at 10:34:42AM -0600, Doug Goldstein wrote: >>>>>> On 3/8/16 9:38 AM, Wei Liu wrote: >>>>>>> On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >>>>>>>> The build should not use -O0 as that results in miscompilations. There >>>>>>> >>>>>>> This needs some (concrete) references. Is that a known issue in gcc? If >>>>>>> so can you reference the bug number? >>>>>> >>>>>> So its not really a bug in GCC but just the complete lack of >>>>>> optimizations in play. inlines aren't inlined. dead code elimination >>>>>> isn't run so things are much bigger. structures aren't padded the same way. >>>>>> >>>>> >>>>> Urgh... >>>>> >>>>>> This came about from reading reports on the -devel and -user's ML that >>>>>> were solved by building Xen with debug=n. I was also striving to reduce >>>>>> the duplication of CFLAGS that are passed on the command line of builds. >>>>>> >>>>> >>>>> I agree this is a good idea. >>>>> >>>>>>> >>>>>>>> have been a few instances on the ML where users were told to switch >>>>>>>> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >>>>>>>> preferred route should be to use -Og if its available, otherwise use >>>>>>>> -O1 which is the default. This change undoes the change from -O1 to -O0 >>>>>>> >>>>>>> gcc manual says -O0 is the default. >>>>>> >>>>>> I wasn't clear about where the 'the default' came from. That's the >>>>>> default in the Xen tree (see: config/StdGNU.mk for example but every >>>>>> platform has -O1 set). >>>>>> >>>>> >>>>> OK. I thought you're talking about something in the manual. >>>>> >>>>>>> >>>>>>> Not that I disagree with this patch in general, but the commit message >>>>>>> seems a bit misleading. >>>>>> >>>>>> I can rewrite it. I'd also be willing to change the patch to prefer -Og >>>>>> if its available and use -O0 if its not. >>>>>> >>>>> >>>>> No need to do it now because ... >>>>> >>>>>>> >>>>>>>> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. >>>>>>>> >>>>>>> >>>>>>> And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for >>>>>>> more input. >>>>>> >>>>>> -O1 can optimize things out when you look at them with gdb but -Og is >>>>>> suppose to do the right thing. >>>>>> >>>>> >>>>> .. I don't know much about gcc so I would like to wait for Ian to give >>>>> some input. >>>>> >>>>> Wei. >>>>> >>>>>>> >>>>>>>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> >>>>>>>> --- >>>>>>>> CC: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>>>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>>>>>> CC: Wei Liu <wei.liu2@citrix.com> >>>>>>>> --- >>>>>>>> tools/Rules.mk | 3 ++- >>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/tools/Rules.mk b/tools/Rules.mk >>>>>>>> index 9ef0b47..ae6b01f 100644 >>>>>>>> --- a/tools/Rules.mk >>>>>>>> +++ b/tools/Rules.mk >>>>>>>> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) >>>>>>>> >>>>>>>> ifeq ($(debug),y) >>>>>>>> # Disable optimizations and enable debugging information for macros >>>>>>>> -CFLAGS += -O0 -g3 >>>>>>>> +$(call cc-option-add,CFLAGS,CC,-Og) >>>>>>>> +CFLAGS += -g3 >>>>>>>> # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. >>>>>>>> PY_CFLAGS += $(PY_NOOPT_CFLAGS) >>>>>>>> endif >>>>>>>> -- >>>>>>>> 2.4.10 >>>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Doug Goldstein >>>>>> >>>>> >>>>> >>>>> >>>> >>>> ping? >>>> >>>> >>> >>> ping the ping? >> >> So just reading through the history -- I'm a bit confused why, if -Og >> is supposed to "do the right thing", why you didn't add that in this >> patch? > > I did. > > +$(call cc-option-add,CFLAGS,CC,-Og) > > That tests to see if the version of GCC you're using supports that flag > and if it does adds it. Otherwise it does nothing. Per the README we > support down to GCC 4.1.2 and -Og was added in 4.7 I believe. For > versions of GCC older than 4.7 this uses the debug=y default of -O1 > instead of using -O0 which is known to break in some cases. Gah -- sorry, my eyes skipped over that when skimming this thread. Sorry about that. :-) So it looks like we're still waiting for someone who knows more about gcc to comment. Andy, you seems to know this sort of thing -- would you be willing take a look? Otherwise we may have to ping IanJ out-of-band. -George
Doug Goldstein writes ("Re: [PATCH 2/2] tools: detect appropriate debug optimization level"): > On 3/8/16 9:38 AM, Wei Liu wrote: > > On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: > >> The build should not use -O0 as that results in miscompilations. There > > > > This needs some (concrete) references. Is that a known issue in gcc? If > > so can you reference the bug number? > > So its not really a bug in GCC but just the complete lack of > optimizations in play. inlines aren't inlined. dead code elimination > isn't run so things are much bigger. structures aren't padded the same way. My initial reaction is that I any actual problems are bugs either in the compiler or in Xen, which should be fixed. There should be nothing wrong with lack of inlining or dead code elimination. If you can give an example of structure padding going wrong, please do. Ian.
Ian Jackson writes ("Re: [PATCH 2/2] tools: detect appropriate debug optimization level"): > My initial reaction is that I any actual problems are bugs either in > the compiler or in Xen, which should be fixed. > > There should be nothing wrong with lack of inlining or dead code > elimination. If you can give an example of structure padding going > wrong, please do. Having said that, the reason for specifying -O0 is the use case that gcc now provides -Og for. So I see no harm and some benefit in using -Og if it is supported. Ian.
On 3/30/16 11:00 AM, Ian Jackson wrote: > Doug Goldstein writes ("Re: [PATCH 2/2] tools: detect appropriate debug optimization level"): >> On 3/8/16 9:38 AM, Wei Liu wrote: >>> On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >>>> The build should not use -O0 as that results in miscompilations. There >>> >>> This needs some (concrete) references. Is that a known issue in gcc? If >>> so can you reference the bug number? >> >> So its not really a bug in GCC but just the complete lack of >> optimizations in play. inlines aren't inlined. dead code elimination >> isn't run so things are much bigger. structures aren't padded the same way. > > My initial reaction is that I any actual problems are bugs either in > the compiler or in Xen, which should be fixed. > > There should be nothing wrong with lack of inlining or dead code > elimination. If you can give an example of structure padding going > wrong, please do. > > Ian. > Ok fine, I'm just confused why we're insisting on using -O0 over -Og? From the gcc manual: -Og Optimize debugging experience. -Og enables optimizations that do not interfere with debugging. It should be the optimization level of choice for the standard edit-compile-debug cycle, offering a reasonable level of optimization while maintaining fast compilation and a good debugging experience.
On Wed, Mar 30, 2016 at 5:00 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Doug Goldstein writes ("Re: [PATCH 2/2] tools: detect appropriate debug optimization level"): >> On 3/8/16 9:38 AM, Wei Liu wrote: >> > On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >> >> The build should not use -O0 as that results in miscompilations. There >> > >> > This needs some (concrete) references. Is that a known issue in gcc? If >> > so can you reference the bug number? >> >> So its not really a bug in GCC but just the complete lack of >> optimizations in play. inlines aren't inlined. dead code elimination >> isn't run so things are much bigger. structures aren't padded the same way. > > My initial reaction is that I any actual problems are bugs either in > the compiler or in Xen, which should be fixed. > > There should be nothing wrong with lack of inlining or dead code > elimination. If you can give an example of structure padding going > wrong, please do. I know in the Linux kernel the level of optimization must be -O2, because there are certain things that rely on dead code elimination to function properly. I'm not sure if the Xen hypervisor has similar requirements, but I'd be rather surprised if it didn't. It would be nice to know what functionality in the tools relied on inlining and/or dead code elimination (and under what circumstances), but we probably have better things to do than make -O0 to work. :-) -George
Doug Goldstein writes ("[PATCH 2/2] tools: detect appropriate debug optimization level"): > The build should not use -O0 as that results in miscompilations. There > have been a few instances on the ML where users were told to switch > from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The > preferred route should be to use -Og if its available, otherwise use > -O1 which is the default. This change undoes the change from -O1 to -O0 > in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. To summarise: I agree with using -Og if it is available. But I disagree with reverting 1166ecf7 in the case where it isn't; if -Og is not available, we should stick with -O0. I'm open to persuasion in the latter point but I would like specific examples of problems (and then I would form an opinion about the specific problems), not generalities. Doug, would you like to, for now, propose a patch that uses -Og if it is available, but otherwise falls back to -O0 ? NB that if convenient, this can be done simply by always specifying -O0 and putting -Og after it if it is supported. Thanks, Ian.
On Wed, Apr 6, 2016 at 3:07 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Doug Goldstein writes ("[PATCH 2/2] tools: detect appropriate debug optimization level"): >> The build should not use -O0 as that results in miscompilations. There >> have been a few instances on the ML where users were told to switch >> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >> preferred route should be to use -Og if its available, otherwise use >> -O1 which is the default. This change undoes the change from -O1 to -O0 >> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. > > To summarise: I agree with using -Og if it is available. But I > disagree with reverting 1166ecf7 in the case where it isn't; if -Og is > not available, we should stick with -O0. > > I'm open to persuasion in the latter point but I would like specific > examples of problems (and then I would form an opinion about the > specific problems), not generalities. > > Doug, would you like to, for now, propose a patch that uses -Og if it > is available, but otherwise falls back to -O0 ? NB that if > convenient, this can be done simply by always specifying -O0 and > putting -Og after it if it is supported. I agree that it would be good to include specific bugs that this fixes. Presuming that they are as described (compilation errors for reasonable setups), I continue to think that making things compile for everyone is more important than making them debuggable via gdb for people running compilers that don't support -Og. -George
George Dunlap writes ("Re: [Xen-devel] [PATCH 2/2] tools: detect appropriate debug optimization level"): > On Wed, Apr 6, 2016 at 3:07 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > > Doug, would you like to, for now, propose a patch that uses -Og if it > > is available, but otherwise falls back to -O0 ? NB that if > > convenient, this can be done simply by always specifying -O0 and > > putting -Og after it if it is supported. > > I agree that it would be good to include specific bugs that this > fixes. Presuming that they are as described (compilation errors for > reasonable setups), I continue to think that making things compile for > everyone is more important than making them debuggable via gdb for > people running compilers that don't support -Og. I think making debug builds debuggable for people with non-broken compilers is more important than making debug builds build for people with broken compilers. (Under some unstated but I think reasonable, and rebuttable, assumptions about the sizes of the two sets of people.) Non-debug builds are, after all, not affected by any of this. Ian.
diff --git a/tools/Rules.mk b/tools/Rules.mk index 9ef0b47..ae6b01f 100644 --- a/tools/Rules.mk +++ b/tools/Rules.mk @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN) ifeq ($(debug),y) # Disable optimizations and enable debugging information for macros -CFLAGS += -O0 -g3 +$(call cc-option-add,CFLAGS,CC,-Og) +CFLAGS += -g3 # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>. PY_CFLAGS += $(PY_NOOPT_CFLAGS) endif
The build should not use -O0 as that results in miscompilations. There have been a few instances on the ML where users were told to switch from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The preferred route should be to use -Og if its available, otherwise use -O1 which is the default. This change undoes the change from -O1 to -O0 in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. Signed-off-by: Doug Goldstein <cardoe@cardoe.com> --- CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- tools/Rules.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)