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

Fix: Factor out integer-only stdio into a common header #1537

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

ALTracer
Copy link
Contributor

Detailed description

  • The previous PR Fix: Decouple headers general.h/platform.h #1530 reduced visibility of "newlib integer-only stdio" redefinitions to only those translation units which #include platform.h This lead to flash size increase for 7 platforms (all not using newlib-nano) by 37 KiB because of linker pulling float/double capable stdio implementations (and softfp routines for double etc).
  • This PR ties these redefinitions back to platform_support.h visible via general.h to every platform except hosted.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Closing issues

Testing

To reproduce/verify, make all_platforms and compare binary sizes of files under src/artifacts/v1.9.0-... for relevant revisions.
Also Puncover to check for functions pulled from libc.
I don't know yet how to easily automatically perform checks for ENABLE_DEBUG=1 builds without encountering build failures or invoking make per-platform manually, but these binaries are Big anyways.

Miscellaneous

If there is a better header to place this in than platform_support.h, then I'll edit this branch.
I could also edit the copyright string, author and filename for "stdio_newlib.h" as I'm only reorganizing the header contents and not actually writing implementation code in this PR. The touched platform.h headers contain at least three authors in "Written by" strings.

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We were contemplating if "stdio_newlib.h" is necessarily the best name for this new header - perhaps "newlib_stdio_defs.h" or something might be better.. but as this fixes both a huge amount of duplication and a bug, we're going to get this merged so main gets unbroken.

@dragonmux dragonmux merged commit 86c9588 into blackmagic-debug:main Jul 12, 2023
6 checks passed
@dragonmux dragonmux added this to the v1.10 milestone Jul 12, 2023
@dragonmux dragonmux added Bug Confirmed bug BMP Firmware Black Magic Probe Firmware (not PC hosted software) Regression Bug caused by a regression labels Jul 12, 2023
@ALTracer ALTracer deleted the newlib-stdio-int branch July 14, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BMP Firmware Black Magic Probe Firmware (not PC hosted software) Bug Confirmed bug Regression Bug caused by a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants