[libc-commits] [PATCH] D73472: [libc] Add utils for memory functions

Wed Jan 29 06:44:55 PST 2020

gchatelet marked 28 inline comments as done.
gchatelet added inline comments.

================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:320
   foreach(dep IN LISTS LIBC_UNITTEST_DEPENDS)
-    get_target_property(dep_type ${dep} "TARGET_TYPE")
-    if (dep_type)
-      string(COMPARE EQUAL ${dep_type} ${ENTRYPOINT_OBJ_TARGET_TYPE} dep_is_entrypoint)
-      if(dep_is_entrypoint)
-        get_target_property(obj_file ${dep} "OBJECT_FILE_RAW")
-        list(APPEND library_deps ${obj_file})
-        continue()
+    # If the dep is a normal CMake library target then add it to the list of
+    # library_deps.
----------------
sivachandra wrote:
> gchatelet wrote:
> > I had to test for non entrypoint libraries first as the `INTERFACE` do not have the `"TARGET_TYPE"` property.
> This is OK. I have another change to add header only libraries which should improve the situation a little.
Sounds good to me. I'm waiting for it to be in and I'll revert this change.

================
Comment at: libc/src/CMakeLists.txt:4
 add_subdirectory(string)
+add_subdirectory(memory)
 # TODO: Add this target conditional to the target OS.
----------------
sivachandra wrote:
> Sorry, I did not catch it in the first pass: I think the memory functions are declared in `string.h`. In which case, you should not be adding a new directory called `memory`, but make use of the existing `string` directory.
I expect quite a few files in here, how about a `details` subfolder?

================
Comment at: libc/src/memory/CMakeLists.txt:8
+target_include_directories(memory_utils INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
+target_link_libraries(memory_utils INTERFACE memory_constants)
----------------
sivachandra wrote:
> Its odd that you have to add link libraries for header only libraries. Is it because this is the only way to setup the dependencies correctly?
It seems to be the way to declare transitive dependency on header only libraries yes.
https://cmake.org/pipermail/cmake/2017-October/066366.html

If someone knows a better way I'm happy to take any suggestions.

================
Comment at: libc/src/memory/constants.h:1
+//===-------------------------- Memory constants --------------------------===//
+//
----------------
sivachandra wrote:
> gchatelet wrote:
> > I assume these files will stay as implementation details and are not to be exposed but feel free to challenge this assumption.
> This is OK. A nit: the use of plural "constants" made me look for more than one constants. But, if I am reading correctly, there is only one constant. So, unless you think that there will be more constants added in the near future, why not call it just `cacheline_size.h`? 
Yes definitely. Thx!

================
Comment at: libc/src/memory/utils.h:20
+// Making sure these functions are inlined by using a private namespace
+namespace {
+
----------------
sivachandra wrote:
> abrachet wrote:
> > gchatelet wrote:
> > > Do we prefer to annotate all functions `static` instead?
> > The coding standard says so, yes.
> To avoid `static`, would an `always_inline` attribute help?
Let's stick to `static` for now, I'll probably need the `always_inline` attribute at some point though.

================
Comment at: libc/src/memory/utils.h:57
+// Returns the offset from `ptr` to the next cache line.
+inline intptr_t offset_to_next_cache_line(const void *ptr) {
+  return offset_to_next_aligned<LLVM_LIBC_CACHELINE_SIZE>(ptr);
----------------
abrachet wrote:
> Is `inline` necessary?
no indeed

================
Comment at: libc/test/src/memory/utils_test.cpp:12
+
+#include <array>
+
----------------
sivachandra wrote:
> abrachet wrote:
> > gchatelet wrote:
> > > Is this ok to rely on the libc++ for the tests?
> > No. In any case the burden to use `int [N]` instead of `std::array<int, N>` is very small.
> Agree with @abrachet. However, if you want convenience functions like `size` et al, I am OK with adding a class like LLVM's `ArrayRef`. In fact, I have another patch in the works to help with your prep here, so I can add it there.
Keeping as is for now and waiting for D73530 to be in to move to `ArrayRef`

================
Comment at: libc/test/src/memory/utils_test.cpp:24
+  });
+  for (size_t i = 0; i < kExpectedValues.size(); ++i) {
+    ASSERT_EQ((int)is_power2_or_zero(i), kExpectedValues[i]);
----------------
abrachet wrote:
> No need for the brackets.
> 
> Not that it matters here but I did end up finding the mailing list archive where they talked about preferring signed types for loops if you're curious. http://lists.llvm.org/pipermail/llvm-dev/2019-June/132890.html
> Not that it matters here but I did end up finding the mailing list archive where they talked about preferring signed types for loops if you're curious. http://lists.llvm.org/pipermail/llvm-dev/2019-June/132890.html

Yeah I've followed the thread :) I've not been convinced by the signed arguments: I've seen code where using `size_t` instead  of plain `int` lead to faster code.

================
Comment at: libc/test/src/memory/utils_test.cpp:25
+  for (size_t i = 0; i < kExpectedValues.size(); ++i) {
+    ASSERT_EQ((int)is_power2_or_zero(i), kExpectedValues[i]);
+  }
----------------
sivachandra wrote:
> gchatelet wrote:
> > @sivachandra there are no `ASSERT_TRUE`/`ASSERT_FALSE` and `ASSERT_EQ` do not handle `bool` values so I had to cast `is_power2_ro_zero` to `int`
> The reason why I have chosen not to have `ASSERT_[TRUE|FALSE]`, and not to support `ASSERT_EQ` for `bool` is this: In a C library, a return value is often more than tri-state. Hence, not having support for `bool` forces one to check for the exact return value.
> 
> I can see that it is being an annoyance for cases such as yours. But, can you actually make functions like `is_power2_or_zero` return an `int` value?
Thx for the explanation I understand the rationale now.

This makes sense when testing pure `C` code but most of the implementation for the `llvm-libc` will heavily rely on `C++` so it seems like an unnecessary burden.

For instance `utils.h` has a templated function so it does not compile as pure `C`.

I believe all tests are written in `C++` as well (`LibCUnitTest` is itself `C++`)

> I can see that it is being an annoyance for cases such as yours. But, can you actually make functions like is_power2_or_zero return an int value?

Then I'd have to think about the tri-state in the implementation as well which is not ideal. `bool` is a great `C++` tool that I want to use.

How about the opposite?
We could have macros to disable the `bool` versions when testing the `C` functions themselves?

```
TEST(TestCFunc, strlen) {
START_LIBC_UNIT_TEST_C
/// ASSERT_[TRUE|FALSE] and ASSERT_EQ for bool are disabled
STOP_LIBC_UNIT_TEST_C
}
```

I might be biased but I think that most of the unittests will be devoted to the internals of the library and not the actual lic functions.

We could go even simpler with a one liner at the top of the test file `UNIT_TESTS_FOR_C_FUNCTION` to disable the problematic checks. WDTY?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73472/new/

https://reviews.llvm.org/D73472