Merge pull request #20156 from MathiasVP/value-numbering-for-noop-casts

MathiasVP · web-flow · commit ccfcd90f082d · 2025-08-11T10:33:58.000+02:00
C++: Value numbering for casts that only modify specifiers
diff --git a/cpp/ql/lib/change-notes/2025-08-02-gvn.md b/cpp/ql/lib/change-notes/2025-08-02-gvn.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The global value numbering library (`semmle.code.cpp.valuenumbering.GlobalValueNumbering` and `semmle.code.cpp.ir.ValueNumbering`) has been improved so more expressions are assigned the same value number.
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingInternal.qll b/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingInternal.qll
@@ -43,6 +43,23 @@ newtype TValueNumber =
   } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
+/**
+ * A `ConvertInstruction` which converts data of type `T` to data of type `U`
+ * where `T` and `U` only differ in specifiers. For example, if `T` is `int`
+ * and `U` is `const T` this is a conversion from a non-const integer to a
+ * const integer.
+ *
+ * Generally, the value number of a converted value is different from the value
+ * number of an unconverted value, but conversions which only modify specifiers
+ * leave the resulting value bitwise identical to the old value.
+ */
+class TypePreservingConvertInstruction extends ConvertInstruction {
+  TypePreservingConvertInstruction() {
+    pragma[only_bind_out](this.getResultType().getUnspecifiedType()) =
+      pragma[only_bind_out](this.getUnary().getResultType().getUnspecifiedType())
+  }
+}
+
 /**
  * A `CopyInstruction` whose source operand's value is congruent to the definition of that source
  * operand.
@@ -216,6 +233,7 @@ private predicate unaryValueNumber(
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
   not instr instanceof FieldAddressInstruction and
+  not instr instanceof TypePreservingConvertInstruction and
   instr.getOpcode() = opcode and
   tvalueNumber(instr.getUnary()) = operand
 }
@@ -351,6 +369,10 @@ private TValueNumber nonUniqueValueNumber(Instruction instr) {
       or
       // The value number of a copy is just the value number of its source value.
       result = tvalueNumber(instr.(CongruentCopyInstruction).getSourceValue())
+      or
+      // The value number of a type-preserving conversion is just the value
+      // number of the unconverted value.
+      result = tvalueNumber(instr.(TypePreservingConvertInstruction).getUnary())
     )
   )
 }
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingInternal.qll b/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingInternal.qll
@@ -43,6 +43,23 @@ newtype TValueNumber =
   } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
+/**
+ * A `ConvertInstruction` which converts data of type `T` to data of type `U`
+ * where `T` and `U` only differ in specifiers. For example, if `T` is `int`
+ * and `U` is `const T` this is a conversion from a non-const integer to a
+ * const integer.
+ *
+ * Generally, the value number of a converted value is different from the value
+ * number of an unconverted value, but conversions which only modify specifiers
+ * leave the resulting value bitwise identical to the old value.
+ */
+class TypePreservingConvertInstruction extends ConvertInstruction {
+  TypePreservingConvertInstruction() {
+    pragma[only_bind_out](this.getResultType().getUnspecifiedType()) =
+      pragma[only_bind_out](this.getUnary().getResultType().getUnspecifiedType())
+  }
+}
+
 /**
  * A `CopyInstruction` whose source operand's value is congruent to the definition of that source
  * operand.
@@ -216,6 +233,7 @@ private predicate unaryValueNumber(
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
   not instr instanceof FieldAddressInstruction and
+  not instr instanceof TypePreservingConvertInstruction and
   instr.getOpcode() = opcode and
   tvalueNumber(instr.getUnary()) = operand
 }
@@ -351,6 +369,10 @@ private TValueNumber nonUniqueValueNumber(Instruction instr) {
       or
       // The value number of a copy is just the value number of its source value.
       result = tvalueNumber(instr.(CongruentCopyInstruction).getSourceValue())
+      or
+      // The value number of a type-preserving conversion is just the value
+      // number of the unconverted value.
+      result = tvalueNumber(instr.(TypePreservingConvertInstruction).getUnary())
     )
   )
 }
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingInternal.qll b/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingInternal.qll
@@ -43,6 +43,23 @@ newtype TValueNumber =
   } or
   TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }
 
+/**
+ * A `ConvertInstruction` which converts data of type `T` to data of type `U`
+ * where `T` and `U` only differ in specifiers. For example, if `T` is `int`
+ * and `U` is `const T` this is a conversion from a non-const integer to a
+ * const integer.
+ *
+ * Generally, the value number of a converted value is different from the value
+ * number of an unconverted value, but conversions which only modify specifiers
+ * leave the resulting value bitwise identical to the old value.
+ */
+class TypePreservingConvertInstruction extends ConvertInstruction {
+  TypePreservingConvertInstruction() {
+    pragma[only_bind_out](this.getResultType().getUnspecifiedType()) =
+      pragma[only_bind_out](this.getUnary().getResultType().getUnspecifiedType())
+  }
+}
+
 /**
  * A `CopyInstruction` whose source operand's value is congruent to the definition of that source
  * operand.
@@ -216,6 +233,7 @@ private predicate unaryValueNumber(
   not instr instanceof InheritanceConversionInstruction and
   not instr instanceof CopyInstruction and
   not instr instanceof FieldAddressInstruction and
+  not instr instanceof TypePreservingConvertInstruction and
   instr.getOpcode() = opcode and
   tvalueNumber(instr.getUnary()) = operand
 }
@@ -351,6 +369,10 @@ private TValueNumber nonUniqueValueNumber(Instruction instr) {
       or
       // The value number of a copy is just the value number of its source value.
       result = tvalueNumber(instr.(CongruentCopyInstruction).getSourceValue())
+      or
+      // The value number of a type-preserving conversion is just the value
+      // number of the unconverted value.
+      result = tvalueNumber(instr.(TypePreservingConvertInstruction).getUnary())
     )
   )
 }
diff --git a/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/ir_gvn.expected b/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/ir_gvn.expected
@@ -1145,3 +1145,43 @@ test.cpp:
 #  152|     v152_7(void)       = ReturnVoid                : 
 #  152|     v152_8(void)       = AliasedUse                : ~m156_7
 #  152|     v152_9(void)       = ExitFunction              : 
+
+#  166| void test_constMemberFunction()
+#  166|   Block 0
+#  166|     v166_1(void)                                 = EnterFunction                        : 
+#  166|     m166_2(unknown)                              = AliasedDefinition                    : 
+#  166|         valnum = unique
+#  166|     m166_3(unknown)                              = InitializeNonLocal                   : 
+#  166|         valnum = unique
+#  166|     m166_4(unknown)                              = Chi                                  : total:m166_2, partial:m166_3
+#  166|         valnum = unique
+#  167|     r167_1(glval<StructWithConstMemberFunction>) = VariableAddress[s]                   : 
+#  167|         valnum = r167_1, r168_2, r169_1, r169_2
+#  167|     m167_2(StructWithConstMemberFunction)        = Uninitialized[s]                     : &:r167_1
+#  167|         valnum = m167_2, m168_4, r168_3
+#  167|     m167_3(unknown)                              = Chi                                  : total:m166_4, partial:m167_2
+#  167|         valnum = unique
+#  168|     r168_1(glval<StructWithConstMemberFunction>) = VariableAddress[s2]                  : 
+#  168|         valnum = unique
+#  168|     r168_2(glval<StructWithConstMemberFunction>) = VariableAddress[s]                   : 
+#  168|         valnum = r167_1, r168_2, r169_1, r169_2
+#  168|     r168_3(StructWithConstMemberFunction)        = Load[s]                              : &:r168_2, m167_2
+#  168|         valnum = m167_2, m168_4, r168_3
+#  168|     m168_4(StructWithConstMemberFunction)        = Store[s2]                            : &:r168_1, r168_3
+#  168|         valnum = m167_2, m168_4, r168_3
+#  169|     r169_1(glval<StructWithConstMemberFunction>) = VariableAddress[s]                   : 
+#  169|         valnum = r167_1, r168_2, r169_1, r169_2
+#  169|     r169_2(glval<StructWithConstMemberFunction>) = Convert                              : r169_1
+#  169|         valnum = r167_1, r168_2, r169_1, r169_2
+#  169|     r169_3(glval<unknown>)                       = FunctionAddress[constMemberFunction] : 
+#  169|         valnum = unique
+#  169|     v169_4(void)                                 = Call[constMemberFunction]            : func:r169_3, this:r169_2
+#  169|     m169_5(unknown)                              = ^CallSideEffect                      : ~m167_3
+#  169|         valnum = unique
+#  169|     m169_6(unknown)                              = Chi                                  : total:m167_3, partial:m169_5
+#  169|         valnum = unique
+#  169|     v169_7(void)                                 = ^IndirectReadSideEffect[-1]          : &:r169_2, ~m169_6
+#  170|     v170_1(void)                                 = NoOp                                 : 
+#  166|     v166_5(void)                                 = ReturnVoid                           : 
+#  166|     v166_6(void)                                 = AliasedUse                           : ~m169_6
+#  166|     v166_7(void)                                 = ExitFunction                         : 
diff --git a/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/test.cpp b/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/test.cpp
@@ -156,4 +156,15 @@ void test_read_global_different(int n) {
   global_a->y = n;
 
   int d = global_a->x;
+}
+
+struct StructWithConstMemberFunction {
+  int x;
+  void constMemberFunction() const;
+};
+
+void test_constMemberFunction() {
+  StructWithConstMemberFunction s;
+  StructWithConstMemberFunction s2 = s;
+  s.constMemberFunction();
 }
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected
@@ -13,7 +13,6 @@
 | test.cpp:49:2:49:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. |
 | test.cpp:62:3:62:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. |
 | test.cpp:65:3:65:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. |
-| test.cpp:70:2:70:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. |
 | test.cpp:81:3:81:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. |
 | test.cpp:84:3:84:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. |
 | test.cpp:105:2:105:10 | call to wcsxfrm_l | Potentially unsafe call to wcsxfrm_l; third argument should be size of destination. |

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: minorAnalysis
 +---
 +* The global value numbering library (`semmle.code.cpp.valuenumbering.GlobalValueNumbering` and `semmle.code.cpp.ir.ValueNumbering`) has been improved so more expressions are assigned the same value number.