-
Notifications
You must be signed in to change notification settings - Fork 90
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
Support picolib #357
base: master
Are you sure you want to change the base?
Support picolib #357
Conversation
b303b2c
to
c480eba
Compare
Needs a formatting pass? Having a hard time parsing the CI output. It's very exciting to see picolibc support, though it'll be interesting if this build also alleviates the malloc concerns of #305 (comment). |
The problem with this is that picolibc seems to want core functions without underscores: int fstat(int fd, struct stat *st) { return _fstat(fd, st); }
int isatty(int fd) { return _isatty(fd); }
int read(int fd, void *buf, uint32_t count) { return _read(fd, buf, count); }
int write(int fd, const void *buf, uint32_t count) { return _write(fd, buf, count); }
int lseek(int fd, uint32_t offset, int whence) { return _lseek(fd, offset, whence); }
int close(int fd) { return _close(fd); }
caddr_t sbrk(int incr) { return _sbrk(incr); } whereas newlib wants the versions with the underscore. My attempt to include both causes some apps to fail to compile. In any case, we probably don't want both. I have no idea why picolib deviates here, or if there is a way to request the _ versions. It's worth considering the picolibc flags I am using:
I don't know what In any case, I've looked over the picolibc build options a couple times and I can't figure out what would have an impact on _ system names versus not. |
AFAICT, the presence/absence of the leading underscore is some unimportant fight among various library/tool vendors: https://stackoverflow.com/questions/36438732/close-vs-close-read-vs-read-write-vs-write-what-is-difference — neither is [in]correct. Best (sigh) solution here is probably to |
oh gross
I think we can add this in a reasonable way |
549d516
to
eb82f24
Compare
Hurrah this passes CI! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not look into the C bindings in depth just yet, but I think this is a great step in the right direction and wouldn't want to block this on a more in-depth review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add text for missing comments.
This needs a rebase onto the new library build infrastructure, which I think should be stable now.
6d8245c
to
bc3497a
Compare
Rebase and updated. |
Doesn't work on a fresh checkout currently. Had enough time to debug quickly but not push fix sorry. Need to add
|
Does something like d4cb0c3 work? |
7a7b781
to
b7b8c02
Compare
Some functions differ between newlib and picolib. Those are now in tock_sys.c.
Switch to TOCK_LIBC variables rather than assuming newlib
Co-authored-by: Pat Pannuto <[email protected]>
The newlib and picolib targets can't match both rules.
b7b8c02
to
f04ba37
Compare
Beyond the CI error, I don't think the rules are right either. Without debugging deeply, on a fresh checkout it will try to fetch picolib twice:
which ultimately causes the build to hang because of this question. full output[-bash] Fri 14 Jun 12:36 [[make-precompiled-picolib] /tmp/libtock-c/examples/blink] |
Ah, I looked at d4cb0c3 now. Short answer, no. That's actually what I tried the first time for newlib/libc++, and how I eventually ended up with the extra directory. The easy answer is to just embrace the subdirectory like I originally had. It's what |
This uses the new precompiled newlib system to create a compiled picolib for libtock-c.
Hopefully this sweetens the deal for #353.