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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka requested changes to this revision.
vitalybuka added a comment.
This revision now requires changes to proceed.

In D149698#4313832 <https://reviews.llvm.org/D149698#4313832>, @caojoshua wrote:

> 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.

Comment in https://godbolt.org/z/MvdzdKovz is incorrect, x is zero initialized as it's global. But I am not sure what this proves.

There is c/c++ standard and the is compiler. Standard explain how such expressions are evaluated. I believe this is short circuited. Compiler can do shortcuts if results the same.

However Msan goal is to detect undefined behaviors in user code when the user violates standard.
D138526 <https://reviews.llvm.org/D138526> brakes msan, making it falsely complaining when violation is not really happening, like here.

foo in your example is not a problem as select is pass-through for msan. it complains about branches.
When you replace select with branch, we have a report.



================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:466
     IntervalNode *Node = nullptr;
     PointType Point;
     unsigned Index = 0;
----------------
it should be here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149698/new/

https://reviews.llvm.org/D149698