MISRA-C Checking function return #9930

ceolin · 2018-09-12T02:55:52Z

This pr is part of #9884 feature. There still violations that will be addressed in others commits/pr.

Important points about this pr:

Commit changing signature of __swap()
- This function can return -EAGAIN but its return value was unsigned int. Nevertheless, this value was being propagate as int.
Change printk()/vnprintk() signature to void

codecov-io · 2018-09-12T03:57:56Z

Codecov Report

Merging #9930 into master will decrease coverage by <.01%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master    #9930      +/-   ##
==========================================
- Coverage    52.5%   52.49%   -0.01%     
==========================================
  Files         213      213              
  Lines       26078    26077       -1     
  Branches     5624     5623       -1     
==========================================
- Hits        13691    13690       -1     
  Misses      10136    10136              
  Partials     2251     2251

Impacted Files	Coverage Δ
subsys/net/lib/coap/coap_link_format.c	`15.74% <ø> (ø)`	⬆️
subsys/net/lib/app/net_app.c	`41.07% <ø> (ø)`	⬆️
subsys/bluetooth/host/uuid.c	`0% <0%> (ø)`	⬆️
subsys/bluetooth/host/hci_core.c	`2.56% <0%> (ø)`	⬆️
subsys/net/lib/http/http_parser_url.c	`72.57% <0%> (ø)`	⬆️
kernel/init.c	`72.58% <0%> (ø)`	⬆️
subsys/net/ip/dhcpv4.c	`6.98% <0%> (ø)`	⬆️
subsys/net/lib/websocket/websocket.c	`17.3% <100%> (ø)`	⬆️
lib/posix/fs.c	`48.29% <100%> (ø)`	⬆️
boards/posix/native_posix/irq_handler.c	`84.52% <100%> (ø)`	⬆️
... and 42 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 81b2721...282e76c. Read the comment docs.

pfalcon · 2018-09-12T04:16:07Z

To the best of my knowledge, the MISRA C experiment (or whatever the right name) should have been applied to "kernel/*". This is explicitly stated in #9552

It would be nice to indeed follow that schedule, or otherwise we get these patches with 237 changed files, attempt to looking thru which shows memset, memset... Again, #9552 states "Adhere to MISRA-C 2012 where it makes sense and provide deviations and documentation of rules", and one of obvious solution for memset is not patching it all the way thru, but leaving alone. it would be nice to see that choice be followed/discussed too.

jhedberg

Doing a void cast on memset seems rather pointless to me, i.e. sounds like a good exception to be made from the MISRA rules. The return value of memset does not exist to denote error/success (which would be a valid reason to acknowledge it when doing the call) rather it's always the same thing, i.e. a pointer to the given memory area, which is rarely useful.

jukkar

I agree with @jhedberg and @pfalcon here. At this first stage we should only concentrate changes in the kernel code. Also changing memset() calls seems quite pointless.

jhedberg · 2018-09-13T11:17:03Z

I guess we're going to have to agree to disagree on whether this memset cast actually makes the code any more safe or secure :) Btw, has anyone considered using __attribute__((warn_unused_result)) instead to improve the code quality? At least both gcc and llvm supports it.

nashif · 2018-09-13T12:48:12Z

I guess we're going to have to agree to disagree on whether this memset cast actually makes the code any more safe or secure :)

nobody is saying that. Following a coding standard or guideline does not make the code safer, and I know it looks really bad with all the voids for memset. This is analog to why we have a coding style and why we format the code the way we do, more or less and provides a standard and consistent way for coding that help spot issues and know the original intention of the developer who wrote the code without too much guessing.

jhedberg · 2018-09-13T13:48:45Z

@nashif understood, but I highly doubt we'd be coming up with this convention (or being blindly strict about it) if it wasn't for MISRA-C

jhedberg

Approving, but if CI doesn't start blocking new PRs which don't follow these conventions we'll soon again have violations of them in the tree. E.g. I'm aware of at least one pending PR that adds a memset (without the cast).

ceolin · 2018-09-13T14:22:24Z

@jhedberg agree, this will need to end up being enforced by our CI. My intention is enforce things using compiler options, Coccinelle scripts (@himanshujha199640 is working to add infra to this), ... It will depend of the guideline. Definitely we need to enforce it.

ceolin · 2018-09-13T15:17:52Z

@jhedberg #9884 contains more information about it, but yeah, I'll add attribute((warn_unused_result)) in functions that must have their return checked.

ceolin · 2018-09-13T17:39:27Z

just one more point about using attribute((warn_unused_result)). When we add this for a function we can't ignore its result doing (void), so we can use this only for the functions that we really should handle their return.

The result of both printk and vprintk are not used in any place. MISRA-C says that the return of every non void function must be checked. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Some syscacll return value through parameters and for these functions the return of _arch_syscall_invoke* are not used. MISRA requires that all return values be checked. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

__swap function was returning -EAGAIN in some case, though its return value was declared as unsigned int. This commit changes this function to return int since it can return a negative value and its return was already been propagate as int. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Ignoring _Swap return where there is no treatment or nothing to do. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

The return of memset is never checked. This patch explicitly ignore the return to avoid MISRA-C violations. The only directory excluded directory was ext/* since it contains only imported code. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

There are some cases that there is nothing to do with _pend_current_thread() return (that is _Swap return value). As MISRA-C requires that all non-void functions have their return value checked, we are explicitly ignoring it when there is nothing to do. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Ignoring the return of _abort_timeout when there is nothing to do. Either because the condition to return something different from 0 was prior checked or because it was called during come cleanup. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

There are some cases where atomic_and/or don't need to be checked. Actively acknowledge these cases. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

_reschedule return's value is not used anywhere, except erroneously by pthread_barrier_wait. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

A lot of times this API is called during some cleanup even if the timeout was not set to make the code simpler. In these cases it's not necessary checking the return. Adding a cast to acknowledge it. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

k_thread_create is used only in k_word_q_start that has no error handling, so the return of that function is not used. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Checking the return of some scattered functions across kernel. MISRA-C requires that all non-void functions have their return value checked, though, in some cases there is nothing to do. Just acknowledging it. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

ceolin · 2018-09-13T18:38:40Z

Added one more commit with leftovers. Now, all these cases on kernel are fixed.

ceolin · 2018-09-14T20:38:04Z

@nashif can we merge it ?

jhedberg requested changes Sep 12, 2018

View reviewed changes

jukkar requested changes Sep 12, 2018

View reviewed changes

ceolin requested a review from cvinayak as a code owner September 12, 2018 21:53

ceolin force-pushed the check_return branch from 282e76c to c832dae Compare September 13, 2018 05:53

jukkar approved these changes Sep 13, 2018

View reviewed changes

jhedberg approved these changes Sep 13, 2018

View reviewed changes

nashif approved these changes Sep 13, 2018

View reviewed changes

Flavio Ceolin added 12 commits September 13, 2018 11:12

misc: printk: Change function return

c81a763

The result of both printk and vprintk are not used in any place. MISRA-C says that the return of every non void function must be checked. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

kernel: syscall: Explicitly ignoring not used return

bfcd047

Some syscacll return value through parameters and for these functions the return of _arch_syscall_invoke* are not used. MISRA requires that all return values be checked. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

kernel: Explicitly ignoring _Swap return

99184f7

Ignoring _Swap return where there is no treatment or nothing to do. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

coccicnelle: Ignore return of memset

6bf7d0a

The return of memset is never checked. This patch explicitly ignore the return to avoid MISRA-C violations. The only directory excluded directory was ext/* since it contains only imported code. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

kernel: atomic: Ignore atomic_and/or return when not used

e616303

There are some cases where atomic_and/or don't need to be checked. Actively acknowledge these cases. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

kernel: Change _reschedule signature

04a802c

_reschedule return's value is not used anywhere, except erroneously by pthread_barrier_wait. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

kernel: Check k_thread_create return value

8ed4493

k_thread_create is used only in k_word_q_start that has no error handling, so the return of that function is not used. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

ceolin force-pushed the check_return branch from c832dae to cde5ff0 Compare September 13, 2018 18:37

nashif merged commit 0a44784 into zephyrproject-rtos:master Sep 14, 2018

ceolin deleted the check_return branch September 20, 2018 04:16

mped-oticon added the area: MISRA-C label Nov 22, 2018

ceolin added the area: Coding Guidelines Coding guidelines and style label Nov 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MISRA-C Checking function return #9930

MISRA-C Checking function return #9930

ceolin commented Sep 12, 2018

codecov-io commented Sep 12, 2018 •

edited

Loading

pfalcon commented Sep 12, 2018

jhedberg left a comment

jukkar left a comment

jhedberg commented Sep 13, 2018 •

edited

Loading

nashif commented Sep 13, 2018

jhedberg commented Sep 13, 2018

jhedberg left a comment

ceolin commented Sep 13, 2018 •

edited

Loading

ceolin commented Sep 13, 2018

ceolin commented Sep 13, 2018

ceolin commented Sep 13, 2018

ceolin commented Sep 14, 2018

MISRA-C Checking function return #9930

MISRA-C Checking function return #9930

Conversation

ceolin commented Sep 12, 2018

codecov-io commented Sep 12, 2018 • edited Loading

Codecov Report

pfalcon commented Sep 12, 2018

jhedberg left a comment

Choose a reason for hiding this comment

jukkar left a comment

Choose a reason for hiding this comment

jhedberg commented Sep 13, 2018 • edited Loading

nashif commented Sep 13, 2018

jhedberg commented Sep 13, 2018

jhedberg left a comment

Choose a reason for hiding this comment

ceolin commented Sep 13, 2018 • edited Loading

ceolin commented Sep 13, 2018

ceolin commented Sep 13, 2018

ceolin commented Sep 13, 2018

ceolin commented Sep 14, 2018

codecov-io commented Sep 12, 2018 •

edited

Loading

jhedberg commented Sep 13, 2018 •

edited

Loading

ceolin commented Sep 13, 2018 •

edited

Loading