Skip to content

src: fix StringSearch compiler warning#31798

Closed
sam-github wants to merge 1 commit intonodejs:masterfrom
sam-github:fix-stringsearch-warning
Closed

src: fix StringSearch compiler warning#31798
sam-github wants to merge 1 commit intonodejs:masterfrom
sam-github:fix-stringsearch-warning

Conversation

@sam-github
Copy link
Contributor

@sam-github sam-github commented Feb 14, 2020 •
edited
Loading

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

@sam-github
src: fix StringSearch compiler warning
9697fac 
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

sam-github commented Feb 14, 2020

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

addaleax commented Feb 14, 2020

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

lundibundi commented Feb 14, 2020

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
@bnoordhuis
src: fix -Wmaybe-uninitialized compiler warning
ed8b671 
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 bnoordhuis mentioned this pull request Feb 15, 2020
Closed
@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 15, 2020

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
@bnoordhuis @addaleax
src: fix -Wmaybe-uninitialized compiler warning
d2e9177 
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
@bnoordhuis @MylesBorins
src: fix -Wmaybe-uninitialized compiler warning
fa376f4 
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
@bnoordhuis @targos
src: fix -Wmaybe-uninitialized compiler warning
a53980d 
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

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sam-github @addaleax @lundibundi @bnoordhuis @nodejs-github-bot