[libcxx-commits] [PATCH] D128146: [libc++] Use uninitialized algorithms for vector

Fri Jul 22 07:06:43 PDT 2022

ldionne added a comment.

In D128146#3669977 <https://reviews.llvm.org/D128146#3669977>, @augusto2112 wrote:

> It shouldn't be LLDB's responsibility to track down who is responsible after and it's not healthy for the project to xfail tests that are broken by one of its dependencies and investigate later on, which would just lead to more and more xfails.

That is only true if those XFAILs are not investigated and fixed in a timely fashion. And in that case, I would say there is a larger issue that nobody's responsible for fixing those. If we have bugs in Clang/LLDB, we should have folks ready to jump in and investigate them -- otherwise, **that** is what's unhealthy for the project.

In D128146#3670231 <https://reviews.llvm.org/D128146#3670231>, @augusto2112 wrote:

> Ok, I tested your change and removing the assert and it seems to work fine. It's not great that the assertion is failing though, the `ASTContext::getInjectedClassNameType` code hasn't been touched since 2010 so I doubt that it's broken (the assert checks the of the decl is an InjectedClassNameType. Before your change it's an `InjectedClassName` after your change it somehow becomes a `Record`, given the function is called `getInjectedClassNameType` that assert seems correct to me). If this is urgent you can xfail the test, and if you could include a bug report number with your xfail that'd be great.

The point we are trying to make is that the libc++ code itself is correct. The tests are passing, it's valid C++ and it does what it should. The fact that it happens to start crashing Clang is an issue, but it doesn't mean that we should prevent ourselves from using this construct. For instance, this is something that users in the wild could write themselves and it would crash Clang just the same. So instead of bending backwards or reverting the patch in libc++, the correct thing to do is to acknowledge that there's a Clang (or LLDB) bug and try to fix it as soon as possible. And in the meantime, to ensure that the CI stays meaningful, mark the test as `XFAIL`ing due to that bug.

If this were a minor unimportant patch, I wouldn't be spending so much time arguing -- we all have more important stuff to do. However, this happens to be an incredibly important patch if we want to add support for `constexpr std:::vector`, which we are aiming to land in time for LLVM 15.

In D128146#3671442 <https://reviews.llvm.org/D128146#3671442>, @hans wrote:

> In Chromium we noticed that this adds 65 MB of debug info to one of our binaries (we noticed because that pushed it over the 4GB limit, so we'll need to do something about that anyway).

Now, that is an actual issue that mandates our attention. Thanks for the heads up. I suspect it has to do with the fact that we instantiate more templates before we get to the `memmove` optimization in `std::copy`. @hans Do you have any way to tell what's in the 65 MB of debug information? Or a reproducer so we can try a few things out and see what impact it has on the size of the debug info? I assume this problem wouldn't be visible on a simple reproducer hand-written from scratch.

@philnik Let's re-land this with an XFAIL for the LLDB test and work with the Chromium folks to decrease the amount of debug information this creates. We can do this after the release and cherry-pick back the improvements.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128146