fix a bug that leads to get many incorrect switch target case value -1

[canliture]

When I use SVF to do some research on static (sparse) symbolic execution, e.g., translate SVFG/VFGNode to Z3 Formula. I think the the following code will lead to some incorrect formulas.

Given the code:

#include <stdio.h>


void test(int a) {
    switch (a)
    {
        case 0: printf("0\n"); break;
        case 1:
        case 2:
        case 3: printf("a >=1 && a <= 3\n"); break;
        case 4:
        case 6:
        case 7:  printf("a >= 4 && a <=7\n"); break;
        default: printf("a < 0 || a > 7"); break;
    }
}

SVF will get many incorrect target case value -1 due to use of API SwitchInst::findCaseDest. Here is comments about SwitchInst::findCaseDest:

/// Finds the unique case value for a given successor. Returns null if the
/// successor is not found, not unique, or is the default case.
ConstantInt *findCaseDest(BasicBlock *BB) {
  // ...
}

Find the unique case value .... .Return null if ... , not unique ...

[codecov]

Codecov Report

Merging #1191 (f691c36) into master (324794b) will increase coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1191      +/-   ##
==========================================
+ Coverage   64.64%   64.65%   +0.01%     
==========================================
  Files         222      222              
  Lines       23525    23536      +11     
==========================================
+ Hits        15207    15218      +11     
  Misses       8318     8318              

Files Changed	Coverage Δ
svf-llvm/include/SVF-LLVM/LLVMUtil.h	76.25% <ø> (ø)
svf-llvm/lib/LLVMUtil.cpp	73.31% <100.00%> (+0.63%)	⬆️
svf-llvm/lib/SVFIRBuilder.cpp	78.15% <100.00%> (ø)

📢 Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

[yuleisui]

what does this mean here, why i * 2? Can we use findCaseDest + something else?

[canliture]

In my mind, 'condVal' is target case value. And in LLVM IR:

Operand 0 is condition of switch,
Operand 1 is default BasicBlock Value , and

Operand 2 is target Value of the first Case
Operand 3 is target succussor BasicBlock Value of the first Case's
...
Operand 2n is target Value of the n-th Case
Operand 2n+1 is target succussor BasicBlock Value of the n-th Case's

---

Before we use findCaseDest, source code of findCaseDest is below:

  /// Finds the unique case value for a given successor. Returns null if the
  /// successor is not found, not unique, or is the default case.
  ConstantInt *findCaseDest(BasicBlock *BB) {
    if (BB == getDefaultDest())
      return nullptr;

    ConstantInt *CI = nullptr;
    for (auto Case : cases()) {
      if (Case.getCaseSuccessor() != BB)
        continue;

      if (CI)
        return nullptr; // Multiple cases lead to BB.

      CI = Case.getCaseValue();
    }

    return CI;
  }

we will get -1 findCaseDest of default case, and also get -1 when encounter continuous case without break stmt, the LLVM IR related the given code is

// ... 
  switch i32 %0, label %sw.default [
    i32 0, label %sw.bb                                ; get -1 due to default branch
    i32 1, label %sw.bb1                               ; get -1 without break
    i32 2, label %sw.bb1                              ; get -1  without break
    i32 3, label %sw.bb1                             
    i32 4, label %sw.bb3                             ; get -1 without break
    i32 6, label %sw.bb3                             ; get -1 without break
    i32 7, label %sw.bb3
  ], !dbg !18
// ... 

if we use findCaseDest + something else, let me thought a few moments. I can't give the answer at once...

[canliture]

I give another answer:

void SVFIRBuilder::visitSwitchInst(SwitchInst &inst)
{
    NodeID brinst = getValueNode(&inst);
    NodeID cond = getValueNode(inst.getCondition());

    BranchStmt::SuccAndCondPairVec successors;

    std::vector<std::pair<ConstantInt*, BasicBlock*>> caseValue2SuccBBVec;
    // default case
    caseValue2SuccBBVec.push_back({nullptr, inst.getDefaultDest()});
    // normal case
    for (const SwitchInst::CaseHandle& cs : inst.cases())
    {
        caseValue2SuccBBVec.push_back({cs.getCaseValue(), cs.getCaseSuccessor()});
    }
    for (const auto& caseValue2SuccBB : caseValue2SuccBBVec)
    {
        ConstantInt *caseValue = caseValue2SuccBB.first;
        BasicBlock *succBB = caseValue2SuccBB.second;
        /// default case is set to -1;
        s64_t val = -1;
        if (caseValue != nullptr && succBB != inst.getDefaultDest())
        {
            if (caseValue->getBitWidth() <= 64)
                val = caseValue->getSExtValue();
        }
        const Instruction* succInst = &succBB->front();
        const SVFInstruction* svfSuccInst = LLVMModuleSet::getLLVMModuleSet()->getSVFInstruction(succInst);
        const ICFGNode* icfgNode = pag->getICFG()->getICFGNode(svfSuccInst);
        SVFUtil::outs() << val << ": " << succBB->getName().str() << std::endl;
        successors.push_back(std::make_pair(icfgNode,val));
    }
    addBranchStmt(brinst, cond,successors);
}

output is:

-1: sw.default
0: sw.bb
1: sw.bb1
2: sw.bb1
3: sw.bb1
4: sw.bb3
6: sw.bb3
7: sw.bb3

[yuleisui]

This look good but code is quite a lot. Could we try to simplify it and any helper functions or make the naming more explicit and easy understandable

[canliture]

This look good but code is quite a lot. Could we try to simplify it and any helper functions or make the naming more explicit and easy understandable

Yeah, I think I have make the code more easily understandable by f691c36

[yuleisui]

Thanks and will merge.

[yuleisui]

Are there any test cases you could provide here to showcase the difference between current buggy implementation and your fix.

[canliture]

Are there any test cases you could provide here to showcase the difference between current buggy implementation and your fix.

given the testcase:

#include <stdio.h>

void test(int a) {
    switch (a)
    {
        case 0: printf("0\n"); break;
        case 1:
        case 2:
        case 3: printf("a >=1 && a <= 3\n"); break;
        case 4:
        case 6:
        case 7:  printf("a >= 4 && a <=7\n"); break;
        default: printf("a < 0 || a > 7"); break;
    }
}

generate IR part related switch:

...
  switch i32 %0, label %sw.default [
    i32 0, label %sw.bb
    i32 1, label %sw.bb1
    i32 2, label %sw.bb1
    i32 3, label %sw.bb1
    i32 4, label %sw.bb3
    i32 6, label %sw.bb3
    i32 7, label %sw.bb3
  ], !dbg !18
...

insert a line code into the original codes here:

SVF/svf-llvm/lib/SVFIRBuilder.cpp

Lines 958 to 959 in 0d9f7cf

	const ICFGNode* icfgNode = pag->getICFG()->getICFGNode(svfSuccInst);
	successors.push_back(std::make_pair(icfgNode,val));

inserted code like this:

const ICFGNode* icfgNode = pag->getICFG()->getICFGNode(svfSuccInst);
SVFUtil::outs() << "case value " << val << ": " << inst.getSuccessor(i)->getName().str() << std::endl;
successors.push_back(std::make_pair(icfgNode,val));

run wpa or other tools that will call into SVFIRBuilder::visitSwitchInst to build SVFIR, output like this:

case value -1: sw.default
case value 0: sw.bb
case value -1: sw.bb1
case value -1: sw.bb1
case value -1: sw.bb1
case value -1: sw.bb3
case value -1: sw.bb3
case value -1: sw.bb3

Apparently, the output is not correct. After applying the patch, and insert code at the same place:

const ICFGNode* icfgNode = pag->getICFG()->getICFGNode(svfSuccInst);
SVFUtil::outs() << "case value " << val << ": " << succBB->getName().str() << std::endl;
successors.push_back(std::make_pair(icfgNode, val));

run wpa or other tools that will call into SVFIRBuilder::visitSwitchInst to build SVFIR, new output like this:

case value -1: sw.default
case value 0: sw.bb
case value 1: sw.bb1
case value 2: sw.bb1
case value 3: sw.bb1
case value 4: sw.bb3
case value 6: sw.bb3
case value 7: sw.bb3

Apparently, the output is correct.