threads / patch / 26487

patchDon't optimize code in debug build

Subject: [PATCH] Don't optimize code in debug build

## tl;dr

4 messages between Feb 13, 2011 and Feb 13, 2011. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Piotr Krukowiecki· Feb 13, 2011, 08:32 UTC · lore

Code optimization makes debugging harder.

Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki.news@gmail.com>
---
My first patch to this list, so please be gentle ;)

Patch fixes most important problem. There are other improvement possible:
- "-g" is not needed in normal build IMO, I'd move it to debug
- I'd add -O0 -fno-inline to debug too, but maybe it's too gccish
(OTOH there's -g already)

 Makefile |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Show changes to Makefile +4 −1

diff --git a/Makefile b/Makefile
index ade7923..32d3a69 100644
--- a/Makefile
+++ b/Makefile
@@ -262,7 +262,10 @@ endif

 # CFLAGS and LDFLAGS are for the users to override from the command line.

-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -Wall
+ifndef DEBUG
+CFLAGS += -O2
+endif
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

-- 
1.7.1

-- 
Piotrek

Johannes Sixt· Feb 13, 2011, 09:36 UTC · re: Piotr Krukowiecki · lore

Re: [PATCH] Don't optimize code in debug build

On Sonntag, 13. Februar 2011, Piotr Krukowiecki wrote:

Show 14 quoted lines

> --- a/Makefile
> +++ b/Makefile
> @@ -262,7 +262,10 @@ endif
>
>  # CFLAGS and LDFLAGS are for the users to override from the command line.
>
> -CFLAGS = -g -O2 -Wall
> +CFLAGS = -g -Wall
> +ifndef DEBUG
> +CFLAGS += -O2
> +endif
>  LDFLAGS =
>  ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>  ALL_LDFLAGS = $(LDFLAGS)

Instead of this, you can just write

 CFLAGS = -g -Wall

in your config.mak. Is anything wrong with that?

-- Hannes

Junio C Hamano· Feb 13, 2011, 19:28 UTC · re: Johannes Sixt · lore

Re: [PATCH] Don't optimize code in debug build

Johannes Sixt <j6t@kdbg.org> writes:

Show 18 quoted lines

>> @@ -262,7 +262,10 @@ endif
>>
>>  # CFLAGS and LDFLAGS are for the users to override from the command line.
>>
>> -CFLAGS = -g -O2 -Wall
>> +CFLAGS = -g -Wall
>> +ifndef DEBUG
>> +CFLAGS += -O2
>> +endif
>>  LDFLAGS =
>>  ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>>  ALL_LDFLAGS = $(LDFLAGS)
>
> Instead of this, you can just write
>
>  CFLAGS = -g -Wall
>
> in your config.mak. Is anything wrong with that?

You then need to feed -O2 from the command line when you are not doing a debugging build. On the other hand, with the current Makefile, you need to feed "-g -O0" from the command line when you are doing a debugging build if you and your debugger get confused when seeing an optimized binary. So neither is a very good solution.

But having to feed DEBUG=Yes when running a debug build is not a good solution either. A single toggle is simply too coarse-grained; the next temptation after applying this patch would be to add

	ifdef DEBUG
        CFLAGS += -DDEBUG
        endif

and from there everything goes downhill. That is the last thing we would want to see happen.

So I would say the current Makefile is just fine as is.

Piotr Krukowiecki· Feb 13, 2011, 16:26 UTC · lore

Re: [PATCH] Don't optimize code in debug build

(forgot about the list when replying)

Show 22 quoted lines

> On Sun, Feb 13, 2011 at 10:36 AM, Johannes Sixt <j6t@kdbg.org> wrote:
>> On Sonntag, 13. Februar 2011, Piotr Krukowiecki wrote:
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -262,7 +262,10 @@ endif
>>>
>>>  # CFLAGS and LDFLAGS are for the users to override from the command line.
>>>
>>> -CFLAGS = -g -O2 -Wall
>>> +CFLAGS = -g -Wall
>>> +ifndef DEBUG
>>> +CFLAGS += -O2
>>> +endif
>>>  LDFLAGS =
>>>  ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>>>  ALL_LDFLAGS = $(LDFLAGS)
>>
>> Instead of this, you can just write
>>
>>  CFLAGS = -g -Wall
>>
>> in your config.mak. Is anything wrong with that?

Several things: I need to know I should remove -O2, I need to keep in sync with other default options, current default is less useful, it requires me to do extra steps. DEBUG is kind of standard - Makefile already uses it in Windows specific part. I suppose we want to keep consistent.

-- 
Piotrek