You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/09/08 22:22:49 UTC
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/8015
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
IMPALA-5912: fix crash in trunc(..., "WW") in release build
The bug is with the pattern below:
const date& d = TruncMonth(orig_date).date();
The problem is that 'd' is a reference into the temporary returned from
TruncMonth. But the temporary is only guaranteed to live until the
expression has been evaluated. Thus if the compiler re-uses the register
or stack slot holding the temporary, 'd' may end up pointing to a bogus
value, causing a crash or incorrect results.
The fix is to simply create a local date value with the required date,
which avoids use of references to expression temporary and makes the
logic more obviously correct.
Also remove other uses of references to temporary that were correct but
unnecessary given that the function returned a value and C++11
return value optimisation should kick in.
Testing:
Ran expr-test to completion with a release build. Before this fix it
reliably crashed for me.
Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
---
M be/src/exprs/udf-builtins.cc
1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/8015/1
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8015/1//COMMIT_MSG
Commit Message:
PS1, Line 23: Also remove other uses of references to temporary that were correct but
: unnecessary given that the function returned a value and C++11
: return value optimisation should kick in.
> okay with me (if RVO is applying so no perf impact), but the const-ref-to-t
I guess there's an element of taste to it. It definitely gets confusing.
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
IMPALA-5912: fix crash in trunc(..., "WW") in release build
The bug is with the pattern below:
const date& d = TruncMonth(orig_date).date();
The problem is that 'd' is a reference into the temporary returned from
TruncMonth. But the temporary is only guaranteed to live until the
expression has been evaluated. Thus if the compiler re-uses the register
or stack slot holding the temporary, 'd' may end up pointing to a bogus
value, causing a crash or incorrect results.
The fix is to simply create a local date value with the required date,
which avoids use of references to expression temporary and makes the
logic more obviously correct.
Also remove other uses of references to temporary that were correct but
unnecessary given that the function returned a value and C++11
return value optimisation should kick in.
Testing:
Ran expr-test to completion with a release build. Before this fix it
reliably crashed for me.
Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Reviewed-on: http://gerrit.cloudera.org:8080/8015
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/udf-builtins.cc
1 file changed, 6 insertions(+), 6 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Thomas Tauber-Marshall: Looks good to me, but someone else must approve
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8015/1//COMMIT_MSG
Commit Message:
PS1, Line 23: Also remove other uses of references to temporary that were correct but
: unnecessary given that the function returned a value and C++11
: return value optimisation should kick in.
> I guess there's an element of taste to it. It definitely gets confusing.
Yeah, i wasn't trying to express an opinion. just noting that it's a common pattern.
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1210/
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8015/1//COMMIT_MSG
Commit Message:
PS1, Line 23: Also remove other uses of references to temporary that were correct but
: unnecessary given that the function returned a value and C++11
: return value optimisation should kick in.
> Yeah, i wasn't trying to express an opinion. just noting that it's a common
No worries, I didn't take it that way - I was just thinking that some people must like the pattern since it's been repeatedly used.
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5912: fix crash in trunc(..., "WW") in release build
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5912: fix crash in trunc(..., "WW") in release build
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8015/1//COMMIT_MSG
Commit Message:
PS1, Line 23: Also remove other uses of references to temporary that were correct but
: unnecessary given that the function returned a value and C++11
: return value optimisation should kick in.
okay with me (if RVO is applying so no perf impact), but the const-ref-to-temporary is a pattern we use broadly especially for TimestampValue (for better or worse).
--
To view, visit http://gerrit.cloudera.org:8080/8015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5017518188f5025daa5040ca1943581a0675726
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes