lib: fix a compiler warning: unused GetDevURandom() #17563

vasild commented at 8:30 PM on November 22, 2019: member

~~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 added the label Utils/log/libs on Nov 22, 2019

fanquake commented at 8:34 PM on November 22, 2019: member

What warning are you seeing, compiling for which OS?

in src/random.cpp:252 in 8d2d95de6e outdated

 247 | @@ -248,7 +248,9 @@ static void Strengthen(const unsigned char (&seed)[32], int microseconds, CSHA51
 248 |      memory_cleanse(buffer, sizeof(buffer));
 249 |  }
 250 |  
 251 | -#ifndef WIN32
 252 | +#if !defined(WIN32) && \
 253 | +  !(defined(HAVE_GETENTROPY) && defined(__OpenBSD__)) && \

laanwj commented at 8:42 PM on November 22, 2019:

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 commented at 9:32 PM on November 22, 2019: member

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

vasild force-pushed on Nov 23, 2019

vasild commented at 3:55 PM on November 23, 2019: member

@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 commented at 5:46 PM on November 23, 2019: contributor

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 commented at 8:22 PM on November 23, 2019: member

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.

MarcoFalke commented at 1:44 AM on November 24, 2019: member

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 force-pushed on Nov 24, 2019

vasild commented at 1:03 PM on November 24, 2019: member

3rd attempt (https://github.com/bitcoin/bitcoin/commit/8a16b9d448b90aa5a234dc326ebda45b99b363b7): 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 commented at 12:51 PM on November 27, 2019: member

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 force-pushed on Mar 4, 2020

vasild commented at 12:44 PM on March 4, 2020: member

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

fanquake commented at 7:48 AM on March 17, 2020: member

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

vasild commented at 8:19 AM on March 17, 2020: member

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

Thanks!

DrahtBot commented at 12:15 PM on March 17, 2020: member

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

Conflicts

No conflicts as of last run.

laanwj referenced this in commit 1d64dfe4fa on Mar 17, 2020

DrahtBot added the label Needs rebase on Mar 17, 2020

vasild force-pushed on Mar 17, 2020

vasild commented at 8:12 PM on March 17, 2020: member

Updated, now that #18364 is in.

DrahtBot removed the label Needs rebase on Mar 17, 2020

sidhujag referenced this in commit db7e54243a on Mar 18, 2020

DrahtBot added the label Needs rebase on Mar 19, 2020

vasild force-pushed on Mar 20, 2020

DrahtBot removed the label Needs rebase on Mar 20, 2020

Fix a compiler warning: unused GetDevURandom()

```
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.

ca2e474372

in src/random.cpp:252 in 5d672345a6 outdated

 248 | @@ -249,7 +249,22 @@ static void Strengthen(const unsigned char (&seed)[32], int microseconds, CSHA51
 249 |      memory_cleanse(buffer, sizeof(buffer));
 250 |  }
 251 |  
 252 | -#ifndef WIN32
 253 | +#if defined(WIN32)

sipa commented at 6:50 PM on March 20, 2020:

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.

vasild commented at 8:00 PM on March 20, 2020:

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.

vasild force-pushed on Mar 20, 2020

fanquake commented at 9:53 AM on May 11, 2020: member

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 commented at 3:42 PM on May 11, 2020: member

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 (review)

hebasto approved

hebasto commented at 8:56 AM on June 13, 2020: member

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

hebasto approved

hebasto commented at 5:28 PM on August 9, 2020: member

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

master (e349eeeb2c9219908686f430b3d74d1b2d5c1106)

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 commented at 7:35 PM on August 9, 2020: contributor

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

sipa commented at 8:45 PM on August 9, 2020: member

utACK ca2e47437277ef6851a739f247b44e73a53f21a1

MarcoFalke commented at 12:40 PM on August 10, 2020: member

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

fanquake merged this on Aug 10, 2020

fanquake closed this on Aug 10, 2020

vasild deleted the branch on Aug 10, 2020

sidhujag referenced this in commit 5a1c112b9d on Aug 10, 2020

amitiuttarwar commented at 3:31 AM on August 12, 2020: contributor

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

sidhujag referenced this in commit 7f195d3241 on Nov 10, 2020

PastaPastaPasta referenced this in commit 6ce3b3a436 on Jun 27, 2021

PastaPastaPasta referenced this in commit 48a4d55ac7 on Jun 28, 2021

PastaPastaPasta referenced this in commit d02bd58e74 on Jun 29, 2021

PastaPastaPasta referenced this in commit def51f6842 on Jul 1, 2021

PastaPastaPasta referenced this in commit 401479490b on Jul 1, 2021

PastaPastaPasta referenced this in commit 54e81abc8d on Jul 15, 2021

PastaPastaPasta referenced this in commit 3c79f6120f on Jul 16, 2021

deadalnix referenced this in commit c2bd741169 on Sep 15, 2021

DrahtBot locked this on Feb 15, 2022