Skip to content

android: fix pthread compiler conditionals #12081

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.

Sign up for GitHub

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

Merged
junr03 merged 3 commits into from
Jul 15, 2020
Merged

android: fix pthread compiler conditionals #12081

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14bc8b5
android: fix pthread compiler conditionals
rebello95 Jul 14, 2020
32b79c1
CR
rebello95 Jul 14, 2020
0215637
address naming nits
rebello95 Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 7 additions & 14 deletions include/envoy/common/platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,19 +248,12 @@ struct mmsghdr {
#endif // __ANDROID_API__ < 24
#endif // ifdef __ANDROID_API__

#ifdef __linux__
#define SUPPORTS_PTHREAD_GETNAME_NP 1
#endif

// https://android.googlesource.com/platform/bionic/+/master/docs/status.md
// ``pthread_getname_np`` is introduced in API 26
#ifdef __ANDROID_API__
#if __ANDROID_API__ > 26
#define SUPPORTS_PTHREAD_GETNAME_NP 1
#endif // __ANDROID_API__ > 26
#endif // ifdef __ANDROID_API__

// Ensure `SUPPORTS_PTHREAD_GETNAME_NP` is set
#ifndef SUPPORTS_PTHREAD_GETNAME_NP
#define SUPPORTS_PTHREAD_GETNAME_NP 0
#endif
#if defined(__ANDROID_API__)
#if __ANDROID_API__ >= 26
#define SUPPORTS_PTHREAD_GETNAME_NP
#endif // __ANDROID_API__ >= 26
#elif defined(__linux__)
#define SUPPORTS_PTHREAD_GETNAME_NP
#endif // defined(__ANDROID_API__)
Comment on lines +253 to +262

@goaway goaway Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jmarantz requested on the other PR that these be set to numerical values for (potential) boolean logic comparisons.

@rebello95 rebello95 Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I opted for basic #defines here to keep the logic in this block simple. Using numeric-backed definitions would make this significantly more complicated and less readable. If people feel strongly I can change it.

@jmarantz jmarantz Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think that would change the logic at all; you'd just be adding a 1 to the end of a couple of lines.

@rebello95 rebello95 Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We would also need to either add 2 #else cases to define it as zero in all branches, or add a separate block with an #ifndef, right?

#if defined(__ANDROID_API__)
#if __ANDROID_API__ >= 26
#define SUPPORTS_PTHREAD_GETNAME_NP 1
#else
#define SUPPORTS_PTHREAD_GETNAME_NP 0
#endif // __ANDROID_API__ >= 26
#elif defined(__linux__)
#define SUPPORTS_PTHREAD_GETNAME_NP 1
#else
#define SUPPORTS_PTHREAD_GETNAME_NP 0
#endif // defined(__ANDROID_API__)

@jmarantz jmarantz Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i see. just define it to 0 at the start and undef/redefine it to 1 as needed?

@rebello95 rebello95 Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines +253 to +262

@goaway goaway Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if defined(__ANDROID_API__)
#if __ANDROID_API__ >= 26
#define SUPPORTS_PTHREAD_GETNAME_NP
#endif // __ANDROID_API__ >= 26
#elif defined(__linux__)
#define SUPPORTS_PTHREAD_GETNAME_NP
#endif // defined(__ANDROID_API__)
#define SUPPORTS_PTHREAD_NAMING 0
#ifdef __linux__
#define SUPPORTS_PTHREAD_NAMING 1
#endif
#if defined(__ANDROID_API__) && __ANDROID_API__ < 26
#define SUPPORTS_PTHREAD_NAMING 0
#endif

@rebello95 rebello95 Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we do this the compiler will complain that we're redefining SUPPORTS_PTHREAD_NAMING.

bazel-out/android-x86-fastbuild/bin/external/envoy/include/envoy/common/_virtual_includes/base_includes/envoy/common/platform.h:255:9: error: 'SUPPORTS_PTHREAD_NAMING' macro redefined [-Werror,-Wmacro-redefined]
#define SUPPORTS_PTHREAD_NAMING 1
        ^
bazel-out/android-x86-fastbuild/bin/external/envoy/include/envoy/common/_virtual_includes/base_includes/envoy/common/platform.h:253:9: note: previous definition is here
#define SUPPORTS_PTHREAD_NAMING 0
        ^
bazel-out/android-x86-fastbuild/bin/external/envoy/include/envoy/common/_virtual_includes/base_includes/envoy/common/platform.h:258:9: error: 'SUPPORTS_PTHREAD_NAMING' macro redefined [-Werror,-Wmacro-redefined]
#define SUPPORTS_PTHREAD_NAMING 0
        ^
bazel-out/android-x86-fastbuild/bin/external/envoy/include/envoy/common/_virtual_includes/base_includes/envoy/common/platform.h:255:9: note: previous definition is here
#define SUPPORTS_PTHREAD_NAMING 1
        ^
2 errors generated.

@goaway goaway Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, we're being strict with -Wmacro-redfined.

I would still suggest simplifying and renaming.

Comment on lines +254 to +262

@goaway goaway Jul 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if defined(__ANDROID_API__)
#if __ANDROID_API__ >= 26
#undef SUPPORTS_PTHREAD_GETNAME_NP
#define SUPPORTS_PTHREAD_GETNAME_NP 1
#endif // __ANDROID_API__ >= 26
#elif defined(__linux__)
#undef SUPPORTS_PTHREAD_GETNAME_NP
#define SUPPORTS_PTHREAD_GETNAME_NP 1
#endif // defined(__ANDROID_API__)
#define SUPPORTS_PTHREAD_NAMING 0
#if defined(__linux__) && !defined(__ANDROID_API__) || __ANDROID_API__ >= 26
#undef SUPPORTS_PTHREAD_NAMING
#define SUPPORTS_PTHREAD_NAMING 1
#endif

How about this?

The renaming suggestion to SUPPORTS_PTHREAD_NAMING is because this macro name implies we only care about the existence of one* method, when in fact we're using/guarding two below.

@buildbreaker buildbreaker Jul 14, 2020 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We're currently only guarding against the one method right now I believe

@rebello95 rebello95 Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're currently only guarding against the one method right now I believe

Right, pthread_getname_np is the only one we're missing, so I think the existing name more closely reflects why we're guarding with this conditional.

Also, your suggestion is not equivalent to what I have on this branch @goaway. I'm not requiring __linux__, but instead am prioritizing the value of Android's API version if it's specified (which is more similar to what was in place prior to this change).

@goaway goaway Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

envoy/source/common/common/posix/thread_impl.cc

Line 62 in 8a21fc7

const int set_name_rc = pthread_setname_np(thread_handle_, name_.c_str());

@rebello95 rebello95 Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm aware that pthread_setname_np is being called within the compiler conditional, but pthread_getname_np is the function that is unavailable, as detailed in the docs linked inline. AFAICT pthread_setname_np is gated by the conditional because it shouldn't be called if pthread_getname_np is unavailable.

@goaway goaway Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We use the guard to generally disable the logic around pthread naming - both set and get.

The boolean logic I posted above is equivalent to what you have, it just reduces the number of conditionals.

@rebello95 rebello95 Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't have that strong of an opinion here. Renamed to what you requested.

4 changes: 2 additions & 2 deletions source/common/common/posix/thread_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class ThreadImplPosix : public Thread {
this);
RELEASE_ASSERT(rc == 0, "");

#if SUPPORTS_PTHREAD_GETNAME_NP
#ifdef SUPPORTS_PTHREAD_GETNAME_NP
// If the name was not specified, get it from the OS. If the name was
// specified, write it into the thread, and assert that the OS sees it the
// same way.
Expand Down Expand Up @@ -93,7 +93,7 @@ class ThreadImplPosix : public Thread {
}

private:
#if SUPPORTS_PTHREAD_GETNAME_NP
#ifdef SUPPORTS_PTHREAD_GETNAME_NP
// Attempts to get the name from the operating system, returning true and
// updating 'name' if successful. Note that during normal operation this
// may fail, if the thread exits prior to the system call.
Expand Down