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

src: fix StringSearch compiler warning #31798

Closed

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Feb 14, 2020

StringSearchBase has tables (int array members) that are used only for
some search strategies, but g++-9 (at least) doesn't understand when
they will or will not be used. Default-initialize them in the
constructor to avoid this warning:

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);
			|            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
Checklist

StringSearchBase has tables (int array members) that are used only for
some search strategies, but g++-9 (at least) doesn't understand when
they will or will not be used. Default-initialize them in the
constructor to avoid this warning:

	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);
				|            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 14, 2020
@sam-github
Copy link
Contributor Author

I believe the default initialization is a no-op, so no value will be written to memory, so that there is no performance impact. If that's not correct, some other strategy might be needed (if search construction is in critical path).

@addaleax
Copy link
Member

I believe the default initialization is a no-op, so no value will be written to memory, so that there is no performance impact.

It’s not – this will lead to zero-initialization: https://godbolt.org/z/2TcRgT

@lundibundi
Copy link
Member

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.

From #26733 (comment).

There is also #31532.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request 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
@bnoordhuis
Copy link
Member

Alternative solution: #31809

@sam-github sam-github closed this Feb 18, 2020
@sam-github sam-github deleted the fix-stringsearch-warning branch February 18, 2020 16:12
addaleax pushed a commit that referenced this pull request 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>
MylesBorins pushed a commit that referenced this pull request 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 pull request 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
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants