[libcxx-commits] [libcxx] [libc++] Fix missing 'get_new_handler()' for Windows builds (PR #150182)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> > > > If we do this it definitely requires a release note. That should include what implications this change has w.r.t. STL compatibility.
> > > 
> > > 
> > > @philnik777 care to elaborate more on this? which part of these changes might have implications in regards to compatibility?
> > 
> > 
> > How did this interact with the MSVC STL until now? How does the behaviour change? Why did we not make it compatible with MSVC? All the questions a potential ABI break implies.
> > Looking at this all again, maybe the most important question is "Is this even conforming?". Which `::operator new` do we actually use on Windows and does that use our implementation of `{s,g}et_new_handler`? Looking at the code the answer seems to be "not ours", which would make us non-conforming with this patch. Do we have a test to ensure that the new handler is called when trying to allocate too much memory?
> 
> **In regards to compatibility:**
> 
>     * AFAIK the functionality implemented in [`new.cpp`](https://github.com/llvm/llvm-project/blob/main/libcxx/src/new.cpp#L16) is not even used for `VCRuntime`, therefore there shouldn't be any ABI break. and the header change to use `#include <vcruntime_new.h>` is on par with MSVC STL since it also avoids including `<new.h>`, and we avoid using it because that would result in redeclaration warnings (caused by the one function that is exposed by the UCRT `set_new_handler`).
> 
>     * This patch relies on UCRT functionality which is in line with MSVC STL, and that also means our handler is being called correctly, I've even debugged it using `WinDbg` to make sure it is working correctly while implementing this change.
> 
> 
> So in fact, this patch actually makes it compliant by implementing everything as it was supposed to be.

I didn't realize this patch called MSVC-internal functions before. Now this patch makes a lot more sense.

> **In regards to testing:**
> 
> Currently the only available [test](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp#L16) is expected to fail (which is not conforming), this patch fixes the functionality and now the test succeeds, as a result I've removed the `XFAIL` in that test.
> 
> As for testing if the handler is being called correctly, I'm not aware of any existing tests for such behaviour. However, I have tested it locally in different ways by triggering an impossibly large allocation using `new`:
> 
>     * by setting and retrieving the handler.
> 
>     * by alternating two different handlers repeatedly in a loop.
> 
> 
> But I don't think that this PR should add such tests, they could be added in another PR if needed.

This patch modifies code related to `new`, so this patch should make sure the testing is adequate. Especially since you claim to already have tested it, I don't see why we should separate testing from implementation.

> I hope I have addressed your concerns and recapped some of the reasoning behind this PR, since I ended up implementing more than what was described in the description of the PR in the process of addressing the review comments.

Yes, this addresses my concerns for the most part. What I'd like to see now is extended test coverage. I'd also like to wait with landing this PR until after the release branch, since it's modifying quite low-level stuff and I'd like to have this bake in trunk for a while.

https://github.com/llvm/llvm-project/pull/150182