Add tests for src/snmalloc/override/new.cc

[paulhdk]

First-time contributor taking a stab at #85.

Questions:

• The standard explicitly requests that new and delete don't introduce data races. Should we explicitly test that, e.g., with ThreadSanitizer?
• I noticed in passing that rust.cc does not have any tests. Is there a reason why there aren't any or
  should that be tracked in a separate issue?

closes: #85

[paulhdk]

@microsoft-github-policy-service agree

[mjp41]

Hi @pauhdk, thanks for taking this.

The standard explicitly requests that new and delete don't introduce data races. Should we explicitly test that, e.g., with ThreadSanitizer?

We run TSan in CI. So any concurrent test will get checked with TSan.

I noticed in passing that rust.cc does not have any tests. Is there a reason why there aren't any or
should that be tracked in a separate issue?

This should also be done, but a separate PR.

We still support building with C++17, so that has caused some failures in CI. Also, there should be a clangformat pass. It will build a target with clangformat that formats the code correctly. If you can't get that to work, the CI failure should display a patch.

[mjp41]

Thanks. I have made a few suggestions that will hopefully get it through CI.

The TSan build is failing due to duplicate symbols. We might have to disable this test on TSan sadly.

[paulhdk]

Thanks. I have made a few suggestions that will hopefully get it through CI.

Thank you! You beat me to it. Probably shouldn't have pushed before addressing all of your previous comments.

The TSan build is failing due to duplicate symbols. We might have to disable this test on TSan sadly.

So, no additional data race tests needed in that case?

[paulhdk]

Just pushed another fix, @mjp41.

I believe the Ubuntu builds are failing because they run with SNMALLOC_USE_SELF_VENDORED_STL=ON.

Not sure about the Windows builds.

[paulhdk]

Did some more testing, and I'm able to reproduce the CI failures locally. I stil believe that SNMALLOC_USE_SELF_VENDORED_STL=ON is at fault and that the RTTI of the thrown exception (self-vendored STL) and the expected exception (system STL) don't match, which is why the catch (std::bad_alloc&) falls through when SNMALLOC_USE_SELF_VENDORED_STL=ON is set.

An easy fix to pass the tests would be a plain catch(...) block, but that wouldn't solve the root cause.

Do you consider this a bug, @mjp41? What do you suggest we do?

[SchrodingerZhu]

Self-vendored STL is for compiled with freestanding so I don't expect it will have full exception handling facilities.

[mjp41]

Self-vendored STL is for compiled with freestanding so I don't expect it will have full exception handling facilities.

So should we disable this test if the self-vendored setting is on?

[SchrodingerZhu]

I think so.

[paulhdk]

PTAL