Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debug symbols to release builds (all toolchains) #2139

Closed
wants to merge 1 commit into from

Conversation

andresag01
Copy link

@andresag01 andresag01 commented Jul 11, 2016

Add debug symbols to release builds in all toolchains. Including symbols in the builds significantly improves the debugging experience for all users because they can now relate the program execution to lines of source code rather than just plain assembly while using debugging tools such as GDB. This makes it significantly easier to identify bugs and issues in applications.
Note: Adding debug symbols will increase the size of the resulting elf file produced by the compiler, but should not have an impact in the binary that is flashed onto the target device.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 11, 2016

Could you enable it for all toolchains? We can test if you don't have all toolchains.

@andresag01
Copy link
Author

@0xc0170: Made the change for armcc and iar toolchains as well.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 11, 2016

LGTM

@0xc0170 0xc0170 changed the title Add debug symbols to release builds with gcc Add debug symbols to release builds (all toolchains) Jul 11, 2016
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 11, 2016

cc @screamerbg @sg-

@PrzemekWirkus
Copy link
Contributor

@andresag01 Sorry, but could you update PR description and e.g. write why this change is happening?
This is non-trivial change to how we build our stuff. Why we are changing this? I just want to understand the reasoning.

@andresag01
Copy link
Author

@PrzemekWirkus: Updated PR description.

@PrzemekWirkus
Copy link
Contributor

Sorry, we are adding debug information to release build ?

@andresag01
Copy link
Author

@PrzemekWirkus: Well, we are adding debug information to the elf file generated by the compilers, but this should not much affect the .bin that is actually flashed into the target boards.

@c1728p9
Copy link
Contributor

c1728p9 commented Jul 11, 2016

This will be really nice to have. Good work @andresag01!

@PrzemekWirkus
Copy link
Contributor

PrzemekWirkus commented Jul 11, 2016

Sorry but -g flag may change optimization levels per compiler and has impact on binary (size, performance). Also -g and -O may not like each other in the same context. To make debugging bearable some optimizations are removed from the code.
You've provided no proof there is no impact on release binary yet.

@PrzemekWirkus
Copy link
Contributor

@0xc0170 I would really discuss this change with Bogdan or Marcus before applying it. Better be safe than sorry!

@screamerbg
Copy link
Contributor

screamerbg commented Jul 11, 2016

Agree with @PrzemekWirkus. ARM Compiler 5 adds HUGE debug payload. For a single target the full compiled OS is 280MB with every object file between 1 and 2 MB.

Can you please try the PR locally before submitting it? E.g. running tools/build_release.py -O with -g might easily fill your disk (last time it killed the build bot as it took 14GB space and it didn't build for all targets @bridadan)

@jupe
Copy link
Contributor

jupe commented Jul 12, 2016

I think that release build should not be compiled with debugs because that change binary context a bit and increase valuable size. Of cource we could provide also release builds with debugs but that should be separate build. cc: @kjbracey-arm @SeppoTakalo

@bogdanm
Copy link
Contributor

bogdanm commented Jul 12, 2016

I think that release build should not be compiled with debugs because that change binary context a bit and increase valuable size.

Not sure what you mean. This change will only add debug symbols to the .elf file, not to the actual binary that is flashed, as @0xc0170 already explained.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 12, 2016

cc @mjs-arm

Of cource we could provide also release builds with debugs but that should be separate build.

That would be nice to have (explanation below)

@PrzemekWirkus made some valid points, this change requires more information to be provided (toolchains behavior with debug symbols, how much it affects the build sizes), plus some consideration for this type of a change. Do we want to enable debug info for all builds, or provide a new extension as "debug info" was added. I many times when was catching a bug, had to do this change manually.

Question for us: if we consider having extra debug symbols is useful (not in the form as its in this patch at the moment), how we could enable it? Or "debug-info" and default build is sufficient?

Please could you provide an alternative if you express negative opinion about a patch? that would be helpful

@kjbracey
Copy link
Contributor

I'm also a bit doubtful that -g and optimisation level are totally independent on all 3 toolchains. Certainly this was never historically the case with armcc. A whole bunch of optimisations were inhibited by -g.

However, the current armcc manual does explicitly say that -g does not affect code generation. IAR and GCC manuals don't explicitly say, but suggest it.

Can you definitely experimentally confirm that "should not have an impact in the binary" is "currently does not"?

Also, saying "debug symbols" is a bit misleading. Symbols (ie variable and function locations) should always be available, regardless of compiler -g. -g is adding extra debug information, like variable types and line numbers.

@bogdanm
Copy link
Contributor

bogdanm commented Jul 12, 2016

Question for us: if we consider having debug symbols is useful (not in the form as its in this patch at the moment), how we could enable it? Or "debug-info" and default build is sufficient?

Personally I think that debug-info should be reserved for actual debug builds and that debug symbols should always be included in the .elf file, in both release and debug mode.

Sorry but -g flag may change optimization levels per compiler and has impact on binary (size, performance).

Where did you get that information from? At least for GCC, the manual doesn't say anything about that:

       -g  Produce debugging information in the operating system's native format (stabs, COFF, XCOFF, or DWARF 2).  GDB can work with this debugging information.

           On most systems that use stabs format, -g enables use of extra debugging information that only GDB can use; this extra information makes debugging work better in GDB but probably makes other debuggers crash or refuse to read the program.  If you want to control for certain whether to generate the extra information, use -gstabs+, -gstabs, -gxcoff+, -gxcoff, or -gvms (see below).

           GCC allows you to use -g with -O.  The shortcuts taken by optimized code may occasionally produce surprising results: some variables you declared may not exist at all; flow of control may briefly move where you did not expect it; some statements may not be executed because they compute constant results or their values are already at hand; some statements may execute in different places because they have been moved out of loops.

           Nevertheless it proves possible to debug optimized output.  This makes it reasonable to use the optimizer for programs that might have bugs.

@kjbracey
Copy link
Contributor

Also, as @0xc0170's comments highlight - with this change as proposed, "-o debug-info" becomes a total misnomer (as opposed the partial one it is now), as it's not actually adding debug info, but only changing optimisation level. That would need adjusting in some way.

@PrzemekWirkus
Copy link
Contributor

PrzemekWirkus commented Jul 12, 2016

@bogdanm Bogdan, thank you for the insights ! :)

Where did you get that information from? At least for GCC, the manual doesn't say anything about that:

I read somewhere I guess. My reasoning was based on assumption that compiler may remove some optimizations like inlining to make debug bearable. I had some time and I have read GCC manual. And yes, AFAIK there is no impact on binary size and only extra symbols are added to ELF.
But in that case producing release binary with -g and -O2 makes debugging harder exactly because optimizations may move your code around, inline loops or remove some variables?

It is actually easier to find this information for ARMCC, see Using --debug does not affect optimization settings..

Building for debug purposes

I was thinking the whole point of having virtual --debug and --release (sudo-names) was to manipulate optimizations the way that we have symbols and binary easy to debug (with -g and less optimizations).

And yes, like Kevin said. With this change whole --options debug-init only changes optimization level :)

Some tests

I've also ran test case build for ARMCC, IAR and GCC with -g and without -g and yes, all test case binaries have the same code / memory usage size.

Build size impact

There is a valid point for size of vanilla/release build. This option increases dramatically size of build like @screamerbg said. Basically there should be an option to have vanilla and debug builds on demand with e.g. vanilla/release as default. May I assume that most of the build systems work like this and there will be no confusion for developers where to find and how to enable "--debug" feature ???

For Jenkins/CI build is an artifact. It can weigh ~50MB but not 250MB by default!

@PrzemekWirkus
Copy link
Contributor

@adbridge @mazimkhan FYI

@bogdanm
Copy link
Contributor

bogdanm commented Jul 13, 2016

But in that case producing release binary with -g and -O2 makes debugging harder exactly because optimizations may move your code around, inline loops or remove some variables?

It definiteley does. But this is a common and relatively well understood problem. Having ocasionally dodgy debug information is better than not having any debug information at all.

But in that case producing release binary with -g and -O2 makes debugging harder exactly because optimizations may move your code around, inline loops or remove some variables?

I didn't do any benchmarks in this area personally. Does anyone have some numbers to support this claim?

For Jenkins/CI build is an artifact. It can weigh ~50MB but not 250MB by default!

Ouch! Are those real life numbers?

@adbridge
Copy link
Contributor

I would think standard approach for debugging is to build fully for debug so that each line of code can actually be stepped through. Trying to step through optimised code that just happens to have debug symbols could actually be quite a confusing experience and lead to unexpected values present in registers etc. They way I have seen this approached in the past is to have some kind of post crash processor dump that can be extracted, which includes current memory state and values of all global data and registers (including stack backtrace) . This can then be analysed offline. Just a thought...

@ghost
Copy link

ghost commented Jul 14, 2016

For GCC the switching from -g -O0 to -g -Og would make more sense. I can see no reason to force the compiler down to -O0. 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."""

@kjbracey
Copy link
Contributor

I did try that a few months ago. I wasn't entirely convinced. It was certainly a lot better to debug than -O2, but still had some oddities that -O0 didn't.

@ghost
Copy link

ghost commented Jul 14, 2016

This also raises the question of why (at least for gcc) we are using -O2 instead of -Os in a footprint constrained environment. I wonder is their is a particular historical reason for this choice, if not then we should look towards switching.

@kjbracey
Copy link
Contributor

This just came up here - we want to switch to -Os to squeeze something in to 512K.

There is a comment in the Python about using -O2 instead of -Os to avoid a compiler bug, but that bug appears to have been long since fixed.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 14, 2016

This is already open issue on mbed - #664 please discuss it there why O2, not Og or Os. Look at Adam's response, not certain if there was any improvement in gcc since 2014, if yes, add a comment there. Thanks

@bogdanm
Copy link
Contributor

bogdanm commented Jul 14, 2016

There is a comment in the Python about using -O2 instead of -Os to avoid a compiler bug, but that bug appears to have been long since fixed.

Yes, avoiding that bug was the original reason for using -O2:

https://github.com/mbedmicro/mbed/blob/master/tools/toolchains/gcc.py#L79

At this point, I fully agree that we should switch to -Os.

@pan-
Copy link
Member

pan- commented Jul 15, 2016

After a quick look, it appear that the GCC documentation explicitly state that the generation of debugging information has no impact to the generated binary (here).

The use of the `-g' switch, which is needed for source-level debugging, affects the size of the 
program executable on disk, and indeed the debugging information can be quite large. 
However, it has no effect on the generated code (and thus does not degrade performance)

This PR raise another question for me: Why NDEBUG is not defined for release builds ?

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 15, 2016

This PR raise another question for me: Why NDEBUG is not defined for release builds ?

@pan- #2169

@AlessandroA
Copy link
Contributor

@PrzemekWirkus If the artifacts size is an issue in the CI infrastructure, we could have a build option that disables the build symbols for that specific case (basically the opposite of what we have now, -o "no-debug-info").

I agree with @bogdanm though that in all other cases debug symbols should be present by default, as they do not affect the final code size and performance.

Debug builds (where optimizations are disabled) should have a more explicit build option, like --debug or -d. I think I've seen this somewhere else 😄.

@bogdanm
Copy link
Contributor

bogdanm commented Aug 16, 2016

At this point, the only thing that would prevent me from merging this PR is this:

For Jenkins/CI build is an artifact. It can weigh ~50MB but not 250MB by default!

If this impacts our infrastructure, we need to find a better way to handle it. But I don't know how much of a problem this really is. Maybe @bridadan or someone else can comment? In any case, we can use @AlessandroA's suggestion:

If the artifacts size is an issue in the CI infrastructure, we could have a build option that disables the build symbols for that specific case (basically the opposite of what we have now, -o "no-debug-info").

@bridadan
Copy link
Contributor

I don't have a problem with adding flags to CI builds that prevent the build size from blowing up. But if we're changing the compiler flags and adding new -o options, can we please document all the available options and what option is used by default? I'm still not clear on all of the options available, and I work with this every day 😄

Until there is an option to disable the build symbols, I'd say we'd need to hold off on merging this PR.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 19, 2016

cc @meriac

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 19, 2016

Update: By default ARMCC produces debug macros that increase the output files. Some numbers:

The size of the output folder for mbed-os-blinky with ARMCC:
no debug info: ~7MB
debug info enabled: ~122MB
debug info enabled, but disabled debug macros (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0472j/chr1359124221739.html): ~24MB

If we are happy with --no_debug_macros (Enables and disables the generation of debug table entries for preprocessor macro definitions. - more here), I would define for ARMCC self.flags['common'].extend(["--debug", "--dwarf3", "--no_debug_macros"]) or -g instead of --debug, it's the same.

@bogdanm
Copy link
Contributor

bogdanm commented Sep 19, 2016

If we are happy with --no_debug_macros

That looks like a good compromise to me, but we still need to assess the impact of the increased size on our CIs. @bridadan

@meriac
Copy link
Contributor

meriac commented Sep 19, 2016

@0xc0170 Thanks a lot for running the numbers! Can you please update the pull request accordingly so we can get it merged? @screamerbg @bogdanm

@meriac
Copy link
Contributor

meriac commented Sep 19, 2016

@0xc0170 Might be worth trying --remove_unneeded_entities and --no_debug_macros in combination, too.

@bridadan
Copy link
Contributor

@bogdanm I think the build increase @0xc0170 mentioned is fairly reasonable. It should be ok and shouldn't cause issues with CI. If there are more changes to be made to this PR could you notify me when they're done? It'd be good to run a release build locally too to try and gauge how big it is.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 20, 2016

@0xc0170 Might be worth trying --remove_unneeded_entities and --no_debug_macros in combination, too.

Demo has now 13mb output folder, that means increase only 2x. I'll try to fetch this PR, and update it to the current code base.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 20, 2016

I am going to send a new pull request , rebased on top of master, and applied new flags for ARMCC, will reference it here.

@0xc0170 0xc0170 closed this Sep 20, 2016
JarkkoPaso pushed a commit to JarkkoPaso/mbed-os that referenced this pull request Aug 26, 2019
…..4a19dc4

4a19dc4 Import new thread files
f6a021d Removed test files
b9e842a Merge branch 'release_internal' into release_external
7d5d869 Merge pull request ARMmbed#2167 from ARMmbed/release_internal_merge
26e2d43 Merge branch 'master' into release_internal
f43620f Add support for India band (ARMmbed#2166)
122f158 Merge pull request ARMmbed#2165 from ARMmbed/release_internal_merge
0e65ee5 Added disabling of NA for Thread BR PPP backbone
4c50e52 Added disabling of NA for Wi-SUN BR PPP backbone
d2ea325 Moved DAD enabled check to Ipv6 SLAAC handler
49994fc Added PPP interface to nanostack
3383e91 Merge pull request ARMmbed#2163 from ARMmbed/IOTTHD-3558
81f7511 MAC: print RF configs
397240a MAC: Implemented CCA threshold and TX power setting
5907042 Added check for allocation failures in EAPOL
9ed97c9 ETX update:
b489415 Add own certificate handling APIS (ARMmbed#2149)
888a0fb fhss_ws: check if 0 used as divider
586f2f2 Merge pull request ARMmbed#2160 from hugueskamba/hk-iotcore-1299-remove-fp-usage-ns_monitor
f1d03b1 Remove floating-point usage in Nanostack heap monitor
ef88f64 Removed rank comprae and also probe 5 best on the list.
a2887d6 Clean PAN id compare trace print.
f37dcf2 Wi-SUN NS NUD & Probe send update
f7133f8 Merge pull request ARMmbed#2158 from ARMmbed/remove_temp_debug_traces
2dc1a8e fhss_ws: removed temporary debug traces
96f962a Reduce wi-sun NS Probe
0a1beb2 GTK update trigger fix
a1d172e Limit Pan config sol timeout after 5 solication.
9d7414b Limit PAE supplikant GTK re-use for authentication from 2->1.
662df08 Fixed Key request address set issue if GTK mismatch is detected.
a56b908 Merge pull request ARMmbed#2153 from ARMmbed/IOTTHD-3650
9b33e98 Fixed mac_pd_sap CCA_PREPARE active ACK handler.
035af9a Enhanced ACK GEN and TX update
b1beb5d fhss_ws: typecast drift to int32_t
f786fc9 Merge pull request ARMmbed#2152 from ARMmbed/fhss_coverity_fix
6efff35 fhss_ws: Coverity fixes
d743e91 WS LLC brodacst shedule fix
6a6fb0c Removed old configuration options from Border router API
a051865 Merge pull request ARMmbed#2135 from ARMmbed/IOTTHD-3232
ff771b1 Added empty interface function for network name set
e94da3c Merge pull request ARMmbed#2146 from ARMmbed/IOTTHD-3571_2
234e649 added network name change function to public API
1770465 fhss_ws: Added temporary debug traces (IOTTHD-3571)
d400859 Fix Thread 1.1 unitests (ARMmbed#2145)
38978f3 wi-sun ETX update:
4a71b04 Adjust Thread functions defined for Thread 1.2 (ARMmbed#2139)
4d8dc0d remove border router from pan size calculation
fb3363e Merge pull request ARMmbed#2141 from ARMmbed/IOTTHD-3571
f01c5f2 fhss_ws: conversion macros/functions to support int64
a7b0027 Suprress dio sending whenRPL is not yet ready
f8c9d54 Adjust tracing (ARMmbed#2138)
678eaf8 Moved Thread 1.2 code to to correct place
f39d07e Merge pull request ARMmbed#2136 from ARMmbed/IOTTHD-3571
ab23116 FHSS: temporary debug traces (IOTTHD-3571)
09d4b06 MAC: Implemented PHY statistics

git-subtree-dir: features/nanostack/sal-stack-nanostack
git-subtree-split: 4a19dc4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet