Fix gcc error for static_cast the parameters of DCHECK_GT macro to unsigned in list.h.

Fix gcc error for static_cast the parameters of DCHECK_GT macro to unsigned in list.h.

  The CL #42279 (https://codereview.chromium.org/2619353006 ) caused a gcc error (-Werror=strict-overflow).

  Here is the error message:
  In file included from .././src/globals.h:15:0,
                   from .././src/allocation.h:9,
                   from .././src/profiler/profile-generator.h:9,
                   from ../src/profiler/profile-generator.cc:5:
  .././src/base/logging.h: In member function ‘void v8::internal::ProfileTree::TraverseDepthFirst(Callback*) [with Callback = v8::internal::DeleteNodesCallback]’:
  .././src/base/logging.h:179:70: error: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Werror=strict-overflow]
                             : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg);        \
                                                                            ^
  .././src/base/logging.h:191:1: note: in expansion of macro ‘DEFINE_CHECK_OP_IMPL’
   DEFINE_CHECK_OP_IMPL(GT, > )
   ^
    CXX(target) /home/zxli/work/google-v8/v8/out/x87.optdebug/obj.target/v8_base/src/regexp/regexp-macro-assembler.o
  cc1plus: all warnings being treated as errors

  This CL fix it.

BUG=

Review-Url: https://codereview.chromium.org/2632633002
Cr-Commit-Position: refs/heads/master@{#42318}
Committed: https://chromium.googlesource.com/v8/v8/+/6c67cd1886a6aea1c8a77c65e6d81a81a3a3b470

[zhengxing.li, 2017-01-13 09:15:28]

zhengxing.li@intel.com changed reviewers:
+ chunyang.dai@intel.com, clemensh@chromium.org, titzer@chromium.org,
yangguo@chromium.org

[zhengxing.li, 2017-01-13 09:15:29]

PTAL, thanks!

[Yang, 2017-01-13 09:17:33]

lgtm

[zhengxing.li, 2017-01-13 09:19:14]

The CQ bit was checked by zhengxing.li@intel.com

[commit-bot: I haz the power, 2017-01-13 09:19:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Clemens Hammacher, 2017-01-13 09:20:33]

Wait, can you link the compiler error?
Since http://crrev.com/2526783002, such checks are supposed to work. Maybe we
should rather fix this in the DCHECK macros.

[commit-bot: I haz the power, 2017-01-13 09:21:53]

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

[commit-bot: I haz the power, 2017-01-13 09:21:54]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19067)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15497)

[zhengxing.li, 2017-01-13 09:27:21]

Description was changed from

==========
Fix gcc error for static_cast the parameters of DCHECK_GT macro to unsigned in
list.h.

  The CL #42279 (https://codereview.chromium.org/2619353006 ) caused a gcc error
(-Werror=strict-overflow).
  This CL fix it.

BUG=
==========

to

==========
Fix gcc error for static_cast the parameters of DCHECK_GT macro to unsigned in
list.h.

  The CL #42279 (https://codereview.chromium.org/2619353006 ) caused a gcc error
(-Werror=strict-overflow).

  Here is the error message:
  In file included from .././src/globals.h:15:0,
                   from .././src/allocation.h:9,
                   from .././src/profiler/profile-generator.h:9,
                   from ../src/profiler/profile-generator.cc:5:
  .././src/base/logging.h: In member function ‘void
v8::internal::ProfileTree::TraverseDepthFirst(Callback*) [with Callback =
v8::internal::DeleteNodesCallback]’:
  .././src/base/logging.h:179:70: error: assuming signed overflow does not occur
when assuming that (X - c) > X is always false [-Werror=strict-overflow]
                             : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg);      
 \
                                                                            ^
  .././src/base/logging.h:191:1: note: in expansion of macro
‘DEFINE_CHECK_OP_IMPL’
   DEFINE_CHECK_OP_IMPL(GT, > )
   ^
    CXX(target)
/home/zxli/work/google-v8/v8/out/x87.optdebug/obj.target/v8_base/src/regexp/regexp-macro-assembler.o
  cc1plus: all warnings being treated as errors

  This CL fix it.

BUG=
==========

[zhengxing.li, 2017-01-13 09:28:41]

On 2017/01/13 09:21:54, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
>   v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19067)
>   v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
>   v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
>   v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
>   v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
>   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15497)

Hi, Clements:

I put the compiler error message into the issue description, please check it,
thanks!

[Clemens Hammacher, 2017-01-13 09:55:41]

On 2017/01/13 at 09:28:41, zhengxing.li wrote:
> On 2017/01/13 09:21:54, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
> >   v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19067)
> >   v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
> >   v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
> >   v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
> >   v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
> >   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15497)
> 
> Hi, Clements:
> 
> I put the compiler error message into the issue description, please check it,
thanks!

Thanks!
Interestingly, both length_ and i are signed values, so the code added in the
linked CL is not even triggered.
Does GCC infer from the DCHECK_LE(0, i) that i is unsigned, then generates an
signed-vs-unsigned comparison for DCHECK_GT(length_, i), and them complains
about it??

[zhengxing.li, 2017-01-13 10:11:22]

On 2017/01/13 09:55:41, Clemens Hammacher wrote:
> On 2017/01/13 at 09:28:41, zhengxing.li wrote:
> > On 2017/01/13 09:21:54, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
> > >   v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19067)
> > >   v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
> > >   v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
> > >   v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
> > >   v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
> > >   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > >
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15497)
> > 
> > Hi, Clements:
> > 
> > I put the compiler error message into the issue description, please check
it,
> thanks!
> 
> Thanks!
> Interestingly, both length_ and i are signed values, so the code added in the
> linked CL is not even triggered.
> Does GCC infer from the DCHECK_LE(0, i) that i is unsigned, then generates an
> signed-vs-unsigned comparison for DCHECK_GT(length_, i), and them complains
> about it??

I just did a test, removed the DCHECK_LE(0, i) and still got the gcc error.

Maybe gcc will promote the magic number to unsigned by default. So DCHECK_LE(0,
i) is OK.

[Clemens Hammacher, 2017-01-13 10:27:30]

On 2017/01/13 at 10:11:22, zhengxing.li wrote:
> On 2017/01/13 09:55:41, Clemens Hammacher wrote:
> > On 2017/01/13 at 09:28:41, zhengxing.li wrote:
> > > On 2017/01/13 09:21:54, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
> > > >   v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19067)
> > > >   v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
> > > >   v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
> > > >   v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
> > > >   v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
> > > >   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15497)
> > > 
> > > Hi, Clements:
> > > 
> > > I put the compiler error message into the issue description, please check
it,
> > thanks!
> > 
> > Thanks!
> > Interestingly, both length_ and i are signed values, so the code added in
the
> > linked CL is not even triggered.
> > Does GCC infer from the DCHECK_LE(0, i) that i is unsigned, then generates
an
> > signed-vs-unsigned comparison for DCHECK_GT(length_, i), and them complains
> > about it??
> 
> I just did a test, removed the DCHECK_LE(0, i) and still got the gcc error.
> 
> Maybe gcc will promote the magic number to unsigned by default. So
DCHECK_LE(0, i) is OK.

i is signed, not unsigned. So the DCHECK against (signed) 0 should always be ok.
Just as the DCHECK against the (signed) length_.
Even signed vs unsigned should be fine after the CL I linked above.

I would like to understand what's wrong here, before fixing it by casting two
signed value to unsigned.
I can't really help to find the real problem here however, since I can't
reproduce locally. The error message you posted is all we got? It does not show
the inferred types for Lhs and Rhs?

Is this blocking you? In this case, you can also land this CL and we ignore it
for now.

[zhengxing.li, 2017-01-13 10:48:24]

On 2017/01/13 10:27:30, Clemens Hammacher wrote:
> On 2017/01/13 at 10:11:22, zhengxing.li wrote:
> > On 2017/01/13 09:55:41, Clemens Hammacher wrote:
> > > On 2017/01/13 at 09:28:41, zhengxing.li wrote:
> > > > On 2017/01/13 09:21:54, commit-bot: I haz the power wrote:
> > > > > Try jobs failed on following builders:
> > > > >   v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > > >
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
> > > > >   v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > > >
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19067)
> > > > >   v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > > >
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
> > > > >   v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > > >
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
> > > > >   v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
> > > > >
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
> > > > >   v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > > >
> > >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
> > > > >   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> > > > >
> > >
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15497)
> > > > 
> > > > Hi, Clements:
> > > > 
> > > > I put the compiler error message into the issue description, please
check
> it,
> > > thanks!
> > > 
> > > Thanks!
> > > Interestingly, both length_ and i are signed values, so the code added in
> the
> > > linked CL is not even triggered.
> > > Does GCC infer from the DCHECK_LE(0, i) that i is unsigned, then generates
> an
> > > signed-vs-unsigned comparison for DCHECK_GT(length_, i), and them
complains
> > > about it??
> > 
> > I just did a test, removed the DCHECK_LE(0, i) and still got the gcc error.
> > 
> > Maybe gcc will promote the magic number to unsigned by default. So
> DCHECK_LE(0, i) is OK.
> 
> i is signed, not unsigned. So the DCHECK against (signed) 0 should always be
ok.
> Just as the DCHECK against the (signed) length_.
> Even signed vs unsigned should be fine after the CL I linked above.
> 
> I would like to understand what's wrong here, before fixing it by casting two
> signed value to unsigned.
> I can't really help to find the real problem here however, since I can't
> reproduce locally. The error message you posted is all we got? It does not
show
> the inferred types for Lhs and Rhs?
> 
> Is this blocking you? In this case, you can also land this CL and we ignore it
> for now.

Yes, this issue is blocking the x87 build which must use gcc to compile.
I will land this CL and you can revert it if you find any problem.

Thanks!

[zhengxing.li, 2017-01-13 10:48:38]

The CQ bit was checked by zhengxing.li@intel.com

[zhengxing.li, 2017-01-13 10:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2632633002/#ps20001
(title: "add error message")

[commit-bot: I haz the power, 2017-01-13 10:48:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 11:35:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484304518295990,
"parent_rev": "3badb2369da989370f89740e822da282d2c00f69", "commit_rev":
"6c67cd1886a6aea1c8a77c65e6d81a81a3a3b470"}

[commit-bot: I haz the power, 2017-01-13 11:35:36]

Description was changed from

==========
Fix gcc error for static_cast the parameters of DCHECK_GT macro to unsigned in
list.h.

  The CL #42279 (https://codereview.chromium.org/2619353006 ) caused a gcc error
(-Werror=strict-overflow).

  Here is the error message:
  In file included from .././src/globals.h:15:0,
                   from .././src/allocation.h:9,
                   from .././src/profiler/profile-generator.h:9,
                   from ../src/profiler/profile-generator.cc:5:
  .././src/base/logging.h: In member function ‘void
v8::internal::ProfileTree::TraverseDepthFirst(Callback*) [with Callback =
v8::internal::DeleteNodesCallback]’:
  .././src/base/logging.h:179:70: error: assuming signed overflow does not occur
when assuming that (X - c) > X is always false [-Werror=strict-overflow]
                             : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg);      
 \
                                                                            ^
  .././src/base/logging.h:191:1: note: in expansion of macro
‘DEFINE_CHECK_OP_IMPL’
   DEFINE_CHECK_OP_IMPL(GT, > )
   ^
    CXX(target)
/home/zxli/work/google-v8/v8/out/x87.optdebug/obj.target/v8_base/src/regexp/regexp-macro-assembler.o
  cc1plus: all warnings being treated as errors

  This CL fix it.

BUG=
==========

to

==========
Fix gcc error for static_cast the parameters of DCHECK_GT macro to unsigned in
list.h.

  The CL #42279 (https://codereview.chromium.org/2619353006 ) caused a gcc error
(-Werror=strict-overflow).

  Here is the error message:
  In file included from .././src/globals.h:15:0,
                   from .././src/allocation.h:9,
                   from .././src/profiler/profile-generator.h:9,
                   from ../src/profiler/profile-generator.cc:5:
  .././src/base/logging.h: In member function ‘void
v8::internal::ProfileTree::TraverseDepthFirst(Callback*) [with Callback =
v8::internal::DeleteNodesCallback]’:
  .././src/base/logging.h:179:70: error: assuming signed overflow does not occur
when assuming that (X - c) > X is always false [-Werror=strict-overflow]
                             : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg);      
 \
                                                                            ^
  .././src/base/logging.h:191:1: note: in expansion of macro
‘DEFINE_CHECK_OP_IMPL’
   DEFINE_CHECK_OP_IMPL(GT, > )
   ^
    CXX(target)
/home/zxli/work/google-v8/v8/out/x87.optdebug/obj.target/v8_base/src/regexp/regexp-macro-assembler.o
  cc1plus: all warnings being treated as errors

  This CL fix it.

BUG=

Review-Url: https://codereview.chromium.org/2632633002
Cr-Commit-Position: refs/heads/master@{#42318}
Committed:
https://chromium.googlesource.com/v8/v8/+/6c67cd1886a6aea1c8a77c65e6d81a81a3a...
==========

[commit-bot: I haz the power, 2017-01-13 11:35:38]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/6c67cd1886a6aea1c8a77c65e6d81a81a3a...