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

1. 
   fanquake commented at 10:58 AM on November 29, 2018: member

   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.

2. fanquake added the label Utils/log/libs on Nov 29, 2018
3. 
   practicalswift commented at 12:43 PM on December 4, 2018: contributor

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

4. 
   promag commented at 3:05 PM on December 4, 2018: member

   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)) {	
   

5. 
   theuni commented at 9:58 PM on December 6, 2018: member

   @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:

   • glibc's pthread_setschedparam just forwards the struct
   • musl's pthread_setschedparam also just forwards the struct

   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;
   

6. 
   promag commented at 9:55 AM on December 7, 2018: member

   @theuni LGTM.

7. 
   fanquake commented at 1:08 PM on December 7, 2018: member

   I’ll update this PR next week.

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

   89282379ba
9. fanquake force-pushed on Dec 9, 2018
10. 
    fanquake commented at 6:59 AM on December 10, 2018: member

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

11. MarcoFalke added the label Needs gitian build on Dec 10, 2018
12. DrahtBot removed the label Needs gitian build on Dec 11, 2018
13. 
    laanwj commented at 3:55 PM on January 16, 2019: member

    utACK 89282379baa503156d9b85f116ae5672f8588b39

14. MarcoFalke deleted a comment on Jan 16, 2019
15. MarcoFalke added this to the milestone 0.18.0 on Jan 16, 2019
16. 
    promag commented at 4:05 PM on January 16, 2019: member

    Somehow forgot this.

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

    4. when a named variable (automatic, static, or thread-local) is declared with the initializer consisting of a pair of braces.

    4. otherwise, the object is zero-initialized.

    utACK 8928237.

17. laanwj merged this on Jan 16, 2019
18. laanwj closed this on Jan 16, 2019

    ---

19. laanwj referenced this in commit fcb6694a99 on Jan 16, 2019
20. fanquake deleted the branch on Jan 16, 2019
21. jasonbcox referenced this in commit 4ab217db85 on Sep 13, 2019
22. jonspock referenced this in commit cedb149ee6 on Dec 22, 2019
23. proteanx referenced this in commit 282ed811a8 on Dec 23, 2019
24. PastaPastaPasta referenced this in commit 31e18d0e23 on Jun 26, 2021
25. PastaPastaPasta referenced this in commit f591a545eb on Jun 26, 2021
26. PastaPastaPasta referenced this in commit b6faa97716 on Jun 27, 2021
27. PastaPastaPasta referenced this in commit f2b33a7d34 on Jun 28, 2021
28. DrahtBot locked this on Dec 16, 2021

Milestone
0.18.0