Make syscall internal implementation visibility consistent

[ceolin]

Syscall internal implementations were spread across headers, as static
inline, and C sources. The generated syscall code was declaring all
internal impl as an extern function because of the ones implemented
in C sources. This was violating MISRA-C rule 8.8 in the cases
internal functions were implemented in the headers.

[codecov-io]

Codecov Report

Merging #12220 into master will decrease coverage by 0.05%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master   #12220      +/-   ##
==========================================
- Coverage    48.3%   48.25%   -0.06%     
==========================================
  Files         293      294       +1     
  Lines       44136    44126      -10     
  Branches    10591    10589       -2     
==========================================
- Hits        21318    21291      -27     
- Misses      18552    18566      +14     
- Partials     4266     4269       +3

Impacted Files	Coverage Δ
include/entropy.h	60% <ø> (-6.67%)	⬇️
include/kernel.h	97.36% <ø> (+2.92%)	⬆️
kernel/sem.c	93.75% <100%> (+0.72%)	⬆️
kernel/poll.c	81.87% <100%> (+0.34%)	⬆️
kernel/queue.c	94.3% <100%> (+0.29%)	⬆️
kernel/msg_q.c	96.26% <100%> (+0.14%)	⬆️
kernel/timer.c	95.06% <100%> (+0.46%)	⬆️
kernel/entropy.c	75% <75%> (ø)
lib/work_q.c	41.17% <0%> (-41.18%)	⬇️
kernel/work_q.c	82.22% <0%> (-17.78%)	⬇️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3980e0c...495965b. Read the comment docs.

[andyross]

All those tiny, isolated functions and the calls to them would strike me as a pretty large increase to code size, no? Isn't there a way to do this that preserves the inlinability? That is, it seems you fixed the "function is defined as both extern and static inline" by making everything extern. Can't we do the opposite?

Maybe have a "NODECLARE" syscall declaration variant that doesn't export the API itself and relies on the user to do it?

[pabigot]

I haven't had a chance to delve into this part of Zephyr yet, and don't know if the following is consistent with MISRA-C, but:

Assuming you require C11 conformance, you can just remove static from the inline definition in the header. Translation units that include the header will see the internal definition but it won't be visible outside the translation unit. A single globally visible external definition can (must) be produced by adding an extern declaration in one translation unit.

E.g. in aio_comparator.h:

inline int _impl_aio_cmp_disable(struct device *dev, u8_t index)
	{
		const struct aio_cmp_driver_api *api = dev->driver_api;
	
		return api->disable(dev, index);
	}

Then in aio_comparator.c:

#include <aio_comparator.h>
extern int _impl_aio_cmp_disable(struct device *dev, u8_t index);

which provides an external definition without needing to duplicate the definition body.

In modern C this is the preferred approach over having static inline in the header. See C11 (ISO/IEC 9899:2011) §6.7.4 Function specifiers, particularly paragraph 7 and the example that follows.

It would make things much nicer if Zephyr did require C11. Since it's seven years old we should be able to expect all toolchains to support it.

[ceolin]

I haven't had a chance to delve into this part of Zephyr yet, and don't know if the following is consistent with MISRA-C, but:
Assuming you require C11 conformance, you can just remove static from the inline definition in the header. Translation units that include the header will see the internal definition but it won't be visible outside the translation unit. A single globally visible external definition can (must) be produced by adding an extern declaration in one translation unit.
E.g. in aio_comparator.h:
inline int _impl_aio_cmp_disable(struct device *dev, u8_t index)
{
const struct aio_cmp_driver_api *api = dev->driver_api;

  return api->disable(dev, index);

}

Then in aio_comparator.c:
#include <aio_comparator.h>
extern int _impl_aio_cmp_disable(struct device *dev, u8_t index);

which provides an external definition without needing to duplicate the definition body.
In modern C this is the preferred approach over having static inline in the header. See C11 (ISO/IEC 9899:2011) §6.7.4 Function specifiers, particularly paragraph 7 and the example that follows.
It would make things much nicer if Zephyr did require C11. Since it's seven years old we should be able to expect all toolchains to support it.

Hi, C11 has several cool things but unfortunately MISRA-C is based on either C99 or C89. We're focusing in C99 right now.h

[ceolin]

@andyross That was my second option, the drawback was increase the complexity in scripts that generate the code (but not that much). This one moved more code but is just simpler.

[ceolin]

@andyross That was my second option, the drawback was increase the complexity in scripts that generate the code (but not that much). This one moved more code but is just simpler.

actually my idea was a little bit different. It was adding a new keyword syscall_no_export and let the scripts only include the headers.

[nashif]

why are we moving all those syscall definitions under kernel/?

[ceolin]

why are we moving all those syscall definitions under kernel/?

didn't know a better place to put these implementations. Anyway, I'm changing the approach, I'll send a new pr with the other solution so we can discuss which one is better

[andrewboie]

Closing this.
I spoke to Flavio offline and we need to file a deviation for this.