Detect simple struct changes

.vscode/settings.json

@@ -0,0 +1,77 @@
+{
+    "files.associations": {
+        "common.h": "c",
+        "vector": "cpp",
+        "array": "cpp",
+        "bitset": "cpp",
+        "string_view": "cpp",
+        "initializer_list": "cpp",
+        "span": "cpp",
+        "any": "cpp",
+        "atomic": "cpp",
+        "bit": "cpp",
+        "*.tcc": "cpp",
+        "cctype": "cpp",
+        "charconv": "cpp",
+        "chrono": "cpp",
+        "cinttypes": "cpp",
+        "clocale": "cpp",
+        "cmath": "cpp",
+        "compare": "cpp",
+        "concepts": "cpp",
+        "condition_variable": "cpp",
+        "cstdarg": "cpp",
+        "cstddef": "cpp",
+        "cstdint": "cpp",
+        "cstdio": "cpp",
+        "cstdlib": "cpp",
+        "cstring": "cpp",
+        "ctime": "cpp",
+        "cwchar": "cpp",
+        "cwctype": "cpp",
+        "deque": "cpp",
+        "forward_list": "cpp",
+        "list": "cpp",
+        "map": "cpp",
+        "set": "cpp",
+        "string": "cpp",
+        "unordered_map": "cpp",
+        "unordered_set": "cpp",
+        "exception": "cpp",
+        "algorithm": "cpp",
+        "functional": "cpp",
+        "iterator": "cpp",
+        "memory": "cpp",
+        "memory_resource": "cpp",
+        "netfwd": "cpp",
+        "numeric": "cpp",
+        "optional": "cpp",
+        "random": "cpp",
+        "ratio": "cpp",
+        "system_error": "cpp",
+        "tuple": "cpp",
+        "type_traits": "cpp",
+        "utility": "cpp",
+        "format": "cpp",
+        "fstream": "cpp",
+        "iomanip": "cpp",
+        "iosfwd": "cpp",
+        "iostream": "cpp",
+        "istream": "cpp",
+        "limits": "cpp",
+        "mutex": "cpp",
+        "new": "cpp",
+        "numbers": "cpp",
+        "ostream": "cpp",
+        "semaphore": "cpp",
+        "shared_mutex": "cpp",
+        "sstream": "cpp",
+        "stdexcept": "cpp",
+        "stop_token": "cpp",
+        "streambuf": "cpp",
+        "thread": "cpp",
+        "cfenv": "cpp",
+        "typeinfo": "cpp",
+        "variant": "cpp"
+    }
+}

custom-patterns/config.yml

@@ -0,0 +1,5 @@
+on_parse_failure: ERROR
+patterns:
+  - /home/pavol/RH/diffkemp/custom-patterns/mbedtls_aes_new.ll 
+  - /home/pavol/RH/diffkemp/custom-patterns/mbedtls_aes_old_i32.ll
+  - /home/pavol/RH/diffkemp/custom-patterns/mbedtls_aes_old_ptr.ll

diffkemp/cli.py

@@ -152,7 +152,8 @@ def make_argument_parser():
                         "type-casts",
                         "control-flow-only",
                         "inverse-conditions",
-                        "reordered-bin-ops"]
+                        "reordered-bin-ops",
+                        "struct-changes"]
 
     # Semantic patterns options.
     compare_ap.add_argument("--enable-pattern",

diffkemp/config.py

@@ -23,6 +23,7 @@ def __init__(
         control_flow_only=False,
         inverse_conditions=True,
         reordered_bin_ops=True,
+        struct_changes=True,
     ):
         """
         Create a configuration of built-in patterns.
@@ -41,6 +42,7 @@ def __init__(
         :param control_flow_only: Consider control-flow changes only.
         :param inverse_conditions: Inverted branch conditions.
         :param reordered_bin_ops: Match reordered binary operations.
+        :param struct_changes: Changes in local structs and variables.
         """
         self.settings = {
             "struct-alignment": struct_alignment,
@@ -54,6 +56,7 @@ def __init__(
             "control-flow-only": control_flow_only,
             "inverse-conditions": inverse_conditions,
             "reordered-bin-ops": reordered_bin_ops,
+            "struct-changes": struct_changes,
         }
         self.resolve_dependencies()
 
@@ -101,6 +104,7 @@ def as_ffi_struct(self):
         ffi_struct.ControlFlowOnly = self.settings["control-flow-only"]
         ffi_struct.InverseConditions = self.settings["inverse-conditions"]
         ffi_struct.ReorderedBinOps = self.settings["reordered-bin-ops"]
+        ffi_struct.StructChanges = self.settings["struct-changes"]
         return ffi_struct
 
 

diffkemp/simpll/Config.h

@@ -38,6 +38,7 @@ struct BuiltinPatterns {
     bool ControlFlowOnly = false;
     bool InverseConditions = true;
     bool ReorderedBinOps = true;
+    bool StructChanges = true;
 };
 
 /// Tool configuration parsed from CLI options.

diffkemp/simpll/DifferentialFunctionComparator.cpp

@@ -510,6 +510,13 @@ int DifferentialFunctionComparator::cmpAllocs(const CallInst *CL,
 /// they need a replacement.
 bool DifferentialFunctionComparator::maySkipInstruction(
         const Instruction *Inst) const {
+    if (Inst->isSafeToRemove()
+        && (Inst->getNumUses() == 0
+            || (config.Patterns.StructChanges
+                && isOnlyUsedByIgnorableMemInstructions(Inst)))) {
+        ignoredInstructions.insert(Inst);
+        return true;
+    }
     if (isa<AllocaInst>(Inst)) {
         ignoredInstructions.insert(Inst);
         return true;
@@ -519,16 +526,23 @@ bool DifferentialFunctionComparator::maySkipInstruction(
         ignoredInstructions.insert(Inst);
         return true;
     }
+    if (isZeroGEP(Inst)) {
+        replacedInstructions.insert({Inst, Inst->getOperand(0)});
+        return true;
+    }
     if (isCast(Inst)) {
         if (config.Patterns.TypeCasts) {
             replacedInstructions.insert({Inst, Inst->getOperand(0)});
             return true;
         }
         return maySkipCast(Inst);
     }
-    if (isZeroGEP(Inst)) {
-        replacedInstructions.insert({Inst, Inst->getOperand(0)});
-        return true;
+    if (auto Store = dyn_cast<StoreInst>(Inst)) {
+        if (config.Patterns.StructChanges
+            && hasIgnoredAllocaAsPtrOperand(Store)) {
+            ignoredInstructions.insert(Store);
+            return true;
+        }
     }
     if (auto Load = dyn_cast<LoadInst>(Inst)) {
         return maySkipLoad(Load);
@@ -597,55 +611,68 @@ bool DifferentialFunctionComparator::maySkipCast(const User *Cast) const {
     return false;
 }
 
-/// Check whether the given instruction is a repetitive variant of a previous
-/// load with no store or call instructions in between. Replacements of
-/// ignorable loads are stored inside the replaced instructions map.
-bool DifferentialFunctionComparator::maySkipLoad(const LoadInst *Load) const {
+//isa<StoreInst>(&*it) || (isa<CallInst>(&*it) && !isDebugInfo(*it))
+
+/// Check whether the given load may be skipped, i.e., it is either a repetitive
+/// variant of a previous load, or a load from an ignored local variable.
+/// Load replacements are stored inside the replaced instructions map.
+bool DifferentialFunctionComparator::maySkipLoad(const llvm::LoadInst *Load) const {
     auto BB = Load->getParent();
     if (!BB) {
         return false;
     }
 
-    // Check if all backward paths in the CFG graph lead to a single load from
-    // the same pointer as the one we want to skip. There must be no stores
-    // or function calls on any of the paths.
+    // Check if all backward CFG paths starting in the load lead to either:
+    // - a single load from the same pointer,
+    // - a single ignored store to the same pointer.
+    // There must be no conflicting stores or calls on any of the paths.
     bool first = true;
-    const LoadInst *PreviousLoad = nullptr;
+    const Value *Replacement = nullptr;
     std::deque<const BasicBlock *> blockQueue = {BB};
     std::unordered_set<const BasicBlock *> visitedBlocks = {BB};
     while (!blockQueue.empty()) {
         BB = blockQueue.front();
         blockQueue.pop_front();
 
-        // Look for the load instruction in the current block. If it's not
-        // here, we will have to check that all predecessors have it.
+        // Look for a suitable load or store in the current block. If it's not
+        // here, we will have to check all predecessors.
         bool checkPredecessors = true;
         for (auto it = BB->rbegin(); it != BB->rend(); ++it) {
-            // Skip all instructions before the compared load
-            // when in the first block.
+            // Skip all instructions before the compared load when in the first
+            // block (note that we are iterating in reverse).
             if (first) {
                 if (&*it == Load) {
                     first = false;
                 }
                 continue;
             }
 
-            if (auto Candidate = dyn_cast<LoadInst>(&*it)) {
-                // Found a candidate load; check whether the pointer matches.
+            const Value *ReplacementCandidate = nullptr;
+            if (auto PreviousLoad = dyn_cast<LoadInst>(&*it)) {
                 if (Load->getPointerOperand()
-                    == Candidate->getPointerOperand()) {
-                    if (PreviousLoad) {
-                        // If another suitable load has been found previously
-                        // in a different CFG path, fail. We need a single one.
-                        return false;
-                    }
-                    // Otherwise remember this load.
-                    PreviousLoad = Candidate;
-                    checkPredecessors = false;
-                    break;
+                    == PreviousLoad->getPointerOperand()) {
+                    // Found an identical load, save the candidate.
+                    ReplacementCandidate = PreviousLoad;
+                }
+            } else if (auto Store = dyn_cast<StoreInst>(&*it)) {
+                if (ignoredInstructions.count(Store)
+                    && Store->getPointerOperand()
+                               == Load->getPointerOperand()) {
+                    // Found a previously ignored store, save the candidate.
+                    ReplacementCandidate = Store->getValueOperand();
+                }
+            }
+            if (ReplacementCandidate) {
+                if (Replacement) {
+                    // If another suitable replacement has been found previously
+                    // in a different CFG path, fail. We need a single one.
+                    return false;
                 }
-            } else if (isa<StoreInst>(&*it)
-                       || (isa<CallInst>(&*it) && !isDebugInfo(*it))) {
+                // Otherwise remember this replacement.
+                Replacement = ReplacementCandidate;
+                checkPredecessors = false;
+                break;
+            } else if (mayOverwritePtrData(&*it, Load->getPointerOperand())) {
                 // Fail if a possibly conflicting store or call is found.
                 return false;
             }
@@ -666,10 +693,9 @@ bool DifferentialFunctionComparator::maySkipLoad(const LoadInst *Load) const {
             }
         }
     }
-    // If the load is repeated without stores in between, skip it because
-    // the load is redundant and its removal will cause no semantic difference.
-    if (PreviousLoad) {
-        replacedInstructions.insert({Load, PreviousLoad});
+    // Successfully found a replacement,
+    if (Replacement) {
+        replacedInstructions.insert({Load, Replacement});
         return true;
     }
     return false;
@@ -680,9 +706,12 @@ bool DifferentialFunctionComparator::maySkipLoad(const LoadInst *Load) const {
 /// of the same kind.
 bool DifferentialFunctionComparator::maySkipReorderableBinaryOp(
         const Instruction *Op) const {
-    return std::all_of(Op->user_begin(), Op->user_end(), [Op](auto user) {
-        auto userOp = dyn_cast<BinaryOperator>(user);
-        return userOp && userOp->getOpcode() == Op->getOpcode();
+    return std::all_of(Op->user_begin(), Op->user_end(), [&](auto user) {
+        if (auto userBinOp = dyn_cast<BinaryOperator>(user))
+            return userBinOp->getOpcode() == Op->getOpcode();
+        if (auto userStore = dyn_cast<StoreInst>(user))
+            return hasIgnoredAllocaAsPtrOperand(userStore);
+        return false;
     });
 }
 
@@ -1382,6 +1411,11 @@ int DifferentialFunctionComparator::cmpFieldAccess(
     if (!(GEPL && GEPR))
         RETURN_WITH_LOG(1);
 
+    if (config.Patterns.StructChanges
+        && (hasIgnoredAllocaAsPtrOperand(GEPL)
+            || hasIgnoredAllocaAsPtrOperand(GEPR)))
+        RETURN_WITH_LOG(1);
+
     if (!GEPL->hasAllConstantIndices() || !GEPR->hasAllConstantIndices())
         RETURN_WITH_LOG(1);
 
@@ -2020,3 +2054,47 @@ bool DifferentialFunctionComparator::checkInverseCondUsers(
     }
     return true;
 }
+
+template <typename InstType>
+bool DifferentialFunctionComparator::hasIgnoredAllocaAsPtrOperand(
+        const InstType *Inst) const {
+    const Value *Ptr = Inst->getPointerOperand();
+    if (auto GEP = dyn_cast<GetElementPtrInst>(Ptr))
+        return hasIgnoredAllocaAsPtrOperand(GEP);
+    return isIgnoredAlloca(Ptr);
+}
+
+bool DifferentialFunctionComparator::isOnlyUsedByIgnorableMemInstructions(
+        const Value *Val) const {
+    return std::all_of(
+            Val->user_begin(), Val->user_end(), [&](const User *user) {
+                if (auto store = dyn_cast<StoreInst>(user))
+                    return hasIgnoredAllocaAsPtrOperand(store);
+                if (auto load = dyn_cast<LoadInst>(user))
+                    return hasIgnoredAllocaAsPtrOperand(load);
+                return false;
+            });
+}
+
+bool DifferentialFunctionComparator::isIgnoredAlloca(const Value *Val) const {
+    return isa<AllocaInst>(Val) && ignoredInstructions.count(Val);
+}
+
+bool DifferentialFunctionComparator::mayOverwritePtrData(const Instruction *Inst, const Value *Ptr) {
+    if (auto Store = dyn_cast<StoreInst>(Inst)) {
+        // Check whether the store pointer may alias the given one.
+        return mayAlias(Store->getPointerOperand(), Ptr);
+    }
+    if (auto Call = dyn_cast<CallInst>(Inst)) {
+        // Calls to debug intrinsics will never store anywhere.
+        if (isDebugInfo(*Call))
+            return false;
+        // Check if any pointer function argument may alias the given one.
+        return std::any_of(Call->arg_begin(), Call->arg_end(), [Ptr] (const Use &Arg) {
+            if (!Arg->getType()->isPointerTy())
+                return false;
+            return mayAlias(Arg.get(), Ptr);
+        });
+    }
+    return false;
+}

diffkemp/simpll/DifferentialFunctionComparator.h

@@ -230,7 +230,7 @@ class DifferentialFunctionComparator : public FunctionComparator {
     /// previous load with no store or call instructions in between.
     /// Replacements of ignorable loads are stored inside the replaced
     /// instructions map.
-    bool maySkipLoad(const LoadInst *Load) const;
+    bool maySkipLoad(const llvm::LoadInst *Load) const;
 
     /// Check whether the given reorderable binary operator can be skipped.
     /// It can only be skipped if all its users are binary operations
@@ -288,6 +288,15 @@ class DifferentialFunctionComparator : public FunctionComparator {
     /// Return true if the users are acceptable for inverse conditions pattern,
     /// false otherwise.
     bool checkInverseCondUsers(const Instruction *inst) const;
+
+    bool isIgnoredAlloca(const Value *Val) const;
+
+    template <typename InstType>
+    bool hasIgnoredAllocaAsPtrOperand(const InstType *Inst) const;
+
+    bool isOnlyUsedByIgnorableMemInstructions(const Value *Val) const;
+
+    static bool mayOverwritePtrData(const Instruction *Inst, const Value *Ptr);
 };
 
 #endif // DIFFKEMP_SIMPLL_DIFFERENTIALFUNCTIONCOMPARATOR_H