Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mitigate gcc8 -Wstringop-truncation and -Wignored-qualifiers #14452

Closed
wants to merge 4 commits into from

Conversation

toanju
Copy link
Contributor

@toanju toanju commented Feb 16, 2018

this PR deals with the warnings of gcc8

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@toanju
Copy link
Contributor Author

toanju commented Mar 27, 2018

any updates on this? Is it worth to invest some time to rebase and resolve the conflicts?

@toanju
Copy link
Contributor Author

toanju commented Apr 21, 2018

amended fixes for -Werror=class-memaccess (as well appearing with gcc8)

@toanju
Copy link
Contributor Author

toanju commented May 3, 2018

since some distros now ship gcc8, did anybody have a look at this, yet?

@toanju
Copy link
Contributor Author

toanju commented Jul 19, 2018

I am still willing to update this - though I would need some feedback in case there is something off. #15443 has another solution to this issue. Could anyone comment please @dgquintas @nicolasnoble

@toanju
Copy link
Contributor Author

toanju commented Aug 22, 2018

updated to master

@toanju
Copy link
Contributor Author

toanju commented Dec 5, 2018

can anybody please comment how to proceed with this one @dgquintas @nicolasnoble @vjpai ?

@matoro
Copy link
Contributor

matoro commented Dec 11, 2018

Does this PR address -Wno-invalid-source-encoding?

@toanju
Copy link
Contributor Author

toanju commented Dec 12, 2018

no it does not, which compiler and version do you use?

@toanju toanju requested a review from apolcyn as a code owner December 12, 2018 09:27
@toanju
Copy link
Contributor Author

toanju commented Dec 12, 2018

since I just updated this branch, I am using gcc (GCC) 8.2.1 20181105 (Red Hat 8.2.1-5) and currently the errors dealt with are:

-Werror=tautological-compare
-Werror=stringop-truncation
-Werror=ignored-qualifiers
-Werror=class-memaccess

(each in a separate commit)

@matoro
Copy link
Contributor

matoro commented Dec 12, 2018

@toanju This is a build issue more than a compile one. I am on gcc (Gentoo Hardened 8.2.0-r5 p1.6) 8.2.0 so it may be a change between 8.2.1 -> 8.2.0. Here's what it spits out in the c-ares build:

[C]       Compiling third_party/cares/cares/ares_library_init.c
mkdir -p `dirname /var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/objs/opt/third_party/cares/cares/ares_library_init.o`                                                                                                                            
armv7a-unknown-linux-gnueabihf-gcc -Ithird_party/googletest/googletest/include -Ithird_party/googletest/googlemock/include -Ithird_party/boringssl/include -Ithird_party/cares -Ithird_party/cares/cares -Ithird_party/address_sorting/include -DGPR_BACKWARDS_COMPATIBILITY_MODE -g -Wall -Wextra -Werror -Wno-long-long
-Wno-unused-parameter -DOSATOMIC_USE_INLINED=1 -Wno-deprecated-declarations -Ithird_party/nanopb -DPB_FIELD_32BIT -O2 -Wframe-larger-than=16384 -fPIC -I. -Iinclude -I/var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/gens -DNDEBUG                
-DINSTALL_PREFIX=\"/usr/local\"   -Ithird_party/zlib -Ithird_party/cares -Ithird_party/cares/cares -fvisibility=hidden -D_GNU_SOURCE   -Ithird_party/cares/config_linux  -DWIN32_LEAN_AND_MEAN -D_HAS_EXCEPTIONS=0 -DNOMINMAX -DHAVE_CONFIG_H -O3 -pipe -march=native -funroll-loops -flto=8 -floop-block               
-floop-interchange -floop-strip-mine -ftree-loop-distribution -std=c99 -Wsign-conversion -Wconversion     -Wno-sign-conversion  -Wno-invalid-source-encoding -MMD -MF                                                                                                                                                   
/var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/objs/opt/third_party/cares/cares/ares_library_init.dep -c -o                                                                                                                                       
/var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/objs/opt/third_party/cares/cares/ares_library_init.o third_party/cares/cares/ares_library_init.c                                                                                                   
third_party/cares/cares/ares_init.c: In function ‘ares_dup’:
third_party/cares/cares/ares_init.c:301:17: error: argument to ‘sizeof’ in ‘strncpy’ call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess]
           sizeof(src->local_dev_name));
                 ^
[C]       Compiling third_party/cares/cares/ares_llist.c
mkdir -p `dirname /var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/objs/opt/third_party/cares/cares/ares_llist.o`
armv7a-unknown-linux-gnueabihf-gcc -Ithird_party/googletest/googletest/include -Ithird_party/googletest/googlemock/include -Ithird_party/boringssl/include -Ithird_party/cares -Ithird_party/cares/cares -Ithird_party/address_sorting/include -DGPR_BACKWARDS_COMPATIBILITY_MODE -g -Wall -Wextra -Werror -Wno-long-long
-Wno-unused-parameter -DOSATOMIC_USE_INLINED=1 -Wno-deprecated-declarations -Ithird_party/nanopb -DPB_FIELD_32BIT -O2 -Wframe-larger-than=16384 -fPIC -I. -Iinclude -I/var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/gens -DNDEBUG
-DINSTALL_PREFIX=\"/usr/local\"   -Ithird_party/zlib -Ithird_party/cares -Ithird_party/cares/cares -fvisibility=hidden -D_GNU_SOURCE   -Ithird_party/cares/config_linux  -DWIN32_LEAN_AND_MEAN -D_HAS_EXCEPTIONS=0 -DNOMINMAX -DHAVE_CONFIG_H -O3 -pipe -march=native -funroll-loops -flto=8 -floop-block
-floop-interchange -floop-strip-mine -ftree-loop-distribution -std=c99 -Wsign-conversion -Wconversion     -Wno-sign-conversion  -Wno-invalid-source-encoding -MMD -MF
/var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/objs/opt/third_party/cares/cares/ares_llist.dep -c -o
/var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/objs/opt/third_party/cares/cares/ares_llist.o third_party/cares/cares/ares_llist.c
third_party/cares/cares/ares_init.c: At top level:
cc1: error: unrecognized command line option ‘-Wno-invalid-source-encoding’ [-Werror]
cc1: all warnings being treated as errors
make[2]: *** [Makefile:2896: /var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0/src/ruby/ext/grpc/objs/opt/third_party/cares/cares/ares_init.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0'
*** extconf.rb failed ***
Could not create Makefile due to some reason, probably lack of necessary
libraries and/or headers.  Check the mkmf.log file for more details.  You may
need configuration options.

Provided configuration options:
        --with-opt-dir
        --without-opt-dir
        --with-opt-include
        --without-opt-include=${opt-dir}/include
        --with-opt-lib
        --without-opt-lib=${opt-dir}/lib
        --with-make-prog
        --without-make-prog
        --srcdir=.
        --curdir
        --ruby=/usr/bin/$(RUBY_BASE_NAME)24

extconf failed, exit code 1

Gem files will remain installed in /var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/gems/grpc-1.15.0 for inspection.
Results logged to /var/tmp/portage/dev-vcs/gitlab-gitaly-0.129.0/work/ruby/vendor/bundle/ruby/2.4.0/extensions/armv7a-linux/2.4.0/grpc-1.15.0/gem_make.out

An error occurred while installing grpc (1.15.0), and Bundler cannot continue.
Make sure that `gem install grpc -v '1.15.0' --source 'https://rubygems.org/'` succeeds before bundling.

In Gemfile:
  gitaly-proto was resolved to 0.123.0, which depends on
    grpc
make[1]: *** [Makefile:32: ../.ruby-bundle] Error 5

This could be fixable by patching the makefile, but I don't know how to do that when gem is invoked from bundle.

@matoro
Copy link
Contributor

matoro commented Dec 12, 2018

Same error as this guy: #15520 so it is not specific to Gentoo or to gcc 8.2.0. Gentoo has a number of gcc8-related patches: https://gitweb.gentoo.org/repo/gentoo.git/tree/net-libs/grpc/files but because this is pulled from rubygems these would not be applied. This is also on a significantly newer ruby version (2.4) and grpc (1.15.0)

@toanju
Copy link
Contributor Author

toanju commented Dec 12, 2018

Nice to see that the patches here helped at least the Gentoo folks. With regards to your ruby build @matoro I am not sure what the best procedure is to fix this. In general I'd say this has to be fixed in the c-ares repo and then the ref here should be updated.

@matoro
Copy link
Contributor

matoro commented Dec 12, 2018

The problem is that this flag is added somewhere downstream, not in the vanilla c-ares build. Looking at line 10252 of the grpc makefile, I would think that it would only be added if a mingw environment is detected, but I cannot find anywhere else in the build stream that this flag is mentioned.

@toanju
Copy link
Contributor Author

toanju commented Jan 18, 2019

Rebased to master. Even if it's almost a year I am looking for feedback.

@AspirinSJL AspirinSJL added the release notes: no Indicates if PR should not be in release notes label Jan 18, 2019
@AspirinSJL
Copy link
Member

@nicolasnoble Friendly reminder.

example error:

src/cpp/common/channel_filter.cc:33:48: error: ‘void* memset(void*, int,
size_t)’ clearing an object of non-trivial type ‘grpc_linked_mdelem’
{aka ‘struct grpc_linked_mdelem’}; use assignment or
value-initialization instead [-Werror=class
-memaccess]
   memset(storage, 0, sizeof(grpc_linked_mdelem));
                                                ^
In file included from ./src/core/lib/transport/transport.h:34,
                 from ./src/core/lib/channel/channel_stack.h:48,
                 from src/cpp/common/channel_filter.cc:21:
./src/core/lib/transport/metadata_batch.h:33:16: note:
‘grpc_linked_mdelem’ {aka ‘struct grpc_linked_mdelem’} declared here
 typedef struct grpc_linked_mdelem {
                ^~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
@stale
Copy link

stale bot commented Sep 4, 2019

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!

@stale stale bot closed this Sep 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants