Skip to content

ci: coccinelle: false positives hindering development flow #77103

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

Closed
cfriedt opened this issue Aug 14, 2024 · 0 comments · Fixed by #77104
Closed

ci: coccinelle: false positives hindering development flow #77103

cfriedt opened this issue Aug 14, 2024 · 0 comments · Fixed by #77104
Assignees
Labels
area: Continuous Integration Enhancement Changes/Updates/Additions to existing features

Comments

@cfriedt
Copy link
Member

cfriedt commented Aug 14, 2024

Describe the bug
Coccinelle is useful for a number of reasons, one of which is to ensure that library and application code do not unintentionally override reserved names. Reserved names are identifiers that are part of the ISO C (and to some extent POSIX) specifications.

This makes sense for libraries that do not implement ISO C and POSIX functionality, but does not make any sense for libraries that do implement ISO C and POSIX functionality.

In those specific cases, many PRs are met with Coding Guidelines false positive failures in CI, even when the PR is legitimately adding or repairing a standard function. In fact, false positives are even generated when reserved identifiers are not even modified.

This has been an ongoing struggle for at least 4 years, leading to a number of negative consequences (listed below).

Please also mention any information which could help others to understand
the problem you're facing:

  • What target platform are you using? All
  • What have you tried to diagnose or workaround this issue?

Aside from the linked Pull Request, the only current workaround is to ping a project member with merge "superpowers" endlessly, until they tire of being pinged. Often this leads to a number of negative consequences such as:

  • Pull requests growing stale, because no Zephyr project member with "superpower" merge privileges merges the PR (many times, in spite of repeated requests and reminders)
  • Features not making it into scheduled releases (including LTS) in spite of being approved and ready to merge for weeks.
  • Needlessly slowing down developer velocity in spite of sufficient testing.
  • Needlessly wasting reviewers' time due to constantly having to rebase if such a PR has grown stale or developed a merge conflict while waiting for a project member with "superpowers" to merge the PR.
  • Making more work than necessary for project members with "superpowers" since all of said PRs tend to funnel toward them.
  • Deterring contributors (both new and experienced) from seeing their pull requests through review to merge stages of the review process.
  • Frustrating other project members, making project members with "superpowers" irritated due to incessant pings, and creating unnecessary divisions between project members.
  • Is this a regression? No - it's been a problem for at least 4 years.

To Reproduce
See "Logs and console output"

Expected behavior
Being able to review / merge code in a timely manner according to Zephyr's usual development flow. Not being penalized by CI at literally every turn.

Impact
This is often a showstopper, so fairly critical.

Logs and console output

  1. False positive for a PR that does not even touch the rename() function:
lib/posix/options/fs.c:66:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - rename
  1. False positives for a PR that is adding the raise() and signal() function prototypes, as per the ISO C specification.

a)

include/zephyr/posix/signal.h:123:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - raise

b)

include/zephyr/posix/signal.h:134:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - signal
  1. False positives for a PR that is adding the remove() function, as per the ISO C specification

a)

lib/libc/common/source/stdio/remove.c:27:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - remove

b)

lib/libc/minimal/include/stdio.h:63:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - remove
  1. False positives for a PR that is adding the asctime(), asctime_r(), cmtime(), cmtime_r(), localtime(), and localtime_r() functions as per the ISO C and POSIX specifications

a)

lib/libc/common/source/time/asctime.c:32:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - asctime

b)

WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - asctime_r

c) Note: the clock() reserved identifier is not referenced in the PR, it simply exists as a function parameter, matching the specification

lib/libc/common/source/time/ctime.c:14:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - clock

d) Note: the clock() reserved identifier is not referenced in the PR, it simply exists as a function parameter, matching the specification

lib/libc/common/source/time/ctime.c:20:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - clock

e)

WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - ctime_r

Environment (please complete the following information):

  • OS: All
  • Toolchain: All
  • Commit SHA or Version used: All

Additional context

@cfriedt cfriedt added bug The issue is a bug, or the PR is fixing a bug area: Continuous Integration labels Aug 14, 2024
@cfriedt cfriedt added the priority: medium Medium impact/importance bug label Aug 14, 2024
@nashif nashif added Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug labels Aug 15, 2024
@nashif nashif assigned cfriedt and unassigned nashif and stephanosio Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Continuous Integration Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants