[libc] Added missing operator delete generated by gcc/clang #67457

mikhailramalho · 2023-09-26T17:04:08Z

This patch adds two operator delete functions that are being generated by clang 15 on rv32 (operator delete(void *mem, std::align_val_t)) and by gcc 13 on intel 64 (operator delete(void *mem, unsigned long)).

llvmbot · 2023-09-26T17:05:17Z

@llvm/pr-subscribers-libc

Changes

This patch adds two operator delete functions that are being generated by clang 15 on rv32 (operator delete(void *mem, std::align_val_t)) and by gcc 13 on intel 64 (operator delete(void *mem, unsigned long)).

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

1 Files Affected:

(modified) libc/test/UnitTest/HermeticTestUtils.cpp (+19)

diff --git a/libc/test/UnitTest/HermeticTestUtils.cpp b/libc/test/UnitTest/HermeticTestUtils.cpp
index 73d54b9eeb5ecc9..d4628091f30fac2 100644
--- a/libc/test/UnitTest/HermeticTestUtils.cpp
+++ b/libc/test/UnitTest/HermeticTestUtils.cpp
@@ -109,3 +109,22 @@ void operator delete(void *) {
   // we just trap here to catch any such accidental usages.
   __builtin_trap();
 }
+
+// Defining members in the std namespace is not preferred. But, we do it here
+// so that we can use it to define the operator new which takes std::align_val_t
+// argument.
+namespace std {
+enum class align_val_t : size_t {};
+} // namespace std
+
+void operator delete(void *mem, std::align_val_t) noexcept {
+  // The libc runtime should not use the global delete operator. Hence,
+  // we just trap here to catch any such accidental usages.
+  __builtin_trap();
+}
+
+void operator delete(void *mem, unsigned long) noexcept {
+  // The libc runtime should not use the global delete operator. Hence,
+  // we just trap here to catch any such accidental usages.
+  __builtin_trap();
+}

jhuber6

Wouldn't we also need this in the IntegrationTests?

mikhailramalho · 2023-09-26T17:11:58Z

Wouldn't we also need this in the IntegrationTests?

It's not an issue in the integration tests currently

jhuber6 · 2023-09-26T17:13:12Z

Wouldn't we also need this in the IntegrationTests?

It's not an issue in the integration tests currently

Do you happen to know why this appears here and not there? I remember there was a recent patch that made the version with the alignment the default somewhere. Maybe we don't have any tests in integration that call delete?

mikhailramalho · 2023-09-28T13:29:03Z

Do you happen to know why this appears here and not there?

I'm still investigating, but it seems that the missing references are coming from the test classes, like StrtoTest, LlvmLibcStrtoimaxTest, etc.:

/usr/bin/ld: projects/libc/test/src/inttypes/CMakeFiles/libc.test.src.inttypes.strtoimax_test.__hermetic__.__build__.dir/strtoimax_test.cpp.o: in function `__llvm_libc_18_0_0_git::testing::Test::~Test()':
/home/mgadelha/tools/llvm-project/libc/test/UnitTest/LibcTest.h:112: undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: projects/libc/test/src/inttypes/CMakeFiles/libc.test.src.inttypes.strtoimax_test.__hermetic__.__build__.dir/strtoimax_test.cpp.o: in function `StrtoTest<long>::~StrtoTest()':
/home/mgadelha/tools/llvm-project/libc/test/src/stdlib/StrtolTest.h:29: undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: projects/libc/test/src/inttypes/CMakeFiles/libc.test.src.inttypes.strtoimax_test.__hermetic__.__build__.dir/strtoimax_test.cpp.o: in function `LlvmLibcStrtoimaxTest_AutomaticBaseSelection::~LlvmLibcStrtoimaxTest_AutomaticBaseSelection()':
/home/mgadelha/tools/llvm-project/libc/test/src/inttypes/strtoimax_test.cpp:15: undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: projects/libc/test/src/inttypes/CMakeFiles/libc.test.src.inttypes.strtoimax_test.__hermetic__.__build__.dir/strtoimax_test.cpp.o: in function `LlvmLibcStrtoimaxTest_MessyBaseSixteenDecode::~LlvmLibcStrtoimaxTest_MessyBaseSixteenDecode()':
/home/mgadelha/tools/llvm-project/libc/test/src/inttypes/strtoimax_test.cpp:15: undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: projects/libc/test/src/inttypes/CMakeFiles/libc.test.src.inttypes.strtoimax_test.__hermetic__.__build__.dir/strtoimax_test.cpp.o: in function `LlvmLibcStrtoimaxTest_CleanBaseSixteenDecode::~LlvmLibcStrtoimaxTest_CleanBaseSixteenDecode()':
/home/mgadelha/tools/llvm-project/libc/test/src/inttypes/strtoimax_test.cpp:15: undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: projects/libc/test/src/inttypes/CMakeFiles/libc.test.src.inttypes.strtoimax_test.__hermetic__.__build__.dir/strtoimax_test.cpp.o:/home/mgadelha/tools/llvm-project/libc/test/src/inttypes/strtoimax_test.cpp:15: more undefined references to `operator delete(void*, unsigned long)' follow

I remember there was a recent patch that made the version with the alignment the default somewhere. Maybe we don't have any tests in integration that call delete?

I think @sivachandra mentioned on discord that changes were not required in the end. I can't find any commit that mentions the operator delete

mikhailramalho · 2023-10-02T19:50:21Z

These operator delete(void*, unsigned long) calls are only being generated for test classes with arrays in their run() methods, e.g.:

bsearch_test.cpp:

TEST(LlvmLibcBsearchTest, IntegerArray) {
  constexpr int ARRAY[25] = {10,   23,   33,    35,   55,   70,   71,
                             100,  110,  123,   133,  135,  155,  170,
                             171,  1100, 1110,  1123, 1133, 1135, 1155,
                             1170, 1171, 11100, 12310};

qsort_r_test.cpp:

TEST(LlvmLibcQsortRTest, ReverseSortedArray) {
  int array[25] = {25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13,
                   12, 11, 10, 9,  8,  7,  6,  5,  4,  3,  2,  1};

bitset_test.cpp:

TEST(LlvmLibcBitsetTest, SettingBitXDoesNotResetBitY) {
  LIBC_NAMESPACE::cpp::bitset<128> bitset;

mikhailramalho · 2023-10-02T19:51:07Z

I also just noticed a new warning by GCC 13:

libc/test/UnitTest/HermeticTestUtils.cpp:107:6: warning: the program should also define ‘void operator delete(void*, long unsigned int)’ [-Wsized-deallocation]
  107 | void operator delete(void *) {
      |      ^~~~~~~~

This patch adds two operator delete functions that are being generated by clang 15 on rv32 (operator delete(void *mem, std::align_val_t)) and by gcc 13 on intel 64 (operator delete(void *mem, unsigned long)).

mikhailramalho · 2024-07-11T17:21:00Z

ping

We need this patch to enable fullbuild for rv32

jhuber6

You likely need to add these to the integration test.cpp as well.

mikhailramalho · 2024-07-11T17:39:10Z

You likely need to add these to the integration test.cpp as well.

integration tests build even without this patch

jhuber6 · 2024-07-11T17:40:09Z

You likely need to add these to the integration test.cpp as well.

integration tests build even without this patch

Probably because they don't hit the same code paths, but I feel like we should do it just in case.

mikhailramalho · 2024-07-11T18:02:02Z

Where do you want this added exactly? I can only find ./test/integration/scudo/integration_test.cpp in libc's codebase. Em qui., 11 de jul. de 2024 às 14:40, Joseph Huber ***@***.***> escreveu:

…

You likely need to add these to the integration test.cpp as well. integration tests build even without this patch Probably because they don't hit the same code paths, but I feel like we should do it just in case. — Reply to this email directly, view it on GitHub <#67457 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKEJHYJC5MGN45PWXWIVS3ZL27Q7AVCNFSM6AAAAABKNEDSJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRTGUYTOMJUGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Mikhail R. Gadelha, Ph.D.

mikhailramalho · 2024-07-15T18:49:09Z

You likely need to add these to the integration test.cpp as well.

integration tests build even without this patch

Probably because they don't hit the same code paths, but I feel like we should do it just in case.

Do yo mean

llvm-project/libc/test/IntegrationTest/test.cpp

Line 4 in 148d907

// See https://llvm.org/LICENSE.txt for license information.

?

I still prefer not to add code unless it's needed. Will you block the PR if I don't add the code there?

jhuber6 · 2024-07-15T18:59:13Z

You likely need to add these to the integration test.cpp as well.

integration tests build even without this patch

Probably because they don't hit the same code paths, but I feel like we should do it just in case.

Do yo mean

llvm-project/libc/test/IntegrationTest/test.cpp

Line 4 in 148d907

// See https://llvm.org/LICENSE.txt for license information.

?

I still prefer not to add code unless it's needed. Will you block the PR if I don't add the code there?

Honestly, these files are practically the same so we could probably just merge them into a single object library or something.

jhuber6

The integration test stuff doesn't have any of these operators, so I guess it's moot. That being said, this is pretty much just a wrapper to work in a hermetic environment given LIBC_NAMESPACE qualified identifiers, we could merge them in the future.

Summary: This patch adds two operators delete that are being generated by clang 15 on rv32 (operator delete(void *mem, std::align_val_t)) and by gcc 13 on intel 64 (operator delete(void *mem, unsigned long)). Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251538

mikhailramalho requested review from sivachandra and michaelrj-google September 26, 2023 17:04

llvmbot added the libc label Sep 26, 2023

jhuber6 reviewed Sep 26, 2023

View reviewed changes

[libc] Added missing operator deletes generated by gcc/clang

6dd3fd7

This patch adds two operator delete functions that are being generated by clang 15 on rv32 (operator delete(void *mem, std::align_val_t)) and by gcc 13 on intel 64 (operator delete(void *mem, unsigned long)).

mikhailramalho force-pushed the libc-operator-delete branch from 2d0e2f7 to 6dd3fd7 Compare July 5, 2024 12:35

mikhailramalho requested a review from lntue July 5, 2024 12:36

lntue requested a review from jhuber6 July 5, 2024 13:53

jhuber6 reviewed Jul 11, 2024

View reviewed changes

jhuber6 approved these changes Jul 15, 2024

View reviewed changes

mikhailramalho merged commit 73799b4 into llvm:main Jul 17, 2024
6 checks passed

mikhailramalho deleted the libc-operator-delete branch July 17, 2024 15:20

[libc] Added missing operator delete generated by gcc/clang #67457

[libc] Added missing operator delete generated by gcc/clang #67457

Uh oh!

Conversation

mikhailramalho commented Sep 26, 2023

Uh oh!

llvmbot commented Sep 26, 2023

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

mikhailramalho commented Sep 26, 2023

Uh oh!

jhuber6 commented Sep 26, 2023

Uh oh!

mikhailramalho commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikhailramalho commented Oct 2, 2023

Uh oh!

mikhailramalho commented Oct 2, 2023

Uh oh!

mikhailramalho commented Jul 11, 2024

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

mikhailramalho commented Jul 11, 2024

Uh oh!

jhuber6 commented Jul 11, 2024

Uh oh!

mikhailramalho commented Jul 11, 2024 via email

Uh oh!

mikhailramalho commented Jul 15, 2024

Uh oh!

jhuber6 commented Jul 15, 2024

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikhailramalho commented Sep 28, 2023 •

edited

Loading