[PATCH] newlib: Remove debug flag for --enable-target-optspace

[PATCH] newlib: Remove debug flag for --enable-target-optspace

[Stephanos Ioannidis]

--enable-target-optspace adds -g (debug) option flag to the CFLAGS
alongside -Os (optimise for size) for no good reason.

Since the -g option flag is not required by the -Os option flag and it
not contribute to what --enable-target-optspace tries to achieve (i.e.
optimise for size), this flag should be removed.

It is also worth noting that many newlib users (e.g. GNU ARM Embedded)
are manually adding -Os to CFLAGS because the --enable-target-optspace
does not achieve what it should.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
---
 config/mt-ospace | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config/mt-ospace b/config/mt-ospace
index ce29ff431..95be8259d 100644
--- a/config/mt-ospace
+++ b/config/mt-ospace
@@ -1,3 +1,3 @@
 # Build libraries optimizing for space, not speed.
- CFLAGS_FOR_TARGET += -g -Os
- CXXFLAGS_FOR_TARGET += -g -Os
+ CFLAGS_FOR_TARGET += -Os
+ CXXFLAGS_FOR_TARGET += -Os
-- 
2.17.1

RE: [PATCH] newlib: Remove debug flag for --enable-target-optspace

[Jon Beniston]

Hi Stephanos,

I don't think this should be required. -g generally shouldn't cause code
size to be bigger when used with -Os. The resulting ELF files will be
bigger, as they include debug info, but this debug info is not burnt to
flash or loaded in to RAM. If you want the ELF files smaller because they
need to go on the targets filesystem, then you can run the strip program on
them to remove the debug info.

Cheers,
Jon

-----Original Message-----
From: newlib-owner@sourceware.org <newlib-owner@sourceware.org> On Behalf Of
Stephanos Ioannidis
Sent: 29 November 2019 03:47
To: newlib@sourceware.org
Cc: Stephanos Ioannidis <root@stephanos.io>
Subject: [PATCH] newlib: Remove debug flag for --enable-target-optspace

--enable-target-optspace adds -g (debug) option flag to the CFLAGS alongside
-Os (optimise for size) for no good reason.

Since the -g option flag is not required by the -Os option flag and it not
contribute to what --enable-target-optspace tries to achieve (i.e.
optimise for size), this flag should be removed.

It is also worth noting that many newlib users (e.g. GNU ARM Embedded) are
manually adding -Os to CFLAGS because the --enable-target-optspace does not
achieve what it should.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
---
 config/mt-ospace | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config/mt-ospace b/config/mt-ospace index ce29ff431..95be8259d
100644
--- a/config/mt-ospace
+++ b/config/mt-ospace
@@ -1,3 +1,3 @@
 # Build libraries optimizing for space, not speed.
- CFLAGS_FOR_TARGET += -g -Os
- CXXFLAGS_FOR_TARGET += -g -Os
+ CFLAGS_FOR_TARGET += -Os
+ CXXFLAGS_FOR_TARGET += -Os
--
2.17.1

RE: [PATCH] newlib: Remove debug flag for --enable-target-optspace

[Stephanos Ioannidis]

Hi Jon,

While `-g` does not increase the actual code size since it is intended to add debug information only, `--enable-target-optspace` semantically has nothing to do with adding debug information and keeping `-g` here only introduces an unexpected and unintentional behaviour (and confusion).

As you suggested, the debug information added by `--enable-target-optspace` can be stripped, but why add it in the first place if it is not necessary?

Unless there is a good reason to keep `-g` and `-Os` together in `--enable-target-optspace`, I strongly believe `-g` should be removed and moved into a separate configuration option.

p.s. It looks like this was passed down from gcc and other GNU toolchains.

Regards,

Stephanos

-----Original Message-----
From: Jon Beniston <jon@beniston.com> 
Sent: Friday, November 29, 2019 6:56 PM
To: Stephanos Ioannidis <root@stephanos.io>; newlib@sourceware.org
Subject: RE: [PATCH] newlib: Remove debug flag for --enable-target-optspace

Hi Stephanos,

I don't think this should be required. -g generally shouldn't cause code size to be bigger when used with -Os. The resulting ELF files will be bigger, as they include debug info, but this debug info is not burnt to flash or loaded in to RAM. If you want the ELF files smaller because they need to go on the targets filesystem, then you can run the strip program on them to remove the debug info.

Cheers,
Jon

-----Original Message-----
From: newlib-owner@sourceware.org <newlib-owner@sourceware.org> On Behalf Of Stephanos Ioannidis
Sent: 29 November 2019 03:47
To: newlib@sourceware.org
Cc: Stephanos Ioannidis <root@stephanos.io>
Subject: [PATCH] newlib: Remove debug flag for --enable-target-optspace

--enable-target-optspace adds -g (debug) option flag to the CFLAGS alongside -Os (optimise for size) for no good reason.

Since the -g option flag is not required by the -Os option flag and it not contribute to what --enable-target-optspace tries to achieve (i.e.
optimise for size), this flag should be removed.

It is also worth noting that many newlib users (e.g. GNU ARM Embedded) are manually adding -Os to CFLAGS because the --enable-target-optspace does not achieve what it should.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
---
 config/mt-ospace | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config/mt-ospace b/config/mt-ospace index ce29ff431..95be8259d
100644
--- a/config/mt-ospace
+++ b/config/mt-ospace
@@ -1,3 +1,3 @@
 # Build libraries optimizing for space, not speed.
- CFLAGS_FOR_TARGET += -g -Os
- CXXFLAGS_FOR_TARGET += -g -Os
+ CFLAGS_FOR_TARGET += -Os
+ CXXFLAGS_FOR_TARGET += -Os
--
2.17.1

Re: [PATCH] newlib: Remove debug flag for --enable-target-optspace

[Andrew Pinski]

On Fri, Nov 29, 2019 at 2:31 AM Stephanos Ioannidis <root@stephanos.io> wrote:
>
> Hi Jon,
>
> While `-g` does not increase the actual code size since it is intended to add debug information only, `--enable-target-optspace` semantically has nothing to do with adding debug information and keeping `-g` here only introduces an unexpected and unintentional behaviour (and confusion).
>
> As you suggested, the debug information added by `--enable-target-optspace` can be stripped, but why add it in the first place if it is not necessary?
>
> Unless there is a good reason to keep `-g` and `-Os` together in `--enable-target-optspace`, I strongly believe `-g` should be removed and moved into a separate configuration option.
>
> p.s. It looks like this was passed down from gcc and other GNU toolchains.

Note I looked into the history of this file and found it originally
did = rather than += but when it changed to being +=, the -g part was
not removed.

This can be seen here:
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02062.html

Also note the upstream of this file is in the GCC sources, so it might
be best if this patch is sent there instead and then sync'ed into the
newlib directory.

Thanks,
Andrew Pinski


>
> Regards,
>
> Stephanos
>
> -----Original Message-----
> From: Jon Beniston <jon@beniston.com>
> Sent: Friday, November 29, 2019 6:56 PM
> To: Stephanos Ioannidis <root@stephanos.io>; newlib@sourceware.org
> Subject: RE: [PATCH] newlib: Remove debug flag for --enable-target-optspace
>
> Hi Stephanos,
>
> I don't think this should be required. -g generally shouldn't cause code size to be bigger when used with -Os. The resulting ELF files will be bigger, as they include debug info, but this debug info is not burnt to flash or loaded in to RAM. If you want the ELF files smaller because they need to go on the targets filesystem, then you can run the strip program on them to remove the debug info.
>
> Cheers,
> Jon
>
> -----Original Message-----
> From: newlib-owner@sourceware.org <newlib-owner@sourceware.org> On Behalf Of Stephanos Ioannidis
> Sent: 29 November 2019 03:47
> To: newlib@sourceware.org
> Cc: Stephanos Ioannidis <root@stephanos.io>
> Subject: [PATCH] newlib: Remove debug flag for --enable-target-optspace
>
> --enable-target-optspace adds -g (debug) option flag to the CFLAGS alongside -Os (optimise for size) for no good reason.
>
> Since the -g option flag is not required by the -Os option flag and it not contribute to what --enable-target-optspace tries to achieve (i.e.
> optimise for size), this flag should be removed.
>
> It is also worth noting that many newlib users (e.g. GNU ARM Embedded) are manually adding -Os to CFLAGS because the --enable-target-optspace does not achieve what it should.
>
> Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
> ---
>  config/mt-ospace | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/config/mt-ospace b/config/mt-ospace index ce29ff431..95be8259d
> 100644
> --- a/config/mt-ospace
> +++ b/config/mt-ospace
> @@ -1,3 +1,3 @@
>  # Build libraries optimizing for space, not speed.
> - CFLAGS_FOR_TARGET += -g -Os
> - CXXFLAGS_FOR_TARGET += -g -Os
> + CFLAGS_FOR_TARGET += -Os
> + CXXFLAGS_FOR_TARGET += -Os
> --
> 2.17.1
>
>

RE: [PATCH] newlib: Remove debug flag for --enable-target-optspace

[Jon Beniston]

Hi Stephanos,

>As you suggested, the debug information added by `--enable-target-optspace`
can be stripped, but why add it in the first place if it is not necessary?

Because many people want to be able to debug their code!

>Unless there is a good reason to keep `-g` and `-Os` together in
`--enable-target-optspace`, 
>I strongly believe `-g` should be removed and moved into a separate
configuration option.

Currently the default for CFLAGS_FOR_TARGET is "-g -O2".
--enable-target-optspace Changes this to "-g -Os"

If -g is to be controlled by a separate option, then it should apply in both
cases. And this new option should probably be enabled by default to maintain
existing behaviour (so perhaps add a --disable-target-debug option that
removes -g)

Cheers,
Jon

Re: [PATCH] newlib: Remove debug flag for --enable-target-optspace

[Andrew Pinski]

On Fri, Nov 29, 2019 at 3:19 AM Jon Beniston <jon@beniston.com> wrote:
>
> Hi Stephanos,
>
> >As you suggested, the debug information added by `--enable-target-optspace`
> can be stripped, but why add it in the first place if it is not necessary?
>
> Because many people want to be able to debug their code!
>
> >Unless there is a good reason to keep `-g` and `-Os` together in
> `--enable-target-optspace`,
> >I strongly believe `-g` should be removed and moved into a separate
> configuration option.
>
> Currently the default for CFLAGS_FOR_TARGET is "-g -O2".
> --enable-target-optspace Changes this to "-g -Os"

Actually you are wrong.  --enable-target-optspace changes the (normal)
CFLAGS to be "-g -O2 -g -Os".  The "-g" in the config/mt-ospace is
redundant now.
Please see https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02062.html as
that changed CFLAGS from being destroyed to be additional.

The reasoning Stephanos is giving of removing the "-g" there is not
correct though.  I am saying the patch is correct but the REASON
behind of why it is correct is NOT.  The "-g" is redudant and can be
removed from config/mt-ospace.

Thanks,
Andrew Pinski

>
> If -g is to be controlled by a separate option, then it should apply in both
> cases. And this new option should probably be enabled by default to maintain
> existing behaviour (so perhaps add a --disable-target-debug option that
> removes -g)
>
> Cheers,
> Jon
>
>

Re: [PATCH] newlib: Remove debug flag for --enable-target-optspace

[Stephanos Ioannidis]

Hi Andrew and Jon,

> Actually you are wrong.  --enable-target-optspace changes the (normal)
CFLAGS to be "-g -O2 -g -Os".  The "-g" in the config/mt-ospace is
redundant now.

It does seem that way (very messy though ...).

> The reasoning Stephanos is giving of removing the "-g" there is not
correct though.

If you are overriding CFLAGS for other purposes and want to _only_ enable `-Os`, you cannot use `--enable-target-optspace` because it has an unwanted side effect of enabling debug information output alongside optimisation for size. After all, the option is called `--enable-target-optspace`, not `--enable-target-optspace-and-debug-info`.

What is more frustrating is that the documentation for `--enable-target-optspace` mentions nothing about enabling debug information output and this can be very confusing and misleading to users; I spent hours trying out many different build options in attempt to figure out why the compiled libraries included debug information in spite of never being instructed to do so, until I finally decided to have a look at the newlib build scripts.

*By removing `-g` in `mt-ospace`, we can support both cases:*

1. CFLAGS is not overridden: Since the default CFLAGS is `-g -O2`, `-Os` will override`-O2` to optimise for size. 
2. CFLAGS is overridden: It will simply add `-Os` to optimise for size. If user wanted debug information output, he/she will add "-g" in the CFLAGS separately.

One could possibly argue that user can simply put `-Os` in the overridden CFLAGS instead of using `--enable-target-optspace`, but that is not a valid excuse for the latter to misbehave.

> Because many people want to be able to debug their code!

Some might, some might not- especially if they are compiling newlib to be used by someone else, which would often be the case; the debug information is not very useful unless you have a local copy of the newlib source code.

For instance, GNU ARM Embedded build script is setting CFLAGS to `-Os` instead of using `--enable-target-optspace` because the latter is not achieving the intended behavior.

More importantly, I do not think any normal average person would somehow automatically assume `--enable-target-optspace` must mean enabling both optimisation for size and debugging information output!

Regards,

Stephanos
________________________________________
From: Andrew Pinski <pinskia@gmail.com>
Sent: 30 November 2019 10:13
To: Jon Beniston <jon@beniston.com>
Cc: Stephanos Ioannidis <root@stephanos.io>; <newlib@sourceware.org> <newlib@sourceware.org>
Subject: Re: [PATCH] newlib: Remove debug flag for --enable-target-optspace 
 
On Fri, Nov 29, 2019 at 3:19 AM Jon Beniston <jon@beniston.com> wrote:
>
> Hi Stephanos,
>
> >As you suggested, the debug information added by `--enable-target-optspace`
> can be stripped, but why add it in the first place if it is not necessary?
>
> Because many people want to be able to debug their code!
>
> >Unless there is a good reason to keep `-g` and `-Os` together in
> `--enable-target-optspace`,
> >I strongly believe `-g` should be removed and moved into a separate
> configuration option.
>
> Currently the default for CFLAGS_FOR_TARGET is "-g -O2".
> --enable-target-optspace Changes this to "-g -Os"

Actually you are wrong.  --enable-target-optspace changes the (normal)
CFLAGS to be "-g -O2 -g -Os".  The "-g" in the config/mt-ospace is
redundant now.
Please see https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02062.html as
that changed CFLAGS from being destroyed to be additional.

The reasoning Stephanos is giving of removing the "-g" there is not
correct though.  I am saying the patch is correct but the REASON
behind of why it is correct is NOT.  The "-g" is redudant and can be
removed from config/mt-ospace.

Thanks,
Andrew Pinski

>
> If -g is to be controlled by a separate option, then it should apply in both
> cases. And this new option should probably be enabled by default to maintain
> existing behaviour (so perhaps add a --disable-target-debug option that
> removes -g)
>
> Cheers,
> Jon
>
>