[libc-commits] [PATCH] D148290: Support custom attributes in pthread_create

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Sorry for the long delay here. Part of the reason why I have been putting this off is because you mixed up some nice cleanups with the actual feature enhancements. I have not yet gone through the tests.



================
Comment at: libc/src/__support/threads/linux/thread.cpp:88
+  int prot = guardsize ? PROT_NONE : PROT_READ | PROT_WRITE;
+  // TODO: Do we need to check for overflow?
+  auto size_or_err = add_no_overflow(stacksize, guardsize);
----------------
You already check for overflow isn't it? So, do we need to keep this TODO? This pattern repeats at many places in this patch.


================
Comment at: libc/src/__support/threads/linux/thread.cpp:230
+
+  static constexpr size_t kInternalStackDataSize =
+      sizeof(StartArgs) + sizeof(ThreadAttributes) +
----------------
Nit: Because of the naming convention we use for constants, this should be named `INTTERNAL_STACK_DATA_SIZE`.


================
Comment at: libc/src/__support/threads/thread.h:140
+  static constexpr size_t kDefaultGuardSize = 0;
+  static constexpr bool kDefaultDetached = false;
+
----------------
Nit: Naming style for all of these constants should be fixed.


================
Comment at: libc/src/pthread/pthread_attr_getstacksize.cpp:23
+  // Probably unnecessary as we throw error if user tries to set invalid
+  // stacksize.
+  if (attr->__stacksize < PTHREAD_STACK_MIN)
----------------
Can you point me to documentation which says the `getstack*` functions should fail with `EINVAL`? They way I read the POSIX docs, I thought the `setstack*` functions fail with `EINVAL` on improper stack sizes.


================
Comment at: libc/src/pthread/pthread_attr_init.cpp:20
+  // NB: This is exceedingly small compared to the 2MB norm and will break many
+  // programs expecting the full 2MB.
+  static constexpr size_t DEFAULT_STACK_SIZE = 1 << 16;
----------------
We will do something better after this change lands.


================
Comment at: libc/src/pthread/pthread_attr_init.cpp:21
+  // programs expecting the full 2MB.
+  static constexpr size_t DEFAULT_STACK_SIZE = 1 << 16;
+
----------------
Can we use the value in the `Thread` class?


================
Comment at: libc/src/pthread/pthread_create.cpp:46
+
+  if (__llvm_libc::pthread_attr_getstack(attr, &stack, &stacksize))
+    return EINVAL;
----------------
Going by my comment above, do we need this? Also, considering that all `pthread_attr_set*` functions fail anyway on bad inputs, do we need do these checks here at all?


================
Comment at: libc/src/pthread/pthread_create.cpp:58
+    // can only succeed.
+    if (__llvm_libc::pthread_attr_destroy(&default_attr))
+      return EINVAL;
----------------
You can use `LIBC_UNLIKELY`:  https://github.com/llvm/llvm-project/blob/main/libc/src/__support/macros/optimization.h#L25


================
Comment at: libc/src/threads/thrd_create.cpp:14
 #include <errno.h>
-#include <threads.h> // For thrd_* type definitions.
+#include <linux/param.h> // For EXEC_PAGESIZE.
+#include <threads.h>     // For thrd_* type definitions.
----------------
Do we need this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148290