net::WindowedFilterTest: Add explicit overflow checks

net::WindowedFilterTest: Add explicit overflow checks

Aggressive inlining causes -Wstrict-overflow to be triggered.

BUG=616957
BUG=internal b/29059135

Committed: https://crrev.com/d66635d239b6f91dcb7e11160a0bee7d99bb2e92
Cr-Commit-Position: refs/heads/master@{#398092}

[bcf, 2016-06-02 01:33:09]

Description was changed from

==========
net::WindowedFilter::Update: Ignore -Wstrict-overflow for GCC

Agressive inlining causes -Wstrict-overflow to be triggered.

BUG=internal b/29059135
==========

to

==========
net::WindowedFilter::Update: Ignore -Wstrict-overflow for GCC

Agressive inlining causes -Wstrict-overflow to be triggered.

BUG=internal b/29059135
==========

[bcf, 2016-06-02 01:33:09]

bcf@chromium.org changed reviewers:
+ fayang@chromium.org, rch@chromium.org, rtenneti@chromium.org

[bcf, 2016-06-02 01:34:41]

Thoughts on this?

Using the compiler
armv7a-cros-linux-gnueabi-g++.real.elf (4.9.2_cos_gg_21201ea_4.9.2-r116)
4.9.x-google 20150123 (prerelease)

We can't build windowed_filter_test.cc:

In file included from
../../net/quic/congestion_control/windowed_filter_test.cc:5:0:
../../net/quic/congestion_control/windowed_filter.h: In member function 'virtual
void
net::test::{anonymous}::WindowedFilterTest_SampleChangesThirdBestMin_Test::TestBody()':
../../net/quic/congestion_control/windowed_filter.h:84:12: error: assuming
signed overflow does not occur when assuming that (X - c) <= X is always true
[-Werror=strict-overflow]
     } else if (Compare()(new_sample, estimates_[2].sample)) {
            ^
../../net/quic/congestion_control/windowed_filter.h: In member function 'virtual
void
net::test::{anonymous}::WindowedFilterTest_SampleChangesSecondBestMin_Test::TestBody()':
../../net/quic/congestion_control/windowed_filter.h:81:5: error: assuming signed
overflow does not occur when assuming that (X - c) <= X is always true
[-Werror=strict-overflow]
     if (Compare()(new_sample, estimates_[1].sample)) {
     ^
../../net/quic/congestion_control/windowed_filter.h: In member function 'virtual
void
net::test::{anonymous}::WindowedFilterTest_SampleChangesAllMins_Test::TestBody()':
../../net/quic/congestion_control/windowed_filter.h:74:5: error: assuming signed
overflow does not occur when assuming that (X - c) <= X is always true
[-Werror=strict-overflow]
     if (estimates_[0].sample == zero_value_ ||
     ^
../../net/quic/congestion_control/windowed_filter.h: In member function 'virtual
void net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
../../net/quic/congestion_control/windowed_filter.h:84:12: error: assuming
signed overflow does not occur when assuming that (X + c) < X is always false
[-Werror=strict-overflow]
     } else if (Compare()(new_sample, estimates_[2].sample)) {

[Ryan Hamilton, 2016-06-02 03:19:55]

rch@chromium.org changed reviewers:
+ jri@chromium.org

[Ryan Hamilton, 2016-06-02 03:20:47]

On 2016/06/02 01:34:41, bcf wrote:
> Thoughts on this?
> 
> Using the compiler
> armv7a-cros-linux-gnueabi-g++.real.elf (4.9.2_cos_gg_21201ea_4.9.2-r116)
> 4.9.x-google 20150123 (prerelease)
> 
> We can't build windowed_filter_test.cc:
> 
> In file included from
> ../../net/quic/congestion_control/windowed_filter_test.cc:5:0:
> ../../net/quic/congestion_control/windowed_filter.h: In member function
'virtual
> void
>
net::test::{anonymous}::WindowedFilterTest_SampleChangesThirdBestMin_Test::TestBody()':
> ../../net/quic/congestion_control/windowed_filter.h:84:12: error: assuming
> signed overflow does not occur when assuming that (X - c) <= X is always true
> [-Werror=strict-overflow]
>      } else if (Compare()(new_sample, estimates_[2].sample)) {

I don't understand this error, do you? I don't see any subtraction on that line.
Can you explain what it's complaining about?

[bcf, 2016-06-02 04:01:08]

> I don't understand this error, do you? I don't see any subtraction on that
line.
> Can you explain what it's complaining about?

The error happens in SampleChangesThirdBestMin
https://chromium.googlesource.com/chromium/src/+/ee1e90c49ebd7bd9204ffd601be1...
====
TEST_F(WindowedFilterTest, SampleChangesThirdBestMin) {
  ...
  QuicTime::Delta rtt_sample = windowed_min_rtt_.GetThirdBest().Subtract(
      QuicTime::Delta::FromMilliseconds(5));
  ...
  QuicTime now = QuicTime::Zero().Add(QuicTime::Delta::FromMilliseconds(101));
  windowed_min_rtt_.Update(rtt_sample, now);

====
T GetThirdBest() const { return estimates_[2].sample; }
https://chromium.googlesource.com/chromium/src/+/ee1e90c49ebd7bd9204ffd601be1...

so this means
rtt_sample = estimates_[2].sample - QuicTime::Delta::FromMilliseconds(5)): 

The error is caused in the call to windowed_max_bw_.Update.

rtt_sample corresponds to new_sample in the line

} else if (Compare()(new_sample, estimates_[2].sample)) {

So expanding compare as <=
(https://chromium.googlesource.com/chromium/src/+/ee1e90c49ebd7bd9204ffd601be1...),

this becomes

} else if ((estimates_[2].sample - QuicTime::Delta::FromMilliseconds(5)) <=
estimates_[2].sample) {

[bcf, 2016-06-02 04:06:59]

On 2016/06/02 04:01:08, bcf wrote:
> The error is caused in the call to windowed_max_bw_.Update.

Correction: windowed_min_rtt_.Update(rtt_sample, now);

[Jana, 2016-06-02 21:29:14]

On 2016/06/02 04:06:59, bcf wrote:
> On 2016/06/02 04:01:08, bcf wrote:
> > The error is caused in the call to windowed_max_bw_.Update.
> 
> Correction: windowed_min_rtt_.Update(rtt_sample, now);

That makes sense. Though, I don't think this error should be fixed in the
windowed_filter.h, since the thing that's triggering the strict-overflow check
to fail comes from the Subtract line that you point out in
windowed_filter_test.cc. There are three such subtractions, which can trigger
this check to fail. We do know for fact that the subtractions won't overflow,
since InitializeMinFilter(), which is called from the tests, has an EXPECT
confirming that the value in estimates_[2].sample is 50ms.

Can you check if the following works? Add the following three ASSERTs just after
Subtract()s are called in windowed_filter_test.cc:
line 155: ASSERT_GT(QuicTime::Delta::FromMilliseconds(5),
windowed_min_rtt_.GetThirdBest());
line 184: ASSERT_GT(QuicTime::Delta::FromMilliseconds(5),
windowed_min_rtt_.GetSecondBest());
line 212: ASSERT_GT(QuicTime::Delta::FromMilliseconds(5),
windowed_min_rtt_.GetBest());

Hopefully that will help?

[bcf, 2016-06-02 23:35:12]

On 2016/06/02 21:29:14, Jana wrote:
> On 2016/06/02 04:06:59, bcf wrote:
> > On 2016/06/02 04:01:08, bcf wrote:
> > > The error is caused in the call to windowed_max_bw_.Update.
> > 
> > Correction: windowed_min_rtt_.Update(rtt_sample, now);
> 
> That makes sense. Though, I don't think this error should be fixed in the
> windowed_filter.h, since the thing that's triggering the strict-overflow check
> to fail comes from the Subtract line that you point out in
> windowed_filter_test.cc. There are three such subtractions, which can trigger
> this check to fail. We do know for fact that the subtractions won't overflow,
> since InitializeMinFilter(), which is called from the tests, has an EXPECT
> confirming that the value in estimates_[2].sample is 50ms.
> 
> Can you check if the following works? Add the following three ASSERTs just
after
> Subtract()s are called in windowed_filter_test.cc:
> line 155: ASSERT_GT(QuicTime::Delta::FromMilliseconds(5),
> windowed_min_rtt_.GetThirdBest());
> line 184: ASSERT_GT(QuicTime::Delta::FromMilliseconds(5),
> windowed_min_rtt_.GetSecondBest());
> line 212: ASSERT_GT(QuicTime::Delta::FromMilliseconds(5),
> windowed_min_rtt_.GetBest());
> 
> Hopefully that will help?

Looks like the adding explicit overflow checks like this help. PTAL.

[bcf, 2016-06-02 23:38:43]

Description was changed from

==========
net::WindowedFilter::Update: Ignore -Wstrict-overflow for GCC

Agressive inlining causes -Wstrict-overflow to be triggered.

BUG=internal b/29059135
==========

to

==========
net::WindowedFilterTest: Add explicit overflow checks

Aggressive inlining causes -Wstrict-overflow to be triggered.

BUG=616957
BUG=internal b/29059135
==========

[Jana, 2016-06-02 23:39:37]

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
File net/quic/congestion_control/windowed_filter_test.cc (right):

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
net/quic/congestion_control/windowed_filter_test.cc:300:
QuicTime::Delta::FromMilliseconds(5).ToMicroseconds()));
Why is this necessary? Does the compiler complain here as well?

[bcf, 2016-06-02 23:42:47]

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
File net/quic/congestion_control/windowed_filter_test.cc (right):

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
net/quic/congestion_control/windowed_filter_test.cc:300:
QuicTime::Delta::FromMilliseconds(5).ToMicroseconds()));
On 2016/06/02 23:39:37, Jana wrote:
> Why is this necessary? Does the compiler complain here as well?

Yes, See on the first comment:

../../net/quic/congestion_control/windowed_filter.h: In member function 'virtual
void net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
../../net/quic/congestion_control/windowed_filter.h:84:12: error: assuming
signed overflow does not occur when assuming that (X + c) < X is always false
[-Werror=strict-overflow]
     } else if (Compare()(new_sample, estimates_[2].sample)) {

[Jana, 2016-06-03 00:14:26]

Thanks! Suggestion below

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
File net/quic/congestion_control/windowed_filter_test.cc (right):

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
net/quic/congestion_control/windowed_filter_test.cc:300:
QuicTime::Delta::FromMilliseconds(5).ToMicroseconds()));
On 2016/06/02 23:42:47, bcf wrote:
> On 2016/06/02 23:39:37, Jana wrote:
> > Why is this necessary? Does the compiler complain here as well?
> 
> Yes, See on the first comment:
> 
> ../../net/quic/congestion_control/windowed_filter.h: In member function
'virtual
> void
net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
> ../../net/quic/congestion_control/windowed_filter.h:84:12: error: assuming
> signed overflow does not occur when assuming that (X + c) < X is always false
> [-Werror=strict-overflow]
>      } else if (Compare()(new_sample, estimates_[2].sample)) {

Summarizing our offline chat: QuicTime::Delta::Add() and Subtract() are defined
in quic_time.h, which probably gets inlined due to optimizations at compile
time. This should have been happening with the various QuicBandwidth::Add()
calls above as well, but the compiler seems happy there likely because the
definition of QuicBandwidth::Add() is in quic_bandwidth.cc, which gets pulled in
at link time. (Thanks, rch, for helping figure this out.)

I think you can simplify this ASSERT here to not need a Max() at all. Can you
try the following instead:
ASSERT_LT(windowed_min_rtt_.GetThirdBest(), rtt_sample);

[bcf, 2016-06-03 00:25:38]

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
File net/quic/congestion_control/windowed_filter_test.cc (right):

https://codereview.chromium.org/2036613002/diff/20001/net/quic/congestion_con...
net/quic/congestion_control/windowed_filter_test.cc:300:
QuicTime::Delta::FromMilliseconds(5).ToMicroseconds()));
On 2016/06/03 00:14:26, Jana wrote:
> On 2016/06/02 23:42:47, bcf wrote:
> > On 2016/06/02 23:39:37, Jana wrote:
> > > Why is this necessary? Does the compiler complain here as well?
> > 
> > Yes, See on the first comment:
> > 
> > ../../net/quic/congestion_control/windowed_filter.h: In member function
> 'virtual
> > void
> net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
> > ../../net/quic/congestion_control/windowed_filter.h:84:12: error: assuming
> > signed overflow does not occur when assuming that (X + c) < X is always
false
> > [-Werror=strict-overflow]
> >      } else if (Compare()(new_sample, estimates_[2].sample)) {
> 
> Summarizing our offline chat: QuicTime::Delta::Add() and Subtract() are
defined
> in quic_time.h, which probably gets inlined due to optimizations at compile
> time. This should have been happening with the various QuicBandwidth::Add()
> calls above as well, but the compiler seems happy there likely because the
> definition of QuicBandwidth::Add() is in quic_bandwidth.cc, which gets pulled
in
> at link time. (Thanks, rch, for helping figure this out.)
> 
> I think you can simplify this ASSERT here to not need a Max() at all. Can you
> try the following instead:
> ASSERT_LT(windowed_min_rtt_.GetThirdBest(), rtt_sample);
> 
> 

It seems like this successfully satisfies the compiler, but this isn't
technically a valid overflow check because if rtt_sample overflowed the result
is undefined.

I guess it's not a big deal because we know it won't overflow.

What do you think?

[Jana, 2016-06-03 00:29:11]

lgtm

Thanks much for fixing this! 

I'm comfortable with just pacifying the compiler, since we have an EXPECT in
InitializeMinFilter that checks that the values are well within range.

[Ryan Hamilton, 2016-06-03 16:27:45]

lgtm

[ramant (doing other things), 2016-06-03 16:28:33]

rtenneti@chromium.org changed reviewers:
- rtenneti@chromium.org

[bcf, 2016-06-03 18:14:48]

The CQ bit was checked by bcf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 18:15:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036613002/40001

[commit-bot: I haz the power, 2016-06-03 18:50:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 18:50:11]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[bcf, 2016-06-03 18:57:49]

https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
File net/quic/congestion_control/windowed_filter_test.cc (right):

https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
net/quic/congestion_control/windowed_filter_test.cc:297:
ASSERT_LT(windowed_min_rtt_.GetThirdBest(), rtt_sample);
On 2016/06/03 18:50:11, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...

In file included from
../../net/quic/congestion_control/windowed_filter_test.cc:11:0:
../../testing/gtest/include/gtest/gtest.h: In member function 'virtual void
net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
../../testing/gtest/include/gtest/gtest.h:1514:3: error: assuming signed
overflow does not occur when assuming that (X + c) >= X is always true
[-Werror=strict-overflow]
   if (val1 op val2) {\
   ^
../../testing/gtest/include/gtest/gtest.h:1530:1: note: in expansion of macro
'GTEST_IMPL_CMP_HELPER_'
 GTEST_IMPL_CMP_HELPER_(LT, <);

Looks like this is failing for the same reason, as it expands to this:

ASSERT_LT(windowed_min_rtt_.GetThirdBest(), windowed_min_rtt_.GetThirdBest() +
QuicTime::Delta::FromMilliseconds(5));

Interestingly this doesn't happen on our compiler.

[bcf, 2016-06-03 19:32:52]

On 2016/06/03 18:57:49, bcf wrote:
>
https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
> File net/quic/congestion_control/windowed_filter_test.cc (right):
> 
>
https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
> net/quic/congestion_control/windowed_filter_test.cc:297:
> ASSERT_LT(windowed_min_rtt_.GetThirdBest(), rtt_sample);
> On 2016/06/03 18:50:11, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
> 
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...
> 
> In file included from
> ../../net/quic/congestion_control/windowed_filter_test.cc:11:0:
> ../../testing/gtest/include/gtest/gtest.h: In member function 'virtual void
> net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
> ../../testing/gtest/include/gtest/gtest.h:1514:3: error: assuming signed
> overflow does not occur when assuming that (X + c) >= X is always true
> [-Werror=strict-overflow]
>    if (val1 op val2) {\
>    ^
> ../../testing/gtest/include/gtest/gtest.h:1530:1: note: in expansion of macro
> 'GTEST_IMPL_CMP_HELPER_'
>  GTEST_IMPL_CMP_HELPER_(LT, <);
> 
> Looks like this is failing for the same reason, as it expands to this:
> 
> ASSERT_LT(windowed_min_rtt_.GetThirdBest(), windowed_min_rtt_.GetThirdBest() +
> QuicTime::Delta::FromMilliseconds(5));
> 
> Interestingly this doesn't happen on our compiler.

Some possible WARs:

// Prevent compiler from doing the compile time optimization with volatile
TEST_F(WindowedFilterTest, ExpireAllMins) {
  ...
  const volatile int kOffsetMs = 5;
  QuicTime::Delta rtt_sample = windowed_min_rtt_.GetThirdBest().Add(
      QuicTime::Delta::FromMilliseconds(kOffsetMs));


// Define an arbitrary max value in windowed_filter_test.cc and validate against
that value
const int kMaxQuicDeltaSeconds = 1000000;
...
TEST_F(WindowedFilterTest, ExpireAllMins) {
  ...
  ASSERT_LT(windowed_min_rtt_.GetThirdBest(),
            QuicTime::Delta::FromSeconds(kMaxQuicDeltaSeconds));
  QuicTime::Delta rtt_sample = windowed_min_rtt_.GetThirdBest().Add(
      QuicTime::Delta::FromMilliseconds(5));


Thoughts?

[Jana, 2016-06-03 22:03:13]

On 2016/06/03 19:32:52, bcf wrote:
> On 2016/06/03 18:57:49, bcf wrote:
> >
>
https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
> > File net/quic/congestion_control/windowed_filter_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
> > net/quic/congestion_control/windowed_filter_test.cc:297:
> > ASSERT_LT(windowed_min_rtt_.GetThirdBest(), rtt_sample);
> > On 2016/06/03 18:50:11, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
> > 
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...
> > 
> > In file included from
> > ../../net/quic/congestion_control/windowed_filter_test.cc:11:0:
> > ../../testing/gtest/include/gtest/gtest.h: In member function 'virtual void
> > net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
> > ../../testing/gtest/include/gtest/gtest.h:1514:3: error: assuming signed
> > overflow does not occur when assuming that (X + c) >= X is always true
> > [-Werror=strict-overflow]
> >    if (val1 op val2) {\
> >    ^
> > ../../testing/gtest/include/gtest/gtest.h:1530:1: note: in expansion of
macro
> > 'GTEST_IMPL_CMP_HELPER_'
> >  GTEST_IMPL_CMP_HELPER_(LT, <);
> > 
> > Looks like this is failing for the same reason, as it expands to this:
> > 
> > ASSERT_LT(windowed_min_rtt_.GetThirdBest(), windowed_min_rtt_.GetThirdBest()
+
> > QuicTime::Delta::FromMilliseconds(5));
> > 
> > Interestingly this doesn't happen on our compiler.
> 
> Some possible WARs:
> 
> // Prevent compiler from doing the compile time optimization with volatile
> TEST_F(WindowedFilterTest, ExpireAllMins) {
>   ...
>   const volatile int kOffsetMs = 5;
>   QuicTime::Delta rtt_sample = windowed_min_rtt_.GetThirdBest().Add(
>       QuicTime::Delta::FromMilliseconds(kOffsetMs));
> 
> 
> // Define an arbitrary max value in windowed_filter_test.cc and validate
against
> that value
> const int kMaxQuicDeltaSeconds = 1000000;
> ...
> TEST_F(WindowedFilterTest, ExpireAllMins) {
>   ...
>   ASSERT_LT(windowed_min_rtt_.GetThirdBest(),
>             QuicTime::Delta::FromSeconds(kMaxQuicDeltaSeconds));
>   QuicTime::Delta rtt_sample = windowed_min_rtt_.GetThirdBest().Add(
>       QuicTime::Delta::FromMilliseconds(5));
> 
> 
> Thoughts?

If the ASSERT earlier that I suggested doesn't work, using "volatile" sounds
good to me.

[Jana, 2016-06-03 22:03:26]

https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
File net/quic/congestion_control/windowed_filter_test.cc (right):

https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
net/quic/congestion_control/windowed_filter_test.cc:297:
ASSERT_LT(windowed_min_rtt_.GetThirdBest(), rtt_sample);
On 2016/06/03 18:57:49, bcf wrote:
> On 2016/06/03 18:50:11, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
> 
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...
> 
> In file included from
> ../../net/quic/congestion_control/windowed_filter_test.cc:11:0:
> ../../testing/gtest/include/gtest/gtest.h: In member function 'virtual void
> net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
> ../../testing/gtest/include/gtest/gtest.h:1514:3: error: assuming signed
> overflow does not occur when assuming that (X + c) >= X is always true
> [-Werror=strict-overflow]
>    if (val1 op val2) {\
>    ^
> ../../testing/gtest/include/gtest/gtest.h:1530:1: note: in expansion of macro
> 'GTEST_IMPL_CMP_HELPER_'
>  GTEST_IMPL_CMP_HELPER_(LT, <);
> 
> Looks like this is failing for the same reason, as it expands to this:
> 
> ASSERT_LT(windowed_min_rtt_.GetThirdBest(), windowed_min_rtt_.GetThirdBest() +
> QuicTime::Delta::FromMilliseconds(5));
> 
> Interestingly this doesn't happen on our compiler.

How about the following ASSERT:
ASSERT_LT(windowed_min_rtt_.GetThirdBest(),
QuicTime::Delta::Infinite().Subtract(QuicTime::Delta::FromMilliSeconds(5)))

[bcf, 2016-06-03 23:45:09]

On 2016/06/03 22:03:26, Jana wrote:
>
https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
> File net/quic/congestion_control/windowed_filter_test.cc (right):
> 
>
https://codereview.chromium.org/2036613002/diff/40001/net/quic/congestion_con...
> net/quic/congestion_control/windowed_filter_test.cc:297:
> ASSERT_LT(windowed_min_rtt_.GetThirdBest(), rtt_sample);
> On 2016/06/03 18:57:49, bcf wrote:
> > On 2016/06/03 18:50:11, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
> > 
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...
> > 
> > In file included from
> > ../../net/quic/congestion_control/windowed_filter_test.cc:11:0:
> > ../../testing/gtest/include/gtest/gtest.h: In member function 'virtual void
> > net::test::{anonymous}::WindowedFilterTest_ExpireAllMins_Test::TestBody()':
> > ../../testing/gtest/include/gtest/gtest.h:1514:3: error: assuming signed
> > overflow does not occur when assuming that (X + c) >= X is always true
> > [-Werror=strict-overflow]
> >    if (val1 op val2) {\
> >    ^
> > ../../testing/gtest/include/gtest/gtest.h:1530:1: note: in expansion of
macro
> > 'GTEST_IMPL_CMP_HELPER_'
> >  GTEST_IMPL_CMP_HELPER_(LT, <);
> > 
> > Looks like this is failing for the same reason, as it expands to this:
> > 
> > ASSERT_LT(windowed_min_rtt_.GetThirdBest(), windowed_min_rtt_.GetThirdBest()
+
> > QuicTime::Delta::FromMilliseconds(5));
> > 
> > Interestingly this doesn't happen on our compiler.
> 
> How about the following ASSERT:
> ASSERT_LT(windowed_min_rtt_.GetThirdBest(),
> QuicTime::Delta::Infinite().Subtract(QuicTime::Delta::FromMilliSeconds(5)))

This seems to work, I'll try another CQ Dry run

[bcf, 2016-06-03 23:45:21]

The CQ bit was checked by bcf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 23:45:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036613002/80001

[commit-bot: I haz the power, 2016-06-04 01:09:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-04 01:09:01]

Dry run: This issue passed the CQ dry run.

[bcf, 2016-06-06 17:20:04]

On 2016/06/04 01:09:01, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Does this look okay to submit?

[Jana, 2016-06-06 17:48:45]

lgtm

[Jana, 2016-06-06 17:52:32]

Can you add a comment above each ASSERT saying something about why it's
required?

[bcf, 2016-06-06 18:08:26]

On 2016/06/06 17:52:32, Jana wrote:
> Can you add a comment above each ASSERT saying something about why it's
> required?

Done.

[bcf, 2016-06-06 18:09:06]

The CQ bit was checked by bcf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-06 18:09:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036613002/100001

[commit-bot: I haz the power, 2016-06-06 19:23:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-06 19:23:11]

Dry run: This issue passed the CQ dry run.

[bcf, 2016-06-06 19:25:58]

The CQ bit was checked by bcf@chromium.org

[bcf, 2016-06-06 19:25:59]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org,
jri@chromium.org
Link to the patchset: https://codereview.chromium.org/2036613002/#ps100001
(title: "Add comments for ASSERTS")

[commit-bot: I haz the power, 2016-06-06 19:26:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036613002/100001

[commit-bot: I haz the power, 2016-06-06 19:31:17]

Description was changed from

==========
net::WindowedFilterTest: Add explicit overflow checks

Aggressive inlining causes -Wstrict-overflow to be triggered.

BUG=616957
BUG=internal b/29059135
==========

to

==========
net::WindowedFilterTest: Add explicit overflow checks

Aggressive inlining causes -Wstrict-overflow to be triggered.

BUG=616957
BUG=internal b/29059135
==========

[commit-bot: I haz the power, 2016-06-06 19:31:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-06 19:32:58]

Description was changed from

==========
net::WindowedFilterTest: Add explicit overflow checks

Aggressive inlining causes -Wstrict-overflow to be triggered.

BUG=616957
BUG=internal b/29059135
==========

to

==========
net::WindowedFilterTest: Add explicit overflow checks

Aggressive inlining causes -Wstrict-overflow to be triggered.

BUG=616957
BUG=internal b/29059135

Committed: https://crrev.com/d66635d239b6f91dcb7e11160a0bee7d99bb2e92
Cr-Commit-Position: refs/heads/master@{#398092}
==========

[commit-bot: I haz the power, 2016-06-06 19:32:59]

Patchset 6 (id:??) landed as
https://crrev.com/d66635d239b6f91dcb7e11160a0bee7d99bb2e92
Cr-Commit-Position: refs/heads/master@{#398092}