You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2019/05/27 21:42:22 UTC
[kudu-CR] Apply gcc 9x patch to lss dependency
sdrreynolds+gh@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13448
Change subject: Apply gcc 9x patch to lss dependency
......................................................................
Apply gcc 9x patch to lss dependency
Summary:
Google team applied this patch to allow gcc9x to compile. My
understanding is this code was poping the stack pointer which has
dangerous behavior and now gcc is yelling loudly and refusing to
compile when this occurs.
Reference patch:
https://chromium.googlesource.com/linux-syscall-support/+/8048ece6c16c91acfe0d36d1d3cc0890ab6e945c%5E%21/#F0
KUDU-2829
Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/breakpad-syscall-rsp-clobber-fix.patch
2 files changed, 25 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/13448/1
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sd...@gmail.com>
[kudu-CR] KUDU-2829: Apply gcc 9x patch to lss dependency
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
sdrreynolds+gh@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/13448 )
Change subject: KUDU-2829: Apply gcc 9x patch to lss dependency
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG@12
PS1, Line 12: now gcc is yelling loudly and refusing to
: compile when this occurs.
> Reading through https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52813, looks l
Interesting, my skills in dealing with View CVS have diminished so I cheated and went to github mirror.
https://github.com/gcc-mirror/gcc/commit/9d1cdb749a1ee071f8f4ba7bf3a0368d0b1342e9
This is what I think is the CVS link:
https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/c-family/ChangeLog?view=markup&pathrev=267941
seems to be in gcc 9.1 according to github tags?
This is my version
gutil|gcc-9x-patch? ? gcc --version
gcc (GCC) 9.1.1 20190503 (Red Hat 9.1.1-1)
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 01 Jun 2019 18:36:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] Apply gcc 9x patch to lss dependency
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13448 )
Change subject: Apply gcc 9x patch to lss dependency
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG@7
PS1, Line 7: Apply gcc 9x patch to lss dependency
nit: can you put the bug number in this line of the commit message? eg "KUDU-2829: Apply gcc 9x patch to lss dependency"
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 28 May 2019 20:00:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2829: Apply gcc 9x patch to lss dependency
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has abandoned this change. ( http://gerrit.cloudera.org:8080/13448 )
Change subject: KUDU-2829: Apply gcc 9x patch to lss dependency
......................................................................
Abandoned
Resolved via https://github.com/apache/kudu/commit/c5fd2e44245502c3c7f8dd0f2cd7639815fef0aa
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 2
Gerrit-Owner: Scott Reynolds <sd...@gmail.com>
Gerrit-Reviewer: Adar Lieber-Dembo <ad...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Scott Reynolds <sd...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2829: Apply gcc 9x patch to lss dependency
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13448
to look at the new patch set (#2).
Change subject: KUDU-2829: Apply gcc 9x patch to lss dependency
......................................................................
KUDU-2829: Apply gcc 9x patch to lss dependency
Summary:
Google team applied this patch to allow gcc9x to compile. My
understanding is this code was poping the stack pointer which has
dangerous behavior and now gcc is yelling loudly and refusing to
compile when this occurs.
This commit includes updates to gutil fork maintained within the Kudu
code base as well as fix to the thirdparty dep gperftools
Reference patch:
https://chromium.googlesource.com/linux-syscall-support/+/8048ece6c16c91acfe0d36d1d3cc0890ab6e945c%5E%21/#F0
Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
---
M src/kudu/gutil/linux_syscall_support.h
M thirdparty/download-thirdparty.sh
A thirdparty/patches/breakpad-syscall-rsp-clobber-fix.patch
A thirdparty/patches/gperftools-gcc9-support.patch
4 files changed, 54 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/13448/2
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2829: Apply gcc 9x patch to lss dependency
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
sdrreynolds+gh@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/13448 )
Change subject: KUDU-2829: Apply gcc 9x patch to lss dependency
......................................................................
Patch Set 2:
I believe I still need to updated breakpad patch to include the chrome team's commit message.
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 01 Jun 2019 18:39:36 +0000
Gerrit-HasComments: No
[kudu-CR] Apply gcc 9x patch to lss dependency
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13448 )
Change subject: Apply gcc 9x patch to lss dependency
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13448/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
http://gerrit.cloudera.org:8080/#/c/13448/1/thirdparty/download-thirdparty.sh@384
PS1, Line 384: BREAKPAD_PATCHLEVEL=1
this needs to increment to 2
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 May 2019 04:18:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] Apply gcc 9x patch to lss dependency
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13448 )
Change subject: Apply gcc 9x patch to lss dependency
......................................................................
Patch Set 1:
(3 comments)
I took a look at our codebase and found a few other copies of LSS. Here's the summary:
1. The google-breakpad project expresses LSS as a third party dependency, and then Kudu expresses google-breakpad as its own third party dependency. That's what you're addressing in this change, and the approach makes sense as upstream google-breakpad has not yet revved its LSS dependency to pick up the fix (so upgrading our google-breakpad won't help).
2. Kudu itself has an old copy of LSS in src/kudu/gutil. Could you make sure it is patched as well?
3. Kudu's gperftools dependency has an embedded LSS. This was fixed in an upstream gperftools commit[1], but that has yet to make it into a gperftools release, so upgrading to gperftools 2.7 won't help us. I recommend we backport that commit into our gperftools patch list.
1. https://github.com/gperftools/gperftools/commit/9e5b1628737c67b4587f937164572774592978c4
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG@7
PS1, Line 7: Apply gcc 9x patch to lss dependency
> nit: can you put the bug number in this line of the commit message? eg "KUD
Once you do that, you can remove the "KUDU-2829" line from L18.
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG@11
PS1, Line 11: poping
popping
http://gerrit.cloudera.org:8080/#/c/13448/1//COMMIT_MSG@12
PS1, Line 12: now gcc is yelling loudly and refusing to
: compile when this occurs.
Reading through https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52813, looks like they downgraded the error to a -Wdeprecated warning. Do you know whether that also landed in gcc9? We should still follow through with this commit; just wondering exactly what the severity is.
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 29 May 2019 17:53:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2829: Apply gcc 9x patch to lss dependency
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13448 )
Change subject: KUDU-2829: Apply gcc 9x patch to lss dependency
......................................................................
Patch Set 2:
(1 comment)
> I believe I still need to updated breakpad patch to include the chrome team's commit message.
Agreed.
http://gerrit.cloudera.org:8080/#/c/13448/2/src/kudu/gutil/linux_syscall_support.h
File src/kudu/gutil/linux_syscall_support.h:
http://gerrit.cloudera.org:8080/#/c/13448/2/src/kudu/gutil/linux_syscall_support.h@1661
PS2, Line 1661: : "esp", "memory"); \
My ASM intrinsics knowledge is not so good, but doesn't this change mean we're telling the compiler that we _will_ clobber esp? Isn't that the opposite of what we want?
--
To view, visit http://gerrit.cloudera.org:8080/13448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d1381454d570716692f850d30b53b01b1a94358
Gerrit-Change-Number: 13448
Gerrit-PatchSet: 2
Gerrit-Owner: Scott Reynolds <sd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Scott Reynolds <sd...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 02 Jun 2019 21:29:56 +0000
Gerrit-HasComments: Yes