Skip to content

Conversation

LauraLMann
Copy link
Collaborator

I know it's just a little thing, but hopefully the warning will save someone else the pain we just went through of trying to figure out what is going on.
The issue is that when the coverage plugin is loaded (it doesn't even have to be enabled), it inserts calls into the TCG instruction stream. This not only impacts the TCG optimizations, but also these calls get instrumented by the taint system. So, you get different taint than if you ran the same scenario without the coverage plugin loaded.
Although it is possible to make the taint system recognize the functions inserted by the coverage plugin and not instrument them, this only reduces the number of taint differences - it doesn't entirely eradicate them. One would also need to build PANDA without TCG optimizations in order to get rid of all the taint differences. Neither adjustment is sufficient on its own to make all the taint differences go away.
It doesn't seem like a good idea to permanently disable TCG optimizations (things are slow enough with them, and the number of taint reports increases with TCG optimizations off), and with recordings you shouldn't need to run coverage with taint at the same time (you can run them separately if you really want output from both plugins), so it seemed best just to warn the user "don't do that".

@LauraLMann
Copy link
Collaborator Author

As all this change does is modify a markdown file, the build errors must be totally unrelated to the changes made.

@AndrewQuijano
Copy link
Collaborator

AndrewQuijano commented Oct 9, 2025

@LauraLMann, yes, at the moment, PANDA has no way to securely test external pull requests. That is on my bucket list of things I want to try testing in the near future!

@lacraig2
Copy link
Member

I think we can test external PRs as of today. Though we haven't verified this yet.

"Securely" is always tough to guarantee, but with the current set of rules we can check that things compile at least and an org member has to approve runs.

You'd have to rebase on dev to try it out.

@LauraLMann LauraLMann force-pushed the a896_coverage_impacts_taint branch from 8530d16 to 34682df Compare October 13, 2025 18:07
@LauraLMann
Copy link
Collaborator Author

@lacraig2 This one is ready for review. All it does is change a documentation file, so I think the CI failures are unrelated to my change. (One is lint complaints about Python code - didn't touch that. The other is authorization failure when tries to push something to harbor - sounds like a CI configuration problem.)

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.

3 participants