This is an archive of the discontinued LLVM Phabricator instance.

[libc] customizable namespace 1/4
AbandonedPublic

Authored by gchatelet on Aug 29 2023, 8:33 AM.

Details

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=<...>).

Diff Detail

Event Timeline

gchatelet created this revision.Aug 29 2023, 8:33 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 29 2023, 8:33 AM
gchatelet requested review of this revision.Aug 29 2023, 8:33 AM
sivachandra added inline comments.
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

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

gchatelet added inline comments.Aug 30 2023, 7:21 AM
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

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 added inline comments.Aug 30 2023, 11:17 AM
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

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 updated this revision to Diff 555010.Aug 31 2023, 6:15 AM
  • Address comments
  • Support bazel as well
gchatelet updated this revision to Diff 555011.Aug 31 2023, 6:16 AM
  • fix typo
gchatelet updated this revision to Diff 555014.Aug 31 2023, 6:30 AM
  • make bazel load statement a one liner
libc/CMakeLists.txt
58

Alright, let's make it simple then.

gchatelet updated this revision to Diff 555017.Aug 31 2023, 6:31 AM
  • fail on unsupported compiler
gchatelet updated this revision to Diff 555020.Aug 31 2023, 6:34 AM
  • fix auto-formatting
gchatelet updated this revision to Diff 555022.Aug 31 2023, 6:36 AM
  • Fixing messed up file

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 added inline comments.Aug 31 2023, 2:31 PM
libc/CMakeLists.txt
67

I think I am wrong here. Scrub this!

gchatelet updated this revision to Diff 555306.Sep 1 2023, 1:55 AM
gchatelet marked 2 inline comments as done.
  • use add_compile_definitions instead of add_definitions
gchatelet marked 2 inline comments as done.Sep 1 2023, 1:55 AM
gchatelet added inline comments.
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)

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 retitled this revision from [libc] POC for namespace customization to [libc] customizable namespace 1/4.Sep 1 2023, 2:22 AM
gchatelet edited the summary of this revision. (Show Details)
gchatelet updated this revision to Diff 555313.Sep 1 2023, 2:23 AM
gchatelet edited the summary of this revision. (Show Details)
  • Remove guard from libc/src/__support/common.h

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 added inline comments.Sep 4 2023, 2:09 AM
libc/CMakeLists.txt
56

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

gchatelet updated this revision to Diff 555690.Sep 4 2023, 2:10 AM
  • Addess comments
sivachandra accepted this revision.Sep 4 2023, 10:16 PM
This revision is now accepted and ready to land.Sep 4 2023, 10:16 PM
gchatelet marked an inline comment as done.Sep 5 2023, 5:31 AM

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

gchatelet abandoned this revision.Sep 5 2023, 5:47 AM

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

This revision was landed with ongoing or failed builds.Sep 6 2023, 1:28 AM