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

[rebase] threads: fix unitialized members in sched_param #14839

Merged
merged 1 commit into from Jan 16, 2019

Conversation

Projects
None yet
7 participants
@fanquake
Copy link
Member

commented Nov 29, 2018

Rebased theuni's #14342.

Building with gcc 8.2 against musl libc, which apparently has more attributes available in its sched_param. The following warnings were produced:

warning: missing initializer for member 'sched_param::sched_ss_low_priority' [-Wmissing-field-initializers]
warning: missing initializer for member 'sched_param::sched_ss_repl_period' [-Wmissing-field-initializers]
warning: missing initializer for member 'sched_param::sched_ss_init_budget' [-Wmissing-field-initializers]
warning: missing initializer for member 'sched_param::sched_ss_max_repl' [-Wmissing-field-initializers]

Since the current thread may have interesting non-zero values for these fields, we want to be sure to only change the intended one. Query and modify the current sched_param rather than starting from a zeroed one.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

@fanquake I'm trying to reproduce. Can you provide the options you used when building?

@promag
Copy link
Member

left a comment

Those fields are only considered when policy is set to SCHED_SPORADIC, see http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sched.h.html.

Removing compiler warning should be enough:

    sched_param param;
    param.sched_priority = 0;
    if (int ret = pthread_setschedparam(pthread_self(), SCHED_BATCH, &param)) {	
@theuni

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

@promag You're right, but that's a little over-simplified :).
SCHED_BATCH is non-standard, so it's not immediately clear if the optional fields would override any current thread params. Also, other libc's may have additional non-standard fields.

The two major pthread implementations in question:

Since SCHED_BATCH is Linux-specific, and the Linux kernel discards everything other than priority and policy, we know that no libc-specific sched_param members will be used. So the my proposed change was overly paranoid. All we really need to do is zero the whole struct rather than just the first member to silence the warnings:

diff --git a/src/util/system.cpp b/src/util/system.cpp
index 8e201ec..4309da3 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -1293,7 +1293,7 @@ fs::path AbsPathForConfigVal(const fs::path& path, bool net_specific)
 int ScheduleBatchPriority()
 {
 #ifdef SCHED_BATCH
-    const static sched_param param{0};
+    const static sched_param param{};
     if (int ret = pthread_setschedparam(pthread_self(), SCHED_BATCH, &param)) {
         LogPrintf("Failed to pthread_setschedparam: %s\n", strerror(errno));
         return ret;
@promag

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

@theuni LGTM.

@fanquake

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2018

I’ll update this PR next week.

threads: fix unitialized members in sched_param
Building with gcc 8.2 against musl libc, which apparently has more attributes
available in its sched_param. The following warnings were produced:

    warning: missing initializer for member 'sched_param::sched_ss_low_priority' [-Wmissing-field-initializers]
    warning: missing initializer for member 'sched_param::sched_ss_repl_period' [-Wmissing-field-initializers]
    warning: missing initializer for member 'sched_param::sched_ss_init_budget' [-Wmissing-field-initializers]
    warning: missing initializer for member 'sched_param::sched_ss_max_repl' [-Wmissing-field-initializers]

Since the current thread may have interesting non-zero values for these fields,
we want to be sure to only change the intended one. Query and modify the
current sched_param rather than starting from a zeroed one.

@fanquake fanquake force-pushed the fanquake:rebased-fix-musl-sched branch to 8928237 Dec 9, 2018

@fanquake

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2018

@theuni I've updated this with your suggested change.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

utACK 8928237

@bitcoin bitcoin deleted a comment from DrahtBot Jan 16, 2019

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Jan 16, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Somehow forgot this.

From https://en.cppreference.com/w/cpp/language/value_initialization

  1. when a named variable (automatic, static, or thread-local) is declared with the initializer consisting of a pair of braces.
  1. otherwise, the object is zero-initialized.

utACK 8928237.

@laanwj laanwj merged commit 8928237 into bitcoin:master Jan 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 16, 2019

Merge #14839: [rebase] threads: fix unitialized members in sched_param
8928237 threads: fix unitialized members in sched_param (Cory Fields)

Pull request description:

  Rebased theuni's #14342.

  Building with gcc 8.2 against musl libc, which apparently has more attributes available in its sched_param. The following warnings were produced:

      warning: missing initializer for member 'sched_param::sched_ss_low_priority' [-Wmissing-field-initializers]
      warning: missing initializer for member 'sched_param::sched_ss_repl_period' [-Wmissing-field-initializers]
      warning: missing initializer for member 'sched_param::sched_ss_init_budget' [-Wmissing-field-initializers]
      warning: missing initializer for member 'sched_param::sched_ss_max_repl' [-Wmissing-field-initializers]

  Since the current thread may have interesting non-zero values for these fields, we want to be sure to only change the intended one. Query and modify the current sched_param rather than starting from a zeroed one.

Tree-SHA512: a0bedbcf0130b3ee8261bb704e4bf6c9b760ad377c8a28c258765d54e54462b76707efc188b936b0a635cdd2bdf6b3b9298ab06ba361dc4806150b670d9702a3

@fanquake fanquake deleted the fanquake:rebased-fix-musl-sched branch Jan 16, 2019

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.