Skip to content

ports/rp2: Use nano.specs for C++ modules. #11143

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gadgetoid
Copy link
Contributor

Remove C++ stack unwinding and exception handling in all cases on user C++ modules. There's no hook to surface exceptions to MicroPython so they're basically useless?

Set options as per https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_cxx_options/CMakeLists.txt

Despite the options linked above, without the addition of -specs=nano.specs, this doesn't make much difference. Since Pico SDK sets nosys.specs elsewhere this is slightly odd, but I'm in a little over my head. Does anyone know what could be happening here?

Save around 10,880 bytes of RAM in our Pico builds (with a number of C++ modules) and returns it to MicroPython's heap for end-users.

@stinos
Copy link
Contributor

stinos commented Jul 3, 2024

There's no hook to surface exceptions to MicroPython so they're basically useless?

I can imagine people would want to use

try
{
  //do C++
}
catch(const std::exception&)
{
  mp_raise_msg(&mp_type_RuntimeError, "oops your C++ code threw an exception");
}

Not sure what would happen in such case with this PR, but probably won't work as expected, so I'd say this should be optional?

@Gadgetoid
Copy link
Contributor Author

I'd say this should be optional?

Probably, I have no idea what I'm doing and why nosys.specs seems to be the only way I can disable the exception handling stuff here.

All I know for sure is that our USER_C_MODULES builds (a lotta modules to be fair) bloat up and overflow the flash app size when these features are enabled, and we don't ever use C++ style exception handling.

I guess ideally this would be possible to disable per module? In fact I wonder if I've ever actually tried just adding this to our USER_C_MODULES cmake files...

It would probably also be nice if builds didn't build at all when User Flash overlaps the App, but that's been quite tricky. See this saga: #8761

Right now it'll happily build a MicroPython port where the user filesystem simply overwrites the end of the app, corrupts it and renders the Pico unbootable until it's reflashed.

@stinos
Copy link
Contributor

stinos commented Jul 3, 2024

I wonder if I've ever actually tried just adding this to our USER_C_MODULES cmake files...

Ah, I though you tried that first then made it into a PR because it worked :)

@Gadgetoid
Copy link
Contributor Author

Ah, I though you tried that first then made it into a PR because it worked :)

I've only ever added it directly to the CMakeLists.txt. Turns out there's no real need for it to be there, other than as a hint/helper/optional thing to prevent future C++ module users running into the same trouble I did- not thinking to put it into our downstream cmake files has caused me untold pain over the last few years. 🤦

pimoroni/pimoroni-pico#968

I guess it would make more sense as a snippet in examples/usercmodule/cppexample/micropython.cmake with appropriate comments.

Though I'm still not sure why nano.specs is needed and how that might conflict with nosys.specs used elsewhere.

@dpgeorge
Copy link
Member

dpgeorge commented Jul 4, 2024

I guess it would make more sense as a snippet in examples/usercmodule/cppexample/micropython.cmake with appropriate comments.

Yes, that would make sense. If you want to change this PR to do that, I'd be happy to merge it.

Though I'm still not sure why nano.specs is needed and how that might conflict with nosys.specs used elsewhere.

I also have no idea about this.

@Gadgetoid Gadgetoid force-pushed the patch-cxx-nano-specs branch from b7a8978 to 1fddc99 Compare July 12, 2024 16:00
@Gadgetoid
Copy link
Contributor Author

I have re-targeted this but I'm still not sure it's suitable since:

  1. I don't know why it works
  2. The commented-out snippet isn't run in CI, so we wont know if/when it breaks

But at least, for now, it's here for posterity.

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.42%. Comparing base (ee10360) to head (65913d8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11143   +/-   ##
=======================================
  Coverage   98.42%   98.42%           
=======================================
  Files         161      161           
  Lines       21253    21253           
=======================================
  Hits        20919    20919           
  Misses        334      334           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Document how to reemove C++ stack unwinding and exception handling.

Set options as per https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_cxx_options/CMakeLists.txt

Saves around 10,880 bytes of RAM in our batteries-included build.

Signed-off-by: Phil Howard <phil@gadgetoid.com>
@Gadgetoid Gadgetoid force-pushed the patch-cxx-nano-specs branch from 1fddc99 to 65913d8 Compare July 12, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants