C++: Fix IR generation for switch statements

[MathiasVP]

IR generation for switch statements with only one case (or multiple cases with fallthrough) such as:

void switch1Case(int x) {
    int y = 0;
    int z;
    switch(x) {
        case 1:
        y = 2;
    }
    z = y;
}

was incorrectly being generated to something with the semantics of:

void switch1Case(int x) {
    int y = 0;
    int z;
    switch(x) {
        case 1:
        y = 2;
        z = y;
    }
}

This caused a false positive in cpp/pointer-overflow-check on php/php-src since the IR-based GVN would assign the same value number to a and b in the < expression in a function like:

void foo(int n, char* a, char* b) {
    switch (n)
    {
    case 0:
        a = b;
    }
    int c = b + 1 < a;
}

With this PR the IR generated by a switch statement will contain a Goto edge if there's no default case in the switch statement. Another choice would also be to generate the new Goto edge as a Default edge, meaning that all switch statements in the IR would have a Default edge.

[jbj]

Should it now be the case that all SwitchInstructions have a DefaultEdge out of them? If so, you can add a sanity check for it at the top of Instruction.qll.

[MathiasVP]

Should it now be the case that all SwitchInstructions have a DefaultEdge out of them? If so, you can add a sanity check for it at the top of Instruction.qll.

If we change it so that the IR generates a DefaultEdge, then yes. And I think like that idea. Unless it should be visible from the IR whether the edge was generated by the IR construction or was already present in the source file.

[rdmarsh2]

It should definitely be a DefaultEdge by current IR semantics. If we really want to distinguish edges to a default: label from edges to the end of a switch, that would need to be a separate edge kind, since GotoEdge is unconditional. I'd prefer having a noop for the default label rather than a special edge kind, though.

[jbj]

If we really want to distinguish edges to a default: label from edges to the end of a switch, that would need to be a separate edge kind

I don't see a need to distinguish those. The AST is available for queries that are interested in surface syntax.

[MathiasVP]

I've changed the IR generation so that a DefaultEdge is generated, and I've added a sanity check in Instruction.qll that verifies that each SwitchInstruction now has a DefaultEdge. However, as the files are synchronized with C#, I'm worried that this will not be the case for C#.

Specifically, looking at https://github.com/Semmle/ql/blob/master/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedStmt.qll#L830 it seems like a similar bug as this one exists in that translation. Can you confirm this @Semmle/cs?

[jbj]

The C# IR is not really owned by the C# team. It's effectively owned by @dbartol. So feel free to either apply the same fix to C# or leave the sanity test there as documentation that this needs fixing.

[MathiasVP]

The C# IR is not really owned by the C# team. It's effectively owned by @dbartol. So feel free to either apply the same fix to C# or leave the sanity test there as documentation that this needs fixing.

Ah, I see. I'm pretty sure it needs the same fix (the original code is identical to the C++ one), so I'll apply the same fix to the C# translation and let @dbartol verify that it's the correct thing to do.

[jbj]

The test results LGTM now, but there's one issue with the implementation.