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 2017/05/27 06:51:20 UTC
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Jim Apple has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7008
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/parquet-column-stats.inline.h
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/coding-util.cc
M be/src/util/internal-queue-test.cc
M be/src/util/streaming-sampler.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
14 files changed, 39 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/1
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/4/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
PS4, Line 74: bytes_needed > 0
> I agree, let's add a DCHECK.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 8: Code-Review+2
carry Dan's +2
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7008
to look at the new patch set (#4).
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This was recognized by the C++ standards committe and fixed in
Committee Draft 2008. See LWG Defect Report 464:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/aggregation-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-join-node-ir.cc
M be/src/exec/hash-join-node.cc
M be/src/exec/hash-table.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-parquet-scanner-ir.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/unnest-node.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-context.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/tuple-row-compare.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
48 files changed, 113 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/4
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7008
to look at the new patch set (#7).
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This was recognized by the C++ standards committe and fixed in
Committee Draft 2008. See LWG Defect Report 464:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/aggregation-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-join-node-ir.cc
M be/src/exec/hash-join-node.cc
M be/src/exec/hash-table.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-parquet-scanner-ir.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.h
M be/src/exec/parquet-column-stats.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/unnest-node.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-context.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/tuple-row-compare.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
49 files changed, 115 insertions(+), 110 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/7
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 3:
(9 comments)
Thank you for fixing this. Please see my inline comments.
http://gerrit.cloudera.org:8080/#/c/7008/3//COMMIT_MSG
Commit Message:
Line 22:
You could add a link to http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464 which mentions that this was one of the reasons for introducing data().
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
Line 731: // ctx_vector = build_expr_ctxs_.data() / ctx_vector = probe_expr_ctxs_.data()
I'm not sure why this line is here at all. Should we remove it?
Line 1093: // ctx_vector = build_expr_ctxs_.data()
same here?
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:
Line 379: row_end_locations_.data(),
Do you want to fix the line wrapping while you're here?
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/old-hash-table.cc
File be/src/exec/old-hash-table.cc:
Line 301: // Load build_expr_ctxs_.data() / probe_expr_ctxs_.data()
I'm not sure that the replacement preserves the comment well. Do you know what the intent is here? Can you rephrase?
Line 643: // Load build_expr_ctxs_.data()
same here?
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
Line 75: v, bytes_needed, reinterpret_cast<uint8_t*>(&(*out)[0]));
Out cannot be empty here, but the code looks suspicious. Should we change it?
There are two other places with similar code: git grep "&(\*\([A-Za-z0-9_]\+\))\[0\]"
be/src/exec/hdfs-parquet-scanner-ir.cc:30: ExprContext* const* conjunct_ctxs = &(*scanner_conjunct_ctxs_)[0];
be/src/exec/hdfs-scanner.h:366: return ExecNode::EvalConjuncts(&(*scanner_conjunct_ctxs_)[0],
be/src/exec/parquet-column-stats.inline.h:75: v, bytes_needed, reinterpret_cast<uint8_t*>(&(*out)[0]));
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/experiments/tuple-splitter-test.cc
File be/src/experiments/tuple-splitter-test.cc:
PS3, Line 284: split_counts_
Will this work if split_counts_ is empty()? Should we have a comment to explain why this is not UB?
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:
Line 72: string_val.ptr = string_val.len > 0 ? &string_data[0] : nullptr;
Nice. Does this get us around the const cast?
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7008
to look at the new patch set (#5).
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This was recognized by the C++ standards committe and fixed in
Committee Draft 2008. See LWG Defect Report 464:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/aggregation-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-join-node-ir.cc
M be/src/exec/hash-join-node.cc
M be/src/exec/hash-table.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-parquet-scanner-ir.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.h
M be/src/exec/parquet-column-stats.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/unnest-node.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-context.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/tuple-row-compare.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
49 files changed, 113 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/5
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/6/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
Line 983: DCHECK(hay_len == 0 || haystack != NULL);
> haystack can be NULL only if 'hay_len' == 0, in which case, the loop below
Done
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7008
to look at the new patch set (#6).
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This was recognized by the C++ standards committe and fixed in
Committee Draft 2008. See LWG Defect Report 464:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/aggregation-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-join-node-ir.cc
M be/src/exec/hash-join-node.cc
M be/src/exec/hash-table.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-parquet-scanner-ir.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.h
M be/src/exec/parquet-column-stats.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/unnest-node.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-context.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/tuple-row-compare.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
49 files changed, 115 insertions(+), 110 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/6
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/6/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
Line 983: if (haystack == NULL) return NULL;
This overly-restrictive DCHECK was exposed by the other parts of the change.
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 8:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/784/
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 3: Code-Review+2
LGTM other than comments already raised.
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This was recognized by the C++ standards committe and fixed in
Committee Draft 2008. See LWG Defect Report 464:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Reviewed-on: http://gerrit.cloudera.org:8080/7008
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-parquet-scanner-ir.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.h
M be/src/exec/parquet-column-stats.inline.h
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
31 files changed, 70 insertions(+), 67 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Jim Apple: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#3).
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/aggregation-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-join-node-ir.cc
M be/src/exec/hash-join-node.cc
M be/src/exec/hash-table.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/unnest-node.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-context.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/internal-queue-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/tuple-row-compare.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
47 files changed, 111 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/3
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7008
to look at the new patch set (#8).
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This was recognized by the C++ standards committe and fixed in
Committee Draft 2008. See LWG Defect Report 464:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-parquet-scanner-ir.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.h
M be/src/exec/parquet-column-stats.inline.h
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
31 files changed, 70 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/8
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/4/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
PS4, Line 74: bytes_needed > 0
> how can bytes_needed be 0? when does that happen?
Yes, this has to be > 0. I'm not sure a DCHECK prevents the compiler from inferring UB in release builds, so we may need the additional check. Adding a DCHECK would still be a good idea.
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/6/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
Line 983: if (haystack == NULL) return NULL;
> This overly-restrictive DCHECK was exposed by the other parts of the change
haystack can be NULL only if 'hay_len' == 0, in which case, the loop below won't be entered. So, rather than add this branch on each iteration of SplitPart(), it should just be: DCHECK(hay_len == 0 || haystack != NULL);
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7008/3//COMMIT_MSG
Commit Message:
Line 22:
> You could add a link to http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-def
Done
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:
Line 379: row_end_locations_.data(),
> Do you want to fix the line wrapping while you're here?
Done
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/old-hash-table.cc
File be/src/exec/old-hash-table.cc:
Line 301: // Load build_expr_ctxs_.data() / probe_expr_ctxs_.data()
> I'm not sure that the replacement preserves the comment well. Do you know w
I think this is the same meaning as in the other codegen earlier in this patch that Tim responded to you about.
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
Line 75: v, bytes_needed, reinterpret_cast<uint8_t*>(&(*out)[0]));
> Out cannot be empty here, but the code looks suspicious. Should we change i
data() returns a const pointer for strings until C++17, but the other two places I changed.
For here, I added a conditional guard to make sure we don't hit the UB.
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:
Line 72: string_val.ptr = string_val.len > 0 ? &string_data[0] : nullptr;
> Nice. Does this get us around the const cast?
Yes - data() returns a const char * (until c++17), but string_val.ptr is a char *.
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/util/internal-queue-test.cc
File be/src/util/internal-queue-test.cc:
Line 127: queue.Enqueue(nodes.data());
> These aren't necessary because of the resize aboveand inconsistent with usi
Done
Line 194: queue.Enqueue(nodes.data());
> Revert this one? It's not necessary and inconsistent with the lines immedia
Done
Line 202: queue.Enqueue(nodes.data());
> Same here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 4: -Code-Review
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/4/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
PS4, Line 74: bytes_needed > 0
> I believe the UB sanitizer is a runtime thing, not a compile time checker.
I agree, let's add a DCHECK.
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#2).
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
IMPALA-5031: Remove undefined behavior "reference binding to null"
When p has type T* and p is nullptr, then T x = p[0] has undefined
behavior (obviously). Less obviously, T& x = p[0] also has undefined
behavior -- references cannot be null. That undefined behavior can be
caused by expressions like &v[0] when v is a vector of size 0. The
issue is that the expression is parsed as &(v[0]), aka
&(v.operator[](0)). The return type of the operator[] method is T&,
and when v is empty the return value is a null reference.
This patch was created by running the following command, then fixing
the resulting compile errors:
find be/src -type f -execdir \
sed -i 's/&\([A-Za-z0-9_]\+\)\[0\]/\1.data()/g' \{\} \;
Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
---
M be/src/exec/aggregation-node.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/delimited-text-parser-test.cc
M be/src/exec/hash-join-node-ir.cc
M be/src/exec/hash-join-node.cc
M be/src/exec/hash-table.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-table-writer.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/parquet-column-stats.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/unnest-node.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/expr-context.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-util-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/parallel-executor-test.cc
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/udf/udf-test-harness.h
M be/src/util/bitmap.h
M be/src/util/coding-util-test.cc
M be/src/util/coding-util.cc
M be/src/util/dict-encoding.h
M be/src/util/internal-queue-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/streaming-sampler.h
M be/src/util/tuple-row-compare.h
M be/src/util/uid-util.h
M be/src/util/webserver.cc
47 files changed, 111 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7008/2
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/4/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
PS4, Line 74: bytes_needed > 0
how can bytes_needed be 0? when does that happen?
From Lars' earlier comment, it sounds like it can't, so this isn't undefined behavior, and this should just DCHECK_GT(bytes_needed, 0) to prove that (and func comment should document). But Lars should confirm.
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 3: Code-Review+1
(6 comments)
Seems good to me aside from a couple of places where the search/replace was overzealous.
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
Line 731: // ctx_vector = build_expr_ctxs_.data() / ctx_vector = probe_expr_ctxs_.data()
> I'm not sure why this line is here at all. Should we remove it?
Looks like it's documenting what code the line below is mean to generate.
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
Line 75: v, bytes_needed, reinterpret_cast<uint8_t*>(&(*out)[0]));
> Out cannot be empty here, but the code looks suspicious. Should we change i
out->data() would be more readable anyway IMO.
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/experiments/tuple-splitter-test.cc
File be/src/experiments/tuple-splitter-test.cc:
PS3, Line 284: split_counts_
> Will this work if split_counts_ is empty()? Should we have a comment to exp
Using sizeof() like this is always valid (it never actually evaluates the expression). Seems ok to me.
http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/util/internal-queue-test.cc
File be/src/util/internal-queue-test.cc:
Line 127: queue.Enqueue(nodes.data());
These aren't necessary because of the resize aboveand inconsistent with using &nodes[1]
Line 194: queue.Enqueue(nodes.data());
Revert this one? It's not necessary and inconsistent with the lines immediately below.
Line 202: queue.Enqueue(nodes.data());
Same here.
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior "reference binding to null"
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null"
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7008/4/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
PS4, Line 74: bytes_needed > 0
> Yes, this has to be > 0. I'm not sure a DCHECK prevents the compiler from i
I believe the UB sanitizer is a runtime thing, not a compile time checker. So, this shouldn't be an issue for UBSAN since we can never get here with bytes_needed == 0. Let's make it a DCHECK then.
--
To view, visit http://gerrit.cloudera.org:8080/7008
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes