Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Coverage: remove function signature lines that are never report as hit #2156

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

neewy
Copy link
Contributor

@neewy neewy commented Mar 11, 2019

Description of the Change

Some time ago, I wanted to get the latest data about project coverage and was confused with the results. As stated in https://github.com/JeremyAgost/lcov-llvm-function-mishit-filter repo: "The lcov test coverage utility is designed to work with gcc and has some issued with running on binaries generated by the llvm compiler. One of these issues is that function signatures are output as lines that should be hit. However, while the binary is running those function signature lines are never registered as hit. Here's a simple python script to filter those lines from the lcov output."

What I did is that I added a script that would remove partial hits from function signatures. Python script won't get updated a lot, I believe; however, we should monitor lcov issue for some time here: linux-test-project/lcov#30

Benefits

Honest coverage results

Possible Drawbacks

Possibly manual maintenance of the script is required.

Usage Examples or Tests [optional]

remove-function-lines.py lcov_output.info cleaned_output.info

@neewy neewy changed the base branch from master to develop March 11, 2019 13:03
@neewy neewy requested review from BulatSaif and MBoldyrev March 11, 2019 13:03
lineno = line[3:].split(',')[0]
if lineno in defs:
if verbose:
printf('Ignoring: {srcfile}:{lineno}:{defs[lineno]}')
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work? I see such syntax for the first time and did not find any docs on such printf function.

Copy link
Contributor

Choose a reason for hiding this comment

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

this did not work on my setup. I suggest removing verbose stuff: srcfile, demangle, printf, cli arg, etc. because we do not use it and it is bogous.

for line in lines:
if line.startswith('SF:'):
defs = {}
srcfile = line[3:].strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

this line can be placed behind if verbose

@neewy
Copy link
Contributor Author

neewy commented Mar 12, 2019

@MBoldyrev TBH I did not write the code myself — I reused the script from https://github.com/JeremyAgost/lcov-llvm-function-mishit-filter repo. We can test it with your suggestions, but it seem to work now with python3.5

@MBoldyrev
Copy link
Contributor

@MBoldyrev TBH I did not write the code myself — I reused the script from https://github.com/JeremyAgost/lcov-llvm-function-mishit-filter repo. We can test it with your suggestions, but it seem to work now with python3.5

I see, but it as fa as I can see, the code is bogous, and one of the purposes of the PR review concept is preventing bad things from entering our repo. I see that the script contains some bad stuff, and I'd like to remove it prior to merging this. The script seems simple enough to rewrite it ourselves in a more robust way.

import subprocess

def demangle(symbol):
p = subprocess.Popen(['c++filt','-n'], stdin=subprocess.PIPE, stdout=subprocess.PIPE)
Copy link
Contributor

@BulatSaif BulatSaif Mar 13, 2019

Choose a reason for hiding this comment

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

Coverage almost always runs at gcc5, but it possible to run coverage at any other compiler
with Custom command:
ex. parameters to run build:

custom_cmd: x64linux_compiler_list = ['clang7']; coverage = true

or on mac:

custom_cmd: x64linux_compiler_list = []; mac_compiler_list = ['appleclang']; coverage = true

This python code looks simple and hope it won't fail on mac, also until now nobody used coverage at mac

Copy link
Contributor

@BulatSaif BulatSaif left a comment

Choose a reason for hiding this comment

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

We can improve script as @MBoldyrev suggested, but as I can see it already works

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants