[clang] [analyzer] Order UninitializedObject notes deterministically (PR #214208)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Balázs Benics (steakhal)

<details>
<summary>Changes</summary>

UninitFieldMap is keyed by FieldRegion pointers, so iterating it to emit the notes ordered them by where those regions happened to be allocated. The order therefore varied between runs: on cxx-uninitialized-object.cpp the two notes of the report at line 363 swapped in roughly 3 of 12 runs.

- Emit the notes in source order instead, tie-broken by the note message.
- -verify matches notes by line, text and count and ignores their order, so the new test pins the order down with FileCheck.

Assisted-By: claude

---
Full diff: https://github.com/llvm/llvm-project/pull/214208.diff


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp (+33-4) 
- (added) clang/test/Analysis/cxx-uninitialized-object-note-order.cpp (+75) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
index 4d54f11efe158..6585a4e0b0df8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -201,11 +201,40 @@ void UninitializedObjectChecker::checkEndFunction(
       BT_uninitField, WarningOS.str(), Node, LocUsedForUniqueing,
       Node->getStackFrame()->getDecl());
 
-  for (const auto &Pair : UninitFields) {
-    Report->addNote(Pair.second,
-                    PathDiagnosticLocation::create(Pair.first->getDecl(),
-                                                   Context.getSourceManager()));
+  using NoteTy = std::pair<PathDiagnosticLocation, StringRef>;
+  SmallVector<NoteTy> Notes;
+  const auto &SM = Context.getSourceManager();
+  for (const auto &[FieldRegion, NoteMsg] : UninitFields) {
+    auto FieldLoc = PathDiagnosticLocation::create(FieldRegion->getDecl(), SM);
+    Notes.emplace_back(FieldLoc, NoteMsg);
   }
+
+  // Make the order deterministic.
+  llvm::sort(Notes, [](const NoteTy &LHS, const NoteTy &RHS) {
+    FullSourceLoc L = LHS.first.asLocation();
+    FullSourceLoc R = RHS.first.asLocation();
+    if (L != R)
+      return L.isBeforeInTranslationUnitThan(R);
+    // Comparing the field locs might not be enough:
+    //   struct TwoInstances {
+    //     Inner first;
+    //     Inner second;
+    //     int z;
+    //     TwoInstances() { z = 0; } // warn: 4 uninitialized fields
+    //   };
+    // Then creating an instance of `TwoInstances` would trigger 4 notes:
+    //   - note: uninitialized field 'this->first.x'  <-- FieldDecl{Inner.x}
+    //   - note: uninitialized field 'this->second.x' <-- FieldDecl{Inner.x}
+    //   - note: uninitialized field 'this->first.y'  <-- FieldDecl{Inner.y}
+    //   - note: uninitialized field 'this->second.y' <-- FieldDecl{Inner.y}
+    // Note that the FieldDecls are pairwise the same, thus we need a
+    // tie breaker: the note message.
+    return LHS.second < RHS.second;
+  });
+
+  for (const auto &[Loc, NoteMsg] : Notes)
+    Report->addNote(NoteMsg, Loc);
+
   Context.emitReport(std::move(Report));
 }
 
diff --git a/clang/test/Analysis/cxx-uninitialized-object-note-order.cpp b/clang/test/Analysis/cxx-uninitialized-object-note-order.cpp
new file mode 100644
index 0000000000000..9557028f628fc
--- /dev/null
+++ b/clang/test/Analysis/cxx-uninitialized-object-note-order.cpp
@@ -0,0 +1,75 @@
+// DEFINE: %{run} = %clang_analyze_cc1 \
+// DEFINE:   -analyzer-checker=core,optin.cplusplus.UninitializedObject \
+// DEFINE:   -analyzer-output=text -fno-caret-diagnostics %s
+
+// RUN: %{run} -verify
+// RUN: %{run} 2>&1 | FileCheck %s
+
+struct MultipleSiblings {
+  int a; // expected-note {{uninitialized field 'this->a'}}
+  int b; // expected-note {{uninitialized field 'this->b'}}
+  int c; // expected-note {{uninitialized field 'this->c'}}
+  int d;
+  MultipleSiblings() { d = 0; }
+  // expected-warning at -1 {{3 uninitialized fields}}
+  // expected-note at -2    {{3 uninitialized fields}}
+};
+
+void fMultipleSiblings() {
+  MultipleSiblings s; // expected-note {{Calling default constructor for 'MultipleSiblings'}}
+}
+
+// CHECK-LABEL: warning: 3 uninitialized fields at the end of the constructor call
+// CHECK-NEXT: note: uninitialized field 'this->a'
+// CHECK-NEXT: note: uninitialized field 'this->b'
+// CHECK-NEXT: note: uninitialized field 'this->c'
+
+struct Inner {
+  int x;
+  // expected-note at -1 {{uninitialized field 'this->i.x'}}
+  // expected-note at -2 {{uninitialized field 'this->first.x'}}
+  // expected-note at -3 {{uninitialized field 'this->second.x'}}
+  int y;
+  // expected-note at -1 {{uninitialized field 'this->i.y'}}
+  // expected-note at -2 {{uninitialized field 'this->first.y'}}
+  // expected-note at -3 {{uninitialized field 'this->second.y'}}
+};
+
+struct Nested {
+  Inner i;
+  int z;
+  Nested() { z = 0; }
+  // expected-warning at -1 {{2 uninitialized fields}}
+  // expected-note at -2    {{2 uninitialized fields}}
+};
+
+void fNested() {
+  Nested n; // expected-note {{Calling default constructor for 'Nested'}}
+}
+
+// CHECK-LABEL: warning: 2 uninitialized fields at the end of the constructor call
+// CHECK-NEXT: note: uninitialized field 'this->i.x'
+// CHECK-NEXT: note: uninitialized field 'this->i.y'
+
+struct TwoInstances {
+  Inner first;
+  Inner second;
+  int z;
+  TwoInstances() { z = 0; }
+  // expected-warning at -1 {{4 uninitialized fields}}
+  // expected-note at -2    {{4 uninitialized fields}}
+};
+
+void fTwoInstances() {
+  TwoInstances t; // expected-note {{Calling default constructor for 'TwoInstances'}}
+}
+
+// 'first' and 'second' have the same type, so all four notes point at the two members of Inner.
+// Ordering by source location alone does not separate them.
+
+// CHECK-LABEL: warning: 4 uninitialized fields at the end of the constructor call
+// CHECK-NEXT: note: uninitialized field 'this->first.x'
+// CHECK-NEXT: note: uninitialized field 'this->second.x'
+// CHECK-NEXT: note: uninitialized field 'this->first.y'
+// CHECK-NEXT: note: uninitialized field 'this->second.y'
+

``````````

</details>


https://github.com/llvm/llvm-project/pull/214208