You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/01/30 04:05:23 UTC
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12302
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
[thirdparty] detection of broken std::regex in libstdc++
Added detection of broken std::regex and friends in older
versions of libstdc++ (< 4.9) into the cpplint tool. For details,
see [1].
The motivation for this change is the fact that Kudu still compiles
and links against older libstdc++ at RHEL/CentOS 7, Ubuntu 14.04
and SLES 12, but using C++11 std::regex could lead to unexpected
crash if the binary is linked aginst libstdc++ versions < 4.9.
References:
[1] https://gcc.gnu.org/onlinedocs/gcc-4.8.2/libstdc++/manual/manual/status.html#status.iso.2011
section 28 'Regular Expressions'
Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
---
M build-support/lint.sh
M thirdparty/download-thirdparty.sh
M thirdparty/patches/google-styleguide-cpplint.patch
3 files changed, 76 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/12302/1
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12302
to look at the new patch set (#2).
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
[thirdparty] detection of broken std::regex in libstdc++
Added detection of broken std::regex and friends in older
versions of libstdc++ (< 4.9) into the cpplint tool. For details,
see [1].
The motivation for this change is the fact that Kudu still compiles
and links against older libstdc++ at RHEL/CentOS 7, Ubuntu 14.04
and SLES 12, but using C++11 std::regex could lead to unexpected
crash if the binary is linked aginst libstdc++ versions < 4.9.
References:
[1] https://gcc.gnu.org/onlinedocs/gcc-4.8.2/libstdc++/manual/manual/status.html#status.iso.2011
section 28 'Regular Expressions'
Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
---
M build-support/lint.sh
M thirdparty/download-thirdparty.sh
M thirdparty/patches/google-styleguide-cpplint.patch
3 files changed, 88 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/12302/2
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12302 )
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12302/1/build-support/lint.sh
File build-support/lint.sh:
http://gerrit.cloudera.org:8080/#/c/12302/1/build-support/lint.sh@51
PS1, Line 51: if [[ "$OSTYPE" =~ ^linux ]]; then
: distro=$(lsb_release --short --id)
: release=$(lsb_release --short --release)
: if ([[ $distro == "Ubuntu" ]] && [[ $release =~ 14\.[[:digit:]]+ ]]) || \
: ([[ $distro =~ (CentOS|RedHatEnterpriseServer) ]] && [[ $release =~ 7\.[[:digit:]]+ ]]) || \
: ([[ $distro =~ "SUSE" ]] && [[ $release =~ 12(\.[[:digit:]]+)? ]]); then
: filter="+runtime/broken_libstdcpp_regex,${filter}"
: fi
: fi
:
> Why condition this at all? As long as Kudu needs to support these older pla
Hrm, indeed. That's a bit funny that I didn't realize that myself :)
I'll remove that, sure.
http://gerrit.cloudera.org:8080/#/c/12302/1/thirdparty/patches/google-styleguide-cpplint.patch
File thirdparty/patches/google-styleguide-cpplint.patch:
PS1:
> You wrote this patch, right? Would it be possible to add comments or someth
Ah, indeed. Somehow I missed the details, providing them only in the commit message.
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 30 Jan 2019 22:25:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12302 )
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
[thirdparty] detection of broken std::regex in libstdc++
Added detection of broken std::regex and friends in older
versions of libstdc++ (< 4.9) into the cpplint tool. For details,
see [1]. I tried to find some warning flags for the GNU compiler
and/or linker to warn about non- or partially-implemented features
of GNU libstdc++ that doesn't yield compilation errors as is,
however I could not find anything relevant.
The motivation for this change is the fact that Kudu still compiles
and links against older libstdc++ at RHEL/CentOS 7, Ubuntu 14.04
and SLES 12, but using C++11 std::regex functionality could lead
to unexpected crash if the binary is linked aginst libstdc++ of
versions prior 4.9. Yes, there are tests, but:
* The issue with std::regex in older libstdc++ affects only the
platforms mentioned above.
* It's much better to detect and warn about the already known issue
sooner to avoid wasting time on troubleshooting and debugging.
References:
[1] https://gcc.gnu.org/onlinedocs/gcc-4.8.2/libstdc++/manual/manual/status.html#status.iso.2011
section 28 'Regular Expressions'
Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Reviewed-on: http://gerrit.cloudera.org:8080/12302
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M build-support/lint.sh
M thirdparty/download-thirdparty.sh
M thirdparty/patches/google-styleguide-cpplint.patch
3 files changed, 125 insertions(+), 7 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12302 )
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
Patch Set 3:
(2 comments)
Thank you for fast review!
http://gerrit.cloudera.org:8080/#/c/12302/3/thirdparty/patches/google-styleguide-cpplint.patch
File thirdparty/patches/google-styleguide-cpplint.patch:
PS3:
> You should consider submitting this new functionality as a PR upstream.
OK, that makes sense. I'll separate that and try to submit as PR upstream this week or so.
http://gerrit.cloudera.org:8080/#/c/12302/3/thirdparty/patches/google-styleguide-cpplint.patch@116
PS3, Line 116: - ('<utility>', ('pair',)),
: + ('<utility>', ('pair', 'swap',)),
: ('<vector>', ('vector',)),
:
: # gcc extensions.
: @@ -5501,8 +5587,7 @@
: _RE_PATTERN_STRING = re.compile(r'\bstring\b')
:
: _re_pattern_algorithm_header = []
: -for _template in ('copy', 'max', 'min', 'min_element', 'sort', 'swap',
: - 'transform'):
: +for _template in ('copy', 'max', 'min', 'min_element', 'sort', 'transform'):
> Are these changes related to the broken std::regex check?
Nope, those are old changes related to std::swap being migrated from <algorithm> to <util> or something like that in newer C++ toolchains. I added the changes for std::regex into the same patch because that's about patching the same cpplint.py file.
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Jan 2019 00:11:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12302
to look at the new patch set (#3).
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
[thirdparty] detection of broken std::regex in libstdc++
Added detection of broken std::regex and friends in older
versions of libstdc++ (< 4.9) into the cpplint tool. For details,
see [1]. I tried to find some warning flags for the GNU compiler
and/or linker to warn about non- or partially-implemented features
of GNU libstdc++ that doesn't yield compilation errors as is,
however I could not find anything relevant.
The motivation for this change is the fact that Kudu still compiles
and links against older libstdc++ at RHEL/CentOS 7, Ubuntu 14.04
and SLES 12, but using C++11 std::regex functionality could lead
to unexpected crash if the binary is linked aginst libstdc++ of
versions prior 4.9. Yes, there are tests, but:
* The issue with std::regex in older libstdc++ affects only the
platforms mentioned above.
* It's much better to detect and warn about the already known issue
sooner to avoid wasting time on troubleshooting and debugging.
References:
[1] https://gcc.gnu.org/onlinedocs/gcc-4.8.2/libstdc++/manual/manual/status.html#status.iso.2011
section 28 'Regular Expressions'
Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
---
M build-support/lint.sh
M thirdparty/download-thirdparty.sh
M thirdparty/patches/google-styleguide-cpplint.patch
3 files changed, 125 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/12302/3
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12302 )
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12302/3/thirdparty/patches/google-styleguide-cpplint.patch
File thirdparty/patches/google-styleguide-cpplint.patch:
PS3:
You should consider submitting this new functionality as a PR upstream.
http://gerrit.cloudera.org:8080/#/c/12302/3/thirdparty/patches/google-styleguide-cpplint.patch@116
PS3, Line 116: - ('<utility>', ('pair',)),
: + ('<utility>', ('pair', 'swap',)),
: ('<vector>', ('vector',)),
:
: # gcc extensions.
: @@ -5501,8 +5587,7 @@
: _RE_PATTERN_STRING = re.compile(r'\bstring\b')
:
: _re_pattern_algorithm_header = []
: -for _template in ('copy', 'max', 'min', 'min_element', 'sort', 'swap',
: - 'transform'):
: +for _template in ('copy', 'max', 'min', 'min_element', 'sort', 'transform'):
Are these changes related to the broken std::regex check?
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Jan 2019 00:04:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12302 )
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
Patch Set 1:
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/16151/ : FAILURE
whoops, it seems I used 'make ilint' instead of 'make lint' to verify that it works; I'll update this.
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 30 Jan 2019 18:04:27 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] detection of broken std::regex in libstdc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12302 )
Change subject: [thirdparty] detection of broken std::regex in libstdc++
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12302/1/build-support/lint.sh
File build-support/lint.sh:
http://gerrit.cloudera.org:8080/#/c/12302/1/build-support/lint.sh@51
PS1, Line 51: if [[ "$OSTYPE" =~ ^linux ]]; then
: distro=$(lsb_release --short --id)
: release=$(lsb_release --short --release)
: if ([[ $distro == "Ubuntu" ]] && [[ $release =~ 14\.[[:digit:]]+ ]]) || \
: ([[ $distro =~ (CentOS|RedHatEnterpriseServer) ]] && [[ $release =~ 7\.[[:digit:]]+ ]]) || \
: ([[ $distro =~ "SUSE" ]] && [[ $release =~ 12(\.[[:digit:]]+)? ]]); then
: filter="+runtime/broken_libstdcpp_regex,${filter}"
: fi
: fi
:
Why condition this at all? As long as Kudu needs to support these older platforms, std::regex should be forbidden, regardless of what machine you happen to be running on.
http://gerrit.cloudera.org:8080/#/c/12302/1/thirdparty/patches/google-styleguide-cpplint.patch
File thirdparty/patches/google-styleguide-cpplint.patch:
PS1:
You wrote this patch, right? Would it be possible to add comments or something to it to provide context?
--
To view, visit http://gerrit.cloudera.org:8080/12302
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb7d01c536a3e2c059c9c9f6a8d14dcb6d6acad1
Gerrit-Change-Number: 12302
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 30 Jan 2019 18:08:54 +0000
Gerrit-HasComments: Yes