android: fix pthread compiler conditionals by rebello95 · Pull Request #12081 · envoyproxy/envoy

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

Conversation

rebello95 commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buildbreaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goaway Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buildbreaker Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rebello95 commented Jul 14, 2020 •

edited

Loading

goaway Jul 14, 2020 •

edited

Loading

buildbreaker Jul 14, 2020 •

edited

Loading