[libc] Allow regex in entrypoint list

[abrachet]

Summary

Don't need to list out entrypoints one by one, we can now glob for them.

The glob is kind of a hack, libc.src.ctype.* works because we conveniently have the .. it would be intuitive if you wanted all the is* functions in ctype to write libc.src.ctype.is* but that would not work. I don't imagine this is a situation that would ever come up though.

A current issue is that string functions have many different targets like there is libc.src.string.bcmp and libc.src.string.bcmp_x86_64_opt_sse2, both define the bcmp symbol. I'm not sure how we want to deal with this. At present the targets with different compile options do not make their way to libllvmlibc.a.

[michaelrj]

If this patch is going to land, there are a few things to fix first, specifically the entrypoints files for the other targets (aarch64 and windows) and the cmake file for the tests (line 54 in libc/test/src/CMakeLists.txt references TARGET_LLVMLIBC_ENTRYPOINTS specifically and throws errors when I try to build with your patch).

But I'm not sure that this patch matches with the design of the entrypoints file, specifically because not all of the functions are currently available for all platforms. For example, the malloc dependant functions aren't available on AArch64 yet, meaning they'd need to be specifically excluded. This means that we'd effectively be swapping a list of functions which are included for a list of functions which are excluded. While this doesn't particularly cause problems for already established platforms, I think it would make bringing up a new platform harder.

I do think having support for globbing could be useful though, specifically for the functions that are very platform independent, such as the ctype.h functions, so perhaps using both globs and explicit lists will be the eventual solution.

[abrachet]

• Fix failing build when building with LLVM_LIBC_FULL_BUILD defined
• Add include subdirectory after source directories are finished adding all symbols, so all used symbols can be aggregated before header generation
• Don't require HdrGen to need one or more --e flags. src/threads/linux/thread_start_args.h is generated under src/ but doesn't need any symbols, so it doesn't need --e.

[abrachet]

In D116679#3223105, @michaelrj wrote:

If this patch is going to land, there are a few things to fix first, specifically the entrypoints files for the other targets (aarch64 and windows)

The other targets don't _need_ to be updated, they still work as is. I think it makes more sense to only change the one file, to limit the scope of this patch. But it is trivial to change the other platforms here if we want that.

and the cmake file for the tests (line 54 in libc/test/src/CMakeLists.txt references TARGET_LLVMLIBC_ENTRYPOINTS specifically and throws errors when I try to build with your patch).

Fixed

But I'm not sure that this patch matches with the design of the entrypoints file, specifically because not all of the functions are currently available for all platforms. For example, the malloc dependant functions aren't available on AArch64 yet, meaning they'd need to be specifically excluded. This means that we'd effectively be swapping a list of functions which are included for a list of functions which are excluded. While this doesn't particularly cause problems for already established platforms, I think it would make bringing up a new platform harder.

Malloc and friends are not included on aarch64 because I only touched the x86 config file. Excluding in that file would be the same as what is done here, list(APPEND TARGET_LIBC_OMIT_ENTRYPOINTS ${MALLOC_DEPENDANT_ENTRYPOINTS})

I'm not sure I see why it makes it harder for other platforms. On the contrary I made this change to make it easier for us to support other platforms and add new symbols without needing to remember to add it to every platform where it should be on.

[michaelrj]

The patch is better, although there are still a few things that need fixing:

The malloc dependent functions don't work, specifically when building with -DLLVM_LIBC_INCLUDE_SCUDO=ON then strdup gets the error no member named 'malloc' in the global namespace.

On Aarch64 the unmodified entrypoints.txt seems to be causing issues, I've copied the error below, I think the problem is that specifically listed functions aren't being properly included (and this is true on x86_64 as well).

CMake Error at ~/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:88 (add_library):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:libc.src.math.aarch64.ceil>

  Objects of target "libc.src.math.aarch64.ceil" referenced but is not an
  OBJECT library.
Call Stack (most recent call first):
  ~/llvm-project/libc/lib/CMakeLists.txt:3 (add_entrypoint_library)


CMake Error at ~/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:88 (add_library):
  No SOURCES given to target: llvmlibc
Call Stack (most recent call first):
  ~/llvm-project/libc/lib/CMakeLists.txt:3 (add_entrypoint_library)

I htink I understand what your intent is a bit better, and I agree that this patch is useful, but I would like to adjust which headers are marked as "all" vs which have their functions listed. I think that headers that are safely and trivially cross-platform can be marked as all, such as ctype.h, in addition I think that headers that have significant platform differences should have their functions listed, such as math.h. The reason behind this is that I agree that not having to add the functions to each list specifically would be nice, but I also think that for functions that may have cross-platform difficulties then adding that function to a platform that you haven't tested is likely to cause problems. With the current setup, if I want to land a function on just x86_64 because it will require tweaking for aarch64 and I want to do that separately, then I just need to remember to add the function to x86_64, which is hard to miss because if I don't then I can't test it. If we switch all functions to being enabled by default on all platforms, then I won't have to remember to add my function to the allow list for x86_64, but I will have to add it to the omit list for all of the other platforms if I don't want to break those platforms' builds.

libc/cmake/modules/LLVMLibCObjectRules.cmake

287

I think you need to add set_property(GLOBAL APPEND PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS ${fq_target_name}) to this function.

[abrachet]

• Fix bug when building with -DLLVM_LIBC_INCLUDE_SCUDO=ON
• Fix aliasee targets not being included
• Only use target globbing for libc.src.ctype.*

[gchatelet]

@michaelrj I concur. I think it's better to be explicit and list every single functions rather than globing.
Globing simplifies the configuration a bit but I suspect it will have surprising effect in the future.

@abrachet is there anything else you're trying to achieve with this patch?

[abrachet]

Globing simplifies the configuration a bit but I suspect it will have surprising effect in the future.

If we don't think the tradeoff is worth it, which it may not be, I'm fine abandoning this patch. I do agree that this could potentially bite us later.

However I think that the current situation is also error prone. For example for our Linux x86 and ARM64 configurations,

memccpy
mempcpy
stpcpy
stpncpy
strncat
atof
strtod
strtof
strtold

Are all missing for ARM despite having pure C++ implementations for these.

For Windows it's worse, granted I don't know how supported Windows even is. All of the above symbols plus these, which are OS agnostic are missing.

imaxdiv
abs
bsearch
div
labs
ldiv
llabs
lldiv
qsort
fedisableexcept
feenableexcept
fegetexcept
cos
llrint
llrintf
llrintl
lrint
lrintf
lrintl
sin
tan

@abrachet is there anything else you're trying to achieve with this patch?

No, we're just planning on adding a new OS target in the future and this would make it simpler to maintain as we increase the number of OS+architecture configurations. Moreover these is no canonical list of all entrypoints, linux/x86_64 essentially acts like this, but it isn't programmatically guaranteed.

[sivachandra]

I am with @gchatelet and @michaelrj wrt scalability issues and wrt globbing causing subtle issues in future. I agree it is not ideal and may be a bit too verbose. I don't necessarily view this verbosity as a problem as I prefer explicit vs implicit in such matters. That said, if you want to redesign this setup at a higher level, feel free to propose alternates.

[abrachet]

An alternative is having a ground truth list of entrypoints that we update explicitly, basically what linux/x86_64/entrypoints.txt is now, and then the OS/arch specific files could use regex. That way the list of entrypoints is created before add_entrypoint_object What you all think about that? Or should we stick with how things are now?

[michaelrj]

While I'm not opposed to having a ground truth list of entrypoints, in fact I think it would be a good idea, I am concerned that if we're doing regexes against that then the end result might be the same as just using regexes as was previously proposed, except that there's another new place to remember to add the entrypoint.

libc/CMakeLists.txt

 include(${entrypoint_file})
 
 if(EXISTS "${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/${LIBC_TARGET_ARCHITECTURE}/headers.txt")
   include("${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/${LIBC_TARGET_ARCHITECTURE}/headers.txt")
 elseif(EXISTS "${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/headers.txt")
   include("${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/headers.txt")
 endif()
 
+if(LLVM_LIBC_FULL_BUILD)
+  # We need to set up hdrgen first since other targets depend on it.
+  add_subdirectory(utils/LibcTableGenUtil)
+  add_subdirectory(utils/HdrGen)
+endif()
+
+define_property(GLOBAL PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS BRIEF_DOCS "List of targets" FULL_DOCS "List of targets")
+set_property(GLOBAL PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS "")
+
+add_subdirectory(config)
+add_subdirectory(src)
+add_subdirectory(utils)
+
+get_property(libc_dependencies GLOBAL PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS)
+
 set(TARGET_ENTRYPOINT_NAME_LIST "")
-foreach(entrypoint IN LISTS TARGET_LLVMLIBC_ENTRYPOINTS)
+foreach(entrypoint IN LISTS libc_dependencies)
   string(FIND ${entrypoint} "." last_dot_loc REVERSE)
   if(${last_dot_loc} EQUAL -1)
     message(FATAL "Invalid entrypoint target name ${entrypoint}; Expected a '.' "
                   "(dot) in the name.")
   endif()
   math(EXPR name_loc "${last_dot_loc} + 1")
   string(SUBSTRING ${entrypoint} ${name_loc} -1 entrypoint_name)
   list(APPEND TARGET_ENTRYPOINT_NAME_LIST ${entrypoint_name})
 endforeach()
 
-if(LLVM_LIBC_FULL_BUILD)
-  # We need to set up hdrgen first since other targets depend on it.
-  add_subdirectory(utils/LibcTableGenUtil)
-  add_subdirectory(utils/HdrGen)
-endif()
-
 add_subdirectory(include)
-add_subdirectory(config)
-add_subdirectory(src)
-add_subdirectory(utils)
 
 if(LLVM_LIBC_FULL_BUILD)
   # The loader can potentially depend on the library components so add it
   # after the library implementation directories.
   add_subdirectory(loader)
 endif()
 
 # The lib and test directories are added at the very end as tests

libc/cmake/modules/LLVMLibCObjectRules.cmake

set_target_properties(
     ${fq_target_name}
     PROPERTIES
       "TARGET_TYPE" ${OBJECT_LIBRARY_TARGET_TYPE}
       "OBJECT_FILES" "$<TARGET_OBJECTS:${fq_target_name}>"
       "DEPS" "${fq_deps_list}"
   )
 endfunction(add_object_library)
 
+function(_target_matches_glob output_var target)
+  foreach(glob_expr IN LISTS ARGN)
+    if(${target} MATCHES "^${glob_expr}$")
+      set(${output_var} TRUE PARENT_SCOPE)
+      return()
+    endif()
+  endforeach()
+
+  set(${output_var} FALSE PARENT_SCOPE)
+endfunction()
+
 set(ENTRYPOINT_OBJ_TARGET_TYPE "ENTRYPOINT_OBJ")
 
 # A rule for entrypoint object targets.
 # Usage:
 #     add_entrypoint_object(
 #       <target_name>
 #       [ALIAS|REDIRECTED] # Specified if the entrypoint is redirected or an alias.
 #       [NAME] <the C name of the entrypoint if different from target_name>
function(add_entrypoint_object target_name)
   )
 
   get_fq_target_name(${target_name} fq_target_name)
   set(entrypoint_name ${target_name})
   if(ADD_ENTRYPOINT_OBJ_NAME)
     set(entrypoint_name ${ADD_ENTRYPOINT_OBJ_NAME})
   endif()
 
-  list(FIND TARGET_ENTRYPOINT_NAME_LIST ${entrypoint_name} entrypoint_name_index)
-  if(${entrypoint_name_index} EQUAL -1)
-    add_custom_target(${fq_target_name})
-    set_target_properties(
-      ${fq_target_name}
-      PROPERTIES
-        "ENTRYPOINT_NAME" ${entrypoint_name}
-        "TARGET_TYPE" ${ENTRYPOINT_OBJ_TARGET_TYPE}
-        "OBJECT_FILE" ""
-        "OBJECT_FILE_RAW" ""
-        "DEPS" ""
-        "SKIPPED" "YES"
-    )
-    message(STATUS "Skipping libc entrypoint ${fq_target_name}.")
-    return()
+  _target_matches_glob(include_target ${fq_target_name} ${TARGET_LLVMLIBC_ENTRYPOINTS})
+  _target_matches_glob(exclude_target ${fq_target_name} ${TARGET_LIBC_OMIT_ENTRYPOINTS})
+  if(${include_target} AND NOT ${exclude_target})
+    set_property(GLOBAL APPEND PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS ${fq_target_name})
   endif()
 
   if(ADD_ENTRYPOINT_OBJ_ALIAS)
     # Alias targets help one add aliases to other entrypoint object targets.
     # One can use alias targets setup OS/machine independent entrypoint targets.
     list(LENGTH ADD_ENTRYPOINT_OBJ_DEPENDS deps_size)
     if(NOT (${deps_size} EQUAL "1"))
       message(FATAL_ERROR "An entrypoint alias should have exactly one dependency.")
function(add_entrypoint_external target_name)
 
   add_custom_target(${fq_target_name})
   set_target_properties(
     ${fq_target_name}
     PROPERTIES
       "ENTRYPOINT_NAME" ${entrypoint_name}
       "TARGET_TYPE" ${ENTRYPOINT_EXT_TARGET_TYPE}
       "DEPS" "${ADD_ENTRYPOINT_EXT_DEPENDS}"
   )
 
+  set_property(GLOBAL APPEND PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS ${fq_target_name})
+
 endfunction(add_entrypoint_external)
 
 # Rule build a redirector object file.
 function(add_redirector_object target_name)
   cmake_parse_arguments(
     "REDIRECTOR_OBJECT"
     "" # No optional arguments
     "SRC" # The cpp file in which the redirector is defined.

libc/cmake/modules/LLVMLibCTestRules.cmake

foreach(dep IN LISTS ARGN)
     endif()
 
     if(${dep_type} STREQUAL ${OBJECT_LIBRARY_TARGET_TYPE})
       get_target_property(dep_object_files ${dep} "OBJECT_FILES")
       if(dep_object_files)
         list(APPEND object_files ${dep_object_files})
       endif()
     elseif(${dep_type} STREQUAL ${ENTRYPOINT_OBJ_TARGET_TYPE})
-      get_target_property(is_skipped ${dep} "SKIPPED")
-      if(is_skipped)
+      get_property(libc_dependencies GLOBAL PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS)
+      list(FIND libc_dependencies ${dep} entrypoint_name_index)
+      if(${entrypoint_name_index} EQUAL -1)
         list(APPEND skipped_list ${dep})
         continue()
       endif()
       get_target_property(object_file_raw ${dep} "OBJECT_FILE_RAW")
       if(object_file_raw)
         list(APPEND object_files ${object_file_raw})
       endif()
     endif()

libc/config/linux/x86_64/entrypoints.txt

 set(TARGET_LIBC_ENTRYPOINTS
     # ctype.h entrypoints
-    libc.src.ctype.isalnum
-    libc.src.ctype.isalpha
-    libc.src.ctype.isascii
-    libc.src.ctype.isblank
-    libc.src.ctype.iscntrl
-    libc.src.ctype.isdigit
-    libc.src.ctype.isgraph
-    libc.src.ctype.islower
-    libc.src.ctype.isprint
-    libc.src.ctype.ispunct
-    libc.src.ctype.isspace
-    libc.src.ctype.isupper
-    libc.src.ctype.isxdigit
-    libc.src.ctype.toascii
-    libc.src.ctype.tolower
-    libc.src.ctype.toupper
+    libc.src.ctype.*
 
     # errno.h entrypoints
     libc.src.errno.__errno_location
 
     # string.h entrypoints
     libc.src.string.bcmp
     libc.src.string.bzero
     libc.src.string.memccpy

libc/lib/CMakeLists.txt

+get_property(libc_dependencies GLOBAL PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS)
+
 add_entrypoint_library(
   llvmlibc
   DEPENDS
-  ${TARGET_LLVMLIBC_ENTRYPOINTS}
+  ${libc_dependencies}
 )
 
 if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR)
   set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE})
 else()
   set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX})
 endif()
 

libc/test/src/CMakeLists.txt

 add_subdirectory(stdio)
 add_subdirectory(sys)
 add_subdirectory(threads)
 add_subdirectory(time)
 add_subdirectory(unistd)
 
 set(public_test ${CMAKE_CURRENT_BINARY_DIR}/public_integration_test.cpp)
 
+get_property(libc_dependencies GLOBAL PROPERTY TARGET_LLVMLIBC_USED_ENTRYPOINTS)
+
 set(entrypoints_name_list "")
-foreach(entry IN LISTS TARGET_LLVMLIBC_ENTRYPOINTS)
+foreach(entry IN LISTS libc_dependencies)
   get_target_property(entry_name ${entry} "ENTRYPOINT_NAME")
   list(APPEND entrypoints_name_list ${entry_name})
 endforeach()
 
 # TODO: Remove these when they are added to the TableGen.
 list(REMOVE_ITEM entrypoints_name_list "__assert_fail" "__errno_location")
 list(TRANSFORM entrypoints_name_list PREPEND "-e=")
 

libc/utils/HdrGen/Main.cpp

HeaderDefFile("def", llvm::cl::desc("Path to the .h.def file."),
                   llvm::cl::value_desc("<filename>"), llvm::cl::Required);
 llvm::cl::opt<std::string> StandardHeader(
     "header",
     llvm::cl::desc("The standard header file which is to be generated."),
     llvm::cl::value_desc("<header file>"));
 llvm::cl::list<std::string> EntrypointNamesOption(
     "e", llvm::cl::value_desc("<list of entrypoints>"),
     llvm::cl::desc(
-        "Each --e is one entrypoint (generated from entrypoints.txt)"),
-    llvm::cl::OneOrMore);
+        "Each --e is one entrypoint (generated from entrypoints.txt)"));
 llvm::cl::list<std::string> ReplacementValues(
     "args", llvm::cl::desc("Command separated <argument name>=<value> pairs."),
     llvm::cl::value_desc("<name=value>[,name=value]"));
 
 void ParseArgValuePairs(std::unordered_map<std::string, std::string> &Map) {
   for (std::string &R : ReplacementValues) {
     auto Pair = llvm::StringRef(R).split('=');
     Map[std::string(Pair.first)] = std::string(Pair.second);