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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Few more minor comments, but overall LGTM now.



================
Comment at: libc/src/__support/threads/linux/thread.cpp:493
+  // TODO: We may have deallocated the stack. Can we reference local variables
+  // that may have spilled?
   if (style == ThreadStyle::POSIX)
----------------
This is a good catch! What do you think of just exiting with a return code of say -1 if the thread is detached? We can take this up separately.


================
Comment at: libc/src/pthread/pthread_attr_setstack.cpp:29
     return EINVAL;
+  if (int result = __llvm_libc::pthread_attr_setstacksize(attr, stacksize))
+    return result;
----------------
We avoid calling one entrypoint from another. So, we should keep this as before.


================
Comment at: libc/src/threads/thrd_create.cpp:14
 #include <errno.h>
-#include <threads.h> // For thrd_* type definitions.
 
----------------
This is there for IWYU which we try to enforce in general. But, few parts of the libc have likely regressed.


================
Comment at: libc/test/integration/src/pthread/pthread_create_test.cpp:39
+struct TestThreadArgs {
+  pthread_attr_t Attrs;
+  void *Ret;
----------------
General style fixes for this file:

* Member names should be in `snake_case` style.
* Function and method names should be in `snake_case` style.
* Variable names are in `lower_case` style.


================
Comment at: libc/test/integration/src/pthread/pthread_create_test.cpp:61
+
+  ASSERT_FALSE(__llvm_libc::pthread_attr_getstack(ExpecAttrs, &ExpecStack,
+                                                  &ExpecStackSize));
----------------
I am a bit surprised this is not leading to a build failure. We should do an equality test here:

```
ASSERT_EQ(__llvm_libc::pthread_attr_getstack(...), 0);
```

Same with other similar checks below.


================
Comment at: libc/test/integration/src/pthread/pthread_create_test.cpp:218
+
+            // TODO: I can't figure out how to test a user allocated stack along
+            // with detached pthread safely. We can't let the thread deallocate
----------------
Change the "person" of your comments, here and elsewhere are necessary, by using "we" instead of "I".


================
Comment at: libc/test/integration/src/pthread/pthread_create_test.cpp:285
+  // should guard against that, but people might accidentaly use uninitialized
+  // memory or w.e)
+}
----------------
You can add such tests which are reasonable separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148290