Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Work around a bug in ancient lcov #8926

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Work around a bug in ancient lcov #8926

merged 1 commit into from
Mar 13, 2024

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Mar 13, 2024 •
edited
Loading

  1. We added multithreaded tests. They cause 
    lcov: WARNING: negative counts found in tracefile Coverage/tmp/tests.info
    This is known to be caused by a bug in gcov, possibly due to a race condition. We may be getting slightly bad data about a few test cases, but we can live with that.
  2. Lcov had a bug in a warning handler that caused it to change to the root directory before writing the output file. 
    lcov: ERROR: cannot write to Coverage/tmp/all.info!
    So when the warning triggers, we don't get output at all. We can't live with that.

As we are nearing a release, I am proposing a minimalistic workaround that just prevents the warning from triggering the error. The proper fix is to move away from ancient tooling but I do not propose to do that the week before a release.

PR checklist

  • changelog no
  • backport not needed (no multithreaded tests causing the lcov warning in 2.28)
  • tests Run the code-coverage job until there's a WARNING but no ERROR (the failure is non-deterministic so this may require a few tries). The normal pull request testing does not run the code-coverage job. https://mbedtls.trustedfirmware.org/job/mbedtls-release-ci-testing/152

@gilles-peskine-arm
Work around a bug in ancient lcov
539d7d5 
lcov had a bug whereby it tries to create the output file relative to /
if it has emitted a warning. We do CI runs on Ubuntu 16.04 which is too
old to have the fix. As a quick fix for the CI, work around the bug.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review component-test Test framework and CI scripts priority-very-high Highest priority - prioritise this over other review work size-xs Estimated task size: extra small (a few hours at most) labels Mar 13, 2024
paul-elliott-arm
paul-elliott-arm approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@Ryan-Everett-arm Ryan-Everett-arm self-requested a review March 13, 2024 16:50
Ryan-Everett-arm
Ryan-Everett-arm approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@Ryan-Everett-arm Ryan-Everett-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@Ryan-Everett-arm Ryan-Everett-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Mar 13, 2024
@gilles-peskine-arm
Copy link
Contributor Author

I've tested locally, but let's wait for CI confirmation to merge. Currently running https://mbedtls.trustedfirmware.org/job/mbedtls-release-ci-testing/152/

This was referenced Mar 13, 2024
Open
Open
@gilles-peskine-arm
Copy link
Contributor Author

From https://mbedtls.trustedfirmware.org/job/mbedtls-release-ci-testing/152/console :

 lcov: WARNING: negative counts found in tracefile /var/lib/build/Coverage/tmp/tests.info

but the script continued and succeeded. So we probably have slightly bad data (I think what happens is that the functions called while there are multiple threads might be miscounted), but at least we don't have a CI failure that could hide other problems.

@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Mar 13, 2024
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Mar 13, 2024
Merged via the queue into Mbed-TLS:development with commit 5c77ad0 Mar 13, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ryan-Everett-arm Ryan-Everett-arm Ryan-Everett-arm approved these changes

@paul-elliott-arm paul-elliott-arm paul-elliott-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-test Test framework and CI scripts priority-very-high Highest priority - prioritise this over other review work size-xs Estimated task size: extra small (a few hours at most)
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gilles-peskine-arm @paul-elliott-arm @Ryan-Everett-arm