[libcxx-commits] [PATCH] D107712: Fix possible infinite loop in itanium demangler

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks a lot for looking into this! When you re-upload the patch, can you please do it with `arc diff` so that context is included in the diff? See https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line for instructions.



================
Comment at: libcxxabi/test/test_demangle.pass.cpp:29847
 
+    // This should be invalid, but it is currently not recognized as such
+    // See https://bugs.llvm.org/show_bug.cgi?id=51407
----------------
We need to add this at the top of this file:

```
// https://llvm.org/PR51407 was not fixed in some previously-released demanglers, which
// causes them to run into the infinite loop.
// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx11.0
```

Otherwise, we'll try to run the test on those configurations and we'll run into the infinite loop. Incidentally, this just stalled the CI for 24 hours (I'll put a timeout on our tests).


================
Comment at: libcxxabi/test/test_demangle.pass.cpp:29848
+    // This should be invalid, but it is currently not recognized as such
+    // See https://bugs.llvm.org/show_bug.cgi?id=51407
+    {"_Zcv1BIRT_EIS1_E", "operator B<><>"},
----------------
Please normalize to https://llvm.org/PR51407.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:112-229
+template <class T, size_t N>
+class PODSmallVector {
+  static_assert(std::is_pod<T>::value,
+                "T is required to be a plain old data type");
+
+  T* First = nullptr;
+  T* Last = nullptr;
----------------
borman wrote:
> I could submit this change as a NFC to clean up the actual patch; however the move itself would be out of context.
I would rather the move be performed as a separate NFC. The commit message can mention it's for a follow-up change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107712