Re: [Xen-devel] [PATCH 2/2] tools: detect appropriate debug optimization level

[Doug Goldstein <cardoe@xxxxxxxxxx>]

On 3/29/16 6:44 AM, George Dunlap wrote:
> On Mon, Mar 28, 2016 at 4:01 PM, Doug Goldstein <cardoe@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>>>>>>> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>>>>>>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>>>>>>> ---
>>>>>>>  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
> 


-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel