Skip to content

Conversation

MathiasVP
Copy link
Contributor

On some projects (notably git/git) it can happen that a PhiInstruction has inexact operands (See https://github.com/github/codeql-c-analysis-team/issues/30):

#  353|     m353_17(unknown)     = Chi               : total:m353_14, partial:m353_16
#-----|   Goto -> Block 2
...
#  352|     m352_15(unknown)     = Chi                : total:m352_13, partial:m352_14
#  352|     v352_16(void)        = ConditionalBranch  : r352_7
#-----|   False -> Block 2
#-----|   True -> Block 1
...
#  355|   Block 2
#  355|     m355_1(unknown)      = Phi        : from 1:~m353_17, from 55:~m352_15

This causes isChiForAllAliasedMemory to fail because it expects the Chi chain to have exact operands for PhiInstructions.

The real fix is most likely to avoid this situation altogether, but this easy workaround PR modifies the definition of isChiForAllAliasedMemory to allow recursion through inexact PhiInstruction operands. I haven't observed any need to modify the case for ChiInstructions, so I'll leave that one to only recurse through exact operands.

@MathiasVP MathiasVP added the C++ label Feb 26, 2020
@MathiasVP MathiasVP requested a review from a team as a code owner February 26, 2020 09:24
@MathiasVP
Copy link
Contributor Author

MathiasVP commented Feb 26, 2020

I don't expect crazy performance changes, but I've started a CPP-difference run to be sure: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/863/

Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

LGTM

@MathiasVP
Copy link
Contributor Author

I'm not exactly sure why linux is performing that much worse.
Everything else seems to be about the same, if not a slight increase in analysis time. I'm guessing this is caused chasing down longer chains of Chi and Phi instructions.

Screenshot 2020-02-27 at 09 36 38

@jbj
Copy link
Contributor

jbj commented Feb 27, 2020

The Linux performance regression looks bad enough that it needs to be investigated. I'll be surprised if it's caused by the number of tuples in the modified predicate -- it's practically impossible for a unary linear predicate to be slow unless there's a bad join order inside it.

@MathiasVP
Copy link
Contributor Author

The Linux performance regression looks bad enough that it needs to be investigated. I'll be surprised if it's caused by the number of tuples in the modified predicate -- it's practically impossible for a unary linear predicate to be slow unless there's a bad join order inside it.

I'll take a look at it later today then.

@MathiasVP MathiasVP added the WIP This is a work-in-progress, do not merge yet! label Feb 27, 2020
@MathiasVP MathiasVP removed the WIP This is a work-in-progress, do not merge yet! label Mar 3, 2020
@MathiasVP
Copy link
Contributor Author

Removing the WIP label as the performance regressions won't be investigated more as part of this PR.

@jbj
Copy link
Contributor

jbj commented Mar 3, 2020

We've failed to isolate the performance bug. It doesn't seem to be connected to this PR in any meaningful way even though it's somehow triggered by it. See https://github.slack.com/archives/CP0LHP150/p1583231417117500.

@jbj jbj merged commit 88c74b2 into github:master Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants