[libc] customizable namespace 1/4

[gchatelet]

Summary

This implements the first step of https://discourse.llvm.org/t/rfc-customizable-namespace-to-allow-testing-the-libc-when-the-system-libc-is-also-llvms-libc/73079

Make required Bazel and CMake preparatory changes which define the var for the customizable namespace and use it (pass it as a compile option -DLIBC_NAMESPACE=<...>).

[sivachandra]

libc/CMakeLists.txt

58

Can we replace lines 51 to 58 with:

set(LIBC_NAMESPACE
  "__llvm_libc_${LLVM_VERSION_MAJOR}_${LLVM_VERSION_MINOR}_${LLVM_VERSION_PATCH}_${LLVM_VERSION_SUFFIX}"
  CACHE STRING "The namespace to use to enclose internal implementations. Must start with '__llvm_libc'."
)

The effective changes are:

1. Call the CMake variable just LIBC_NAMESPACE.
2. Since it is a CACHE var, it will be set only if it is not being set on the command line.
3. We don't need FORCE as we do not really want to change the cached value.

libc/cmake/modules/LLVMLibCCompileOptions.cmake

10 ↗

(On Diff #554367)

Instead of adding a compile option to every target, can we just do one add_definitions call in libc/CMakeLists.txt?

[gchatelet]

libc/CMakeLists.txt

58

I initially did the two step version with FORCE to make sure that CMake does not retain an old value.

i.e., if we run cmake -DLIBC_NAMESPACE=__llvm_libc ... then cmake ... I would expect the second run to use the autogenerated namespace value. WDYT?

libc/cmake/modules/LLVMLibCCompileOptions.cmake

10 ↗

(On Diff #554367)

We could. For large projects I tend to prefer being explicit rather than implicit and local rather than global.
LIBC_NAMESPACE might be a could use of add_definitions though.

I think we should still have a file like LLVMLibCCompileOptions.cmake for other options like -fno-expections, -ffreestanding or -fno-rtti. We currently have ad-hoc target_compile_options all over the place which will fail when compiling with MSVC for instance.

[sivachandra]

libc/CMakeLists.txt

58

I initially did the two step version with FORCE to make sure that CMake does not retain an old value.

The way I understand it, the FORCE on line 52 just clears out the command line arg also?

i.e., if we run cmake -DLIBC_NAMESPACE=__llvm_libc ... then cmake ... I would expect the second run to use the autogenerated namespace value. WDYT?

I don't think this is possible in general at all? As in, I am not sure there is a way to overwrite cache variables that way at all other than FORCE of course, but FORCE erases the command line value the first time as well?

If one is making another CMake run with a different set of command line options, they should cleanup the files from the previous run. I went looking for an option and found the --fresh option but it is available only from 3.24: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-fresh.

libc/cmake/modules/LLVMLibCCompileOptions.cmake

10 ↗

(On Diff #554367)

We could. For large projects I tend to prefer being explicit rather than implicit and local rather than global.
LIBC_NAMESPACE might be a could use of add_definitions though.

There cannot be two different namespaces during a single build so I think just adding via add_definitions would be sufficient. If we hit problems, we can always make it more complicated at that time.

I think we should still have a file like LLVMLibCCompileOptions.cmake for other options like -fno-expections, -ffreestanding or -fno-rtti. We currently have ad-hoc target_compile_options all over the place which will fail when compiling with MSVC for instance.

Acknowledged. I think we can do it irrespective of this POC.

[gchatelet]

• Address comments
• Support bazel as well

[gchatelet]

• fix typo

[gchatelet]

• make bazel load statement a one liner

libc/CMakeLists.txt

58

Alright, let's make it simple then.

[gchatelet]

• fail on unsupported compiler

[gchatelet]

• fix auto-formatting

[gchatelet]

• Fixing messed up file

[sivachandra]

How do you want to take this forward?

libc/CMakeLists.txt

67

If we use add_definitions, we shouldn't use -D or the /D syntax. CMake will use the appropriate way to add a macro-def based on the compiler we use.

[sivachandra]

libc/CMakeLists.txt

67

I think I am wrong here. Scrub this!

[gchatelet]

• use add_compile_definitions instead of add_definitions

[gchatelet]

libc/CMakeLists.txt

67

That's actually the case when using add_compile_definitions (doc) instead of add_definitions (doc). I've used the more modern version since we can support it (3.20 > 3.12)

[gchatelet]

In D159112#4632351, @sivachandra wrote:

How do you want to take this forward?

Let's stick to the plan in https://discourse.llvm.org/t/rfc-customizable-namespace-to-allow-testing-the-libc-when-the-system-libc-is-also-llvms-libc/73079.
I will update this patch to conform to the first step (removing the guard that is) and submit.
Then I'll create a second patch to add the guard.

[gchatelet]

• Remove guard from libc/src/__support/common.h

[sivachandra]

Have an inline question but LGTM.

libc/CMakeLists.txt

56

May be I am missing something. Wouldn't the FORCE make it take the else path always?

[gchatelet]

libc/CMakeLists.txt

56

Hmm I thought I used your previous suggestion but it seems I messed up. Thx for noticing.

[gchatelet]

• Addess comments

[gchatelet]

FYI I'll submit this patch as a github PR.

[gchatelet]

Abandoning the review in Phabricator.
Review in github : https://github.com/llvm/llvm-project/pull/65321

libc/CMakeLists.txt

if(("libc" IN_LIST LLVM_ENABLE_RUNTIMES AND NOT LLVM_RUNTIMES_BUILD) OR
   return()
 endif()
 
 option(LIBC_CMAKE_VERBOSE_LOGGING
        "Log details warnings and notifications during CMake configuration." OFF)
 # Path libc/scripts directory.
 set(LIBC_BUILD_SCRIPTS_DIR "${LIBC_SOURCE_DIR}/utils/build_scripts")
 
+# Defining a global namespace to enclose all libc functions.
+set(LIBC_CMAKE_NAMESPACE "" CACHE STRING "The namespace to use to enclose internal implementations. Must start with '__llvm_libc'." FORCE)
+
+if(LIBC_CMAKE_NAMESPACE)
+  set(LLVM_LIBC_NAMESPACE "${LIBC_CMAKE_NAMESPACE}")
+else()
+  set(LLVM_LIBC_NAMESPACE "__llvm_libc_${LLVM_VERSION_MAJOR}_${LLVM_VERSION_MINOR}_${LLVM_VERSION_PATCH}_${LLVM_VERSION_SUFFIX}")
+endif()
+
+if(NOT LLVM_LIBC_NAMESPACE MATCHES "^__llvm_libc")
+  message(FATAL_ERROR "Invalid LLVM_LIBC_NAMESPACE. Must start with '__llvm_libc' was '${LLVM_LIBC_NAMESPACE}'")
+endif()
+
+message(STATUS "Setting LLVM_LIBC_NAMESPACE namespace to '${LLVM_LIBC_NAMESPACE}'")
+
+if(LLVM_COMPILER_IS_GCC_COMPATIBLE)
+  add_definitions("-DLIBC_NAMESPACE=${LLVM_LIBC_NAMESPACE}")
+elseif(MSVC)
+  add_definitions("/DLIBC_NAMESPACE=${LLVM_LIBC_NAMESPACE}")
+endif()
+
 # Flags to pass down to the compiler while building the libc functions.
 set(LIBC_COMPILE_OPTIONS_DEFAULT "" CACHE STRING "Architecture to tell clang to optimize for (e.g. -march=... or -mcpu=...)")
 
 include(common_libc_tuners.cmake)
 
 list(APPEND LIBC_COMPILE_OPTIONS_DEFAULT ${LIBC_COMMON_TUNE_OPTIONS})
 
 # Check --print-resource-dir to find the compiler resource dir if this flag

libc/src/__support/common.h

 //===-- Common internal contructs -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_SUPPORT_COMMON_H
 #define LLVM_LIBC_SUPPORT_COMMON_H
 
+#ifndef LIBC_NAMESPACE
+#error "LIBC_NAMESPACE macro is not defined."
+#endif
+
 #include "src/__support/macros/attributes.h"
 #include "src/__support/macros/properties/architectures.h"
 
 #ifndef LLVM_LIBC_FUNCTION_ATTR
 #define LLVM_LIBC_FUNCTION_ATTR
 #endif
 
 // MacOS needs to be excluded because it does not support aliasing.

utils/bazel/llvm-project-overlay/libc/BUILD.bazel

 # This file is licensed under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 # LLVM libc project.
 load(
     ":libc_build_rules.bzl",
     "libc_function",
     "libc_math_function",
     "libc_support_library",
 )
 load(":platforms.bzl", "PLATFORM_CPU_ARM64", "PLATFORM_CPU_X86_64")
 load("@bazel_skylib//lib:selects.bzl", "selects")
 load("@bazel_skylib//rules:common_settings.bzl", "string_flag")
+load("//:vars.bzl", "LLVM_VERSION_MAJOR", "LLVM_VERSION_MINOR", "LLVM_VERSION_PATCH")
 
 package(
     default_visibility = ["//visibility:public"],
     features = [
         "-use_header_modules",
         "-layering_check",
     ],
 )
 
 licenses(["notice"])
 
 PRINTF_COPTS = [
     "LIBC_COPT_PRINTF_USE_SYSTEM_FILE",
     "LIBC_COPT_PRINTF_DISABLE_INDEX_MODE",
     "LIBC_COPT_PRINTF_DISABLE_WRITE_INT",
 ]
 
 MEMORY_COPTS = [
     # "LIBC_COPT_MEMCPY_X86_USE_REPMOVSB_FROM_SIZE=0",
     # "LIBC_COPT_MEMCPY_X86_USE_SOFTWARE_PREFETCHING",
 ]
 
+llvm_libc_namespace = "__llvm_libc_{}_{}_{}_git".format(LLVM_VERSION_MAJOR, LLVM_VERSION_MINOR, LLVM_VERSION_PATCH)
+
 # A flag to pick which `mpfr` to use for math tests.
 # Usage: `--@llvm-project//libc:mpfr=<disable|external|system>`.
 # Flag documentation: https://bazel.build/extending/config
 string_flag(
     name = "mpfr",
     build_setting_default = "external",
     values = [
         "disable",  # Skip tests that need mpfr

 
 # This empty root library helps us add an include path to this directory
 # using the 'includes' attribute. The strings listed in the includes attribute
 # are relative paths wrt this library but are inherited by the dependents
 # appropriately. Hence, using this as a root dependency avoids adding include
 # paths of the kind "../../" to other libc targets.
 cc_library(
     name = "libc_root",
+    defines = ["LIBC_NAMESPACE=" + llvm_libc_namespace],
     includes = ["."],
 )
 
 ############################## Support libraries #############################
 
 libc_support_library(
     name = "__support_macros_properties_architectures",
     hdrs = ["src/__support/macros/properties/architectures.h"],