fix a bug: 'getExitBB' of SVFFunction may get incorrect exit block. #1262

canliture · 2023-11-29T01:13:08Z

reproduce the bug:
given the code in file bb.c:

int print();

void test()
{
    for (int i = 0 ; i < 100 ; i++)
    {
        print();
    }
}

compile and optimize the code with the following commands:

clang -S -c -Xclang -disable-O0-optnone -fno-discard-value-names -emit-llvm -o bb.ll bb.c
opt -loop-unroll -unroll-count=3 -S -o bb.ll.ll bb.ll

we get the final optimized bitcode file bb.ll.ll and get the code of test function:

; Function Attrs: noinline nounwind ssp uwtable
define void @test() #0 {
entry:
  %i = alloca i32, align 4
  store i32 0, i32* %i, align 4
  br label %for.cond

for.cond:                                         ; preds = %for.inc.2, %entry
  %0 = load i32, i32* %i, align 4
  %cmp = icmp slt i32 %0, 100
  br i1 %cmp, label %for.body, label %for.end

for.body:                                         ; preds = %for.cond
  %call = call i32 (...) @print()
  br label %for.inc

for.inc:                                          ; preds = %for.body
  %1 = load i32, i32* %i, align 4
  %inc = add nsw i32 %1, 1
  store i32 %inc, i32* %i, align 4
  %2 = load i32, i32* %i, align 4
  %cmp.1 = icmp slt i32 %2, 100
  br i1 %cmp.1, label %for.body.1, label %for.end

for.end:                                          ; preds = %for.inc.1, %for.inc, %for.cond
  ret void

for.body.1:                                       ; preds = %for.inc
  %call.1 = call i32 (...) @print()
  br label %for.inc.1

for.inc.1:                                        ; preds = %for.body.1
  %3 = load i32, i32* %i, align 4
  %inc.1 = add nsw i32 %3, 1
  store i32 %inc.1, i32* %i, align 4
  %4 = load i32, i32* %i, align 4
  %cmp.2 = icmp slt i32 %4, 100
  br i1 %cmp.2, label %for.body.2, label %for.end

for.body.2:                                       ; preds = %for.inc.1
  %call.2 = call i32 (...) @print()
  br label %for.inc.2

for.inc.2:                                        ; preds = %for.body.2
  %5 = load i32, i32* %i, align 4
  %inc.2 = add nsw i32 %5, 1
  store i32 %inc.2, i32* %i, align 4
  br label %for.cond, !llvm.loop !6
}

so, the basic block vector is:

[
  entry,
  for.cond,
  for.body,
  for.inc,
  for.end,
  for.body.1,
  for.inc.1,
  for.body.2,
  for.inc.2,
]

in the code above, we know: for.end basic block is the exit basic block of the function test, so I think we can't get exit basic block by simply retrieving the last element of the basic block vector

by pulling request, I unify to set entry and exit basic block according to the definition of the entry/exit basic block of a function: no preds/succs.

canliture · 2023-11-29T04:58:29Z

maybe multi exit blocks leads to assert crashes

yuleisui · 2023-11-29T05:16:26Z

maybe multi exit blocks leads to assert crashes

Could you help fix this?

canliture · 2023-11-29T05:18:53Z

maybe multi exit blocks leads to assert crashes

Could you help fix this?

Yes, I currently work on this.

…and connect all exit basic blocks to it.

jumormt · 2023-11-29T11:35:10Z

Hi @canliture , which LLVM version are you using to compile this case?

jumormt · 2023-11-29T11:38:58Z

svf/include/SVFIR/SVFValue.h

@@ -320,6 +320,15 @@ class SVFFunction : public SVFValue
    std::vector<const SVFBasicBlock*> allBBs;   /// all BasicBlocks of this function
    std::vector<const SVFArgument*> allArgs;    /// all formal arguments of this function
    std::vector<std::string> annotations; /// annotations of this function
+    SVFBasicBlock *entryBlock;                  /// entry BasicBlock of this function


I think we don't need an entryBlock because it is always the first block in allBBs. This is consistent with LLVM's API called getEntryBlock(), which simply returns the front of its basic blocks.

Thank you for your review. Yeah, here is redundant.

jumormt · 2023-11-29T11:42:49Z

svf-llvm/lib/LLVMModule.cpp

+        }
+        if (svfbb->getSuccessors().empty())
+        {
+            svfFun->setExitBlock(svfbb);


We should consider the unreachable blocks introduced by instructions such as exit(0). These blocks are not function exit blocks and cannot be connected to the unique exit block. A valid function exit should end with a return instruction.

To my knowledge, each llvm function can only have one return instruction. This means there should be only one function exit block? In that way, no extra unique function exit block is needed.

Yeah, we do not need an extra unique exit block theoretically.

I would like to improve robustness of SVF: when someone generate a valid bitcode file like above, we should handle it correctly.

And I have a question, why my first pr can't pass the testcases. I guess the following assert leads to fail to pass the testcase (maybe multi exit basic block)

inline void setExitBlock(SVFBasicBlock *bb) { assert(!exitBlock && "function already has a exit Basicblock."); exitBlock = bb; }

Because I have no testcases, so I can only guess the reason...

canliture · 2023-11-30T06:52:08Z

Hi @canliture , which LLVM version are you using to compile this case?

I use opt command of llvm 13

opt -loop-unroll -unroll-count=3 -S -o bb.ll.ll bb.ll

…ncisely

canliture · 2023-11-30T08:45:20Z

I find another testcase here:

https://github.com/SVF-tools/Test-Suite/blob/ac3865cafc6f5ab3af86e5e923aa4f1462704583/test_cases_bc/basic_cpp_tests/vector-4.cpp.bc#L339

jumormt · 2023-11-30T08:57:23Z

I find another testcase here:

https://github.com/SVF-tools/Test-Suite/blob/ac3865cafc6f5ab3af86e5e923aa4f1462704583/test_cases_bc/basic_cpp_tests/vector-4.cpp.bc#L339

Thank you for your time. That's a good catch! It is not rigorous to return the last block as the exit block.

It seems that some cases fail the CI because of no return instructions residing in the function (e.g., https://github.com/SVF-tools/Test-Suite/blob/ac3865cafc6f5ab3af86e5e923aa4f1462704583/test_cases_bc/basic_cpp_tests/deque-1.cpp.bc#L981).

jumormt · 2023-11-30T09:33:04Z

Confirming that all 27 failed cases are caused by the Assertion `exitBlock && "have not yet set exit Basicblock?"'. This happens when analyzing the following functions:

define linkonce_odr hidden void @__clang_call_terminate(i8* %0) #6 comdat {
%2 = call i8* @__cxa_begin_catch(i8* %0) #11
call void @_ZSt9terminatev() #12
unreachable
}

I think there should be no exit BBs for this function. Also, there should be no return edge from the callee back to the caller. Therefore, I suppose that the assertion in getExitBB() for checking null for exitBB can be removed.

yuleisui · 2023-11-30T11:08:43Z

Need to revise the assert to be:

assert((function.hasReturn() && exitBlock) && "have not yet set exit Basicblock?");

…e function must have return instruction

yuleisui · 2023-11-30T11:46:08Z

svf-llvm/lib/LLVMModule.cpp

@@ -319,6 +320,17 @@ void LLVMModuleSet::initSVFBasicBlock(const Function* func)
            const SVFBasicBlock* svf_pred_bb = getSVFBasicBlock(*pred_it);
            svfbb->addPredBasicBlock(svf_pred_bb);
        }
+
+        // set exit block
+        if (svfbb->getSuccessors().empty() && !bb->getInstList().empty())


Can you sure this will be the exit bb? Better to add assertions in your code to make it more robust.

Thanks for review.

Yeah, an exit basic block is a 'single' basic block in a function:

having no successors

containing return instruction in a function, and

ret instruction is not neccessarily the last instruction in the basic block

Looks to me the crests are failed. Please help fix them.

Looks to me the crests are failed. Please help fix them.

Yes, I have fixed all the test failures.

…nction has ret instruction

codecov · 2023-12-01T02:30:30Z

Codecov Report

Merging #1262 (6ea914d) into master (0e6935b) will increase coverage by 0.00%.
Report is 5 commits behind head on master.
The diff coverage is 87.03%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1262   +/-   ##
=======================================
  Coverage   64.50%   64.51%           
=======================================
  Files         223      223           
  Lines       23767    23781   +14     
=======================================
+ Hits        15331    15342   +11     
- Misses       8436     8439    +3

Files	Coverage Δ
svf-llvm/include/SVF-LLVM/LLVMUtil.h	`73.68% <ø> (ø)`
svf-llvm/lib/LLVMModule.cpp	`82.74% <100.00%> (-0.48%)`	⬇️
svf-llvm/lib/LLVMUtil.cpp	`75.73% <100.00%> (-0.10%)`	⬇️
svf/include/MemoryModel/AccessPath.h	`83.33% <ø> (ø)`
svf/include/SVFIR/SVFStatements.h	`83.00% <ø> (ø)`
svf/include/SVFIR/SVFType.h	`95.65% <100.00%> (ø)`
svf/include/SVFIR/SVFValue.h	`89.04% <100.00%> (+0.15%)`	⬆️
svf/include/SVFIR/SymbolTableInfo.h	`92.30% <ø> (ø)`
svf/lib/Graphs/ICFG.cpp	`71.09% <100.00%> (+0.50%)`	⬆️
svf/lib/SVFIR/SVFFileSystem.cpp	`78.73% <100.00%> (ø)`
... and 4 more

... and 2 files with indirect coverage changes

canliture · 2023-12-01T02:37:54Z

I think maybe there are some potential crashes of all callers of getExitBB().

But we have guard assert in getExitBB(), these crashes may be left for more real projects test, but currently, It's very nice.😄

yuleisui · 2023-12-01T02:40:30Z

svf-llvm/lib/LLVMModule.cpp

+        /// set exit block
+        if (svfbb->getSuccessors().empty())
+        {
+            if (LLVMUtil::basicBlockHasRet(bb))


basicBlockHasRet=>basicBlockHasRetInst

yuleisui · 2023-12-01T02:40:48Z

svf-llvm/lib/LLVMModule.cpp

+        {
+            if (LLVMUtil::basicBlockHasRet(bb))
+            {
+                // exit basic block must have no successors and have a return instruction


move this comment to lin324

jumormt · 2023-12-01T03:02:43Z

svf/include/SVFIR/SVFValue.h

@@ -458,6 +468,11 @@ class SVFFunction : public SVFValue
        return isNotRet;
    }

+    inline bool hasReturn() const


Can hasReturn() and isNotRetFunction() be merged into one? hasReturn() seems redundant to me.

could you keep hasReturn and remove the other one?

Thanks for reviewing. Yeah, I think so.

yuleisui · 2023-12-01T03:49:22Z

svf/lib/Graphs/ICFG.cpp

@@ -54,7 +54,11 @@ FunExitICFGNode::FunExitICFGNode(NodeID id, const SVFFunction* f)
    // if function is implemented
    if (f->begin() != f->end())
    {
-        bb = f->getExitBB();
+        // ensure the enclosing function has exit basic block
+        if (!f->isNotRetFunction())


if (f->hasReturn)

sorry for late to commit. 😂 now Is it necessary to commit? I have committed to my forked repository

fix a bug: 'getExitBlock' of SVFFunction may get incorrect exit block.

5cce7dd

handle multi exit basic blocks: introduce a unique exit basic block, …

5443381

…and connect all exit basic blocks to it.

jumormt reviewed Nov 29, 2023

View reviewed changes

1. remove redundant entry block setting 2. setting exit block more co…

965c5a1

…ncisely

remove assert about nullness of exit basic block

57ff663

revise assertion:When caller calls 'getExitBB', let caller ensures th…

0da2dd6

…e function must have return instruction

yuleisui reviewed Nov 30, 2023

View reviewed changes

canliture added 5 commits December 1, 2023 09:01

make recognizing exit basic block more robust

7a0b4d6

fix crash: ensure the function has an exit basic block

a7e0a0f

fix incorrect recognizing exit basic block

dcaf079

revise assert about SVFFunction#back()

5a9fd4f

fix a crash: before call SVFFunction#getExitBB, need to ensure the fu…

408dfc0

…nction has ret instruction

yuleisui reviewed Dec 1, 2023

View reviewed changes

more clearly setting exit Basic Block

f899a36

jumormt reviewed Dec 1, 2023

View reviewed changes

remove redundant method 'hasReturn' in class SVFFunction

6ea914d

yuleisui reviewed Dec 1, 2023

View reviewed changes

yuleisui merged commit ccbff5c into SVF-tools:master Dec 1, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a bug: 'getExitBB' of SVFFunction may get incorrect exit block. #1262

fix a bug: 'getExitBB' of SVFFunction may get incorrect exit block. #1262

canliture commented Nov 29, 2023

canliture commented Nov 29, 2023 •

edited

Loading

yuleisui commented Nov 29, 2023

canliture commented Nov 29, 2023

jumormt commented Nov 29, 2023

jumormt Nov 29, 2023

canliture Nov 30, 2023

jumormt Nov 29, 2023

jumormt Nov 29, 2023

canliture Nov 30, 2023

canliture commented Nov 30, 2023

canliture commented Nov 30, 2023

jumormt commented Nov 30, 2023

jumormt commented Nov 30, 2023

yuleisui commented Nov 30, 2023

yuleisui Nov 30, 2023

canliture Dec 1, 2023

yuleisui Dec 1, 2023

canliture Dec 1, 2023

codecov bot commented Dec 1, 2023 •

edited

Loading

canliture commented Dec 1, 2023

yuleisui Dec 1, 2023

yuleisui Dec 1, 2023

jumormt Dec 1, 2023

yuleisui Dec 1, 2023

canliture Dec 1, 2023

yuleisui Dec 1, 2023

canliture Dec 1, 2023 •

edited

Loading

fix a bug: 'getExitBB' of SVFFunction may get incorrect exit block. #1262

fix a bug: 'getExitBB' of SVFFunction may get incorrect exit block. #1262

Conversation

canliture commented Nov 29, 2023

canliture commented Nov 29, 2023 • edited Loading

yuleisui commented Nov 29, 2023

canliture commented Nov 29, 2023

jumormt commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canliture commented Nov 30, 2023

canliture commented Nov 30, 2023

jumormt commented Nov 30, 2023

jumormt commented Nov 30, 2023

yuleisui commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

canliture commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canliture Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

canliture commented Nov 29, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading

canliture Dec 1, 2023 •

edited

Loading