You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2018/09/08 17:16:46 UTC
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Jim Apple has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11406
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
IMPALA-5031: undefined behavior: codegen signed overflow
This patch changes the way signed integer arithmetic is performed in
generated code. It uses unsigned arithmetic instead, because overflow
causes undefined behavior according to the standard.
Compiling with -full_ubsan, the following types of errors are avoided
by this patch:
exprs/operators-ir.cc:167:803: runtime error: signed integer
overflow: -9223372036854775807 + -9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:168:823: runtime error: signed integer
overflow: -9223372036854775807 - 9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:169:823: runtime error: signed integer
overflow: -9223372036854775807 * -9223372036854775807 cannot be
represented in type 'long'
This is visible when running be/build/latest/exprs/expr-test
--gtest_filter=ExprTest.ArithmeticExprs
To check for performance regressions, I ran TPCH and TPCDS at scale
factor 20 with three minicluster nodes and observed no performance
changes.
Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
---
M be/src/exprs/operators-ir.cc
1 file changed, 51 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/11406/1
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 17:55:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 1:
> Do we have test coverage for overflows of these operators. Would be
> good to test the operators overflowing both in positive and
> negative directions. I looked in expr-test and didn't really see
> any direct tests of this behaviour.
Yes, we do have coverage; see the calls to TestFixedResultTypeOps in the ArithmeticExprs test.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 15 Sep 2018 23:40:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
IMPALA-5031: undefined behavior: codegen signed overflow
This patch changes the way signed integer arithmetic is performed in
generated code. It uses unsigned arithmetic instead, because overflow
causes undefined behavior according to the standard.
Compiling with -full_ubsan, the following types of errors are avoided
by this patch:
exprs/operators-ir.cc:167:803: runtime error: signed integer
overflow: -9223372036854775807 + -9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:168:823: runtime error: signed integer
overflow: -9223372036854775807 - 9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:169:823: runtime error: signed integer
overflow: -9223372036854775807 * -9223372036854775807 cannot be
represented in type 'long'
This is visible when running be/build/latest/exprs/expr-test
--gtest_filter=ExprTest.ArithmeticExprs
To check for performance regressions, I ran TPCH and TPCDS at scale
factor 20 with three minicluster nodes and observed no performance
changes. I also checked the LLVM IR to see that the generated code
after this commit is identical to the generated code at the previous
commit, except that the new code doesn't use the 'nsw' modifier. That
modifier normally indicates that signed wrap does not occur; in this
case the operation happens on unsigned representation of the same
bits, that modifier is no longer relevant.
Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Reviewed-on: http://gerrit.cloudera.org:8080/11406
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exprs/operators-ir.cc
M be/src/util/bit-util.h
2 files changed, 95 insertions(+), 8 deletions(-)
Approvals:
Jim Apple: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 5
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11406
to look at the new patch set (#3).
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
IMPALA-5031: undefined behavior: codegen signed overflow
This patch changes the way signed integer arithmetic is performed in
generated code. It uses unsigned arithmetic instead, because overflow
causes undefined behavior according to the standard.
Compiling with -full_ubsan, the following types of errors are avoided
by this patch:
exprs/operators-ir.cc:167:803: runtime error: signed integer
overflow: -9223372036854775807 + -9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:168:823: runtime error: signed integer
overflow: -9223372036854775807 - 9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:169:823: runtime error: signed integer
overflow: -9223372036854775807 * -9223372036854775807 cannot be
represented in type 'long'
This is visible when running be/build/latest/exprs/expr-test
--gtest_filter=ExprTest.ArithmeticExprs
To check for performance regressions, I ran TPCH and TPCDS at scale
factor 20 with three minicluster nodes and observed no performance
changes. I also checked the LLVM IR to see that the generated code
after this commit is identical to the generated code at the previous
commit, except that the new code doesn't use the 'nsw' modifier. That
modifier normally indicates that signed wrap does not occur; in this
case the operation happens on unsigned representation of the same
bits, that modifier is no longer relevant.
Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
---
M be/src/exprs/operators-ir.cc
M be/src/util/bit-util.h
2 files changed, 95 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/11406/3
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/688/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 16 Sep 2018 23:49:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 1:
Do we have test coverage for overflows of these operators. Would be good to test the operators overflowing both in positive and negative directions. I looked in expr-test and didn't really see any direct tests of this behaviour.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Sep 2018 15:44:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11406
to look at the new patch set (#2).
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
IMPALA-5031: undefined behavior: codegen signed overflow
This patch changes the way signed integer arithmetic is performed in
generated code. It uses unsigned arithmetic instead, because overflow
causes undefined behavior according to the standard.
Compiling with -full_ubsan, the following types of errors are avoided
by this patch:
exprs/operators-ir.cc:167:803: runtime error: signed integer
overflow: -9223372036854775807 + -9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:168:823: runtime error: signed integer
overflow: -9223372036854775807 - 9223372036854775807 cannot be
represented in type 'long'
exprs/operators-ir.cc:169:823: runtime error: signed integer
overflow: -9223372036854775807 * -9223372036854775807 cannot be
represented in type 'long'
This is visible when running be/build/latest/exprs/expr-test
--gtest_filter=ExprTest.ArithmeticExprs
To check for performance regressions, I ran TPCH and TPCDS at scale
factor 20 with three minicluster nodes and observed no performance
changes. I also checked the LLVM IR to see that the generated code
after this commit is identical to the generated code at the previous
commit, except that the new code doesn't use the 'nsw' modifier. That
modifier normally indicates that signed wrap does not occur; in this
case the operation happens on unsigned representation of the same
bits, that modifier is no longer relevant.
Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
---
M be/src/exprs/operators-ir.cc
1 file changed, 66 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/11406/2
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 1:
(1 comment)
Otherwise the code looks fine
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc@65
PS1, Line 65: std::
the std:: doesn't seem necessary
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Sep 2018 15:46:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc@56
PS1, Line 56: converted to a prvalue of another integer type. ... If the destination type is
> did you verify that it's happening? what about in debug builds?
I can see that it's happening in release. In debug, this still generates a few unnecessary IR loads/stores, but no actual calls to memcpy.
Good suggestion about the static cast. A static cast works correctly TO unsigned, but not FROM, I learned, so this new patchset still has one memcpy.
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc@65
PS1, Line 65: docs p
> the std:: doesn't seem necessary
If we #include <string.h>, I agree, but I think cstring puts it in std::, not ::.
https://en.cppreference.com/w/cpp/string/byte/memcpy
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 16 Sep 2018 23:16:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/622/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Sep 2018 17:51:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/731/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 05:26:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc@65
PS1, Line 65: retur
> I'm pretty sure all real implementations put the standard libc functions in
removed
http://gerrit.cloudera.org:8080/#/c/11406/2/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11406/2/be/src/exprs/operators-ir.cc@68
PS2, Line 68: #define BITNOT_FN(TYPE)\
> I'm ok with relying on clang's undocumented behaviour so long as we have a
Added tests that work at compile time.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 04:54:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc@65
PS1, Line 65: docs p
> If we #include <string.h>, I agree, but I think cstring puts it in std::, n
I'm pretty sure all real implementations put the standard libc functions into the global namespace regardless of which header variant you import. Honestly, this makes sense to me - it would be insane for a C++ program to redefine memcpy.
But yeah, this is ok, just slightly inconsistent with the rest of the code base.
http://gerrit.cloudera.org:8080/#/c/11406/2/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11406/2/be/src/exprs/operators-ir.cc@68
PS2, Line 68: // range of the type". However, Clang does not document its implementation-defined
I'm ok with relying on clang's undocumented behaviour so long as we have a test case. I don't see a reason why they would change this going forward, given it's the simplest to implement and most compatible with GCC.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Sep 2018 16:32:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3185/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 4
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 17:57:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 4: Code-Review+2
carry +2 on rebase
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 4
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 17:56:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 4
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 21:38:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 )
Change subject: IMPALA-5031: undefined behavior: codegen signed overflow
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc@56
PS1, Line 56: Modern compilers can "see through" the call to memcpy and generate code with no copies.
did you verify that it's happening? what about in debug builds?
wouldn't a static cast work just the same? i.e. unsigned x = (unsigned)y;
though I don't feel strongly if you're sure the memcpy really goes away in all cases.
--
To view, visit http://gerrit.cloudera.org:8080/11406
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f
Gerrit-Change-Number: 11406
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Sep 2018 17:46:52 +0000
Gerrit-HasComments: Yes