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

Mon Jan 27 11:36:45 PST 2020

sivachandra 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.
----------------
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.

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

================
Comment at: libc/src/memory/constants.h:1
+//===-------------------------- Memory constants --------------------------===//
+//
----------------
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`? 

================
Comment at: libc/src/memory/constants.h:46-48
+#endif
+#endif
+#endif
----------------
abrachet wrote:
> Maybe comment the original `#if`'s, 3 `#endif`'s gets a little hard to follow.
For me, this is OK. But, as @abrachet  points out, to improve readability I prefer making this a generated header. It also makes it consistent with the rest of the code base.

```
$> cat cacheline_size.h.def
<license header>
%%include(${machine_cacheline_size})
```

and add a rule for the header like this:

```
add_gen_hdr(
  cacheline_size,
  DEF_FILE
    cacheline_size.h.def
  GEN_HDR
    cacheline_size.h
  PARAMS
    machine_cacheline_size=${LIBC_TARGET_MACHINE}_cacheline_size.h.inc
  DATA_FILES
    ${LIBC_TARGET_MACHINE}_cacheline_size.h.inc # That is, put machine specific cacheline sizes in different files.
)
```

================
Comment at: libc/src/memory/utils.h:14-15
+
+#include <stddef.h>
+#include <stdint.h>
+
----------------
abrachet wrote:
> We'll need to wait for @sivachandra for this one, because I really don't know. But look how mman.h gets size_t, https://github.com/llvm/llvm-project/blob/master/libc/config/linux/api.td#L7 
> https://github.com/llvm/llvm-project/blob/master/libc/config/linux/api.td#L106
> I don't think this internal header needs to go through the same process but we don't have `stddef` or `stdint`, and I think we stopped including library headers like this.
> 
> Also for @sivachandra the definition of `size_t` includes `stddef` is that ok?
A high level point is, we do not intend to add freestanding C headers like `stddef` and `stdint` to LLVM libc. So, including them directly is fine as they do not interfere with our headers.

About picking `size_t`: in a public header, to avoid name pollution, we need to "pick" only `size_t` and not anything else. In an internal header however, including everything from `stddef` is OK. But, adding a comment to say that `stddef` is being included for `size_t` will be good. [For `stdint`, it is obvious so you need not add such a comment.]

================
Comment at: libc/src/memory/utils.h:20
+// Making sure these functions are inlined by using a private namespace
+namespace {
+
----------------
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?

================
Comment at: libc/test/src/memory/utils_test.cpp:12
+
+#include <array>
+
----------------
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.

================
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]);
+  }
----------------
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?

================
Comment at: libc/test/src/memory/utils_test.cpp:85
+  };
+  ASSERT_EQ(offset_to_next_aligned<16>(forge(0)), 0L);
+  ASSERT_EQ(offset_to_next_aligned<16>(forge(1)), 15L);
----------------
gchatelet wrote:
> @sivachandra `ASSERT_EQ` needs to have the same type on both sides so I had to use the `L` suffix literal. Technically we should write `intptr_t(0)` here and below but it hurts readability.
This was done for making sure that exact types are being compared.  So, using `intptr_t` is the right thing to do. However, to make it less verbose, you can probably define a convenience like:

```
using I = intptr_t;
```

================
Comment at: libc/test/src/memory/utils_test.cpp:101-102
+  ASSERT_EQ(offset_to_next_cache_line(forge(LLVM_LIBC_CACHELINE_SIZE - 1)), 1L);
+}
+} // namespace __llvm_libc
----------------
abrachet wrote:
> Should there be a space here?
Yes :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73472