Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc][pthread] fix -Wmissing-field-initializers #126314

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nickdesaulniers
Copy link
Member

Fixes:

llvm-project/libc/test/integration/src/pthread/pthread_rwlock_test.cpp:59:29:
warning: missing field '__preference' initializer
[-Wmissing-field-initializers]
   59 |   pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER;
      |                             ^

Also, add a test that demonstrates the same issue for
PTHREAD_MUTEX_INITIALIZER, and fix that, too.

PTHREAD_ONCE_INIT does not have this issue and does have test coverage.

Fixes:

    llvm-project/libc/test/integration/src/pthread/pthread_rwlock_test.cpp:59:29:
    warning: missing field '__preference' initializer
    [-Wmissing-field-initializers]
       59 |   pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER;
          |                             ^

Also, add a test that demonstrates the same issue for
PTHREAD_MUTEX_INITIALIZER, and fix that, too.

PTHREAD_ONCE_INIT does not have this issue and does have test coverage.
@llvmbot
Copy link
Member

llvmbot commented Feb 7, 2025

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

Fixes:

llvm-project/libc/test/integration/src/pthread/pthread_rwlock_test.cpp:59:29:
warning: missing field '__preference' initializer
[-Wmissing-field-initializers]
   59 |   pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER;
      |                             ^

Also, add a test that demonstrates the same issue for
PTHREAD_MUTEX_INITIALIZER, and fix that, too.

PTHREAD_ONCE_INIT does not have this issue and does have test coverage.


Full diff: https://github.com/llvm/llvm-project/pull/126314.diff

2 Files Affected:

  • (modified) libc/include/llvm-libc-macros/pthread-macros.h (+2-2)
  • (modified) libc/test/integration/src/pthread/pthread_mutex_test.cpp (+4)
diff --git a/libc/include/llvm-libc-macros/pthread-macros.h b/libc/include/llvm-libc-macros/pthread-macros.h
index 8a144dbd2e611e7..2b1440f9308ca7f 100644
--- a/libc/include/llvm-libc-macros/pthread-macros.h
+++ b/libc/include/llvm-libc-macros/pthread-macros.h
@@ -25,8 +25,8 @@
 #define PTHREAD_PROCESS_PRIVATE 0
 #define PTHREAD_PROCESS_SHARED 1
 
-#define PTHREAD_MUTEX_INITIALIZER {0}
-#define PTHREAD_RWLOCK_INITIALIZER {0}
+#define PTHREAD_MUTEX_INITIALIZER {}
+#define PTHREAD_RWLOCK_INITIALIZER {}
 
 // glibc extensions
 #define PTHREAD_STACK_MIN (1 << 14) // 16KB
diff --git a/libc/test/integration/src/pthread/pthread_mutex_test.cpp b/libc/test/integration/src/pthread/pthread_mutex_test.cpp
index ce2a3538924da84..ec8488da4ead98f 100644
--- a/libc/test/integration/src/pthread/pthread_mutex_test.cpp
+++ b/libc/test/integration/src/pthread/pthread_mutex_test.cpp
@@ -186,6 +186,10 @@ void multiple_waiters() {
   LIBC_NAMESPACE::pthread_mutex_destroy(&counter_lock);
 }
 
+// Test the initializer
+[[gnu::unused]]
+static pthread_mutex_t test_initializer = PTHREAD_MUTEX_INITIALIZER;
+
 TEST_MAIN() {
   relay_counter();
   wait_and_step();

Copy link

github-actions bot commented Feb 7, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 72aa3889fb725ce27817f06c9d7754e78cff9fc2 6ef84f257c414f7f564add2579c8f98fb342bf82 --extensions h,cpp -- libc/include/llvm-libc-macros/pthread-macros.h libc/test/integration/src/pthread/pthread_mutex_test.cpp
View the diff from clang-format here.
diff --git a/libc/include/llvm-libc-macros/pthread-macros.h b/libc/include/llvm-libc-macros/pthread-macros.h
index 2b1440f930..4d68edb75e 100644
--- a/libc/include/llvm-libc-macros/pthread-macros.h
+++ b/libc/include/llvm-libc-macros/pthread-macros.h
@@ -25,8 +25,12 @@
 #define PTHREAD_PROCESS_PRIVATE 0
 #define PTHREAD_PROCESS_SHARED 1
 
-#define PTHREAD_MUTEX_INITIALIZER {}
-#define PTHREAD_RWLOCK_INITIALIZER {}
+#define PTHREAD_MUTEX_INITIALIZER                                              \
+  {                                                                            \
+  }
+#define PTHREAD_RWLOCK_INITIALIZER                                             \
+  {                                                                            \
+  }
 
 // glibc extensions
 #define PTHREAD_STACK_MIN (1 << 14) // 16KB

@nickdesaulniers
Copy link
Member Author

oh gawd clang-format...my eyes!!!

@jhuber6
Copy link
Contributor

jhuber6 commented Feb 7, 2025

Do we really need a macro that uses 10x as many characters as it's replacing.

@nickdesaulniers
Copy link
Member Author

Homey, that boat sailed 100 years ago. That feedback should have been to the austin group back when they made up this shit!

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Feb 7, 2025

Also, I assume that as opaque types, you could perhaps have non-zero values you'd like to initialize these members to, so declaring static pthread_rwlock_t lock; would produce a pthread_rwlock_t with DIFFERENT values than one initialized via pthread_rwlock_init (static pthread_rwlock_t lock = PTHREAD_RWLOCK_INITIALIZER; being the fix, IIUC).

They probably also made this up BEFORE they had designated initializers; gotta love aggregate initializers! Don't mess up the order ever! Refactor struct definitions at your peril!

@nickdesaulniers
Copy link
Member Author

oh gawd clang-format...my eyes!!!

So, echo '#define foo {}' | clang-format - can produce different output based on where in the tree you are. IIUC, clang-format will traverse up until it finds a .clang-format. We have effectively echo '#define foo {}' | clang-format --style='{BasedOnStyle: LLVM}' - due to llvm-project/.clang-format.

But there may be a format option that fixes this (who knows if it's specific enough though, or too broad). Still digging in https://clang.llvm.org/docs/ClangFormatStyleOptions.html.

@nickdesaulniers
Copy link
Member Author

echo '#define foo {}' | clang-format --style='{SkipMacroDefinitionBody: true}' - does it; probably don't want that generally though.

@frobtech
Copy link
Contributor

frobtech commented Feb 8, 2025

Homey, that boat sailed 100 years ago. That feedback should have been to the austin group back when they made up this shit!

This is moot anyway, because you can't use {} in C (maybe C23 allows it or something, but not C99 without GNU extensions, definitely). Short of using pragma magic to just suppress the warning (which I'm not even sure you can do in this context), you just have to actually have a value listed for each member and the right number of commas.

@@ -186,6 +186,10 @@ void multiple_waiters() {
LIBC_NAMESPACE::pthread_mutex_destroy(&counter_lock);
}

// Test the initializer
[[gnu::unused]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not [[maybe_unused]]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants