[ADT] Require base equality in indexed_accessor_iterator::operator==()

[andrey-golubev]

Similarly to operator<(), equality-comparing iterators from different ranges must really be forbidden. The preconditions for being able to do it1 < it2 and it1 != it2 (or it1 == it2 for the matter) ought to be the same. Thus, there's little sense in keeping explicit base object comparison in operator==() whilst having this is a precondition in operator<() and operator-() (e.g. used for std::distance() and such).

[andrey-golubev]

@River707 I think you're effectively the maintainer of this iterator class (and the surrounding machinery), please take a look!

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Andrei Golubev (andrey-golubev)

Changes

Similarly to operator<(), equality-comparing iterators from different ranges must really be forbidden. The preconditions for being able to do it1 < it2 and it1 != it2 (or it1 == it2 for the matter) ought to be the same. Thus, there's little sense in keeping explicit base object comparison in operator==() whilst having this is a precondition in operator<() and operator-() (e.g. used for std::distance() and such).

---

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

1 Files Affected:

• (modified) llvm/include/llvm/ADT/STLExtras.h (+2-1)

diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index e11d6cac7685e4..eb441bb31c9bc8 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -1194,7 +1194,8 @@ class indexed_accessor_iterator
     return index - rhs.index;
   }
   bool operator==(const indexed_accessor_iterator &rhs) const {
-    return base == rhs.base && index == rhs.index;
+    assert(base == rhs.base && "incompatible iterators");
+    return index == rhs.index;
   }
   bool operator<(const indexed_accessor_iterator &rhs) const {
     assert(base == rhs.base && "incompatible iterators");

[dwblaikie]

Worth a go - you've run check-all with this & verified to a reasonable degree that there aren't existing violations of this invariant?

[River707]

LGTM, I don't remember this actually being a requirement in practice, it should be fine!

[andrey-golubev]

Worth a go - you've run check-all with this & verified to a reasonable degree that there aren't existing violations of this invariant?

I did run check-llvm && check-mlir locally under debug (so with assertions). Haven't seen anything suspicious (fwiw, it seems like this iterator type is only used by MLIR as well as the range type around this iterator). I do see some crash in check-llvm though:

# <path>/llvm-project/build/bin/llc -mtriple=x86_64-pc-linux-gnu -enable-new-pm -print-pipeline-passes -filetype=null <path>/llvm-project/llvm/test/tools/llc/new-pm/pipeline.ll

free(): invalid pointer

with stack trace pointing to:

#15 0x0000563920481835 llvm::MachineVerifierPass::~MachineVerifierPass() <path>/llvm-project/llvm/include/llvm/CodeGen/MachineVerifier.h:16:7
#16 0x000056392049510b llvm::detail::PassModel<llvm::MachineFunction, llvm::MachineVerifierPass, llvm::AnalysisManager<llvm::MachineFunction>>::~PassModel() <path>/llvm-project/llvm/include/llvm/IR/PassManagerInternal.h:71:8

but i don't think it's related?

also, could someone click merge for this one (I do not have merge rights)?