Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2526783002:
[base] Define CHECK comparison for signed vs. unsigned (Closed)
|
Created:
4 years ago by Clemens Hammacher Modified:
4 years ago CC:
ahaas, Benedikt Meurer, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[base] Define CHECK comparison for signed vs. unsigned
The current CHECK/DCHECK implementation fails statically if a signed
value is compared against an unsigned value. The common solution is to
cast on each caller, which is tedious and error-prone (might hide bugs).
This CL implements signed vs. unsigned comparisons by executing up to
two comparisons. For example, if i is int32_t and u is uint_32_t, a
DCHECK_LE(i, u) would create the check
i <= 0 || static_cast<uint32_t>(i) <= u.
For checks against constants, at least one of the checks can be removed
by compiler optimizations.
The tradeoff we have to make is to sometimes silently execute an
additional comparison. And we increase code complexity of course, even
though the usage is just as easy (or even easier) as before.
The compile time impact seems to be minimal:
I ran 3 full compilations for Optdebug on my local machine, one time on
the current ToT, one time with this CL plus http://crrev.com/2524093002.
Before: 143.72 +- 1.21 seconds
Now: 144.18 +- 0.67 seconds
In order to check that the new comparisons are working, I refactored
some DCHECKs in wasm to use the new magic, and added unit test cases.
R=ishell@chromium.org, titzer@chromium.org
CC=ahaas@chromium.org, bmeurer@chromium.org
Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc
Committed: https://crrev.com/db0c86fa5f03794838ee6d1d301ca34d41f20fca
Cr-Original-Commit-Position: refs/heads/master@{#41275}
Cr-Commit-Position: refs/heads/master@{#41411}
Patch Set 1 #Patch Set 2 : Rebase & refactor for windows #
Total comments: 4
Patch Set 3 : Address Ben's comments #Patch Set 4 : Fix comment #
Total comments: 2
Patch Set 5 : Add missing undefs #Patch Set 6 : Extend test case #
Total comments: 2
Patch Set 7 : Include cstdint #Patch Set 8 : Extend test case #Patch Set 9 : Rebase #
Depends on Patchset: Messages
Total messages: 74 (58 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13080)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoffs we have to make is that compilation time might degrade because of an additional template being involved. In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. ========== to ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoffs we have to make is that compilation time might degrade because of an additional template being involved, and that sometimes an additional check is silently executed. And increased code complexity of course, even though the usage is just as easy (or even easier) as before. In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoffs we have to make is that compilation time might degrade because of an additional template being involved, and that sometimes an additional check is silently executed. And increased code complexity of course, even though the usage is just as easy (or even easier) as before. In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. ========== to ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. ==========
Description was changed from ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. ========== to ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. R=bmeurer@google.com, titzer@chromium.org ==========
clemensh@chromium.org changed reviewers: + bmeurer@google.com, titzer@chromium.org
Description was changed from ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. R=bmeurer@google.com, titzer@chromium.org ========== to ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. R=bmeurer@chromium.org, titzer@chromium.org ==========
clemensh@chromium.org changed reviewers: + bmeurer@chromium.org - bmeurer@google.com
clemensh@chromium.org changed reviewers: + ishell@chromium.org - bmeurer@chromium.org
+ishell I heard you have fun reviewing C++ template magic :)
https://codereview.chromium.org/2526783002/diff/20001/src/base/logging.h File src/base/logging.h (right): https://codereview.chromium.org/2526783002/diff/20001/src/base/logging.h#newc... src/base/logging.h:118: template <typename Lhs, typename Rhs> This is a nasty little template knot that took me a while to unravel. A couple comments could really make this clearer. Suggest: adding a comment that std::enable_if<> declares a function only if the predicate is true and std::make_unsigned<T> picks the integral unsigned type for the given signed type. https://codereview.chromium.org/2526783002/diff/20001/src/base/logging.h#newc... src/base/logging.h:167: typename std::enable_if<!is_signed_unsigned_mismatch<Lhs, Rhs>::value, \ Do you really need the auxilliary is_signed_unsigned_mismatch struct? Couldn't you just use an && here.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2526783002/diff/20001/src/base/logging.h File src/base/logging.h (right): https://codereview.chromium.org/2526783002/diff/20001/src/base/logging.h#newc... src/base/logging.h:118: template <typename Lhs, typename Rhs> On 2016/11/24 at 12:57:31, titzer wrote: > This is a nasty little template knot that took me a while to unravel. > A couple comments could really make this clearer. > > Suggest: adding a comment that std::enable_if<> declares a function only if the predicate is true and std::make_unsigned<T> picks the integral unsigned type for the given signed type. I added several comments, and the MAKE_UNSIGNED macro to increase readability. https://codereview.chromium.org/2526783002/diff/20001/src/base/logging.h#newc... src/base/logging.h:167: typename std::enable_if<!is_signed_unsigned_mismatch<Lhs, Rhs>::value, \ On 2016/11/24 at 12:57:31, titzer wrote: > Do you really need the auxilliary is_signed_unsigned_mismatch struct? Couldn't you just use an && here. Good suggestion, thanks. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for adding more comments. lgtm with a nit https://codereview.chromium.org/2526783002/diff/60001/src/base/logging.h File src/base/logging.h (right): https://codereview.chromium.org/2526783002/diff/60001/src/base/logging.h#newc... src/base/logging.h:156: DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, GE, CmpLEImpl(rhs, lhs)) #undef DEFINE_SIGNED_MISMATCH_COMP
lgtm
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2526783002/diff/60001/src/base/logging.h File src/base/logging.h (right): https://codereview.chromium.org/2526783002/diff/60001/src/base/logging.h#newc... src/base/logging.h:156: DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, GE, CmpLEImpl(rhs, lhs)) On 2016/11/24 at 15:37:12, Igor Sheludko wrote: > #undef DEFINE_SIGNED_MISMATCH_COMP Oh, sure. And #undef MAKE_UNSIGNED. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2526783002/#ps80001 (title: "Add missing undefs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480009905281670,
"parent_rev": "de25d982d2696e450e6b6e20a6ca86b7b40fd3a5", "commit_rev":
"21cd59056e4ba29c199a745305ec55da9c68d011"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. R=bmeurer@chromium.org, titzer@chromium.org ========== to ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. R=bmeurer@chromium.org, titzer@chromium.org Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc Cr-Commit-Position: refs/heads/master@{#41275} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc Cr-Commit-Position: refs/heads/master@{#41275}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2531533003/ by clemensh@chromium.org. The reason for reverting is: Need to revert previous CL because of Android compile error, and this one depends in it. .
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2526783002/diff/100001/test/unittests/base/lo... File test/unittests/base/logging-unittest.cc (right): https://codereview.chromium.org/2526783002/diff/100001/test/unittests/base/lo... test/unittests/base/logging-unittest.cc:33: CHECK_SUCCEED(EQ, -0.0, -0.0) Maybe also test a case where we compare static const pointer defined in some class with some other pointer.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Will land tomorrow morning then. https://codereview.chromium.org/2526783002/diff/100001/test/unittests/base/lo... File test/unittests/base/logging-unittest.cc (right): https://codereview.chromium.org/2526783002/diff/100001/test/unittests/base/lo... test/unittests/base/logging-unittest.cc:33: CHECK_SUCCEED(EQ, -0.0, -0.0) On 2016/11/29 at 14:12:04, Igor Sheludko wrote: > Maybe also test a case where we compare static const pointer defined in some class with some other pointer. Done.
Description was changed from ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic. R=bmeurer@chromium.org, titzer@chromium.org Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc Cr-Commit-Position: refs/heads/master@{#41275} ========== to ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic, and added unit test cases. R=ishell@chromium.org, titzer@chromium.org CC=ahaas@chromium.org, bmeurer@chromium.org Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc Cr-Commit-Position: refs/heads/master@{#41275} ==========
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2526783002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480582247804940,
"parent_rev": "4f51deef0b6cf07f72815241693730f821e7b175", "commit_rev":
"d121ee9ddf1604703994fe03b89dd28b6b6b76c8"}
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic, and added unit test cases. R=ishell@chromium.org, titzer@chromium.org CC=ahaas@chromium.org, bmeurer@chromium.org Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc Cr-Commit-Position: refs/heads/master@{#41275} ========== to ========== [base] Define CHECK comparison for signed vs. unsigned The current CHECK/DCHECK implementation fails statically if a signed value is compared against an unsigned value. The common solution is to cast on each caller, which is tedious and error-prone (might hide bugs). This CL implements signed vs. unsigned comparisons by executing up to two comparisons. For example, if i is int32_t and u is uint_32_t, a DCHECK_LE(i, u) would create the check i <= 0 || static_cast<uint32_t>(i) <= u. For checks against constants, at least one of the checks can be removed by compiler optimizations. The tradeoff we have to make is to sometimes silently execute an additional comparison. And we increase code complexity of course, even though the usage is just as easy (or even easier) as before. The compile time impact seems to be minimal: I ran 3 full compilations for Optdebug on my local machine, one time on the current ToT, one time with this CL plus http://crrev.com/2524093002. Before: 143.72 +- 1.21 seconds Now: 144.18 +- 0.67 seconds In order to check that the new comparisons are working, I refactored some DCHECKs in wasm to use the new magic, and added unit test cases. R=ishell@chromium.org, titzer@chromium.org CC=ahaas@chromium.org, bmeurer@chromium.org Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc Committed: https://crrev.com/db0c86fa5f03794838ee6d1d301ca34d41f20fca Cr-Original-Commit-Position: refs/heads/master@{#41275} Cr-Commit-Position: refs/heads/master@{#41411} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/db0c86fa5f03794838ee6d1d301ca34d41f20fca Cr-Commit-Position: refs/heads/master@{#41411} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
