Skip to content

[libc][stdlib] Only use freelist_malloc for baremetal targets. #96355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

lntue
Copy link
Contributor

@lntue lntue commented Jun 21, 2024

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Jun 21, 2024

@llvm/pr-subscribers-libc

Author: None (lntue)

Changes

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

2 Files Affected:

  • (modified) libc/src/stdlib/CMakeLists.txt (+15-9)
  • (modified) libc/test/src/__support/CMakeLists.txt (+1-1)
diff --git a/libc/src/stdlib/CMakeLists.txt b/libc/src/stdlib/CMakeLists.txt
index 51d53e0d02ea2..7d99fdf38141a 100644
--- a/libc/src/stdlib/CMakeLists.txt
+++ b/libc/src/stdlib/CMakeLists.txt
@@ -380,18 +380,24 @@ elseif(LIBC_TARGET_OS_IS_GPU)
     aligned_alloc
   )
 else()
-  # Only add malloc in full build mode.  Use the system malloc in overlay mode.
-  if(LLVM_LIBC_FULL_BUILD)
+  # Only use freelist malloc for baremetal targets.
+  add_entrypoint_object(
+    freelist_malloc
+    SRCS
+      freelist_malloc.cpp
+    HDRS
+      malloc.h
+    DEPENDS
+      libc.src.__support.freelist_heap
+    COMPILE_OPTIONS
+      -DLIBC_FREELIST_MALLOC_SIZE=${LIBC_CONF_FREELIST_MALLOC_BUFFER_SIZE}
+  )
+  if(LIBC_TARGET_OS_IS_BAREMETAL)
     add_entrypoint_object(
       malloc
-      SRCS
-        freelist_malloc.cpp
-      HDRS
-        malloc.h
+      ALIAS
       DEPENDS
-        libc.src.__support.freelist_heap
-      COMPILE_OPTIONS
-        -DLIBC_FREELIST_MALLOC_SIZE=${LIBC_CONF_FREELIST_MALLOC_BUFFER_SIZE}
+        .freelist_malloc
     )
   else()
     add_entrypoint_external(
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index 082a002959c95..e224e82c922f0 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -36,7 +36,7 @@ if(LLVM_LIBC_FULL_BUILD)
     DEPENDS
       libc.src.__support.CPP.span
       libc.src.__support.freelist_heap
-      libc.src.stdlib.malloc
+      libc.src.stdlib.freelist_malloc
       libc.src.string.memcmp
       libc.src.string.memcpy
   )

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the problem I had was that the freelist_heap_test is triggered if you have malloc because the source just assumes that if you have malloc then it builds it. Can you also disable the test for now and in the future @PiJoules can move freelist_heap to an object library so we can check if it's built?

@lntue
Copy link
Contributor Author

lntue commented Jun 21, 2024

So, the problem I had was that the freelist_heap_test is triggered if you have malloc because the source just assumes that if you have malloc then it builds it. Can you also disable the test for now and in the future @PiJoules can move freelist_heap to an object library so we can check if it's built?

This PR changes freelist_heap_test to depend on libc.src.stdlib.freelist_malloc instead of libc.src.stdlib.malloc, which shouldn't be added for GPU. Can you double check if that's what it is? @jhuber6

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, reading comprehension.

@lntue lntue merged commit 09bc1e8 into llvm:main Jun 21, 2024
6 of 7 checks passed
@lntue lntue deleted the malloc branch June 21, 2024 20:38
@ilovepi
Copy link
Contributor

ilovepi commented Jun 21, 2024

I think we're seeing some CMake failures in our CI after this patch. Can you take a look, and if it will take a while to fix, revert.

Bot: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8744479865106393873/overview

Error:

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:250 (add_library):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:libc.src.stdlib.freelist_malloc>

  Objects of target "libc.src.stdlib.freelist_malloc" referenced but is not
  one of the allowed target types (EXECUTABLE, STATIC, SHARED, MODULE,
  OBJECT).
Call Stack (most recent call first):
  /b/s/w/ir/x/w/llvm-llvm-project/libc/lib/CMakeLists.txt:22 (add_entrypoint_library)


CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:250 (add_library):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:libc.src.stdlib.freelist_malloc>

  Objects of target "libc.src.stdlib.freelist_malloc" referenced but is not
  one of the allowed target types (EXECUTABLE, STATIC, SHARED, MODULE,
  OBJECT).
Call Stack (most recent call first):
  /b/s/w/ir/x/w/llvm-llvm-project/libc/lib/CMakeLists.txt:22 (add_entrypoint_library)

@lntue
Copy link
Contributor Author

lntue commented Jun 22, 2024

I think we're seeing some CMake failures in our CI after this patch. Can you take a look, and if it will take a while to fix, revert.

Bot: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8744479865106393873/overview

Error:

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:250 (add_library):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:libc.src.stdlib.freelist_malloc>

  Objects of target "libc.src.stdlib.freelist_malloc" referenced but is not
  one of the allowed target types (EXECUTABLE, STATIC, SHARED, MODULE,
  OBJECT).
Call Stack (most recent call first):
  /b/s/w/ir/x/w/llvm-llvm-project/libc/lib/CMakeLists.txt:22 (add_entrypoint_library)


CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:250 (add_library):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:libc.src.stdlib.freelist_malloc>

  Objects of target "libc.src.stdlib.freelist_malloc" referenced but is not
  one of the allowed target types (EXECUTABLE, STATIC, SHARED, MODULE,
  OBJECT).
Call Stack (most recent call first):
  /b/s/w/ir/x/w/llvm-llvm-project/libc/lib/CMakeLists.txt:22 (add_entrypoint_library)

It should be fixed with #96372. Can you double check with the bots? @ilovepi

@ilovepi
Copy link
Contributor

ilovepi commented Jun 24, 2024

Sorry, I didn't see this before signing off for the weekend. #96372 did seem to fix the issue in our CI. Thanks!

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants