lib: fix a compiler warning: unused GetDevURandom() by vasild · Pull Request #17563 · bitcoin/bitcoin

vasild · 2019-11-22T20:30:54Z

~~Only define GetDevURandom() if it is going to be used.~~

Silence by planting a dummy reference to the GetDevURandom symbol
in the places where we don't call the function.

fanquake · 2019-11-22T20:34:30Z

What warning are you seeing, compiling for which OS?

laanwj · 2019-11-22T20:42:45Z

src/random.cpp

~~Concept ACK, but I think there's some redundancy in this experession: at the least, WIN32 will never be set if HAVE_GETENTROPY or HAVE_SYSCTL_ARND.~~

Edit: oh, that doesn't really help. I still think this is too complex an expression, would be good to have a NEED_DEVURANDOM_FALLBACK or such.

laanwj · 2019-11-22T21:32:14Z

See also #13294 for earlier discussion on a similar change (which was eventually dropped from the PR)

vasild · 2019-11-23T15:55:59Z

@fanquake

random.cpp:255:13: error: unused function 'GetDevURandom' [-Werror,-Wunused-function]
static void GetDevURandom(unsigned char *ent32)
            ^
1 error generated.

FreeBSD 12.1.

@laanwj I agree that the "forest" of ifdef is totally unreadable.

I changed the approach to mark the function as possibly unused. After all the compilers provide a way to do that exactly for cases like this. Once we are on C++17, we can use [[maybe_unused]] directly in the code. Before that some magic in src/attributes.h, similarly to NODISCARD.

gmaxwell · 2019-11-23T17:46:43Z

Marking that unused would be a great way to hid an actual bug in the future. Would it be possible to #define a USEGETDEVURANDOM as part of the conditional compilation that makes it unused and ifndef the function out if it won't be used?

vasild · 2019-11-23T20:22:09Z

Marking that unused would be a great way to hid an actual bug in the future.

Like what?

Would it be possible to #define a USEGETDEVURANDOM as part of the conditional compilation that makes it unused and ifndef the function out if it won't be used?

I don't think so. We have:

static GetDevURandom() { ... }
...
GetOSRand() {
#ifdef FOO
... don't use GetDevURandom() ...
#elif BAR
... use GetDevURandom() ...
#elif BAZ
... don't use GetDevURandom() ...
#else whatnot
... use GetDevURandom() ...
#endif
}

GetDevURandom() must be declared before GetOSRand(). If we want to

define the function when it is going to be used and
not define it (not even declare it) when it is not going to be used

then we have to use the forest of ifdefs from the first iteration of this PR - replicate the same conditions as inside GetOSRand() before it.

There are also other ways to silence this:

Tell the compiler to switch off this particular warning for GetDevURandom() (works for GCC and Clang, Visual Studio does not produce warning for this anyway):

#ifdef __GNUC__
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-function"
#endif
static void GetDevURandom() { ... }
#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif

Remove the static qualifier. Possibly add inline hoping that it will not be defined as a global symbol. static inline does not produce a warning with GCC, but still does for Clang.
I am not sure what @laanwj means with NEED_DEVURANDOM_FALLBACK, can you elaborate?

PS this seems to be causing a noise inversely proportional to the complexity of the change.

maflcko · 2019-11-24T01:44:56Z

Just for reference, the warning appears on freebsd 12: https://cirrus-ci.com/task/4881774827536384?command=make#L423

I have no opinion on whether it should be fixed or how.

vasild · 2019-11-24T13:03:27Z

3rd attempt (8a16b9d): no marking as unused (but @gmaxwell what bug could that hide in the future, I am just curious?), no complex negated ifdef conditions that span 3 lines.

laanwj · 2019-11-27T12:51:21Z

Looks good to me now. I don't think the current construction can hide any bug:

If NEED_GET_DEV_URANDOM is set unnecessarily, the compiler warning comes back
If NEED_GET_DEV_URANDOM is left out in error, the compiler will throw an error because the function is used but missing

ACK 8a16b9d448b90aa5a234dc326ebda45b99b363b7

vasild · 2020-03-04T12:44:58Z

Rebased to get appveyor green, its failure was not due to this PR.

fanquake · 2020-03-17T07:48:09Z

I've just opened #18364, which removes GetDevURandom usage on macOS, which will also simplify this change.

vasild · 2020-03-17T08:19:17Z

Yes! I will adjust this PR once #18364 makes it to the tree.

Thanks!

DrahtBot · 2020-03-17T12:15:24Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

f9f210d doc: fix GetTimeMicros() comment in random.cpp (fanquake) a889711 rand: remove getentropy() fallback for macOS < 10.12 (fanquake) Pull request description: We [no longer support macOS < 10.12](#17550) (our binaries will not run), so remove the fallback for when `getentropy()` wasn't available. From the manpage: ```bash HISTORY The getentropy() function appeared in OSX 10.12 ``` Note that compiling on macOS you'll see a new unused function warning: ```bash random.cpp:256:13: warning: unused function 'GetDevURandom' [-Wunused-function] static void GetDevURandom(unsigned char *ent32) ^ 1 warning generated. ``` This will likely be addressed as part of #17563. ACKs for top commit: vasild: ACK f9f210d (code review, not tested) elichai: utACK f9f210d practicalswift: ACK f9f210d -- patch looks correct laanwj: ACK f9f210d hebasto: ACK f9f210d, tested on macOS 10.13.6: compiled Tree-SHA512: 6bd2a721f23605a8bca0b7b51f42d628ebf92a18e74eb43194331ba745ee449223aff84119892781c40b188c70b75417447f4e390e3d9ac549292de2b1e8b308

vasild · 2020-03-17T20:12:47Z

Updated, now that #18364 is in.

… 10.12 f9f210d doc: fix GetTimeMicros() comment in random.cpp (fanquake) a889711 rand: remove getentropy() fallback for macOS < 10.12 (fanquake) Pull request description: We [no longer support macOS < 10.12](bitcoin#17550) (our binaries will not run), so remove the fallback for when `getentropy()` wasn't available. From the manpage: ```bash HISTORY The getentropy() function appeared in OSX 10.12 ``` Note that compiling on macOS you'll see a new unused function warning: ```bash random.cpp:256:13: warning: unused function 'GetDevURandom' [-Wunused-function] static void GetDevURandom(unsigned char *ent32) ^ 1 warning generated. ``` This will likely be addressed as part of bitcoin#17563. ACKs for top commit: vasild: ACK f9f210d (code review, not tested) elichai: utACK f9f210d practicalswift: ACK f9f210d -- patch looks correct laanwj: ACK f9f210d hebasto: ACK f9f210d, tested on macOS 10.13.6: compiled Tree-SHA512: 6bd2a721f23605a8bca0b7b51f42d628ebf92a18e74eb43194331ba745ee449223aff84119892781c40b188c70b75417447f4e390e3d9ac549292de2b1e8b308

sipa · 2020-03-20T18:50:10Z

src/random.cpp

I don't know if I like effectively duplicating the which-random-source-to-use logic in macro statements. It feels like this too risks overlooking things.

One possibility is adding a (void)GetDevURandom; line in the exact places where we know that no /dev/urandom is needed (but only there, so that it doesn't silence an unintended lack of invocation). Such a statement will also silence the warning, and since the method is static, if it's unused, it still won't be included in the object code.

Ok, another ingenious way to silence this warning. The previous one did not gain traction for a few months, so lets see if this one flies. Updated the patch.

I confirm that the warning is gone and that the symbol is not included in the object code with -O1 or higher. It is included with -O0 or in the absence of any -O... option but this is fine.

``` random.cpp:255:13: error: unused function 'GetDevURandom' [-Werror,-Wunused-function] ``` Clang 9.0.0, FreeBSD 12.1 Silence by planting a dummy reference to the `GetDevURandom` symbol in the places where we don't call the function.

fanquake · 2020-05-11T09:53:47Z

I started working on a similar change to remove the warning on macOS, then remembered we had this PR. Will test shortly; if this is the way everyone is happy to solve the issue, let get this in.

vasild · 2020-05-11T15:42:18Z

This can be silenced in a few different ways. I am fine with either one, as long as it gets fixed.

@sipa, what do you think about the last incarnation of this patch? I changed it according to your suggestion at #17563 (comment)

hebasto

ACK ca2e474, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

hebasto

re-ACK ca2e474, tested on macOS 10.15.6 + llvm clang 10.0.0

master (e349eee)

random.cpp:257:13: warning: unused function 'GetDevURandom' [-Wunused-function]
static void GetDevURandom(unsigned char *ent32)
            ^
1 warning generated.

master + this PR -- no warnings

practicalswift · 2020-08-09T19:35:39Z

ACK ca2e474 -- increased signal to noise in compiler diagnostics is good

sipa · 2020-08-09T20:45:00Z

utACK ca2e474

maflcko · 2020-08-10T12:40:46Z

@fanquake Anything left to do here since your last comment?

ca2e474 Fix a compiler warning: unused GetDevURandom() (Vasil Dimov) Pull request description: ~~Only define GetDevURandom() if it is going to be used.~~ Silence by planting a dummy reference to the `GetDevURandom` symbol in the places where we don't call the function. ACKs for top commit: practicalswift: ACK ca2e474 -- increased signal to noise in compiler diagnostics is good sipa: utACK ca2e474 hebasto: re-ACK ca2e474, tested on macOS 10.15.6 + llvm clang 10.0.0 Tree-SHA512: 03c98f00dad5d9a3c5c9f68553d72ad5489ec02f18b9769108a22003ec7be7819a731b1eab6a9f64dafb5be0efddccf6980de7e3bb90cd20d4f4d72f74124675

amitiuttarwar · 2020-08-12T03:31:42Z

just wanna say thanks for this! that warning was annoying.

… 10.12 f9f210d doc: fix GetTimeMicros() comment in random.cpp (fanquake) a889711 rand: remove getentropy() fallback for macOS < 10.12 (fanquake) Pull request description: We [no longer support macOS < 10.12](bitcoin#17550) (our binaries will not run), so remove the fallback for when `getentropy()` wasn't available. From the manpage: ```bash HISTORY The getentropy() function appeared in OSX 10.12 ``` Note that compiling on macOS you'll see a new unused function warning: ```bash random.cpp:256:13: warning: unused function 'GetDevURandom' [-Wunused-function] static void GetDevURandom(unsigned char *ent32) ^ 1 warning generated. ``` This will likely be addressed as part of bitcoin#17563. ACKs for top commit: vasild: ACK f9f210d (code review, not tested) elichai: utACK f9f210d practicalswift: ACK f9f210d -- patch looks correct laanwj: ACK f9f210d hebasto: ACK f9f210d, tested on macOS 10.13.6: compiled Tree-SHA512: 6bd2a721f23605a8bca0b7b51f42d628ebf92a18e74eb43194331ba745ee449223aff84119892781c40b188c70b75417447f4e390e3d9ac549292de2b1e8b308

ca2e474 Fix a compiler warning: unused GetDevURandom() (Vasil Dimov) Pull request description: ~~Only define GetDevURandom() if it is going to be used.~~ Silence by planting a dummy reference to the `GetDevURandom` symbol in the places where we don't call the function. ACKs for top commit: practicalswift: ACK ca2e474 -- increased signal to noise in compiler diagnostics is good sipa: utACK ca2e474 hebasto: re-ACK ca2e474, tested on macOS 10.15.6 + llvm clang 10.0.0 Tree-SHA512: 03c98f00dad5d9a3c5c9f68553d72ad5489ec02f18b9769108a22003ec7be7819a731b1eab6a9f64dafb5be0efddccf6980de7e3bb90cd20d4f4d72f74124675

Summary: > ``` > random.cpp:255:13: error: unused function 'GetDevURandom' [-Werror,-Wunused-function] > > ``` > > Clang 9.0.0, FreeBSD 12.1 > > Silence by planting a dummy reference to the `GetDevURandom` symbol > in the places where we don't call the function. We see the same warnings in our CI logs. This is a backport of [[bitcoin/bitcoin#17563 | core#17563]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10090

fanquake added the Utils/log/libs label Nov 22, 2019

laanwj reviewed Nov 22, 2019

View reviewed changes

vasild force-pushed the unused-GetDevURandom branch from 8d2d95d to 6120aa8 Compare November 23, 2019 15:45

vasild force-pushed the unused-GetDevURandom branch from 6120aa8 to 8a16b9d Compare November 24, 2019 13:00

vasild force-pushed the unused-GetDevURandom branch from 8a16b9d to ab25e34 Compare March 4, 2020 10:57

fanquake mentioned this pull request Mar 17, 2020

random: remove getentropy() fallback for macOS < 10.12 #18364

Merged

DrahtBot added the Needs rebase label Mar 17, 2020

vasild force-pushed the unused-GetDevURandom branch from ab25e34 to 0bbc4c7 Compare March 17, 2020 20:09

DrahtBot removed the Needs rebase label Mar 17, 2020

DrahtBot mentioned this pull request Mar 19, 2020

build: fix sysctl() detection on macOS #18359

Merged

DrahtBot added the Needs rebase label Mar 19, 2020

vasild force-pushed the unused-GetDevURandom branch from 0bbc4c7 to 5d67234 Compare March 20, 2020 15:08

DrahtBot removed the Needs rebase label Mar 20, 2020

sipa reviewed Mar 20, 2020

View reviewed changes

Fix a compiler warning: unused GetDevURandom()

ca2e474

``` random.cpp:255:13: error: unused function 'GetDevURandom' [-Werror,-Wunused-function] ``` Clang 9.0.0, FreeBSD 12.1 Silence by planting a dummy reference to the `GetDevURandom` symbol in the places where we don't call the function.

vasild force-pushed the unused-GetDevURandom branch from 5d67234 to ca2e474 Compare March 20, 2020 19:52

hebasto approved these changes Jun 13, 2020

View reviewed changes

hebasto approved these changes Aug 9, 2020

View reviewed changes

fanquake merged commit 04a0676 into bitcoin:master Aug 10, 2020

vasild deleted the unused-GetDevURandom branch August 10, 2020 14:36

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

vasild commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Nov 22, 2019

Uh oh!

laanwj Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Nov 23, 2019

Uh oh!

gmaxwell commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Nov 23, 2019

Uh oh!

maflcko commented Nov 24, 2019

Uh oh!

vasild commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Mar 4, 2020

Uh oh!

fanquake commented Mar 17, 2020

Uh oh!

vasild commented Mar 17, 2020

Uh oh!

DrahtBot commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild commented Mar 17, 2020

Uh oh!

sipa Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

vasild Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

fanquake commented May 11, 2020

Uh oh!

vasild commented May 11, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Aug 9, 2020

Uh oh!

sipa commented Aug 9, 2020

Uh oh!

maflcko commented Aug 10, 2020

Uh oh!

amitiuttarwar commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

vasild commented Nov 22, 2019 •

edited

Loading

laanwj Nov 22, 2019 •

edited

Loading

laanwj commented Nov 22, 2019 •

edited

Loading

gmaxwell commented Nov 23, 2019 •

edited

Loading

vasild commented Nov 24, 2019 •

edited

Loading

laanwj commented Nov 27, 2019 •

edited

Loading

DrahtBot commented Mar 17, 2020 •

edited

Loading