[PATCH] D149698: [IntervalTree] Initialize find_iterator::Point with default constructor

[Joshua Cao via Phabricator via llvm-commits]

caojoshua created this revision.
Herald added a project: All.
caojoshua added reviewers: vitalybuka, CarlosAlbertoEnciso, clopez.
caojoshua removed a reviewer: clopez.
caojoshua added a comment.
caojoshua published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

If this change looks ok, I would like to reland https://reviews.llvm.org/D138526


caojoshua added a comment.

We can't rely on the compiler to short-circuit the conditional check in `find_iterator::operator==()`.

  return (!LHS.Node && !RHS.Node && !LHS.Index && !RHS.Index) ||
         (LHS.Point == RHS.Point && LHS.Node == RHS.Node &&
          LHS.Index == RHS.Index);

Before the unswitch patch, the compiler was short-circuiting, so we never use the uninitialized `RHS.Point`. After the unswitching patch, the RHS of the `||` gets hoisted to an earlier part of the function.

The compiler is legally allowed to switch the left hand and right hand side of the `||`. I constructed an example in https://godbolt.org/z/MvdzdKovz.


find_iterator IntervalTree::end is initialized with the default
constructor, which does not initialize the Point member variable. This
can cause a use-of-initialized-value in find_iterator::operator==.

This issue was discovered by msan in https://reviews.llvm.org/D138526,
which unswitches invariant selects. The logical or comparison is changed
to a select (`a||b` can be transformed to `select a, true, b`) and
hoisted to an earlier point in the program. Without the patch, the
select is guarded by branches such that the unitialized variable is
never used.

This change is technically not NFC, because it requires template type
PointT to have a default constructor. The few consumers in the repo are
all use primitives, which has default constructors that set them to
zero, so nothing is affected in the repo. External consumers that use a
PointT without a default constructor will be impacted, but this is a
young header and has few consumers in the world.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149698

Files:
  llvm/include/llvm/ADT/IntervalTree.h


Index: llvm/include/llvm/ADT/IntervalTree.h
===================================================================
--- llvm/include/llvm/ADT/IntervalTree.h
+++ llvm/include/llvm/ADT/IntervalTree.h
@@ -541,7 +541,7 @@
       }
     }
 
-    find_iterator() = default;
+    find_iterator() : Point() {}
     explicit find_iterator(const IntervalReferences *Left,
                            const IntervalReferences *Right, IntervalNode *Node,
                            PointType Point)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D149698.518873.patch
Type: text/x-patch
Size: 484 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230502/263bf6f9/attachment.bin>