From 0ef9e176b4d3260421d12019bdf2e4d9c7b9d5b7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 11:10:54 -0400 Subject: [PATCH 01/17] MsToAbsoluteTimespecTest added --- app/tests/time_test.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index 09daf7a2a4..bde8dcbe6e 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -62,6 +62,14 @@ TEST(TimeTests, ComparisonTests) { EXPECT_EQ(firebase::internal::TimespecCmp(t1, t1), 0); EXPECT_EQ(firebase::internal::TimespecCmp(t2, t2), 0); } + +TEST(TimeTests, MsToAbsoluteTimespecTest) { + const timespec t1 = firebase::internal::MsToAbsoluteTimespec(0); + const timespec t2 = firebase::internal::MsToAbsoluteTimespec(10000); + const int64_t ms1 = firebase::internal::TimespecToMs(t1); + const int64_t ms2 = firebase::internal::TimespecToMs(t2); + ASSERT_NEAR(ms1, ms2 - 10000, 300); +} #endif // Test GetTimestamp function From b0203106670771f47b873446f8b251b8135effb9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 11:56:26 -0400 Subject: [PATCH 02/17] enable unit tests for ubuntu x86 --- .github/workflows/desktop.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/desktop.yml b/.github/workflows/desktop.yml index 3d5c5073dd..23e24ed2b7 100644 --- a/.github/workflows/desktop.yml +++ b/.github/workflows/desktop.yml @@ -242,7 +242,7 @@ jobs: # command must be invoked in same shell instance of that runs the # tests. # TODO: Enable tests for x86 once they are all working - if: startsWith(matrix.os, 'ubuntu') && matrix.architecture != 'x86' + if: startsWith(matrix.os, 'ubuntu') env: LANG: en_US run: | From d5fc8d343bab9578e5c59c2ceb1e7b5049063c45 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 12:38:02 -0400 Subject: [PATCH 03/17] specify --output-on-failure to ctest --- .github/workflows/desktop.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/desktop.yml b/.github/workflows/desktop.yml index 23e24ed2b7..db28dc7d28 100644 --- a/.github/workflows/desktop.yml +++ b/.github/workflows/desktop.yml @@ -248,7 +248,7 @@ jobs: run: | ulimit -c unlimited cd build - sudo ctest --repeat until-pass:3 + sudo ctest --repeat until-pass:3 --output-on-failure - name: Prep bins for achive (linux) # Copies all of the binary files into one directory for ease in From b3f2ee7cd653e91eab620bd8bf786414694f14a4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 12:59:47 -0400 Subject: [PATCH 04/17] Patch in the MsToAbsoluteTimespec integer overflow fix --- app/src/time.h | 9 +++++++-- app/tests/time_test.cc | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/src/time.h b/app/src/time.h index c52bd2ad67..a60bd7a3e5 100644 --- a/app/src/time.h +++ b/app/src/time.h @@ -89,8 +89,13 @@ inline timespec MsToTimespec(int milliseconds) { inline timespec MsToAbsoluteTimespec(int milliseconds) { timespec t; clock_gettime(CLOCK_REALTIME, &t); - t.tv_nsec += milliseconds * internal::kNanosecondsPerMillisecond; - NormalizeTimespec(&t); + + const int64_t nanoseconds = + t.tv_nsec + (t.tv_sec * internal::kNanosecondsPerSecond) + + (milliseconds * internal::kNanosecondsPerMillisecond); + + t.tv_sec = nanoseconds / internal::kNanosecondsPerSecond; + t.tv_nsec = nanoseconds % internal::kNanosecondsPerSecond; return t; } diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index bde8dcbe6e..a2a2c0ea77 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -63,6 +63,8 @@ TEST(TimeTests, ComparisonTests) { EXPECT_EQ(firebase::internal::TimespecCmp(t2, t2), 0); } +// This test verifies the fix for the old integer overflow bug on 32-bit +// architectures: https://github.com/firebase/firebase-cpp-sdk/pull/1042. TEST(TimeTests, MsToAbsoluteTimespecTest) { const timespec t1 = firebase::internal::MsToAbsoluteTimespec(0); const timespec t2 = firebase::internal::MsToAbsoluteTimespec(10000); From 53f835965e1e7c681608a81e6fc972705192d1fa Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 14:06:56 -0400 Subject: [PATCH 05/17] time_test.cc: Fix NormalizeTest on 32-bit platforms --- app/tests/time_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index a2a2c0ea77..0f3390e718 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -28,11 +28,11 @@ namespace { TEST(TimeTests, NormalizeTest) { timespec t; t.tv_sec = 2; - t.tv_nsec = firebase::internal::kNanosecondsPerSecond * 5.5; + t.tv_nsec = 2100000000; // Must be <= 2,147,483,647 on 32-bit platforms. firebase::internal::NormalizeTimespec(&t); - EXPECT_EQ(t.tv_sec, 7); - EXPECT_EQ(t.tv_nsec, firebase::internal::kNanosecondsPerSecond * 0.5); + EXPECT_EQ(t.tv_sec, 4); + EXPECT_EQ(t.tv_nsec, 100000000); } // Test the various conversions to and from timespecs. From 851df4b89802fc3ba23c404c5386d7fe6450f1e7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 14:07:27 -0400 Subject: [PATCH 06/17] time_test.cc: Replace printf with std::cout in GetTimestampEpochTest to avoid platform-specific %lu %llu, etc. --- app/tests/time_test.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index 0f3390e718..3cab4a0230 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -16,6 +16,9 @@ #include "app/src/time.h" +#include +#include + #include #include "gmock/gmock.h" @@ -97,11 +100,7 @@ TEST(TimeTests, GetTimestampEpochTest) { // Print out the epoch time so that we can verify the timestamp from the log // This is the easiest way to verify if the function works in all platform -#ifdef __linux__ - printf("%lu -> %lu (%ld)\n", start, end, error); -#else - printf("%llu -> %llu (%lld)\n", start, end, error); -#endif // __linux__ + std::cout << std::to_string(start) << " -> " << std::to_string(end) << " (" << std::to_string(error) << ")" << std::endl; EXPECT_GE(end, start + 500); } From 7827651e7ee73fd5cf386e0d530159f841795b48 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 14:11:49 -0400 Subject: [PATCH 07/17] time_test.cc: format code --- app/tests/time_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index 3cab4a0230..3db8f6964e 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -13,14 +13,13 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - #include "app/src/time.h" +#include + #include #include -#include - #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -31,7 +30,7 @@ namespace { TEST(TimeTests, NormalizeTest) { timespec t; t.tv_sec = 2; - t.tv_nsec = 2100000000; // Must be <= 2,147,483,647 on 32-bit platforms. + t.tv_nsec = 2100000000; // Must be <= 2,147,483,647 on 32-bit platforms. firebase::internal::NormalizeTimespec(&t); EXPECT_EQ(t.tv_sec, 4); @@ -100,7 +99,8 @@ TEST(TimeTests, GetTimestampEpochTest) { // Print out the epoch time so that we can verify the timestamp from the log // This is the easiest way to verify if the function works in all platform - std::cout << std::to_string(start) << " -> " << std::to_string(end) << " (" << std::to_string(error) << ")" << std::endl; + std::cout << std::to_string(start) << " -> " << std::to_string(end) << " (" + << std::to_string(error) << ")" << std::endl; EXPECT_GE(end, start + 500); } From 0c48250f7b93bb6d57cfcdfb0795a006ead0c15b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 14:43:51 -0400 Subject: [PATCH 08/17] server_values_test.cc: fix 32-bit flakiness in ServerTimestamp --- database/tests/desktop/core/server_values_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/database/tests/desktop/core/server_values_test.cc b/database/tests/desktop/core/server_values_test.cc index b0040c78bb..b7610282e9 100644 --- a/database/tests/desktop/core/server_values_test.cc +++ b/database/tests/desktop/core/server_values_test.cc @@ -37,7 +37,7 @@ TEST(ServerValues, ServerTimestamp) { } TEST(ServerValues, GenerateServerValues) { - int64_t current_time_ms = time(nullptr) * 1000; + int64_t current_time_ms = static_cast(time(nullptr)) * 1000L; Variant result = GenerateServerValues(0); From 096a75e3ea2271b8d91118428622a935e8351241 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 14:56:32 -0400 Subject: [PATCH 09/17] server_values_test.cc: fix other uses of time() to work on 32-bit architectures --- database/tests/desktop/core/server_values_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/database/tests/desktop/core/server_values_test.cc b/database/tests/desktop/core/server_values_test.cc index b7610282e9..c772869ac5 100644 --- a/database/tests/desktop/core/server_values_test.cc +++ b/database/tests/desktop/core/server_values_test.cc @@ -50,7 +50,7 @@ TEST(ServerValues, GenerateServerValues) { } TEST(ServerValues, GenerateServerValuesWithTimeOffset) { - int64_t current_time_ms = time(nullptr) * 1000; + int64_t current_time_ms = static_cast(time(nullptr)) * 1000L; Variant result = GenerateServerValues(5000); @@ -160,7 +160,7 @@ TEST(ServerValues, ResolveDeferredValueNestedMap) { } TEST(ServerValues, ResolveDeferredValueTimestamp) { - int64_t current_time_ms = time(nullptr) * 1000; + int64_t current_time_ms = static_cast(time(nullptr)) * 1000L; Variant timestamp = ServerTimestamp(); Variant server_values = GenerateServerValues(0); @@ -171,7 +171,7 @@ TEST(ServerValues, ResolveDeferredValueTimestamp) { } TEST(ServerValues, ResolveDeferredValueSnapshot) { - int64_t current_time_ms = time(nullptr) * 1000; + int64_t current_time_ms = static_cast(time(nullptr)) * 1000L; Variant nested_map_variant = std::map{ std::make_pair("aaa", 100), std::make_pair("bbb", 200), @@ -196,7 +196,7 @@ TEST(ServerValues, ResolveDeferredValueSnapshot) { } TEST(ServerValues, ResolveDeferredValueMerge) { - int64_t current_time_ms = time(nullptr) * 1000; + int64_t current_time_ms = static_cast(time(nullptr)) * 1000L; Variant merge(std::map{ std::make_pair("aaa", 100), std::make_pair("bbb", 200), From 8fc48276a575556d5d5fca492bf53980c8aa8c13 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 15:18:16 -0400 Subject: [PATCH 10/17] desktop.yml: enable x86 unit tests on windows and macOS too --- .github/workflows/desktop.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/desktop.yml b/.github/workflows/desktop.yml index db28dc7d28..0ab3b87cf4 100644 --- a/.github/workflows/desktop.yml +++ b/.github/workflows/desktop.yml @@ -228,8 +228,7 @@ jobs: rm -rf ~/Library/Logs/DiagnosticReports/* - name: Run unit tests (windows & macos) - # TODO: Enable tests for x86 once they are all working - if: (startsWith(matrix.os, 'windows') || startsWith(matrix.os, 'macos')) && matrix.architecture != 'x86' && matrix.architecture != 'arm64' + if: (startsWith(matrix.os, 'windows') || startsWith(matrix.os, 'macos')) && matrix.architecture != 'arm64' env: LANG: en_US run: | @@ -241,7 +240,6 @@ jobs: # platform-specific `ulimit` to enable crash collection. The ulimit # command must be invoked in same shell instance of that runs the # tests. - # TODO: Enable tests for x86 once they are all working if: startsWith(matrix.os, 'ubuntu') env: LANG: en_US From c3afb6f9ec5745d5d8de7473b3b10b7d7c7d5c23 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 15:40:30 -0400 Subject: [PATCH 11/17] pthread_condvar.h: Fix ConditionVariable::TimedWait() on 32-bit platforms --- app/src/pthread_condvar.h | 13 +++---------- app/src/time.h | 6 ------ app/tests/time_test.cc | 11 ----------- 3 files changed, 3 insertions(+), 27 deletions(-) diff --git a/app/src/pthread_condvar.h b/app/src/pthread_condvar.h index b265500c14..e21807595f 100644 --- a/app/src/pthread_condvar.h +++ b/app/src/pthread_condvar.h @@ -58,18 +58,11 @@ class ConditionVariable { // Returns true otherwise template bool TimedWait(pthread_mutex_t* lock, Predicate predicate, int milliseconds) { - timespec end_time, current_time; - clock_gettime(CLOCK_REALTIME, &end_time); - end_time.tv_nsec += milliseconds * kNanosecondsPerMillisecond; - int64_t end_time_ms = TimespecToMs(end_time); - NormalizeTimespec(&end_time); - - clock_gettime(CLOCK_REALTIME, ¤t_time); - int64_t current_time_ms = TimespecToMs(current_time); + int64_t end_time_ms = TimespecToMs(MsToAbsoluteTimespec(milliseconds)); + int64_t current_time_ms = TimespecToMs(MsToAbsoluteTimespec(0)); while (!predicate() && current_time_ms < end_time_ms) { TimedWait(lock, end_time_ms - current_time_ms); - clock_gettime(CLOCK_REALTIME, ¤t_time); - current_time_ms = TimespecToMs(current_time); + current_time_ms = TimespecToMs(MsToAbsoluteTimespec(0)); } // If time isn't up, AND the predicate is true, then we return true. // False otherwise. diff --git a/app/src/time.h b/app/src/time.h index a60bd7a3e5..08f88d538c 100644 --- a/app/src/time.h +++ b/app/src/time.h @@ -63,12 +63,6 @@ inline void Sleep(int64_t milliseconds) { } #if !FIREBASE_PLATFORM_WINDOWS -// Utility function for normalizing a timespec. -inline void NormalizeTimespec(timespec* t) { - t->tv_sec += t->tv_nsec / kNanosecondsPerSecond; - t->tv_nsec %= kNanosecondsPerSecond; -} - // Utility function, for converting a timespec struct into milliseconds. inline int64_t TimespecToMs(timespec tm) { return tm.tv_sec * firebase::internal::kMillisecondsPerSecond + diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index a2a2c0ea77..b439246227 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -24,17 +24,6 @@ namespace { #ifndef WIN32 -// Test that the normalize function works, for timespecs -TEST(TimeTests, NormalizeTest) { - timespec t; - t.tv_sec = 2; - t.tv_nsec = firebase::internal::kNanosecondsPerSecond * 5.5; - firebase::internal::NormalizeTimespec(&t); - - EXPECT_EQ(t.tv_sec, 7); - EXPECT_EQ(t.tv_nsec, firebase::internal::kNanosecondsPerSecond * 0.5); -} - // Test the various conversions to and from timespecs. TEST(TimeTests, ConversionTests) { timespec t; From 7fd70c06089e4baa134c25ddc035f68e5da3cbd9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 15:54:55 -0400 Subject: [PATCH 12/17] pthread_condvar.h: delete the templated TimedWait() method since it's not used anywhere --- app/src/pthread_condvar.h | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/app/src/pthread_condvar.h b/app/src/pthread_condvar.h index e21807595f..de7cbb530b 100644 --- a/app/src/pthread_condvar.h +++ b/app/src/pthread_condvar.h @@ -53,22 +53,6 @@ class ConditionVariable { } } - // Waits for the condition variable to go, AND for the predicate to succeed. - // Returns false if it times out before both those conditions are met. - // Returns true otherwise - template - bool TimedWait(pthread_mutex_t* lock, Predicate predicate, int milliseconds) { - int64_t end_time_ms = TimespecToMs(MsToAbsoluteTimespec(milliseconds)); - int64_t current_time_ms = TimespecToMs(MsToAbsoluteTimespec(0)); - while (!predicate() && current_time_ms < end_time_ms) { - TimedWait(lock, end_time_ms - current_time_ms); - current_time_ms = TimespecToMs(MsToAbsoluteTimespec(0)); - } - // If time isn't up, AND the predicate is true, then we return true. - // False otherwise. - return current_time_ms < end_time_ms; - } - void NotifyOne() { pthread_cond_signal(&cond_); } void NotifyAll() { pthread_cond_broadcast(&cond_); } From 41c2b875c53ba003ceabf639b76581c16ad5950b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 16:03:41 -0400 Subject: [PATCH 13/17] desktop.yml: add --output-on-failure to ctest on windows/macos --- .github/workflows/desktop.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/desktop.yml b/.github/workflows/desktop.yml index 0ab3b87cf4..bc10bfdce4 100644 --- a/.github/workflows/desktop.yml +++ b/.github/workflows/desktop.yml @@ -233,7 +233,7 @@ jobs: LANG: en_US run: | cd build - ctest --repeat until-pass:3 + ctest --repeat until-pass:3 --output-on-failure - name: Run unit tests (linux) # Linux exists as its own standalone execution step in order to invoke From 96bae2d64638ff83bbae02c68835af7c84b6f7a1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 16:08:18 -0400 Subject: [PATCH 14/17] time_test.cc: clean up a confusing comment about tv_nsec's max value on 32-bit platforms --- app/tests/time_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index 3db8f6964e..9a985f114a 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -30,7 +30,10 @@ namespace { TEST(TimeTests, NormalizeTest) { timespec t; t.tv_sec = 2; - t.tv_nsec = 2100000000; // Must be <= 2,147,483,647 on 32-bit platforms. + // The maximum value for `tv_nsec` on 32-bit platforms is 2,147,483,647; + // make sure not to use a value larger than this or else integer overflow + // will occur on x86 and other 32-bit platforms. + t.tv_nsec = 2100000000; firebase::internal::NormalizeTimespec(&t); EXPECT_EQ(t.tv_sec, 4); From f31cb1e3725b4b4ca1733f804e722f52cf7ebde6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 16:33:18 -0400 Subject: [PATCH 15/17] Add back the templated TimedWait() method since it is actually used. --- app/src/pthread_condvar.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/app/src/pthread_condvar.h b/app/src/pthread_condvar.h index de7cbb530b..e21807595f 100644 --- a/app/src/pthread_condvar.h +++ b/app/src/pthread_condvar.h @@ -53,6 +53,22 @@ class ConditionVariable { } } + // Waits for the condition variable to go, AND for the predicate to succeed. + // Returns false if it times out before both those conditions are met. + // Returns true otherwise + template + bool TimedWait(pthread_mutex_t* lock, Predicate predicate, int milliseconds) { + int64_t end_time_ms = TimespecToMs(MsToAbsoluteTimespec(milliseconds)); + int64_t current_time_ms = TimespecToMs(MsToAbsoluteTimespec(0)); + while (!predicate() && current_time_ms < end_time_ms) { + TimedWait(lock, end_time_ms - current_time_ms); + current_time_ms = TimespecToMs(MsToAbsoluteTimespec(0)); + } + // If time isn't up, AND the predicate is true, then we return true. + // False otherwise. + return current_time_ms < end_time_ms; + } + void NotifyOne() { pthread_cond_signal(&cond_); } void NotifyAll() { pthread_cond_broadcast(&cond_); } From da99323bd0392bf754e8561d3bd6a42ee1c88f19 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 20:31:33 -0400 Subject: [PATCH 16/17] desktop.yml: re-disable x86 tests on windows, cuz they're still broken --- .github/workflows/desktop.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/desktop.yml b/.github/workflows/desktop.yml index bc10bfdce4..cacba17827 100644 --- a/.github/workflows/desktop.yml +++ b/.github/workflows/desktop.yml @@ -228,7 +228,8 @@ jobs: rm -rf ~/Library/Logs/DiagnosticReports/* - name: Run unit tests (windows & macos) - if: (startsWith(matrix.os, 'windows') || startsWith(matrix.os, 'macos')) && matrix.architecture != 'arm64' + # TODO: Enable tests for x86 once they are all working + if: (startsWith(matrix.os, 'windows') || startsWith(matrix.os, 'macos')) && matrix.architecture != 'x86' && matrix.architecture != 'arm64' env: LANG: en_US run: | From b4dc432eb10d993a4dbc3eb7464c42e0afa8b9ca Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Aug 2022 20:32:38 -0400 Subject: [PATCH 17/17] time_test.cc: add back a blank line that was needlessly deleted --- app/tests/time_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/app/tests/time_test.cc b/app/tests/time_test.cc index 44db03110e..c2cd626438 100644 --- a/app/tests/time_test.cc +++ b/app/tests/time_test.cc @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + #include "app/src/time.h" #include