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/07/25 03:58:35 UTC
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Jim Apple has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11047
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
IMPALA-5031: Fix undefined behavior: right-shifting too far
In expr.shift, the C++ standard says of right shifts:
The behavior is undefined if the right operand is negative, or
greater than or equal to the length in bits of the promoted left
operand.
In HdfsAvroScannerTest.DecimalTest, this is triggered, and the
interesting part of the backtrace is:
exec/hdfs-avro-scanner-ir.cc:272:18: runtime error: shift exponent 32 is too large for 32-bit type 'int32_t' (aka 'int')
#0 0x1786f65 in HdfsAvroScanner::ReadAvroDecimal(int, unsigned char**, unsigned char*, bool, void*, MemPool*) exec/hdfs-avro-scanner-ir.cc:272:18
#1 0x1617778 in void HdfsAvroScannerTest::TestReadAvroType<DecimalValue<int>, bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long>(bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long, unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:88:20
#2 0x1605705 in void HdfsAvroScannerTest::TestReadAvroDecimal<int>(unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:184:5
Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
---
M be/src/exec/hdfs-avro-scanner-ir.cc
1 file changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/11047/1
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 2:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/50/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 04:19:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11047
to look at the new patch set (#2).
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
IMPALA-5031: Fix undefined behavior: right-shifting too far
In expr.shift, the C++ standard says of right shifts:
The behavior is undefined if the right operand is negative, or
greater than or equal to the length in bits of the promoted left
operand.
In HdfsAvroScannerTest.DecimalTest, this is triggered, and the
interesting part of the backtrace is:
exec/hdfs-avro-scanner-ir.cc:272:18: runtime error: shift exponent 32 is too large for 32-bit type 'int32_t' (aka 'int')
#0 0x1786f65 in HdfsAvroScanner::ReadAvroDecimal(int, unsigned char**, unsigned char*, bool, void*, MemPool*) exec/hdfs-avro-scanner-ir.cc:272:18
#1 0x1617778 in void HdfsAvroScannerTest::TestReadAvroType<DecimalValue<int>, bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long>(bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long, unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:88:20
#2 0x1605705 in void HdfsAvroScannerTest::TestReadAvroDecimal<int>(unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:184:5
Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
---
M be/src/exec/hdfs-avro-scanner-ir.cc
1 file changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/11047/2
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/84/ : 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/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 17:23:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc
File be/src/exec/hdfs-avro-scanner-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc@263
PS3, Line 263: if(LIKELY(slot_byte_size == 4 || slot_byte_size == 8 || slot_byte_size == 16)) {
> Added this line to exactly replicate the behavior below. PS1 & PS2 did not
I think I preferred the old version :). We don't need a runtime check for the invariant (it sort-of happens for free below).
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 17:12:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
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/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
IMPALA-5031: Fix undefined behavior: right-shifting too far
In expr.shift, the C++ standard says of right shifts:
The behavior is undefined if the right operand is negative, or
greater than or equal to the length in bits of the promoted left
operand.
In HdfsAvroScannerTest.DecimalTest, this is triggered, and the
interesting part of the backtrace is:
exec/hdfs-avro-scanner-ir.cc:272:18: runtime error: shift exponent 32 is too large for 32-bit type 'int32_t' (aka 'int')
#0 0x1786f65 in HdfsAvroScanner::ReadAvroDecimal(int, unsigned char**, unsigned char*, bool, void*, MemPool*) exec/hdfs-avro-scanner-ir.cc:272:18
#1 0x1617778 in void HdfsAvroScannerTest::TestReadAvroType<DecimalValue<int>, bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long>(bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long, unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:88:20
#2 0x1605705 in void HdfsAvroScannerTest::TestReadAvroDecimal<int>(unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:184:5
Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Reviewed-on: http://gerrit.cloudera.org:8080/11047
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/hdfs-avro-scanner-ir.cc
1 file changed, 10 insertions(+), 0 deletions(-)
Approvals:
Jim Apple: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 5
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 4
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 21:01:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11047
to look at the new patch set (#3).
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
IMPALA-5031: Fix undefined behavior: right-shifting too far
In expr.shift, the C++ standard says of right shifts:
The behavior is undefined if the right operand is negative, or
greater than or equal to the length in bits of the promoted left
operand.
In HdfsAvroScannerTest.DecimalTest, this is triggered, and the
interesting part of the backtrace is:
exec/hdfs-avro-scanner-ir.cc:272:18: runtime error: shift exponent 32 is too large for 32-bit type 'int32_t' (aka 'int')
#0 0x1786f65 in HdfsAvroScanner::ReadAvroDecimal(int, unsigned char**, unsigned char*, bool, void*, MemPool*) exec/hdfs-avro-scanner-ir.cc:272:18
#1 0x1617778 in void HdfsAvroScannerTest::TestReadAvroType<DecimalValue<int>, bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long>(bool (HdfsAvroScanner::*)(int, unsigned char**, unsigned char*, bool, void*, MemPool*), unsigned long, unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:88:20
#2 0x1605705 in void HdfsAvroScannerTest::TestReadAvroDecimal<int>(unsigned char*, long, DecimalValue<int>, int, TErrorCode::type) exec/hdfs-avro-scanner-test.cc:184:5
Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
---
M be/src/exec/hdfs-avro-scanner-ir.cc
1 file changed, 10 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/11047/3
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2869/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 4
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 17:51:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 1:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/49/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 03:58:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11047/2/be/src/exec/hdfs-avro-scanner-ir.cc
File be/src/exec/hdfs-avro-scanner-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11047/2/be/src/exec/hdfs-avro-scanner-ir.cc@264
PS2, Line 264: if (len.val == 0) {
UNLIKELY()?
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 2
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: Wed, 25 Jul 2018 07:10:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/49/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 04:16:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 3:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/84/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 3
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: Fri, 27 Jul 2018 16:49:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/50/ : 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/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 04:51:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc
File be/src/exec/hdfs-avro-scanner-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc@263
PS3, Line 263: if(LIKELY(slot_byte_size == 4 || slot_byte_size == 8 || slot_byte_size == 16)) {
> It doesn't look sort-of free to me - The Compiler Explorer reports every pa
Oh i guess it's replaced with a constant for codegen anyway
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 17:36:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 4: Code-Review+2
Carry Tim's +2
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 4
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 17:51:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 3:
(2 comments)
I'll wait for another +2 from you, Tim, since this changes things enough to deserve a second quick look.
http://gerrit.cloudera.org:8080/#/c/11047/2/be/src/exec/hdfs-avro-scanner-ir.cc
File be/src/exec/hdfs-avro-scanner-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11047/2/be/src/exec/hdfs-avro-scanner-ir.cc@264
PS2, Line 264: memset(slot, 0, slot_byte_size);
> UNLIKELY()?
done
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc
File be/src/exec/hdfs-avro-scanner-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc@263
PS3, Line 263: if(LIKELY(slot_byte_size == 4 || slot_byte_size == 8 || slot_byte_size == 16)) {
Added this line to exactly replicate the behavior below. PS1 & PS2 did not replicate it in the failing case.
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 16:50:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Fix undefined behavior: right-shifting too far
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11047 )
Change subject: IMPALA-5031: Fix undefined behavior: right-shifting too far
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc
File be/src/exec/hdfs-avro-scanner-ir.cc:
http://gerrit.cloudera.org:8080/#/c/11047/3/be/src/exec/hdfs-avro-scanner-ir.cc@263
PS3, Line 263: if(LIKELY(slot_byte_size == 4 || slot_byte_size == 8 || slot_byte_size == 16)) {
> I think I preferred the old version :). We don't need a runtime check for t
It doesn't look sort-of free to me - The Compiler Explorer reports every path using between one and three cmp calls: https://godbolt.org/g/a1mUiD
--
To view, visit http://gerrit.cloudera.org:8080/11047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5be92912198af2a5e00053ceb9a4fab43ca6bb8
Gerrit-Change-Number: 11047
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
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: Fri, 27 Jul 2018 17:20:08 +0000
Gerrit-HasComments: Yes