sys/newlib: Split stubs and make modules provide non-stubs

[jcarrano]

Background

To get the full standard C library, newlib needs to be provided, at minimum, with the following basic calls (shown here in their reentrant versions):

File handling: _open_r, _close_r, _lseek_r, _read_r, _write_r, _stat_r, _fstat_r, _link_r, _unlink_r, _fcntl_r

Process control: _execve_r, _wait_r, _fork_r, _getpid_r

Memory management: _sbrk_r

Timekeeping: _gettimeofday_r, _times_r

Stubs for these functions can be found in the newlib source, under newlib/libc/reent/.

If these are provided, newlib will implement the rest of the standard functions directly or indirectly based on them. Some of the basic calls are not C but rather POSIX definition, which are generally more strict. For example POSIX's times() is more detailed than C's clock(). By default newlib implements clock() using times().

There is also the possibility of implementing some higher level function. For example, an alternate definition of clock() can be provided that does not use times(), or a version of malloc() that does not use sbrk().

Problem

The current implementation of the above mentioned functions lives in sys/newlib_syscalls_default/syscalls.c. For filesystem functions (and _gettimeofday_r), functioning implementations are provided if VFS (or xtimer) is included, for other functions, there are only stubs. Also, some are missing (see #9051), and some incorrectly defined (see #9187)

The problem with stubs is that, while useful (sometimes we have external packages that depend on having C standard function available and it may be acceptable if these are stubs that always fail) they may turn a linker error into a runtime error. It would be desirable to let the user decide and make him explicitly request to have stubs compiled in.

Contribution

For a detailed explanation, see the commit messages.

I propose that the support for C standard functions is provided directly by whatever module can do it. In the first commit, I'm making xtimer provide clock(). In the next commit, I will make RTC provide for the "absolute time" functions (either _gettimeofday_r or time()).

The implementation for different types of functions (file handling, process control, etc) will be split into several modules to allow the user to decide for what parts of the API he wants stubs. Stub modules will be provided for the different categories.

New modules and submodules:

• newlib_syscalls_riot: the "real" thing
  • clock: relative time using xtimer (the clock() function)
  • time: absolute time using RTC (the time() function)
• newlib_stubs: same as above, but with stubs instead of a real implementation.

Testing

For the clock() implementation:

# test the stub
$ USEMODULE=newlib_stubs_clock BOARD=saml21-xpro make -C tests/libc_time_h_relative all flash term
{ time should be zero all the time }
# test the real thing
$ BOARD=saml21-xpro make -C tests/libc_time_h_relative all flash term
{ times difference should be 5 (because it sleeps for 5 seconds between calls) }

For time():

# test the stub
$ USEMODULE=newlib_stubs_time BOARD=saml21-xpro make -C tests/libc_time_h_absolute all flash term
{ bogus time shown }
# test the real thing
$ BOARD=saml21-xpro make -C tests/libc_time_h_absolute all flash term
{ on each character sent through serial the curent time will be printed, you should be able to see how time advances }

Issues/PRs references

This is a longer term and better solution than my previous PR: #9090
The package that is giving me headaches with the C api: #8847, #9153

If the stubs are split, implementing #10292 becomes easier and nicer.

[jia200x]

The problem with stubs is that, while useful (sometimes we have external packages that depend on having C standard function available and it may be acceptable if these are stubs that always fail) they may turn a linker error into a runtime error. It would be desirable to let the user decide and make him explicitly request to have stubs compiled in.

Agreed

I propose that the support for C standard functions is provided directly by whatever module can do it. In the first commit, I'm making xtimer provide clock(). In the next commit, I will make RTC provide for the "absolute time" functions (either _gettimeofday_r or time()).

I like the idea of implementing these functions in a higher layer thant xxx_r functions, but tend to think these POSIX functions should be implemented on top of RIOT modules and not by the modules. See for instance here.

If there are several modules implementing POSIX functions it could be really hard to configure properly. Otherwise, everything under sys/posix can be configured or arranged as wished.

[jia200x]

@jcarrano on second thoughts (and following the case of sock_t structs), I think the proposal you describe can do the trick. The only issue I could see (fixable though) is a way to prevent to modules implementing the same functions so we don't get duplicated symbols

[jcarrano]

@jia200x Right now I don't think it's a problem because there is no overlapping functionality (we have one vfs, one xtimer, etc). Creating a separate module is a good proposal too for future proofing. We might see a proliferation of modules, though.

[jia200x]

As you say, I think it can solve the problem for some time. If at some point becomes a problem, we can fix it later :)

[jcarrano]

@jia200x Maybe we can mark this as a WIP, and push #9090 in the meanwhile. #9090 does not break anything, and this PR will take me a while to develop.

[jcarrano]

I'm experimenting with a new C library, baselibc (see #10292 ), and having the newlib support split would help me quite a bit.

baselibc does not replace newlib. It sits on top, replacing some of the functions. This means that some adaptors/stubs still need to be there, and some others need to be implemented for baselibc instead.

Regarding this PR:

I'm not 100% sure that implementing clock() in xtimer.c is the right thing to do, instead of creating a new module that implements just that function.

[kaspar030]

I think the ordering needs rework. what's "stub_std_*" and why does it have to be in sys/?

Also I don't think that RIOT modules should provide libc functions just because they re related or can be based on them.

[jcarrano]

stub_std_* is a stub for the standard library. Part of the motivation for this change is to make the use aware when he is using stubs, instead of silently putting in a function that fails.

I don't see any other place to put such a module than in sys.

Also I don't think that RIOT modules should provide libc functions just because they re related or can be based on them.

Ok, so that means that I should split the clock() implementation into it's own module (xtimer_std_clock).

[kaspar030]

stub_std_* is a stub for the standard library.

Noone calls libc "the standard library" or "std".

I don't see any other place to put such a module than in sys.

If they're newlib stubs, sys/newlib-syscall-default/stubs/clock.c.
If they work for more than one libc, how about sys/libc/stubs/clock.c, but do we actually need that?

Ok, so that means that I should split the clock() implementation into it's own module (xtimer_std_clock).

I'd say so. make it a submodule of newlib-syscall-default. put in sys/newlib-syscalls-default/clock_xtimer.c.

[jcarrano]

Noone calls libc "the standard library" or "std".

Is "stdlib" better?

sys/newlib-syscall-default/stubs/clock.c

I want to avoid SUBMODULES. It is an unnecessary complication and there is really no need. If I use a regular module I just have to create a directory and a trivial Makefile and I'm done.

Regarding PSEUDOMODULEs, they should only when they are needed, that is, when they enable or disable code that cannot be split apart.

[kaspar030]

Is "stdlib" better?

No, "stdlib" is ambiguous with stdlib.h. It is libc or C standard library.

I want to avoid SUBMODULES. It is an unnecessary complication and there is really no need. If I use a regular module I just have to create a directory and a trivial Makefile and I'm done.

All is fine and well if you want to avoid submodules. Open an issue so we can discuss and change current best practices. Until then, use submodules.

Why do you want to make "clock()" it's own module?

[jcarrano]

Why do you want to make "clock()" it's own module?

Because that is what it is, a component, which can be depended upon and with dependencies to other modules. I can be taken in and out independently of other modules. Too many issues have arisen from not keeping the module system simple.

[kaspar030]

Because that is what it is, a component, which can be depended upon and with dependencies to other modules. I can be taken in and out independently of other modules.

It is also horribly inefficient. Distinct module gets it's own folder, Makefile, submake instance, ...
It is just overkill to have a module per function, with the current module system.

Too many issues have arisen from not keeping the module system simple.

I tend to agree, but which that has relevance are you referring to?

[jcarrano]

It is also horribly inefficient.

Only because of the current implementation. I'm against architecture our system around limitations that can be avoided (and will eventually be fixed). It just makes the problem worse. It's not a module per function, it's just that this one module happens to have one function. I try to avoid to avoid such subjective grouping based on what "seems" small.

[kaspar030]

It's not a module per function, it's just that this one module happens to have one function.

I was under the impression that there'll be multiple implementations for libc functions coming.
E.g., clock_xtimer, clock_stub, time_rtc, time_stub. I'd rather have them in one newlib folder and make them submodules than clutter the file tree.

I'm against architecture our system around limitations that can be avoided (and will eventually be fixed).

The need for a folder & Makefile & DIRS entry & Makefile.dep entry per module is a technical limitation that will eventually get fixed. Without that, you'd put clock_xtimer.c whereever it fits best and maybe mention it once in a proper build description file. Using submodules is a step towards fixing the modules system.

Why again are submodules too complicated?

[jcarrano]

Following some discussions with @cladmi, I reorganized the module structure and changed the names.

[kaspar030]

IMO, this could be massively simplified by just adding conditional compile time warnings to the stub implementations. No need to touch 19 files and multiply 40 lines by ten.

I'm thinking #warning newlib: xtimer module not selected, using stubs for foo, bar, ...!.
E.g., just add what you're missing: a compile time warning/error when stubs are being used.

Anyhow,

• please keep/move newlib stuff in/to sys/newlib
• whatever made you use UNDEF, drop that, there's probably another way
• please aggregate the tests. Why not make them part of the unittests, or a generic "libc" test, or extend the existing libc_newlib, if the?
• if you go with one function per file, call the file like the function
• now there's "newlib_syscalls_riot" and "newlib_syscalls_default". I'm guessing that "_riot" means "implements newlib functionality using RIOT facilities". In other contexts, "newlib_syscalls_X" points to a platform. I'm not sure we should mix that.

[jcarrano]

IMO, this could be massively simplified by just adding conditional compile time warnings to the stub implementations.

That is not the point. The point is to be able to choose the implementation, and to be able to provide a subset of the functionality. If you look at the MIPS UHI syscalls you will see some of the stuff is copy-pasted from newlib_syscalls_default. By being granular only some syscalls can be replaced.

Also, I don't want to see a warning if I chose to use a stub, and I don't want an app to be built with stubs automatically, because a broken app should not even build. Only if the user decides that it is OK to have a stub for a certain functionality should the compilation proceed, and there is no point in reminding him with a warning of that he himself did.

I think that handling conditional compilation/inclusion using the build system is superior to using pre-processor directives:

• The code is one and easier to understand.
• Dependency issues can be resolved ahead of time.
• The logic is in only one place: the build system.

whatever made you use UNDEF, drop that, there's probably another way

The UNDEF was taken from here:

RIOT/sys/newlib_syscalls_default/Makefile.include

Line 1 in 3df1a82

UNDEF += $(BINDIR)/newlib_syscalls_default/syscalls.o

If it needs to be removed then OK, but I would do that on another PR (and make this one depend on it).

Why not make them part of the unittests

I think I can come up with a self-testing, non interactive test. I don't think I can test the stubs in the unit tests with the rest of the stuff because it requires linking a conflicting set of symbols.

if you go with one function per file, call the file like the function...

I'm not going with one function per line, only in these cases it happens to contain only one, and because they implement different, unrelated functionality with no interdependency, they are split. When I do process management and file system, there will be more than one function. The "clock" stub implements _times_r instead of clock because that way it can cover the times() function too (in fact the clock stub and the xtimer-based clock() are compatible: you get a working implementation of clock() and a times() stub, which makes sense since times() should report information we don't have).

now there's "newlib_syscalls_riot" and "newlib_syscalls_default".

newlib_syscalls_default will go away once I split it completely. I want to change the current behaviour of newlib_syscalls_default of being included by default, to one in which the selection is explicit.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.