You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/09/22 03:07:45 UTC
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4514
to review the following change.
Change subject: thirdparty: stifle unused argument warnings when building with clang
......................................................................
thirdparty: stifle unused argument warnings when building with clang
Dan tested my various thirdparty patches on macOS and reported that clang
emits thousands of unused argument warnings when building llvm, gflags, and
gtest. The first is due to the use of EXTRA_LDFLAGS in the llvm build; the
others have always been there.
We can tackle this in one of two ways:
1. Add -Qunused-arguments to EXTRA_CXXFLAGS. This isn't as easy as it
sounds, because we need to restrict this to clang-based builds (gcc
doesn't recognize the parameter), and sometimes that happens via CC/CXX
environment variables and other times implicitly.
2. Narrow our prolific additions of -L... and -Wl,-rpath,... such that
they're only added when needed.
I chose approach #2, which also meant remembering all of our
interdependencies. As best I can tell, here is the complete list:
- glog depends on gflags
- glog depends on libunwind
- pmemobj depends on pmem
With such a short list, approach #2 isn't actually that bad. I tested it by
building thirdparty with my system's clang. It worked once I disabled
-Werror in the nvml build.
I tried very hard not to regress commit 2567ed0. My system should be using
gold, though I noticed that only the shared objects in
installed-deps-tsan had RUNPATH entries; the ones in installed-deps and
installed used RPATH. Nevertheless, I ran both debug and tsan tests, which
should have exercised all of them.
Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
2 files changed, 36 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/4514/1
--
To view, visit http://gerrit.cloudera.org:8080/4514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: thirdparty: stifle unused argument warnings when building with clang
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4514
to look at the new patch set (#2).
Change subject: thirdparty: stifle unused argument warnings when building with clang
......................................................................
thirdparty: stifle unused argument warnings when building with clang
Dan tested my various thirdparty patches on macOS and reported that clang
emits thousands of unused argument warnings when building llvm, gflags, and
gtest. The first is due to the use of EXTRA_LDFLAGS in the llvm build; the
others have always been there.
We can tackle this in one of two ways:
1. Add -Qunused-arguments to EXTRA_CXXFLAGS. This isn't as easy as it
sounds, because we need to restrict this to clang-based builds (gcc
doesn't recognize the parameter), and sometimes that happens via CC/CXX
environment variables and other times implicitly.
2. Narrow our prolific additions of -L... and -Wl,-rpath,... such that
they're only added when needed.
I chose approach #2, which also meant remembering all of our
interdependencies. As best I can tell, here is the complete list:
- glog depends on gflags
- glog depends on libunwind
- pmemobj depends on pmem
- bitshuffle depends on lz4
With such a short list, approach #2 isn't actually that bad. I tested it by
building thirdparty with my system's clang. It worked once I disabled
-Werror in the nvml build.
I tried very hard not to regress commit 2567ed0. My system should be using
gold, though I noticed that only the shared objects in
installed/tsan had RUNPATH entries; the ones in installed/uninstrumented and
installed/common used RPATH. Nevertheless, I ran both debug and tsan tests,
which should have exercised all of them.
Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
2 files changed, 36 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/4514/2
--
To view, visit http://gerrit.cloudera.org:8080/4514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: thirdparty: stifle unused argument warnings when building with clang
......................................................................
Patch Set 1: Code-Review+2
looks good, although the build failure may be legit.
--
To view, visit http://gerrit.cloudera.org:8080/4514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: thirdparty: stifle unused argument warnings when building with clang
......................................................................
Patch Set 4: Code-Review+2 Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: thirdparty: stifle unused argument warnings when building with clang
......................................................................
thirdparty: stifle unused argument warnings when building with clang
Dan tested my various thirdparty patches on macOS and reported that clang
emits thousands of unused argument warnings when building llvm, gflags, and
gtest. The first is due to the use of EXTRA_LDFLAGS in the llvm build; the
others have always been there.
We can tackle this in one of two ways:
1. Add -Qunused-arguments to EXTRA_CXXFLAGS. This isn't as easy as it
sounds, because we need to restrict this to clang-based builds (gcc
doesn't recognize the parameter), and sometimes that happens via CC/CXX
environment variables and other times implicitly.
2. Narrow our prolific additions of -L... and -Wl,-rpath,... such that
they're only added when needed.
I chose approach #2, which also meant remembering all of our
interdependencies. As best I can tell, here is the complete list:
- glog depends on gflags
- glog depends on libunwind
- pmemobj depends on pmem
- bitshuffle depends on lz4
With such a short list, approach #2 isn't actually that bad. I tested it by
building thirdparty with my system's clang. It worked once I disabled
-Werror in the nvml build.
I tried very hard not to regress commit 2567ed0. My system should be using
gold, though I noticed that only the shared objects in
installed/tsan had RUNPATH entries; the ones in installed/uninstrumented and
installed/common used RPATH. Nevertheless, I ran both debug and tsan tests,
which should have exercised all of them.
Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Reviewed-on: http://gerrit.cloudera.org:8080/4514
Reviewed-by: Dan Burkert <da...@cloudera.com>
Tested-by: Dan Burkert <da...@cloudera.com>
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
2 files changed, 36 insertions(+), 22 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/4514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] thirdparty: stifle unused argument warnings when building with clang
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: thirdparty: stifle unused argument warnings when building with clang
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No