[libc++] Fix vector<const T>

[philnik777]

#80558 introduced code that assumed that the element type of vector is never const. This fixes it and adds a test. Eventually we should remove the allocator<const T> extension.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

#80558 introduced code that assumed that the element type of vector is nenver const. This fixes it and adds a test. Eventually we should remove the allocator<const T> extension,

---

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

2 Files Affected:

• (modified) libcxx/include/__memory/uninitialized_algorithms.h (+1-1)
• (added) libcxx/test/libcxx/containers/sequences/vector/const_T.compile.pass.cpp (+18)

diff --git a/libcxx/include/__memory/uninitialized_algorithms.h b/libcxx/include/__memory/uninitialized_algorithms.h
index 9733bb748f665..7e25a5c5fa19b 100644
--- a/libcxx/include/__memory/uninitialized_algorithms.h
+++ b/libcxx/include/__memory/uninitialized_algorithms.h
@@ -643,7 +643,7 @@ __uninitialized_allocator_relocate(_Alloc& __alloc, _Tp* __first, _Tp* __last, _
     __guard.__complete();
     std::__allocator_destroy(__alloc, __first, __last);
   } else {
-    __builtin_memcpy(__result, __first, sizeof(_Tp) * (__last - __first));
+    __builtin_memcpy(const_cast<__remove_const_t<_Tp>*>(__result), __first, sizeof(_Tp) * (__last - __first));
   }
 }
 
diff --git a/libcxx/test/libcxx/containers/sequences/vector/const_T.compile.pass.cpp b/libcxx/test/libcxx/containers/sequences/vector/const_T.compile.pass.cpp
new file mode 100644
index 0000000000000..62fff96ac5abe
--- /dev/null
+++ b/libcxx/test/libcxx/containers/sequences/vector/const_T.compile.pass.cpp
@@ -0,0 +1,18 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Make sure that `vector<const T>` works
+
+#include <vector>
+
+void test() {
+  std::vector<const int> v;
+  v.emplace_back(1);
+  v.push_back(1);
+  v.resize(3);
+}

[ldionne]

LGTM. I think we have an issue tracking that removal, and I might even have a patch locally.

[pranavk]

Can we merge this? The original commit has broken lot of our internal workloads and we would like to get this in to alleviate some pain.

[philnik777]

Please wait for the CI, and don't ask and then merge without an answer.

[pranavk]

Apologies.

I asked but then thought you may be off for the day (judging by your email ID)

I did check the CI before merging though. Everything was green except one workload which was stuck on "Waiting for the agent" for "FreeBSD 13" for last 7 hours. Since everything else was green, it seemed reasonable to go ahead with this one.

[ldionne]

@philnik777 It’s actually worrying that so many people have commit access to the monorepo. I just checked and about 1800 people have access — arguably just a small fraction of those should really need access now that we have github PRs.

[rnk]

To provide a different perspective, I am extremely happy to learn today that this landed yesterday, and we haven't lost a day's worth of test results buried under std::vector<const T> issues. It seems entirely reasonable to bypass standard procedure to recover from breakage, and then to reflect afterwards on what could have gone better, and to always assume positive intent.

[philnik777]

@rnk I don't think anyone here tried to imply none-positive intent here. I actually watched the CI to land it as soon as the CI is ready. It would have been at most another hour or two until it landed. To me it mostly seemed very weird to first ask whether the patch could be landed and then landing the patch a minute later. I'm not necessarily against landing a patch sooner if it would fix wide-spread breakage (I'm not sure whether this did - I only received a single report), but in this case it wouldn't have been that much longer until the CI was happy and I would have landed it myself. I also would have checked some "land as soon as the CI is green" tick mark if it existed. And FWIW I'm available most of the time during US office hours.

[ldionne]

To provide a different perspective, I am extremely happy to learn today that this landed yesterday, and we haven't lost a day's worth of test results buried under std::vector<const T> issues. It seems entirely reasonable to bypass standard procedure to recover from breakage, and then to reflect afterwards on what could have gone better, and to always assume positive intent.

I wasn’t assuming bad intent, I knew the intent was good. I just think it’s surprising that someone who we’ve never (to my recollection ) interacted with in the context of libc++ would have sufficient permissions to merge code into libc++ just like any regular maintainer, and bypass CI checks. I don’t think my surprise is an overreaction, I would argue that LLVM is an outlier in that respect.

In this case there’s no actual problem, the FreeBSD CI was fine to ignore and this simply returned things to green faster. My observation is a general one about how liberal we are with permissions and I do think that’s something we can improve on to make the project run smoother (even outside of security aspects).

[pranavk]

@philnik777 Thanks for explaining. I will just add more details to my previous comment in case I wasn't clear. I understand it may seem little weird to first ask and then merge but we were unaware of your working hours and if you were actively monitoring this. I commented on the PR to ask, then looked at your email ID (berlin.de), assumed you were in Berlin time (it was past midnight in Berlin at that time) and off for the day, and went ahead merging it after noticing this PR was created and approved 7+ hours ago at that time.

The original commit had quite a huge impact on us, and we couldn't afford waiting until normal Berlin office hours. Also, CI being mostly green and our internal testing on similar platforms gave me enough confidence to go ahead with this one. I hope that explains my rationale.