Skip to content

sys/newlib: Bugfix, add missing syscalls (only stubs)#9090

Merged
danpetry merged 1 commit intoRIOT-OS:masterfrom
jcarrano:newlib-missing-functions
Jun 21, 2018
Merged

sys/newlib: Bugfix, add missing syscalls (only stubs)#9090
danpetry merged 1 commit intoRIOT-OS:masterfrom
jcarrano:newlib-missing-functions

Conversation

@jcarrano
Copy link
Contributor

@jcarrano jcarrano commented May 7, 2018

Add small stubs for _times and _link_r so that code using times() or link()/rename() can still compile. The functions are marked as not implemented and return invalid codes.

Issues/PRs references

Fixes #9051 .

clock_t _times(struct tms *ptms)
{
memset(ptms, 0, sizeof(*ptms));

Copy link
Contributor

Choose a reason for hiding this comment

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

does errno have to be cleared? Spec says, errno indicates error on error...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaspar030 You are right, this is an example from libgloss (part of newlib):

/* _times -- no clock, so return an error.  */
clock_t _times (struct tms *buf)
{
  errno = EINVAL;
  return (-1);
}

I'm fixing it.

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 9, 2018
@kaspar030
Copy link
Contributor

please squash

@jcarrano jcarrano force-pushed the newlib-missing-functions branch from 2f0e331 to f373803 Compare May 9, 2018 11:36
@basilfx
Copy link
Member

basilfx commented May 14, 2018

@jcarrano The build failed, because of unused parameters.

syscalls.c: In function '_times':
syscalls.c:546:28: error: unused parameter 'ptms' [-Werror=unused-parameter]
 clock_t _times(struct tms *ptms)

You can mark them as unused by adding (void) ptms; to _times.

@jcarrano jcarrano force-pushed the newlib-missing-functions branch from f373803 to ef68f86 Compare May 14, 2018 14:05
{
(void)ptms;

errno = EINVAL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also use the errno variable from struct reent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from libgloss, which is part of newlib.

@jcarrano jcarrano force-pushed the newlib-missing-functions branch from ef68f86 to 171170c Compare May 18, 2018 13:42
@jcarrano
Copy link
Contributor Author

jcarrano commented May 18, 2018

@kaspar030 I changed over to _times_r(). What's curious is that in the newlib source, _times_r() is implemented in terms of times() and all the platform-specific syscall implementations in libgloss implement times(). However, after reading https://www.embecosm.com/appnotes/ean9/html/ch05s04.html I learned that the two options are possible depending on how the library is configured.

Add small stubs for _times_r and _link_r so that code using times()
or link() can still compile. The functions are marked as not implemented
and return invalid codes.
@jcarrano jcarrano force-pushed the newlib-missing-functions branch from a614c10 to cce83b4 Compare June 11, 2018 12:55
@jia200x
Copy link
Member

jia200x commented Jun 11, 2018

@jcarrano any news on this?

@jcarrano
Copy link
Contributor Author

jcarrano commented Jun 11, 2018

@jia200x I addressed @kaspar030 review and squashed. Btw, I'm also working intermittently on what I think is a better fix (#9258 ), but this one should work for now.

@tcschmidt
Copy link
Member

@kaspar030 do changes satisfy your concerns?

@jcarrano jcarrano dismissed kaspar030’s stale review June 20, 2018 12:48

Already addressed

@tcschmidt tcschmidt requested a review from danpetry June 20, 2018 18:46
@danpetry danpetry 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 Jun 21, 2018
Copy link
Contributor

@danpetry danpetry left a comment

Choose a reason for hiding this comment

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

OK I'm satisfied with the approach here, on the basis that it replicates the approach elsewhere and that that approach is being addressed more comprehensively in #9258. Implementation is also consistent with other functions in the module. Also, the added functions aren't actually used elsewhere meaning, for example, they won't have a memory impact anywhere except in the Lua interpreter. Would be good to address the above PR ASAP to avoid errors if other people try to use the related newlib functions.

@danpetry danpetry merged commit ce37449 into RIOT-OS:master Jun 21, 2018
@cladmi cladmi added this to the Release 2018.07 milestone Jul 10, 2018
@jcarrano jcarrano deleted the newlib-missing-functions branch November 8, 2018 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants