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

shell/cmds: GNRC: replace puts() with printf() #19327

Merged
merged 3 commits into from Feb 27, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 27, 2023

Contribution description

Strings output with printf() will automatically be placed in the .text section on AVR now.
This is not true for strings output with puts().

Replace puts() with puts() by running

sed -Ei 's/puts\(\"(.*)\"\)/printf\(\"\1\\n\"\)/g'

Testing procedure

Observe the reduced .data RAM usage on AVR:

master

   text	   data	    bss	    dec	    hex	filename
 123950	   6710	  13123	 143783	  231a7	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/avr-rss2/gnrc_networking.elf

this PR

   text	   data	    bss	    dec	    hex	filename
 129840	   2042	  13123	 145005	  2366d	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/avr-rss2/gnrc_networking.elf

Issues/PRs references

@benpicco benpicco requested a review from maribu February 27, 2023 14:37
@github-actions github-actions bot added Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System labels Feb 27, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 27, 2023
@riot-ci
Copy link

riot-ci commented Feb 27, 2023

Murdock results

✔️ PASSED

b1e327d examples/gnrc_networking: remove microduino-corerf from Makefile.ci

Success Failures Total Runtime
6864 0 6864 10m:45s

Artifacts

@bergzand bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 27, 2023
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Since we can rely on the compiler to emit calls to puts("Foo"); when it sees printf("Foo\n");, this should not impact non-AVR platforms. And it still is idiomatic C code.

@benpicco
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Feb 27, 2023
19321: examples/gnrc_border_router: add BLE as downlink option r=benpicco a=benpicco



19325: esptools/install.sh: Fix shellcheck issues r=benpicco a=bergzand

### Contribution description

Quote all the things!


### Testing procedure

The script should still work as before


### Issues/PRs references

None

19327: shell/cmds: GNRC: replace puts() with printf() r=benpicco a=benpicco



19328: pkg/u8g2: bump version r=benpicco a=benpicco



Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
Co-authored-by: Koen Zandberg <koen@bergzand.net>
Co-authored-by: Benjamin Valentin <benpicco@beuth-hochschule.de>
@bors
Copy link
Contributor

bors bot commented Feb 27, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request Feb 27, 2023
19327: shell/cmds: GNRC: replace puts() with printf() r=benpicco a=benpicco



19328: pkg/u8g2: bump version r=benpicco a=benpicco



Co-authored-by: Benjamin Valentin <benpicco@beuth-hochschule.de>
@bors
Copy link
Contributor

bors bot commented Feb 27, 2023

Canceled.

@benpicco
Copy link
Contributor Author

bors cancel
bors merge

@bors
Copy link
Contributor

bors bot commented Feb 27, 2023

Build succeeded:

@bors bors bot merged commit 6b501f7 into RIOT-OS:master Feb 27, 2023
@benpicco benpicco deleted the gnrc_shell-printf branch February 27, 2023 20:02
@OlegHahm
Copy link
Member

Why do strings in a puts() call don't get into the text section? How do we prevent this from being re-introduced in the future?

@benpicco
Copy link
Contributor Author

benpicco commented Feb 27, 2023

We don't enforce the puts() string to be const a string literal but we do for printf() (originally to prevent format string bugs).

There is a case for outputting non-const literal strings with puts() so we can't automatically place the argument in flash.

@OlegHahm
Copy link
Member

Thanks for the explanation. I still wonder if or how we could check for any regressions in this direction.

@maribu
Copy link
Member

maribu commented Feb 27, 2023

On ATmega and Xtensa const char * will still land in RAM due to flash memory not being mapped in the data address space. When declaring and initializing a variable we can add the __flash attribute for those to force it to flash (and the compiler will correctly load them from flash on access), but this requires some set up.

For printf() the first argument has to be a string literal to allow the compiler to validate format specifiers to match the remaining arguments. This is enforced in RIOT with -Wformat=2 for ages anyway. We can levarage this to just force the format string to flash on AVR / Xtensa transparently.

For puts() the argument can be a non-literal. We have no reason to force it to be a literal (well, other than AVR / Xtensa profiting), so we cannot do that. Hence, we cannot do the same trick with puts().

@benpicco
Copy link
Contributor Author

I suppose we could to

#define puts_literal(s) flash_puts(TO_FLASH(s))

but that might be a bit controversial

@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants