Skip to content

Conversation

silas-martens
Copy link
Contributor

CGMerge can now handle call graphs where the meta field is null. This is necessary to merge the patch graphs generated by cgpatch.

Copy link
Member

@jplehr jplehr left a comment

Choose a reason for hiding this comment

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

Please add a test for this case

@silas-martens
Copy link
Contributor Author

Please add a test for this case

From what I understand, the existing tests in cgcollector/test/ generate static call-graphs using cgcollector, which are then merged. However, since cgpatch is not yet available in the repo, and the fix relates to patch-graphs generated by it, I’m unsure where a test involving patch-graphs would fit best.

My idea is to add a separate folder inside cgcollector/test/input to hold test cases with pre-generated call-graphs that do not rely on cgcollector itself. Then, these testcases could be run as a part of run_format_two_test.sh.

@sebastiankreutzer
Copy link
Member

As @silas-martens said, there are currently no explicit test cases for CGMerge. Adding a new test suite for this simple change seems a bit overkill.
Instead, I suggest that we abandon this fix and explicitly require CGMerge2 with CGPatch.

@jplehr
Copy link
Member

jplehr commented May 26, 2025

I'm OK with requiring the new tool for the functionality that cgpatch does. It simply sounded as if this is a general bug, and so this would be a general fix.

So, my question is: What's stopping us from adding a test case that uses cgmerge and have one of the input files trigger the previous error? My understanding was that this currently crashes the tool, which I think should not happen.

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