Solve GCC warnings introduced in commit 968a34ffdaadd7db062a9621dfbdf8b2d16e05af #254

bstaletic · 2019-01-20T23:22:17Z

With GCC 8 the commit of the above hash causes -Wclass-memaccess in create_thread_identity.cc, -Wunused-parameter in time_zone_info.cc and -Wmissing-field-initializers in time_zone_libc.cc which are problematic for projects that use -Werror in CI environments.

GCC8 output while compiling abseil:

/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.cc: In member function ‘virtual bool absl::time_internal::cctz::TimeZoneLibC::NextTransition(absl::time_
internal::cctz::time_point<std::chrono::duration<long int> >&, absl::time_internal::cctz::time_zone::civil_transition*) const’:
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.cc:270:62: warning: unused parameter ‘tp’ [-Wunused-parameter]
 bool TimeZoneLibC::NextTransition(const time_point<seconds>& tp,
                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.cc:271:64: warning: unused parameter ‘trans’ [-Wunused-parameter]
                                   time_zone::civil_transition* trans) const {
                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.cc: In member function ‘virtual bool absl::time_internal::cctz::TimeZoneLibC::PrevTransition(absl::time_
internal::cctz::time_point<std::chrono::duration<long int> >&, absl::time_internal::cctz::time_zone::civil_transition*) const’:
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.cc:275:62: warning: unused parameter ‘tp’ [-Wunused-parameter]
 bool TimeZoneLibC::PrevTransition(const time_point<seconds>& tp,
                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.cc:276:64: warning: unused parameter ‘trans’ [-Wunused-parameter]
                                   time_zone::civil_transition* trans) const {
                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc: In member function ‘virtual bool absl::time_internal::cctz::TimeZoneInfo::NextTransition(absl::time_
internal::cctz::time_point<std::chrono::duration<long int> >&, absl::time_internal::cctz::time_zone::civil_transition*) const’:
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc:924:41: warning: missing initializer for member ‘absl::time_internal::cctz::Transition::type_index’ [
-Wmissing-field-initializers]
   const Transition target = { unix_time };
                                         ^
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc:924:41: warning: missing initializer for member ‘absl::time_internal::cctz::Transition::civil_sec’ [-
Wmissing-field-initializers]
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc:924:41: warning: missing initializer for member ‘absl::time_internal::cctz::Transition::prev_civil_se
c’ [-Wmissing-field-initializers]
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc: In member function ‘virtual bool absl::time_internal::cctz::TimeZoneInfo::PrevTransition(absl::time_
internal::cctz::time_point<std::chrono::duration<long int> >&, absl::time_internal::cctz::time_zone::civil_transition*) const’:
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc:959:41: warning: missing initializer for member ‘absl::time_internal::cctz::Transition::type_index’ [
-Wmissing-field-initializers]
   const Transition target = { unix_time };
                                         ^
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc:959:41: warning: missing initializer for member ‘absl::time_internal::cctz::Transition::civil_sec’ [-
Wmissing-field-initializers]
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/time/internal/cctz/src/time_zone_info.cc:959:41: warning: missing initializer for member ‘absl::time_internal::cctz::Transition::prev_civil_se
c’ [-Wmissing-field-initializers]
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/synchronization/internal/create_thread_identity.cc: In function ‘absl::base_internal::ThreadIdentity* absl::synchronization_internal::NewThrea
dIdentity()’:
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/synchronization/internal/create_thread_identity.cc:93:40: warning: ‘void* memset(void*, int, size_t)’ clearing an object of type ‘struct absl:
:base_internal::ThreadIdentity’ with no trivial copy-assignment; use value-initialization instead [-Wclass-memaccess]
   memset(identity, 0, sizeof(*identity));
                                        ^
In file included from /home/bstaletic/Temp/abseil_test/abseil-cpp/absl/synchronization/internal/create_thread_identity.cc:26:
/home/bstaletic/Temp/abseil_test/abseil-cpp/absl/base/internal/thread_identity.h:136:8: note: ‘struct absl::base_internal::ThreadIdentity’ declared here
 struct ThreadIdentity {
        ^~~~~~~~~~~~~~

Reason for introducing ResetThreadIdentity is because not only does absl::base_internal::ThreadIdentity have no trivial copy assignment operator, it has no copy assignment operator at all, because it uses std::atomics, so, considering that both memset and operator= are not allowed, I went for C style "assign every member separately" approach.

derekmauro · 2019-01-21T02:09:11Z

absl/synchronization/internal/create_thread_identity.cc

@@ -67,6 +67,33 @@ static intptr_t RoundUp(intptr_t addr, intptr_t align) {
  return (addr + align - 1) & ~(align - 1);
 }

+namespace {
+
+  void ResetThreadIdetity(base_internal::ThreadIdentity* identity) {


https://github.com/abseil/abseil-cpp/blob/master/CONTRIBUTING.md#coding-style

Please be sure to follow our style-guide, or use clang-format to automatically format your code. In particular, we don't indent here https://google.github.io/styleguide/cppguide.html#Namespaces.

However, local style within this file uses static instead of the anonymous namespace for functions that aren't exported from a file, so prefer that.

Changed to static void ResetThreadIdentity.

derekmauro · 2019-01-21T02:09:48Z

Thanks for the contribution.

This is going to need to be split up. The changes for the files under CCTZ go to a separate project which we automatically import. Please send those changes to https://github.com/google/cctz.

I'm actually hoping to get a GCC8 continuous build up soon, so thanks for making that easier for me.

derekmauro · 2019-01-21T02:14:14Z

absl/synchronization/internal/create_thread_identity.cc

+    pts.cond_waiter = false;
+    pts.all_locks = nullptr;
+    identity->waiter_state = {};
+    identity->blocked_count_ptr = nullptr;


Please use relaxed stores (store(nullptr, std::memory_order_relaxed)) for this and the other atomics.

blocked_count_ptr is a pointer to an atomic int. Is it okay to just reset the pointer or do I first need to store a 0 into the atomic int?

derekmauro · 2019-01-21T02:42:17Z

absl/synchronization/internal/create_thread_identity.cc

+namespace {
+
+  void ResetThreadIdetity(base_internal::ThreadIdentity* identity) {
+    base_internal::PerThreadSynch& pts = identity->per_thread_synch;


Google style usually uses pointers for mutable "reference" variables and references for constant "reference" variables.

I thought that was just about function parameters. Thanks for correcting me.

bstaletic · 2019-01-21T05:51:51Z

This is going to need to be split up. The changes for the files under CCTZ go to a separate project which we automatically import.

I submitted a PR to CCTZ: google/cctz#89

-- 5804cc13b413412988248835459b90cd15ec43d9 by Abseil Team <absl-team@google.com>: Mark raw_hash_set::clear() with the ABSL_ATTRIBUTE_REINITIALIZES attribute. This prevents false positives in the clang-tidy check bugprone-use-after-move; it allows reset() to be called on a moved-from raw_hash_set without any warnings, and the raw_hash_set will thereafter be regarded as initialized again. PiperOrigin-RevId: 230717196 -- ff5961a5600ae19b69a9cba6912126cdf2858f38 by CJ Johnson <johnsoncj@google.com>: Swaps DisableIfIntegral<> for EnableIfInputIterator<> for Iterator member functions of InlinedVector PiperOrigin-RevId: 230559521 -- 3f9754ccbeecbd40f235c6f2465279e045ff51d9 by Derek Mauro <dmauro@google.com>: Import GitHub PR 254 #254 Fixes warnings from -Wclass-memaccess (base_internal::ThreadIdentity? with no trivial copy-assignment). PiperOrigin-RevId: 230536048 -- 8af03a654ce9a4a7f55384bc7eb1ed64878ac2ec by Chris Kennelly <ckennelly@google.com>: absl: cap SpinLock backoff to 4ms The current backoff logic has 3 problems: 1. It can produce too high values (up to 256ms), which can negatively affect tail latency. The value was chosen long time ago and now it's a good idea to reconsider it. 2. It does not have low bound, so on any iteration it can produce a very small value that will lead to unnecessary cpu consumption. 3. It does not increase low bound with the number of iterations. So if the SpinLock is actually somehow locked for a very prolonged time, a waiter can still wake periodically. Rework the logic to solve these problems. Add lower bound of 128us, no code should rely on absence of episodic delays in this range as they can occur everywhere. Lower upper bound to 4ms. A thread sleeping for 4ms does not consume significant cpu time (see below). Grow lower bound with the number of iterations. This is cpu consumption of a process doing usleep(x) in a loop (sampled with ps): 64us -> 4.0% 128us -> 2.7% 256us -> 3.5% 512us -> 2.8% 1024us -> 1.6% 2048us -> 0.6% 4096us -> 0.3% 8192us -> 0.0% Few millisecond sleeps do not consume significant time. PiperOrigin-RevId: 230534015 -- 37ebba92289ca556cb2412cd8b3cb4c1ead3def7 by Samuel Benzaquen <sbenza@google.com>: Add override and dispose hooks to the hashtable sampler. PiperOrigin-RevId: 230353438 -- 89c8f90175233ce9964eb3412df04e8a3cff0c0f by Andy Getzendanner <durandal@google.com>: Fix a comment typo. PiperOrigin-RevId: 229986838 GitOrigin-RevId: 5804cc13b413412988248835459b90cd15ec43d9 Change-Id: Iedb5e2cc9c0b924635c1c87b537780ab6b5b899f

-- 5804cc13b413412988248835459b90cd15ec43d9 by Abseil Team <absl-team@google.com>: Mark raw_hash_set::clear() with the ABSL_ATTRIBUTE_REINITIALIZES attribute. This prevents false positives in the clang-tidy check bugprone-use-after-move; it allows reset() to be called on a moved-from raw_hash_set without any warnings, and the raw_hash_set will thereafter be regarded as initialized again. PiperOrigin-RevId: 230717196 -- ff5961a5600ae19b69a9cba6912126cdf2858f38 by CJ Johnson <johnsoncj@google.com>: Swaps DisableIfIntegral<> for EnableIfInputIterator<> for Iterator member functions of InlinedVector PiperOrigin-RevId: 230559521 -- 3f9754ccbeecbd40f235c6f2465279e045ff51d9 by Derek Mauro <dmauro@google.com>: Import GitHub PR 254 abseil/abseil-cpp#254 Fixes warnings from -Wclass-memaccess (base_internal::ThreadIdentity? with no trivial copy-assignment). PiperOrigin-RevId: 230536048 -- 8af03a654ce9a4a7f55384bc7eb1ed64878ac2ec by Chris Kennelly <ckennelly@google.com>: absl: cap SpinLock backoff to 4ms The current backoff logic has 3 problems: 1. It can produce too high values (up to 256ms), which can negatively affect tail latency. The value was chosen long time ago and now it's a good idea to reconsider it. 2. It does not have low bound, so on any iteration it can produce a very small value that will lead to unnecessary cpu consumption. 3. It does not increase low bound with the number of iterations. So if the SpinLock is actually somehow locked for a very prolonged time, a waiter can still wake periodically. Rework the logic to solve these problems. Add lower bound of 128us, no code should rely on absence of episodic delays in this range as they can occur everywhere. Lower upper bound to 4ms. A thread sleeping for 4ms does not consume significant cpu time (see below). Grow lower bound with the number of iterations. This is cpu consumption of a process doing usleep(x) in a loop (sampled with ps): 64us -> 4.0% 128us -> 2.7% 256us -> 3.5% 512us -> 2.8% 1024us -> 1.6% 2048us -> 0.6% 4096us -> 0.3% 8192us -> 0.0% Few millisecond sleeps do not consume significant time. PiperOrigin-RevId: 230534015 -- 37ebba92289ca556cb2412cd8b3cb4c1ead3def7 by Samuel Benzaquen <sbenza@google.com>: Add override and dispose hooks to the hashtable sampler. PiperOrigin-RevId: 230353438 -- 89c8f90175233ce9964eb3412df04e8a3cff0c0f by Andy Getzendanner <durandal@google.com>: Fix a comment typo. PiperOrigin-RevId: 229986838 GitOrigin-RevId: 5804cc13b413412988248835459b90cd15ec43d9 Change-Id: Iedb5e2cc9c0b924635c1c87b537780ab6b5b899f

googlebot added the cla: yes label Jan 20, 2019

derekmauro reviewed Jan 21, 2019

View reviewed changes

bstaletic mentioned this pull request Jan 21, 2019

Solve GCC8 warnings google/cctz#89

Merged

bstaletic force-pushed the gcc_warnings branch from fc2eaa2 to 3540ce4 Compare January 21, 2019 05:51

Solve GCC warnings

3540ce4

derekmauro added the kokoro:run label Jan 22, 2019

kokoro-team removed the kokoro:run label Jan 22, 2019

Fix spelling of "Identity"

afe0ee8

derekmauro added the kokoro:run label Jan 22, 2019

kokoro-team removed the kokoro:run label Jan 22, 2019

derekmauro merged commit 6b4201f into abseil:master Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Solve GCC warnings introduced in commit 968a34ffdaadd7db062a9621dfbdf8b2d16e05af #254

Solve GCC warnings introduced in commit 968a34ffdaadd7db062a9621dfbdf8b2d16e05af #254

Uh oh!

bstaletic commented Jan 20, 2019

Uh oh!

derekmauro Jan 21, 2019 •

edited

Loading

Uh oh!

bstaletic Jan 21, 2019

Uh oh!

derekmauro commented Jan 21, 2019

Uh oh!

derekmauro Jan 21, 2019

Uh oh!

bstaletic Jan 21, 2019

Uh oh!

derekmauro Jan 21, 2019

Uh oh!

bstaletic Jan 21, 2019

Uh oh!

bstaletic commented Jan 21, 2019

Uh oh!

Uh oh!

Solve GCC warnings introduced in commit 968a34ffdaadd7db062a9621dfbdf8b2d16e05af #254

Solve GCC warnings introduced in commit 968a34ffdaadd7db062a9621dfbdf8b2d16e05af #254

Uh oh!

Conversation

bstaletic commented Jan 20, 2019

Uh oh!

derekmauro Jan 21, 2019 • 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!

bstaletic Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

derekmauro commented Jan 21, 2019

Uh oh!

derekmauro Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

bstaletic Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

derekmauro Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

bstaletic Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

bstaletic commented Jan 21, 2019

Uh oh!

Uh oh!

derekmauro Jan 21, 2019 •

edited

Loading