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

Fix Compiler Warnings #84

Merged
merged 2 commits into from Oct 25, 2018

Conversation

3 participants
@jmjatlanta
Copy link

commented Oct 22, 2018

Fix for bitshares/bitshares-core#1383

Several compiler warnings have appeared due to upgrades of compilers and OpenSSL.

Synopsis of changes:

rand.hpp - OpenSSL's CRYPTO_pseudo_random_bytes has been deprecated. This is used in only 1 place in Bitshares, and should be replaced with fc::rand_bytes (net/node.cpp).

openssl.cpp - OPENSSL_config(nullptr) is no longer needed with OpenSSL 1.1. It has been replaced with an init function that is automatically called with defaults when needed.

The rest of the changes were simply to squelch compiler warnings and SHOULD NOT change code logic.

@@ -429,7 +429,7 @@ openssl_thread_config::openssl_thread_config()
}
openssl_thread_config::~openssl_thread_config()
{
if (CRYPTO_get_id_callback() == &get_thread_id)
if (CRYPTO_get_id_callback() != NULL && CRYPTO_get_id_callback() == &get_thread_id)

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 22, 2018

Isn't the first check redundant? &get_thread_idwill never be NULL.

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Oct 23, 2018

Author

Yes, it is redundant. It did avoid a warning, but I would prefer to not add the logic. I took it back out, and the warning returned. But I think it better to live with the warning.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 23, 2018

What does the warning say? Are we missing something?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Oct 23, 2018

Author

Yes, including what I am talking about in the note would have been smart. Adding the CRYPTO_get_id_callback() != nullptr && to the if fixed the warning, but I hate adding such code just to squelch a warning.

This is the original warning:

/src/crypto/aes.cpp:432:32: warning: the address of ‘static long unsigned int fc::openssl_thread_config::get_thread_id()’ will never be NULL [-Waddress]
   if (CRYPTO_get_id_callback() == &get_thread_id)

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 23, 2018

CRYPTO_get_id_callback is deprecated - does the warning persist if you switch to CRYPTO_THREADID_[gs]et_callback? https://linux.die.net/man/3/crypto_lock

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Oct 24, 2018

Author

Yes, unfortunately.

@jmjatlanta

This comment has been minimized.

Copy link
Author

commented Oct 23, 2018

I reverted the change for the null check, and the warning has reappeared. Personally, I think the warning should stay there until a better fix is provided.

A different quick fix is to do the compare using a macro, casting to void*. But it seems wrong to me to add such code just to avoid a warning. Please share your thoughts.

@pmconrad
Copy link

left a comment

Ok, after some googling is seems that the warning about the non-null function address is a flaw in g++. We should leave it in, hopefully it'll be fixed in a later gcc version.

@abitmore abitmore merged commit 8b6a2dd into master Oct 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@abitmore abitmore deleted the jmj_1383 branch Oct 25, 2018

@jmjatlanta jmjatlanta referenced this pull request Oct 25, 2018

Closed

BitShares-fc compilation warnings #1383

1 of 17 tasks complete

@oxarbitrage oxarbitrage referenced this pull request Oct 26, 2018

Merged

bump fc and fix node #1405

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.