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

GCC warnings when compiling files in src #26733

Closed
targos opened this issue Mar 18, 2019 · 9 comments
Closed

GCC warnings when compiling files in src #26733

targos opened this issue Mar 18, 2019 · 9 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@targos
Copy link
Member

targos commented Mar 18, 2019

I don't know if they are all fixable, but the first one is very annoying because it is emitted for (almost?) each C++ file:

In file included from ../src/node.h:63,
                 from ../src/api/callback.cc:1:
../deps/v8/include/v8.h: In instantiation of ‘void v8::PersistentBase<T>::SetWeak(P*, typename v8::WeakCallbackInfo<P>::Callback, v8::WeakCallbackType) [with P = node::BaseObject; T = v8::Object; typename v8::WeakCallbackInfo<P>::Callback = void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)]’:
../src/base_object-inl.h:104:42:   required from here
../deps/v8/include/v8.h:9585:16: warning: cast between incompatible function types from ‘v8::WeakCallbackInfo<node::BaseObject>::Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)’} to ‘Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<void>&)’} [-Wcast-function-type]
                reinterpret_cast<Callback>(callback), type);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../src/node_buffer.cc:29:
../src/string_search.h: In function ‘size_t node::stringsearch::SearchString(node::stringsearch::Vector<const Char>, node::stringsearch::Vector<const Char>, size_t) [with Char = short unsigned int]’:
../src/string_search.h:113:30: warning: ‘search’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     return (this->*strategy_)(subject, index);
            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
../src/string_search.h: In function ‘size_t node::stringsearch::SearchString(node::stringsearch::Vector<const Char>, node::stringsearch::Vector<const Char>, size_t) [with Char = unsigned char]’:
../src/string_search.h:113:30: warning: ‘search’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     return (this->*strategy_)(subject, index);
            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
@targos targos added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 18, 2019
@refack
Copy link
Contributor

refack commented Mar 18, 2019

https://gist.github.com/refack/1cc202e9b507d63d108d5ba55293e540 with 167 warnings from MSVC

refack pushed a commit to targos/node that referenced this issue Mar 18, 2019
PR-URL: nodejs#26735
Refs: nodejs#26733
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
zbjornson added a commit to zbjornson/node that referenced this issue Mar 19, 2019
danbev pushed a commit that referenced this issue Mar 22, 2019
PR-URL: #26766
Ref #26733
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit to targos/node that referenced this issue Mar 27, 2019
PR-URL: nodejs#26735
Refs: nodejs#26733
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit to targos/node that referenced this issue Mar 27, 2019
PR-URL: nodejs#26766
Ref nodejs#26733
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit that referenced this issue Mar 27, 2019
PR-URL: #26735
Refs: #26733
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit that referenced this issue Mar 27, 2019
PR-URL: #26766
Ref #26733
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented May 2, 2019

@targos is this still an issue?

@targos
Copy link
Member Author

targos commented May 2, 2019

@cjihrig Yes, I can still reproduce all warnings from the OP with GCC 8 and 9.

@bmsdave
Copy link
Contributor

bmsdave commented Oct 18, 2019

@targos
I would like to try to solve this problem. I'll try it next week.

@bmsdave
Copy link
Contributor

bmsdave commented Oct 28, 2019

@targos hi. Can you describe the steps to reproduce these notifications?

@targos
Copy link
Member Author

targos commented Oct 28, 2019

@bmsdave Hello. I can reproduce the warnings on my system (Fedora 30, GCC 9.2.1) with:

mkdir -p out/Release
cd out/Release
$ c++ -MMD -MF obj/src/libnode.node.o.d -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -DOPENSSL_NO_PINSHARED -DOPENSSL_THREADS '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' -DNODE_WANT_INTERNALS=1 -DV8_DEPRECATION_WARNINGS=1 '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' -DHAVE_INSPECTOR=1 -DNODE_REPORT -D__POSIX__ -DNODE_USE_V8_PLATFORM=1 -DNODE_HAVE_I18N_SUPPORT=1 -DHAVE_OPENSSL=1 -DUCONFIG_NO_SERVICE=1 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION=1 -DU_HAVE_STD_STRING=1 -DUCONFIG_NO_BREAK_ITERATION=0 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_POSIX_C_SOURCE=200112 -DNGHTTP2_STATICLIB -I../../src -Igen -Igen/include -Igen/src -I../../deps/histogram/src -I../../deps/v8/include -I../../deps/icu-small/source/i18n -I../../deps/icu-small/source/common -I../../deps/zlib -I../../deps/llhttp/include -I../../deps/cares/include -I../../deps/uv/include -I../../deps/nghttp2/lib/includes -I../../deps/brotli/c/include -I../../deps/openssl/openssl/include -Wall -Wextra -Wno-unused-parameter -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++1y  -c ../../src/node.cc -o obj/src/libnode.node.o
In file included from ../../src/node.h:63,
                 from ../../src/node.cc:22:
../../deps/v8/include/v8.h: In instantiation of ‘void v8::PersistentBase<T>::SetWeak(P*, typename v8::WeakCallbackInfo<P>::Callback, v8::WeakCallbackType) [with P = node::BaseObject; T = v8::Object; typename v8::WeakCallbackInfo<P>::Callback = void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)]’:
../../src/base_object-inl.h:106:42:   required from here
../../deps/v8/include/v8.h:10100:16: warning: cast between incompatible function types from ‘v8::WeakCallbackInfo<node::BaseObject>::Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)’} to ‘Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<void>&)’} [-Wcast-function-type]
10100 |                reinterpret_cast<Callback>(callback), type);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ c++ -MMD -MF obj/src/libnode.node_buffer.o.d -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -DOPENSSL_NO_PINSHARED -DOPENSSL_THREADS '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' -DNODE_WANT_INTERNALS=1 -DV8_DEPRECATION_WARNINGS=1 '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' -DHAVE_INSPECTOR=1 -DNODE_REPORT -D__POSIX__ -DNODE_USE_V8_PLATFORM=1 -DNODE_HAVE_I18N_SUPPORT=1 -DHAVE_OPENSSL=1 -DUCONFIG_NO_SERVICE=1 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION=1 -DU_HAVE_STD_STRING=1 -DUCONFIG_NO_BREAK_ITERATION=0 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_POSIX_C_SOURCE=200112 -DNGHTTP2_STATICLIB -I../../src -Igen -Igen/include -Igen/src -I../../deps/histogram/src -I../../deps/v8/include -I../../deps/icu-small/source/i18n -I../../deps/icu-small/source/common -I../../deps/zlib -I../../deps/llhttp/include -I../../deps/cares/include -I../../deps/uv/include -I../../deps/nghttp2/lib/includes -I../../deps/brotli/c/include -I../../deps/openssl/openssl/include -Wall -Wextra -Wno-unused-parameter -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++1y  -c ../../src/node_buffer.cc -o obj/src/libnode.node_buffer.o
In file included from ../../src/node.h:63,
                 from ../../src/node_buffer.h:25,
                 from ../../src/node_buffer.cc:22:
../../deps/v8/include/v8.h: In instantiation of ‘void v8::PersistentBase<T>::SetWeak(P*, typename v8::WeakCallbackInfo<P>::Callback, v8::WeakCallbackType) [with P = node::Buffer::{anonymous}::CallbackInfo; T = v8::ArrayBuffer; typename v8::WeakCallbackInfo<P>::Callback = void (*)(const v8::WeakCallbackInfo<node::Buffer::{anonymous}::CallbackInfo>&)]’:
../../src/node_buffer.cc:130:75:   required from here
../../deps/v8/include/v8.h:10100:16: warning: cast between incompatible function types from ‘v8::WeakCallbackInfo<node::Buffer::{anonymous}::CallbackInfo>::Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<node::Buffer::{anonymous}::CallbackInfo>&)’} to ‘Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<void>&)’} [-Wcast-function-type]
10100 |                reinterpret_cast<Callback>(callback), type);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../src/node_buffer.cc:29:
../../src/string_search.h: In function ‘size_t node::stringsearch::SearchString(node::stringsearch::Vector<const Char>, node::stringsearch::Vector<const Char>, size_t) [with Char = short unsigned int]’:
../../src/string_search.h:113:30: warning: ‘search’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  113 |     return (this->*strategy_)(subject, index);
      |            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
../../src/string_search.h: In function ‘size_t node::stringsearch::SearchString(node::stringsearch::Vector<const Char>, node::stringsearch::Vector<const Char>, size_t) [with Char = unsigned char]’:
../../src/string_search.h:113:30: warning: ‘search’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  113 |     return (this->*strategy_)(subject, index);
      |            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~

I removed the other two warnings from the OP because I can't reproduce them anymore.

@gireeshpunathil
Copy link
Member

just wondering between this and #18983 , are we talking about the same thing?

@targos
Copy link
Member Author

targos commented Jan 7, 2020

It doesn't look like the same thing.

@lundibundi
Copy link
Member

Not sure we can do anything about the rest of them:

The search_string ones are caused by uninitialized arrays (they are initialized when needed and not upon object creation) in SearchStringBase which can be initialized (i.e. = {0}) with 1-2% performance impact or suppressed with something like #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" (this should also work for clang). Not sure if this is needed though pragma ignore would probably be fine.

The Wcast-function-type is from the base_object-inl.h specifically the v8::PersistentBase::SetWeak call which uses reinterpret_cast from WeakCallbackInfo<Type> to WeakCallbackInfo<void> which is okay for C++ and is just fine in there IIUC. I don't think this is possible to fix on our end. I'd consider this one as wontfix however annoying it is, unfortunately.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Feb 15, 2020
Turn the `strategy_` method pointer into an enum-based static dispatch.

It's both safer and more secure (no chance of method pointer corruption)
and it helps GCC see that the shift and suffix tables it's complaining
about are unused in single char search mode.

Fixes the following warning:

    ../src/string_search.h:113:30: warning:
    ‘search’ may be used uninitialized in this function [-Wmaybe-uninitialized]
         return (this->*strategy_)(subject, index);

Fixes: nodejs#26733
Fixes: nodejs#31532
Fixes: nodejs#31798
MylesBorins pushed a commit that referenced this issue Mar 11, 2020
Turn the `strategy_` method pointer into an enum-based static dispatch.

It's both safer and more secure (no chance of method pointer corruption)
and it helps GCC see that the shift and suffix tables it's complaining
about are unused in single char search mode.

Fixes the following warning:

    ../src/string_search.h:113:30: warning:
    ‘search’ may be used uninitialized in this function [-Wmaybe-uninitialized]
         return (this->*strategy_)(subject, index);

Fixes: #26733
Refs: #31532
Refs: #31798
PR-URL: #31809
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Apr 22, 2020
Turn the `strategy_` method pointer into an enum-based static dispatch.

It's both safer and more secure (no chance of method pointer corruption)
and it helps GCC see that the shift and suffix tables it's complaining
about are unused in single char search mode.

Fixes the following warning:

    ../src/string_search.h:113:30: warning:
    ‘search’ may be used uninitialized in this function [-Wmaybe-uninitialized]
         return (this->*strategy_)(subject, index);

Fixes: #26733
Refs: #31532
Refs: #31798
PR-URL: #31809
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants