[libcxx] Improve libcxx tests when using Optimizations. #88897

Stylie777 · 2024-04-16T14:05:29Z

Within the libcxx tests, some tests compare a pointer to a Global Variable to one that is stored locally. At high levels of optmization the pointers are the same, but the assert is failing as the compiler cannot properly compare them.

This changes how these variables are defined in specific tests to ensure the asserts pass. This is done by

Updating a variant of DoNotOptimize used to accept L value references. This is a different variant that is currently used to ensure that the compiler cannot follow the data-flow through it by using both the input and output from the inline asm
Add in missing DoNotOptimize calls to tests. When using -O2 optimisations, these are failing because the pointer comparison fails when both values are the same. Adding the function call fixes this.

No change to the tests functionality is included.

llvmbot · 2024-04-16T14:06:00Z

@llvm/pr-subscribers-libcxx

Author: Jack Styles (Stylie777)

Changes

Within the libcxx tests, some tests compare a pointer to a Global Variable to one that is stored locally. This can cause issues where, both are the same value, but the assert is failing as the compiler cannot properly compare them.

This changes how these variables are defined to specific tests to ensure the asserts pass. No change to the tests functionality is included.

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

6 Files Affected:

(modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp (+2-1)
(modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp (+20-5)
(modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align_nothrow.replace.indirect.pass.cpp (+2-1)
(modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align.replace.pass.cpp (+20-5)
(modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.indirect.pass.cpp (+2-1)
(modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.pass.cpp (+20-5)

diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp
index 0b540e09bab3cc..1ccf5501bcea33 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp
@@ -51,7 +51,8 @@ int main(int, char**) {
     // Test with an overaligned type
     {
         new_called = delete_called = 0;
-        OverAligned* x = DoNotOptimize(new OverAligned[3]);
+        OverAligned* dummy_data_block = new (std::nothrow) OverAligned;
+        OverAligned* x = DoNotOptimize(dummy_data_block);
         ASSERT_WITH_OPERATOR_NEW_FALLBACKS(static_cast<void*>(x) == DummyData);
         ASSERT_WITH_OPERATOR_NEW_FALLBACKS(new_called == 1);
 
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp
index 2d021ecb30e793..bdfed1bd8aae18 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp
@@ -27,16 +27,31 @@
 #include "test_macros.h"
 #include "../types.h"
 
+class Data{
+public:
+    Data() = default;
+    ~Data() = default;
+
+    char *getDummyData() {
+        return this->dummy_data_;
+    }
+
+    std::size_t getDummyDataSize() {
+        return sizeof(this->dummy_data_);
+    }
+private:
+    alignas(OverAligned) char dummy_data_[alignof(OverAligned) * 3];
+};
+
 int new_called = 0;
 int delete_called = 0;
-
-alignas(OverAligned) char DummyData[alignof(OverAligned) * 3];
+Data data_class;
 
 void* operator new[](std::size_t s, std::align_val_t a) {
-    assert(s <= sizeof(DummyData));
+    assert(s <= data_class.getDummyDataSize());
     assert(static_cast<std::size_t>(a) == alignof(OverAligned));
     ++new_called;
-    return DummyData;
+    return data_class.getDummyData();
 }
 
 void operator delete[](void*, std::align_val_t) noexcept {
@@ -49,7 +64,7 @@ int main(int, char**) {
     {
         new_called = delete_called = 0;
         OverAligned* x = new OverAligned[3];
-        assert(static_cast<void*>(x) == DummyData);
+        assert(static_cast<void*>(x) == data_class.getDummyData());
         assert(new_called == 1);
 
         delete[] x;
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align_nothrow.replace.indirect.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align_nothrow.replace.indirect.pass.cpp
index 227b20f0b1e18b..6c12a9b65f129a 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align_nothrow.replace.indirect.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align_nothrow.replace.indirect.pass.cpp
@@ -55,7 +55,8 @@ int main(int, char**) {
     // Test with an overaligned type
     {
         new_called = delete_called = 0;
-        OverAligned* x = DoNotOptimize(new (std::nothrow) OverAligned[3]);
+        OverAligned* dummy_data_block = new (std::nothrow) OverAligned[3];
+        OverAligned* x = DoNotOptimize(dummy_data_block);
         ASSERT_WITH_OPERATOR_NEW_FALLBACKS(static_cast<void*>(x) == DummyData);
         ASSERT_WITH_OPERATOR_NEW_FALLBACKS(new_called == 1);
 
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align.replace.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align.replace.pass.cpp
index e5ef5f1669752b..254b864a74341f 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align.replace.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align.replace.pass.cpp
@@ -27,16 +27,31 @@
 #include "test_macros.h"
 #include "../types.h"
 
+class Data{
+public:
+    Data() = default;
+    ~Data() = default;
+
+    char *getDummyData() {
+        return this->dummy_data_;
+    }
+
+    std::size_t getDummyDataSize() {
+        return sizeof(this->dummy_data_);
+    }
+private:
+    alignas(OverAligned) char dummy_data_[alignof(OverAligned)];
+};
+
 int new_called = 0;
 int delete_called = 0;
-
-alignas(OverAligned) char DummyData[alignof(OverAligned)];
+Data data_class;
 
 void* operator new(std::size_t s, std::align_val_t a) {
-    assert(s <= sizeof(DummyData));
+    assert(s <= data_class.getDummyDataSize());
     assert(static_cast<std::size_t>(a) == alignof(OverAligned));
     ++new_called;
-    return DummyData;
+    return data_class.getDummyData();
 }
 
 void operator delete(void*, std::align_val_t) noexcept {
@@ -49,7 +64,7 @@ int main(int, char**) {
     {
         new_called = delete_called = 0;
         OverAligned* x = new OverAligned;
-        assert(static_cast<void*>(x) == DummyData);
+        assert(static_cast<void*>(x) == data_class.getDummyData());
         assert(new_called == 1);
 
         delete x;
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.indirect.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.indirect.pass.cpp
index 7eab0729f9ef1a..df0c454ecd3e17 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.indirect.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.indirect.pass.cpp
@@ -54,7 +54,8 @@ int main(int, char**) {
     // Test with an overaligned type
     {
         new_called = delete_called = 0;
-        OverAligned* x = DoNotOptimize(new (std::nothrow) OverAligned);
+        OverAligned* dummy_data_block = new (std::nothrow) OverAligned;
+        OverAligned* x = DoNotOptimize(dummy_data_block);
         ASSERT_WITH_OPERATOR_NEW_FALLBACKS(static_cast<void*>(x) == DummyData);
         ASSERT_WITH_OPERATOR_NEW_FALLBACKS(new_called == 1);
 
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.pass.cpp
index 9a5b53e039025c..5e7f2523de4b10 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.pass.cpp
@@ -26,16 +26,31 @@
 #include "test_macros.h"
 #include "../types.h"
 
+class Data{
+public:
+    Data() = default;
+    ~Data() = default;
+
+    char *getDummyData() {
+        return this->dummy_data_;
+    }
+
+    std::size_t getDummyDataSize() {
+        return sizeof(this->dummy_data_);
+    }
+private:
+    alignas(OverAligned) char dummy_data_[alignof(OverAligned)];
+};
+
 int new_nothrow_called = 0;
 int delete_called = 0;
-
-alignas(OverAligned) char DummyData[alignof(OverAligned)];
+Data data_class;
 
 void* operator new(std::size_t s, std::align_val_t a, std::nothrow_t const&) noexcept {
-    assert(s <= sizeof(DummyData));
+    assert(s <= data_class.getDummyDataSize());
     assert(static_cast<std::size_t>(a) == alignof(OverAligned));
     ++new_nothrow_called;
-    return DummyData;
+    return data_class.getDummyData();
 }
 
 void operator delete(void*, std::align_val_t) noexcept {
@@ -48,7 +63,7 @@ int main(int, char**) {
     {
         new_nothrow_called = delete_called = 0;
         OverAligned* x = new (std::nothrow) OverAligned;
-        assert(static_cast<void*>(x) == DummyData);
+        assert(static_cast<void*>(x) == data_class.getDummyData());
         ASSERT_WITH_OPERATOR_NEW_FALLBACKS(new_nothrow_called == 1);
 
         delete x;

github-actions · 2024-04-16T14:08:20Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ldionne

Sorry if I am missing something obvious, but I basically don't understand this change. You mention that "the compiler cannot properly compare the values", what do you mean by that?

What issue are you encountering and with what configuration? Can you share a reproducer showcasing the issue?

ldionne · 2024-04-16T14:48:53Z

...support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp

@@ -51,7 +51,8 @@ int main(int, char**) {
    // Test with an overaligned type
    {
        new_called = delete_called = 0;
-        OverAligned* x = DoNotOptimize(new OverAligned[3]);
+        OverAligned* dummy_data_block = new (std::nothrow) OverAligned;


This change uses the nothrow variant of operator new, but that is not what we are trying to test here.

Updated - Copy Paste Error.

ldionne · 2024-04-16T14:49:20Z

...language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp

@@ -27,16 +27,31 @@
 #include "test_macros.h"
 #include "../types.h"

+class Data{


I don't understand why this change is necessary.

See #88897 (comment)

Stylie777 · 2024-04-17T09:47:35Z

Sorry if I am missing something obvious, but I basically don't understand this change. You mention that "the compiler cannot properly compare the values", what do you mean by that?

@ldionne Apologies, not sure I explained it very well there.

The issue that has been observed is that with certain configurations when using C++17, when comparing the pointers of x and DummyData, they result was coming out as false when the pointers are identical (see below). This was occurring on 6 tests, 3 of which can be remedied by utilising a different DoNotOptimize function that uses the input and output from the inline asm. The other three I used the class to define the DummyData, as then this removes the issues that can arise from comparing a global pointer to that of a local one.

Example:

0000000000060940 <- `x`
0000000000060940 <- `DummyData`
0 <- (x == DummyData)

This is running on AArch64 V8 with the following compile options: --target=aarch64-none-elf -march=armv8 -std=c++17 -O2

jthackray

LGTM, once you've clang-formatted the code.

Stylie777 · 2024-04-17T14:16:40Z

Updated. Ran Clang-Format

ldionne · 2024-04-22T13:37:43Z

...support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp

@@ -51,7 +51,8 @@ int main(int, char**) {
    // Test with an overaligned type
    {
        new_called = delete_called = 0;
-        OverAligned* x = DoNotOptimize(new OverAligned[3]);
+        OverAligned* dummy_data_block = new OverAligned;


This is still incorrect. This test is trying to test operator new[], but you are not calling it anymore. You are calling the non-array version.

ldionne · 2024-04-22T13:40:11Z

...language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp

+  std::size_t getDummyDataSize() { return sizeof(this->dummy_data_); }
+
+private:
+  alignas(OverAligned) char dummy_data_[alignof(OverAligned) * 3];


I don't understand why indirecting through a class solves anything here. After all you're still returning the address of a global variable.

I have removed the classes and reverted to the initial behaviour. Please see the initial comment as the changes made are different to those previously reviewed. Thanks

ldionne · 2024-04-25T14:31:34Z

...support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.pass.cpp

@@ -47,7 +47,7 @@ int main(int, char**) {
    // Test with an overaligned type
    {
        new_nothrow_called = delete_called = 0;
-        OverAligned* x = new (std::nothrow) OverAligned;
+        OverAligned* x                     = DoNotOptimize(new (std::nothrow) OverAligned);


Ah! I understand this part of the change -- that looks like an oversight to me. However, I don't understand why you need to add a rvalue-reference overload to the function below, it seems to me that the const& overload should handle this correctly, no?

The change to &&value was not the correct change, so have reverted this back to &value for DoNotOptimize.

In these test cases, we have found that the const& overload was not allowing for comparison of the pointers correctly, even when the values were the same. See explanation from @ostannard below:

"think that's because DoNotOptimize only uses the value as an input. This prevents the compiler from removing and data-flow leading up to it, but it can still "see" that one pointer came from new, and the other came from the address of a global, so they "can't" alias
This works for me, using both input and output from the inline asm, so the compiler can't follow the data-flow through it:"

template <class Tp> inline Tp DoNotOptimize(Tp value) { asm volatile("" : "+r,m"(value) : : "memory"); return value; }

I have now scaled back the changes to only apply to the tests rather than change the function itself.

ldionne · 2024-04-26T13:50:35Z

...support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp

@@ -51,7 +51,8 @@ int main(int, char**) {
    // Test with an overaligned type
    {
        new_called = delete_called = 0;
-        OverAligned* x = DoNotOptimize(new OverAligned[3]);
+        OverAligned* dummy_data_block = new OverAligned[3];


Do you really still see the bug if you change all of those back to OverAligned* x = DoNotOptimize(new ...)?

It seems to me that the only necessary part of this patch is adding the missing DoNotOptimize calls in libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align_nothrow.replace.pass.cpp, libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp and libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.size_align.replace.pass.cpp.

Yes we do - hence the change in these tests to a different variant of the DoNotOptimize function.

Then let's delete dummy_data_block instead of x below.

As part of the libcxx tests, there is a function provided called `DoNotOptimize`. The tests currently utilise a version which only uses the value as an input, leading to the compiler not removing information relating to the data-flow for the value. This leads to issues when comparing the pointer values, where one is a global pointer and the other a pointer which is local to the function being executed. Another version of `DoNotOptimize` is available which allows for the value to be an input and output of the inline assembly, which allows for the pointers to be successfully compared to one another. This function however only accepts an L Value reference, where the values are initialized as R value reference, so use the variant of `DoNotOptmize` described in the first paragraph. To enable the use of the second variant in the tests, the type of reference has been updated within the test to be an L Value reference, to correctly utlize the correct variant of `DoNotOptimize` that allows for successful comparison of the pointers.

…opriate It has been seen, at high levels of optimizations, issues with comparing the pointer values in some libcxx tests attaining to the assert comparing the pointer of a global variable to that of one created locally within the test. These tests follow the same logic as others that utilize the `DoNotOptimize` function, however these tests do not. To ensure the correct behaviour at high levels of optimization, these tests now utilize the function when building the tests. This allows for the correct behaviour to be observed when running the tests.

Stylie777 · 2024-04-30T06:04:21Z

Thanks for merging @ldionne and thank you for the review. Much appreciated!

Stylie777 requested a review from a team as a code owner April 16, 2024 14:05

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 16, 2024

ldionne requested changes Apr 16, 2024

View reviewed changes

jthackray self-requested a review April 16, 2024 14:56

jthackray approved these changes Apr 17, 2024

View reviewed changes

Stylie777 force-pushed the improve_libcxx_tests branch from e32c591 to f765ebe Compare April 17, 2024 12:43

ldionne requested changes Apr 22, 2024

View reviewed changes

Stylie777 force-pushed the improve_libcxx_tests branch from f765ebe to 48a1b8e Compare April 24, 2024 14:52

Stylie777 changed the title ~~[libcxx] Improve handling of DummyData blocks in libcxx tests~~ [libcxx] Improve libcxx tests when using Optimizations. Apr 24, 2024

ldionne requested changes Apr 25, 2024

View reviewed changes

Stylie777 force-pushed the improve_libcxx_tests branch 2 times, most recently from 384ef88 to b5f2d77 Compare April 26, 2024 09:58

ldionne reviewed Apr 26, 2024

View reviewed changes

Stylie777 added 3 commits April 29, 2024 09:08

[NFC] Formatting Changes

ea7a7f2

Stylie777 force-pushed the improve_libcxx_tests branch from b5f2d77 to ea7a7f2 Compare April 29, 2024 08:35

ldionne approved these changes Apr 29, 2024

View reviewed changes

ldionne merged commit c3598b1 into llvm:main Apr 29, 2024

[libcxx] Improve libcxx tests when using Optimizations. #88897

[libcxx] Improve libcxx tests when using Optimizations. #88897

Uh oh!

Conversation

Stylie777 commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 16, 2024

Uh oh!

github-actions bot commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stylie777 commented Apr 17, 2024

Uh oh!

jthackray left a comment

Choose a reason for hiding this comment

Uh oh!

Stylie777 commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stylie777 commented Apr 30, 2024

Uh oh!

Uh oh!

Stylie777 commented Apr 16, 2024 •

edited

Loading

github-actions bot commented Apr 16, 2024 •

edited

Loading