[Mlir-commits] [mlir] 1e60e7a - [mlir][tblgen] Fix undefined behaviour found by MSVC debug iterators

[Markus Böck]

Author: Markus Böck
Date: 2023-01-14T17:49:48+01:00
New Revision: 1e60e7add7ce18a652a726d5ec267547399c744e

URL: https://github.com/llvm/llvm-project/commit/1e60e7add7ce18a652a726d5ec267547399c744e
DIFF: https://github.com/llvm/llvm-project/commit/1e60e7add7ce18a652a726d5ec267547399c744e.diff

LOG: [mlir][tblgen] Fix undefined behaviour found by MSVC debug iterators

Incrementing past the end iterator of any container in C++ is immediate undefined behaviour.
This is guaranteed to occur in the loop condition due to the expression cur = earlyIncIt++, which when earlyIncIt is the end iterator (aka we just did the last iteration of the loop), will do an increment on the end iterator.

To fix this, the patch refactors the loop to a more conventional loop using iterators, with the only difference being that the increment happens through the erase operation, which conveniently returns an iterator to the element after the erased element. Thanks to that guarantee there is also no need to use std::list over std::vector.
I also opted to reduce the inner loop find_if, because I felt splitting the "search" and the effects of if it was successful made the code (subjectively) nicer, and also avoided having to add an extra "bool erased" to the outer loop body.

Differential Revision: https://reviews.llvm.org/D141758

Added: 
    

Modified: 
    mlir/lib/TableGen/Operator.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/TableGen/Operator.cpp b/mlir/lib/TableGen/Operator.cpp
index dd528406fe9a2..1b186f81cc0e6 100644
--- a/mlir/lib/TableGen/Operator.cpp
+++ b/mlir/lib/TableGen/Operator.cpp
@@ -498,29 +498,33 @@ void Operator::populateTypeInferenceInfo(
   }
 
   // Propagate inferredness until a fixed point.
-  std::list<ResultTypeInference *> worklist;
+  std::vector<ResultTypeInference *> worklist;
   for (ResultTypeInference &infer : inference)
     if (!infer.inferred)
       worklist.push_back(&infer);
   bool changed;
   do {
     changed = false;
-    // This is `llvm::make_early_inc_range` but keeps the iterator for erasing.
-    for (auto earlyIncIt = worklist.begin(), cur = earlyIncIt;
-         cur = earlyIncIt++, cur != worklist.end();) {
+    for (auto cur = worklist.begin(); cur != worklist.end();) {
       ResultTypeInference &infer = **cur;
-      for (auto &[idx, source] : llvm::enumerate(infer.sources)) {
-        assert(InferredResultType::isResultIndex(source.getIndex()));
-        if (inference[InferredResultType::unmapResultIndex(source.getIndex())]
-                .inferred) {
-          changed = true;
-          infer.inferred = true;
-          // Make this the only source for the result. This breaks any cycles.
-          infer.sources.assign(1, source);
-          worklist.erase(cur);
-          break;
-        }
+
+      InferredResultType *iter =
+          llvm::find_if(infer.sources, [&](const InferredResultType &source) {
+            assert(InferredResultType::isResultIndex(source.getIndex()));
+            return inference[InferredResultType::unmapResultIndex(
+                                 source.getIndex())]
+                .inferred;
+          });
+      if (iter == infer.sources.end()) {
+        ++cur;
+        continue;
       }
+
+      changed = true;
+      infer.inferred = true;
+      // Make this the only source for the result. This breaks any cycles.
+      infer.sources.assign(1, *iter);
+      cur = worklist.erase(cur);
     }
   } while (changed);