Skip to content

sys/fmt: use fwrite with picolibc#14841

Closed
benpicco wants to merge 1 commit intoRIOT-OS:masterfrom
benpicco:picolibc-fmt
Closed

sys/fmt: use fwrite with picolibc#14841
benpicco wants to merge 1 commit intoRIOT-OS:masterfrom
benpicco:picolibc-fmt

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 24, 2020

Contribution description

Use fwrite() instead of write() to fix linking with picolibc.

(Why even keep the separate write codepath?)

Testing procedure

make -C examples/gnrc_networking BOARD=samr21-xpro PICOLIBC=1 -j

should be working now.

Issues/PRs references

follow-up to #14827
#14843 offers a more comprehensive solution.

@benpicco benpicco requested a review from kaspar030 as a code owner August 24, 2020 17:58
@benpicco benpicco requested a review from bergzand August 24, 2020 17:58
@benpicco benpicco added Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 24, 2020
@benpicco benpicco requested a review from maribu August 24, 2020 18:11
@maribu
Copy link
Member

maribu commented Aug 24, 2020

I assume the reason to use fwrite() is that you prefer buffered I/O, e.g. especially when using USB ACM for stdio?

Wouldn't using fwrite() over write() fix the issues with mixing puts() and printf() with fmt functions for the same reason? As the RIOT code base is full of calls to puts() and printf() anyway, is there any advantage in terms of ROM size in using write() over fwrite()? Otherwise, we might want to consider to just always using fwrite().

@benpicco
Copy link
Contributor Author

benpicco commented Aug 24, 2020

With newlib:

fwrite

  97604	    184	  19236	 117024	  1c920	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

write

   text	   data	    bss	    dec	    hex	filename
  97404	    184	  19236	 116824	  1c858	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

But I keep wondering how fmt keeps working if the vfs module is used.
If 'our' write function is used, this will crash & burn (or corrupt a file) - #14827 (comment)

@maribu
Copy link
Member

maribu commented Aug 24, 2020

With newlib:

fwrite

  97604	    184	  19236	 117024	  1c920	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

write

   text	   data	    bss	    dec	    hex	filename
  97404	    184	  19236	 116824	  1c858	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

But I keep wondering how fmt keeps working if the vfs module is used.
If 'our' write function is used, this will crash & burn (or corrupt a file) - #14827 (comment)

Hmm, 200 B ROM seems to be worth the pain of using either fwrite() or write(). But we certainly should go for fwrite() if vfs is used - at least until vfs takes special care for fds 0-2.

@benpicco
Copy link
Contributor Author

Ok if we do this as an intermediate solution I’m fine.

Use fwrite() instead of write() to fix linking with picolibc.
@benpicco
Copy link
Contributor Author

I was wrong, VFS already does the right thing - no need to avoid write here.
But of course it is preferable to always provide the write function.

@benpicco benpicco 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 Aug 25, 2020
@benpicco
Copy link
Contributor Author

#14843 was merged instead.

@benpicco benpicco closed this Sep 11, 2020
@benpicco benpicco deleted the picolibc-fmt branch September 11, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants