Skip to content

Use O0 for a better debug experience #14562

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

Closed
wants to merge 2 commits into from
Closed

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Apr 19, 2021

Summary of changes

Make Debug profile use optimization level -O0 instead of -O1.

Impact of changes

Previously variables in scope could have been optimized out in Mbed Studio when building with the Debug profile.
Now their values should remain visible.

Migration actions required

Documentation

None


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Apr 20, 2021
@ciarmcom ciarmcom requested a review from a team April 20, 2021 00:00
@ciarmcom
Copy link
Member

@rotu, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 20, 2021

Hi @rotu , what problems you have with O1 optimizations?

https://developer.arm.com/documentation/dui0472/k/Compiler-Coding-Practices/Compiler-optimization-levels-and-the-debug-view - worth reading about good practices.

Is it this one that this PR is addressing:

Values of variables might not be available within their scope after they have been initialized. For example if their assigned location has been reused.

There are implications of switching to O0. The size increase is one of them.

How does your experience change if you add --debug for debug profile ? As suggested for O1:

If used with --debug, this option gives a generally satisfactory debug view with good code density.

Edit: we define -g there

@kjbracey
Copy link
Contributor

kjbracey commented Apr 20, 2021

--debug is just the long form of -g.

In my experience the debuggability of -O1 (or -Og for GCC) is overstated - every time I use it I get irritated by missing info. I would always go for -O0 if I had the ROM space, in both compilers.

But the "debug" profile was changed to -O1 because -O0 code is a LOT bigger, and I believe it was argued that anyone wanting -O0 could make a local edit.

Here was a previous PR wanting to change it by adding a new profile: #12165. Check discussion there. And also discussion on the docs change: ARMmbed/mbed-os-5-docs#1195

@rotu
Copy link
Contributor Author

rotu commented Apr 20, 2021

Hi @rotu , what problems you have with O1 optimizations?

The issue I regularly come up against is variables being optimized out. But all of these caveats negatively impact the debuggability of code:

  • Values of variables might not be available within their scope after they have been initialized. For example if their assigned location has been reused.
  • Functions with no side-effects might be called out of sequence, or might be omitted if the result is not needed.
  • Backtrace might not give the stack of open function activations that is expected from reading the source because of the presence of tailcalls.

When using the debugger, the -O0 is the only armclang optimization level that faithfully reflects the code. If not using the debugger, the "Develop" build profile seems to be most appropriate.

Maybe there's some hidden optimization level that provides a better middle ground?

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 21, 2021

I would rather focus on documentation and custom profiles to edit compiler/linker options. There is always a flag to change. The best customization is achieved by a custom profile.

mbed-cli 1 provides custom profiles. mbed-cli 2 require more work for handling toolchain file that should take care of this. The current CMake has predefined profiles that you can edit until we get toolchain file support.

@rotu
Copy link
Contributor Author

rotu commented Apr 21, 2021

I would rather focus on documentation and custom profiles to edit compiler/linker options. There is always a flag to change. The best customization is achieved by a custom profile.

It's not mutually exclusive. In the linked discussions it seems the general consensus is that O0 is more useful.

That said, it's not obvious to the user that they should RTFM in the first place - it is rather unexpected behavior that a profile called "Debug" will hide information from the "Debugger".

My experience with GCC is that the current profile is good enough; although -Og applies some optimization, it doesn't obstruct debugging in the way that O1 does on ARMC6.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 22, 2021

Based on the feedback, we should rather create an issue to change this in the next major version. As there are consequences (mainly about the final size), I believe this would be a breaking change and it should be in the next major version.

@pan-
Copy link
Member

pan- commented Apr 22, 2021

@0xc0170 With the CMake work, would it be possible to select certain level of optimization for specific libraries ?

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 23, 2021

I remember typing the reply yesterday. Yes it should be. A library can add options via compile options/compiler features, etc. To just keep it for a library, use PRIVATE, so won't affect others that link to it.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 27, 2021

Based on the feedback, we should rather create an issue to change this in the next major version. As there are consequences (mainly about the final size), I believe this would be a breaking change and it should be in the next major version.

@donatieng Please review

@0xc0170 0xc0170 requested a review from donatieng April 27, 2021 12:43
@ciarmcom ciarmcom added the stale Stale Pull Request label May 10, 2021
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. @donatieng, @ARMmbed/mbed-os-maintainers, please complete review of the changes to move the PR forward. Thank you for your contributions.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 14, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 21, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 28, 2021
@donatieng
Copy link
Contributor

Hi folks,

I agree about the sentiment - in practice I've often had to switch to -O0 to do some proper debugging! But @0xc0170 is right that it would amount to a breaking change as it would really increase the size of anything compiled with the debug profile.

@0xc0170 With the CMake work, would it be possible to select certain level of optimization for specific libraries ?

Seems the ideal solution, let's look at how this could be achieved in the next major release.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 4, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 11, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 18, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 25, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jun 30, 2021

I'll close this as it can't be accepted currently due to the breaking change. Please use custom profile meanwhile.

@rotu Please create a tracking issue for this change

I'll close this pull request now.

@0xc0170 0xc0170 closed this Jun 30, 2021
@mergify mergify bot removed component: tools needs: review release-type: patch Indentifies a PR as containing just a patch stale Stale Pull Request labels Jun 30, 2021
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.

6 participants