[libc] replace MutexLock with cpp::lock_guard

[nickdesaulniers]

MutexLock in src/__support/threads/mutex.h smells like std::lock_guard. We should:

1. move it to src/__support/CPP/mutex.h (or better yet, just create a new file. This RAII wrapper is trivial IMO to implement)
2. rename it to lock_guard
3. update the references to MutexLock to now refer to cpp::lock_guard
4. explicitly delete the copy assignment operator

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

MutexLock in src/__support/threads/mutex.h smells like std::lock_guard. We should: 1. move it to src/__support/CPP/mutex.h (or better yet, just create a new file. This RAII wrapper is trivial IMO to implement) 2. rename it to lock_guard 3. update the references to MutexLock to now refer to cpp::lock_guard 4. explicitly delete the copy assignment operator

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Nick Desaulniers (nickdesaulniers)

MutexLock in src/__support/threads/mutex.h smells like std::lock_guard. We should: 1. move it to src/__support/CPP/mutex.h (or better yet, just create a new file. This RAII wrapper is trivial IMO to implement) 2. rename it to lock_guard 3. update the references to MutexLock to now refer to cpp::lock_guard 4. explicitly delete the copy assignment operator

[vmishelcs]

Hello, I'd like to give this issue a try :)

[nickdesaulniers]

You can do it @vmishelcs !

To add more to the spec here, our source dir src/__support/CPP is meant to mirror the C++ standard library, so methods/members/identifiers should all match the C++ standard library, just in the cpp namespace rather than std. We don't express a dependency on libc++, but most of the C++ standard library can be implemented in headers. We're in early discussions to share code across libc++ and libc, but until then we will maintain some reimplementations of the C++ standard library.

We'll also want a test for the newly created cpp::lock_guard. libc/test/src/__support/CPP/mutex_test.cpp would be a good place to put it (a new file).

[vmishelcs]

@nickdesaulniers Thank you for the extra info, I will get started on this today. If I have some questions along the way regarding the implementation, would it be OK if I ping you here or perhaps in the LLVM Discord?

[nickdesaulniers]

Yep, either works. @nickdesaulniers is also my handle on LLVM discord.

[vmishelcs]

Hey @nickdesaulniers, I think I've got the actual cpp::lock_guard class implementation down, however the CMake structure of the project and how the project is built is causing some confusion for me.

After adding the mutex.h file to libc/src/support/CPP (which as far as I understand should be almost identical to libc/src/support/threads/mutex.h), I added the following entry to libc/src/__support/CPP/CMakeLists.txt.

if(TARGET libc.src.__support.threads.${LIBC_TARGET_OS}.mutex)
  add_header_library(
    mutex
    HDRS
      mutex.h
    DEPENDS
      libc.src.__support.threads.${LIBC_TARGET_OS}.mutex
  )
endif()

I figured it would need to look similar to how the header library is declared in libc/src/support/threads/CMakeLists.txt. However, just as a sanity check I tried to write some gibberish inside of my newly created mutex.h file to see if building the project gives me a syntax error and it did not. Is this behaviour expected until I start to replace MutexLock variables for lock_guard variables? After replacing all usages of MutexLock with cpp::lock_guard in the libc/src/support/File directory I am getting CMake errors:

[build] CMake Error at /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:5 (get_target_property):
[build]   get_target_property() called with non-existent target
[build]   "libc.src.__support.File.file".
[build] Call Stack (most recent call first):
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:36 (collect_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:36 (collect_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:63 (collect_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:237 (get_all_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/lib/CMakeLists.txt:22 (add_entrypoint_library)
[build] 
[build] 
[build] CMake Error at /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:5 (get_target_property):
[build]   get_target_property() called with non-existent target
[build]   "libc.src.__support.File.platform_file".
[build] Call Stack (most recent call first):
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:36 (collect_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:36 (collect_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:63 (collect_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/cmake/modules/LLVMLibCLibraryRules.cmake:237 (get_all_object_file_deps)
[build]   /home/vmishel/llvm-project/libc/lib/CMakeLists.txt:22 (add_entrypoint_library)

I also have a question about testing. I see that in the docs the way testing is done is by running various ninja configurations. Is there anything similar to llvm-lit in libc, a binary that takes a test as argument and runs exactly those tests?

Thanks in advance for your time!

[nickdesaulniers]

I added the following entry to libc/src/support/CPP/CMakeLists.txt. libc.src.support.threads.${LIBC_TARGET_OS}.mutex

ah, so while libc/src/__support/threads/mutex.h today currently has:

   40 #if defined(__linux__)                                                          
   41 #include "linux/mutex.h"                                                        
   42 #elif defined(LIBC_TARGET_ARCH_IS_GPU)                                          
   43 #include "gpu/mutex.h"                                                          
   44 #endif // __linux__

we don't want to retain that in the newly created libc/src/__support/CPP/mutex.h. If you look at the interface exposed by std::lock_guard, it doesn't actually depend on std::mutex or even really knowing what a mutex is. Really, it just needs some templated type that has lock and unlock methods. Looking at how it's implemented in libcxx may also provide inspiration (though we can likely get away with a much less complex implementation).

So to get back to cmake, I'd expect:

add_header_library(
  mutex
  HDRS
    mutex.h
)

and maybe some DEPENDS (perhaps none, dunno, that's for you to find out)

However, just as a sanity check I tried to write some gibberish inside of my newly created mutex.h file to see if building the project gives me a syntax error

Good intuition to check that. I do the same. ;)

After replacing all usages of MutexLock with cpp::lock_guard in the libc/src/__support/File directory I am getting CMake errors:

Sort out the above CMake issue first, then show me a working diff of what you have if it's still reproducible.

Is there anything similar to llvm-lit in libc, a binary that takes a test as argument and runs exactly those tests?

Not yet. Generally, I use the cmake to find what test suite something is part of, then use that ninja target. It admittedly runs more tests than just the few I care about. While our unittests look like they use gtest, it's not gtest, so the usual command line flags you'd feed gtest to output the results of just one test aren't recognized in the libc test suite. We've talked about moving testing to llvm-lit based, since IIRC libcxx does that.