Revert "C++: Do not generate IR for functions with multiple entry points"

[calumgrant]

Reverts #17694

I'm reverting this due to https://github.com/github/codeql-c-team/issues/2482. The problem is that it seems that TRAP caching does pull in a few duplicate functions, which is arguably a bug but will take a bit more time to investigate. In the meantime, since #17694 can cause results wobble, I think it would be safest to revert.

@jketema kindly do double-check that this PR is indeed the root cause of these new alert deltas.

[jketema]

Is this a stable regression, or is it actually wobbling from night to night?

[calumgrant]

Is this a stable regression, or is it actually wobbling from night to night?

It's now a "stable regression" - in that the alerts consistently change between cache-read and cache write.

[jketema]

Then the proper solution is to silence the errors in DCA, not reverting changes in the CodeQL repo.

[geoffw0]

Feel free to DM me if you'd like help making changes to the DCA rules for CPP. I've done it quite a few times recently.

[calumgrant]

Feel free to DM me if you'd like help making changes to the DCA rules for CPP. I've done it quite a few times recently.

I don't think these differences should be silenced. What it shows is that we get fewer alerts if the TRAP cache is used, which is definitely not what we want! My main point is that it will take a while to dig into why this is happening and it may not be easy to fix. The "wobble" is that we'll get different alerts on main vs PR branches, which could cause a few problems. It's a "stable wobble" if that makes sense.

My current guess FWIW is that it's something to do with the interaction of linker awareness and TRAP caching.

[jketema]

I don't think these differences should be silenced. What it shows is that we get fewer alerts if the TRAP cache is used, which is definitely not what we want! My main point is that it will take a while to dig into why this is happening and it may not be easy to fix. The "wobble" is that we'll get different alerts on main vs PR branches, which could cause a few problems. It's a "stable wobble" if that makes sense.

This sound like a much deeper problem with TRAP caching than anything related to the PR you're now trying to revert, and it that case both silencing and reverting are both the incorrect solution.

[calumgrant]

This sound like a much deeper problem with TRAP caching than anything related to the PR you're now trying to revert, and it that case both silencing and reverting are both the incorrect solution.

This isn't a permanent revert, it's a temporary revert whilst we investigate the root cause.

[jketema]

This isn't a permanent revert, it's a temporary revert whilst we investigate the root cause.

What do we gain by reverting this?

[calumgrant]

This isn't a permanent revert, it's a temporary revert whilst we investigate the root cause.

What do we gain by reverting this?

We'll regain the lost alerts and main won't be broken.

[jketema]

We'll regain the lost alerts and main won't be broken.

Why will main no longer be broken? From the information you have provided it very much seems that this was a preexisting problem, that was now accidentally exposed because we changed something in a QL library?

[jketema]

I had a chat with Anders. What I hadn't realized is that this might give flip-flopping results between PRs and main when trap caching is used. So from that perspective it indeed seems best for revert. One comment though.

[jketema]

Could we also run some DCA on this, to make sure that does not have any odd interactions with changes that were made after this change originally went in? Ideally both a normal experiment and a trap caching one.

[jketema]

@calumgrant I'm missing a trap-caching DCA experiment that shows that this addresses the problem you observed.