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

[Noah Goldstein via Phabricator via libc-commits]

goldstein.w.n added inline comments.


================
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);
----------------
sivachandra wrote:
> 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.
Sorry, kind of left those in as questions for you (maybe overflow check is unnecessary and such). Will drop those.


================
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)
----------------
sivachandra wrote:
> 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.
Bah, you are right. the `getstack*` manpage re-iterates that `setstack*` can fail. Misread as `getstack*`. Sorry.


================
Comment at: libc/src/pthread/pthread_attr_init.cpp:21
+  // programs expecting the full 2MB.
+  static constexpr size_t DEFAULT_STACK_SIZE = 1 << 16;
+
----------------
sivachandra wrote:
> Can we use the value in the `Thread` class?
Done for all but `PTHREAD_CREATE_JOINABLE` because I believe that one we have no choice about.


================
Comment at: libc/src/pthread/pthread_create.cpp:46
+
+  if (__llvm_libc::pthread_attr_getstack(attr, &stack, &stacksize))
+    return EINVAL;
----------------
sivachandra wrote:
> 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?
> 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?

My feeling is code like this which is far far far from any critical path is best future-proofed.
But if you think its not worth it LMK and I'll drop.



================
Comment at: libc/src/pthread/pthread_create.cpp:58
+    // can only succeed.
+    if (__llvm_libc::pthread_attr_destroy(&default_attr))
+      return EINVAL;
----------------
sivachandra wrote:
> You can use `LIBC_UNLIKELY`:  https://github.com/llvm/llvm-project/blob/main/libc/src/__support/macros/optimization.h#L25
Sure. Generally my feeling was its not worth the clutter in far from critical path code but annotated all the known-to-never fail cases here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148290