[flang-commits] [PATCH] D88793: [flang] Fix assumptions on std::forward_list iterator implementation.

[Michael Kruse via Phabricator via flang-commits]

Meinersbur created this revision.
Meinersbur added reviewers: isuruf, DavidTruby, sscalpone, tskeith, klausler.
Meinersbur added a project: Flang.
Herald added a reviewer: jdoerfert.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Meinersbur requested review of this revision.

The code in the messages class makes several assumptions how `std::forward_list` is implemented which are not guaranteed by the standard. It fails, e.g. with Microsoft's STL <https://github.com/microsoft/STL> implementation.

1. Use of container A's iterator for container B after a move or merge of elements of A into B. This is not allowed for any STL container.
2. Use of B's before_begin sentinel iterator for A after B has been moved-assigned to A. Like the other iterators, the before_begin iterator is specific their container. MS STL's implementation is probably the only correct here since the before_begin iterator has no precondition an should still be valid after its content has been replaced by a move-assignment.

Remove dependence on implementation details by restoring the `last_` from the container's own iterators.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88793

Files:
  flang/include/flang/Parser/message.h
  flang/lib/Parser/message.cpp


Index: flang/lib/Parser/message.cpp
===================================================================
--- flang/lib/Parser/message.cpp
+++ flang/lib/Parser/message.cpp
@@ -262,7 +262,7 @@
 
 void Messages::clear() {
   messages_.clear();
-  ResetLastPointer();
+  RestoreLast();
 }
 
 bool Messages::Merge(const Message &msg) {
@@ -289,7 +289,7 @@
         ++last_;
       }
     }
-    that.ResetLastPointer();
+    that.RestoreLast();
   }
 }
 
Index: flang/include/flang/Parser/message.h
===================================================================
--- flang/include/flang/Parser/message.h
+++ flang/include/flang/Parser/message.h
@@ -214,19 +214,13 @@
 public:
   Messages() {}
   Messages(Messages &&that) : messages_{std::move(that.messages_)} {
-    if (!messages_.empty()) {
-      last_ = that.last_;
-      that.ResetLastPointer();
-    }
+    RestoreLast();
+    that.RestoreLast();
   }
   Messages &operator=(Messages &&that) {
     messages_ = std::move(that.messages_);
-    if (messages_.empty()) {
-      ResetLastPointer();
-    } else {
-      last_ = that.last_;
-      that.ResetLastPointer();
-    }
+    RestoreLast();
+    that.RestoreLast();
     return *this;
   }
 
@@ -240,11 +234,9 @@
   }
 
   void Annex(Messages &&that) {
-    if (!that.messages_.empty()) {
-      messages_.splice_after(last_, that.messages_);
-      last_ = that.last_;
-      that.ResetLastPointer();
-    }
+    messages_.splice_after(last_, that.messages_);
+    RestoreLast();
+    that.RestoreLast();
   }
 
   void Restore(Messages &&that) {
@@ -262,7 +254,15 @@
   bool AnyFatalError() const;
 
 private:
-  void ResetLastPointer() { last_ = messages_.before_begin(); }
+  void RestoreLast() {
+    last_ = messages_.before_begin();
+    auto next{messages_.begin()};
+    auto end{messages_.end()};
+    while (next != end) {
+      last_ = next;
+      ++next;
+    }
+  }
 
   std::forward_list<Message> messages_;
   std::forward_list<Message>::iterator last_{messages_.before_begin()};


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88793.296024.patch
Type: text/x-patch
Size: 2008 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/flang-commits/attachments/20201004/4565529c/attachment.bin>