android: fix pthread compiler conditionals #12081

rebello95 · 2020-07-14T21:48:33Z

This is a follow-up to #12011. The original fix didn't solve the Envoy Mobile issues because Envoy Mobile's Android builds are done on Linux CI, so SUPPORTS_PTHREAD_GETNAME_NP was still being defined as 1.

The changes in this PR ensure that Android API availability supersedes the Linux conditional when defining SUPPORTS_PTHREAD_GETNAME_NP. It also updates us to filter on API version 26 inclusive, since that is the API where this became available.

Signed-off-by: Michael Rebello me@michaelrebello.com

Risk Level: Low
Testing: Local builds / CI in Envoy Mobile
Docs Changes: None

Signed-off-by: Michael Rebello <me@michaelrebello.com>

buildbreaker

Thank you @rebello95

goaway · 2020-07-14T21:54:22Z

include/envoy/common/platform.h

+#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__)


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

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.

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.

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__)

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

goaway · 2020-07-14T21:58:29Z

include/envoy/common/platform.h

+#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__)


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

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.

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

I would still suggest simplifying and renaming.

Signed-off-by: Michael Rebello <me@michaelrebello.com>

goaway · 2020-07-14T22:23:26Z

include/envoy/common/platform.h

+#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__)


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.

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

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).

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());

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.

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.

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

Signed-off-by: Michael Rebello <me@michaelrebello.com>

Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: envoyproxy/envoy#10030 - Fixes for 32 bit archs: envoyproxy/envoy#11726 - Fix for missing posix call on Android: envoyproxy/envoy#12081 - Additional zlib stats: envoyproxy/envoy#11782 Signed-off-by: Mike Schore <mike.schore@gmail.com>

android: fix pthread compiler conditionals Risk Level: Low Testing: Local builds / CI in Envoy Mobile Docs Changes: None Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

android: fix pthread compiler conditionals Risk Level: Low Testing: Local builds / CI in Envoy Mobile Docs Changes: None Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: #10030 - Fixes for 32 bit archs: #11726 - Fix for missing posix call on Android: #12081 - Additional zlib stats: #11782 Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

android: fix pthread compiler conditionals

14bc8b5

Signed-off-by: Michael Rebello <me@michaelrebello.com>

buildbreaker previously approved these changes Jul 14, 2020

View reviewed changes

This was referenced Jul 14, 2020

android: update SUPPORTS_PTHREAD_GETNAME_NP conditional #12079

Closed

upstream: update Envoy upstream sha envoyproxy/envoy-mobile#951

Closed

goaway reviewed Jul 14, 2020

View reviewed changes

CR

32b79c1

Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 dismissed buildbreaker’s stale review via 32b79c1 July 14, 2020 22:15

goaway reviewed Jul 14, 2020

View reviewed changes

address naming nits

0215637

Signed-off-by: Michael Rebello <me@michaelrebello.com>

junr03 approved these changes Jul 14, 2020

View reviewed changes

junr03 merged commit d8b5738 into envoyproxy:master Jul 15, 2020

goaway mentioned this pull request Jul 15, 2020

envoy: update upstream for multiple fixes envoyproxy/envoy-mobile#923

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

android: fix pthread compiler conditionals #12081

android: fix pthread compiler conditionals #12081

rebello95 commented Jul 14, 2020 •

edited

Loading

buildbreaker left a comment

goaway Jul 14, 2020

rebello95 Jul 14, 2020

jmarantz Jul 14, 2020

rebello95 Jul 14, 2020

jmarantz Jul 14, 2020

rebello95 Jul 14, 2020

goaway Jul 14, 2020

rebello95 Jul 14, 2020

goaway Jul 14, 2020

goaway Jul 14, 2020 •

edited

Loading

buildbreaker Jul 14, 2020 •

edited

Loading

rebello95 Jul 14, 2020

goaway Jul 14, 2020

rebello95 Jul 14, 2020

goaway Jul 14, 2020

rebello95 Jul 14, 2020

android: fix pthread compiler conditionals #12081

android: fix pthread compiler conditionals #12081

Conversation

rebello95 commented Jul 14, 2020 • edited Loading

buildbreaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goaway Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

buildbreaker Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebello95 commented Jul 14, 2020 •

edited

Loading

goaway Jul 14, 2020 •

edited

Loading

buildbreaker Jul 14, 2020 •

edited

Loading