Page MenuHomePhabricator

[libc] add malloc funcs as external entrypoints
ClosedPublic

Authored by michaelrj on Oct 19 2021, 4:13 PM.

Details

Summary

malloc, calloc, realloc, and free are all functions that other libc
functions depend on, but are pulled from external sources, instead of
having an internal implementation. This patch adds a way to include
functions like that as entrypoints in the list of external entrypoints,
and includes the malloc functions using this new path.

Diff Detail

Event Timeline

michaelrj created this revision.Oct 19 2021, 4:13 PM
michaelrj requested review of this revision.Oct 19 2021, 4:13 PM

While this is OK logically, I would like to take a step back and design the external entrypoint concept in a more cleaner fashion. With this change as is, there will be an external entrypoint rule and there is also an EXT_DEPS option to add_entrypoint_library. We should unify these. What I mean is the following steps:

  1. Make add_external_entrypoint have a DEPENDS option which captures the external deps. We should move the SCUDO related logic from lib/CMakeLists.txt to src/stdlib/CMakeLists.txt and list the SCUDO entrypoints there with the appropriate DEPENDS list..
  2. Remove EXT_DEPS option to add_entrypoint_library and list the external entrypoint deps as part of the normal DEPENDS option.
  3. add_entrypoint_library should recognize that a DEPENDS entry is an external entrypoint and get the objects appropriately.
  4. We will not need any special handling in libc/CMakeLists.txt as in being done in this change. Also, the external entrypoints can be listed like any other entrypoint in the entrypoints.txt files.
libc/src/stdlib/CMakeLists.txt
184

I think these targets should be added conditionally like this:

if(LLVM_LIBC_INCLUDE_SCUDO)
  ...
endif()

I think I remember the original plan in this case was to use redirectors. We have add_redirector_object, but I don't think it has ever been used.

I think I remember the original plan in this case was to use redirectors. We have add_redirector_object, but I don't think it has ever been used.

Redirectors were for a different use case: When LLVM libc does not provide a function, a redirector still makes it appear as if it provides it by using a "redirector" to the system libc function [1]. The use case relevant for this patch is different. With allocators, it is further more nuanced [2]. LLVM libc provides an allocator, which is the SCUDO standalone allocator. So, there is no need of a redirector. But, we can't use the normal add_entrypoint_object rule for them because LLVM libc doesn't "own" those sources.

[1] For all practical purposes, we have given up on the idea of redirectors. We might use them still, but not sure for what exactly.
[2] Allocators are a little nuanced because when calling the allocator functions from libc code, we want to call them using the public name and not an internal name. This is because we want the users to be able to override the libc allocators.

michaelrj updated this revision to Diff 382118.Oct 25 2021, 2:04 PM
michaelrj marked an inline comment as done.

change the way external entrypoints are handled to be cleaner

This is almost what I had in mind. It is still "almost" because of a question I left inline.

libc/cmake/modules/LLVMLibCLibraryRules.cmake
42

Wouldn't this silently ignore all external objects?

libc/cmake/modules/LLVMLibCObjectRules.cmake
287

External entrypoints will not have an object file produced by the libc build. So, we don't need OBJECT_FILE and OBJECT_FILE_RAW properties.

libc/src/stdlib/CMakeLists.txt
190

Indent this?

michaelrj marked 3 inline comments as done.

fix the external entrypoints not actually being included

sivachandra accepted this revision.Oct 26 2021, 11:21 AM
sivachandra added inline comments.
libc/cmake/modules/LLVMLibCLibraryRules.cmake
42

Add a comment here saying something like,

It is not possible to recursively extract deps of external dependencies.
So, we just accumulate the direct dep and return.
This revision is now accepted and ready to land.Oct 26 2021, 11:21 AM
michaelrj marked an inline comment as done.

add an explaining comment

libc/cmake/modules/LLVMLibCLibraryRules.cmake
42

You're right it does, I'm not sure why this ever worked. It's fixed now (I checked the resulting .a file to be sure).

This revision was automatically updated to reflect the committed changes.