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.

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

coverage: count number of executions per line #1265

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

elopez
Copy link
Member

@elopez elopez commented May 28, 2024

No description provided.

Copy link
Member

@arcz arcz left a comment

Choose a reason for hiding this comment

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

This looks good, I would just benchmark if this doesn't add too much overhead

@ggrieco-tob
Copy link
Member

I think either the workers will be frequently sending events (quite bad) or some counter will not be properly updated (not so bad, still not great).

@ggrieco-tob
Copy link
Member

@elopez can you also include the changes from here? #1110

@rappie
Copy link

rappie commented May 30, 2024

This is awesome, I want this feature so bad 😆

@arcz arcz added this to the Echidna 2.2.4 milestone Jun 21, 2024
@elopez
Copy link
Member Author

elopez commented Jul 8, 2024

I did a second attempt at implementing this a safer way. The code is quite ugly right now but if anyone wants to test/review it before I spend some more time on it I'd appreciate it 👍

@rappie
Copy link

rappie commented Jul 8, 2024

I don't see this branch under Actions to download the binary 🤔

@ggrieco-tob
Copy link
Member

I think it is because it is a draft, let me convert it a proper PR

@ggrieco-tob ggrieco-tob marked this pull request as ready for review July 8, 2024 15:56
@ggrieco-tob ggrieco-tob self-requested a review as a code owner July 8, 2024 15:56
@elopez elopez marked this pull request as draft July 8, 2024 18:05
@elopez
Copy link
Member Author

elopez commented Jul 8, 2024

@rappie There was a small git conflict, hopefully it builds now that I resolved it 👍

@ggrieco-tob
Copy link
Member

@samalws-tob can you take a look to this PR?

lib/Echidna/Exec.hs Outdated Show resolved Hide resolved
lib/Echidna/Types/Coverage.hs Show resolved Hide resolved
@@ -304,13 +314,13 @@ execTxWithCov tx = do
-- bug in another place, investigate.
-- ... this should be fixed now, since we use `codeContract` instead
-- of `contract` for everything; it may be safe to remove this check.
when (pc < VMut.length vec) $
when (pc < VMut.length vec) $ do
VMut.modify (fromJust maybeStatsVec) (\(execQty, revertQty) -> (execQty + 1, revertQty)) opIx
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see anywhere where we increment revertQty

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I have not implemented counting of reverts. I meant to do it at first but then it wasn't clear to me where I should be counting them. If you have any pointers there I'd appreciate them, otherwise I may remove the extra var at this time.

lib/Echidna/Output/Source.hs Outdated Show resolved Hide resolved
lib/Echidna/Output/Source.hs Outdated Show resolved Hide resolved
lib/Echidna/Output/Source.hs Outdated Show resolved Hide resolved
Revert counting is still not implemented.
@samalws-tob
Copy link
Collaborator

samalws-tob commented Sep 3, 2024

I benchmarked this, looks like it cuts performance (calls/s) in half

@samalws-tob
Copy link
Collaborator

in #1305 tried doing type StatsInfo = ExecQty instead of type StatsInfo = (ExecQty, RevertQty) because I thought the issue might've had to do with tuples being non-strict, but it's still far slower than master

@rappie
Copy link

rappie commented Sep 10, 2024

I'm still very interested in this, despite the performance loss. It would be great to have this in master as a togglable feature 🙂

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

Successfully merging this pull request may close these issues.

5 participants