Announcement

Collapse
No announcement yet.

Linux 5.15 Enabling "-Werror" By Default For All Kernel Builds

Collapse
X
 
  • Filter
  • Time
  • Show
Clear All
new posts

  • #31
    Originally posted by coder View Post
    Depends on what you mean by "all warnings". Some developers tend to be fairly aggressive, in terms of which they enable.
    When I wrote "all" I meant all those that are enabled. I do have a lot enabled (most notable exception is "unused parameters"), but not all those that need to be enabled individually.

    Originally posted by coder View Post
    Unfortunately, the more advanced warnings tend to be less reliable, which becomes problematic when someone decides to enact a -Werror policy.

    I've seen significant amounts of time wasted, and bugs actually getting introduced, in the course of fixing false-positive warnings (i.e. those which didn't indicate any real problem). In many instances, the resulting code is worse, in terms of being less readable or maintainable. I would argue that most of these warnings shouldn't even have been enabled.
    For example for which warnings? Maybe that comes with using a lot of external/legacy source code or include files that were written with a lot of warnings disabled?

    Comment


    • #32
      Originally posted by NobodyXu View Post
      Linux kernel doesn’t use standard IO functions provided by C stdio.h, so this option likely do nothing.
      "-Werror=format-security" does not require you to use C stdio.h for it to be useful.

      https://gcc.gnu.org/onlinedocs/gcc-3...ttributes.html

      Code:
      extern int my_printf (void *my_object, const char *my_format, ...) __attribute__ ((format (printf, 2, 3)));
      The fun of the format attribute. You can name your printf/scanf functions what ever you like as long as you include attribute that gcc/clang knows what the it is.

      https://github.com/torvalds/linux/bl...ributes.h#L155
      https://github.com/torvalds/linux/bl...red/user.h#L43

      Yes surprise its already declared all over the place around the Linux kernel. So yes -Werror=format-security if you do turn it on with the Linux kernel it really does something.

      Comment


      • #33
        Originally posted by set135 View Post
        This does not change the bar for code getting in. This is only for robots who otherwise might not flag warnings. Linus has never tolerated warnings and if you ask him to pull code that outputs warnings he will likely not accept it. He also hates useless warnings and patches that do nothing useful except shut them up. So, either the warning is helpful and the code is fixed, or it's useless and shut off.
        This is not quite right. Linus of the lInux kernel when a warning is in fact useless to disable it usage a bug report has to be submitted to the compiler developer and be confirmed that this is a bug even with this its not sure that the warning test will be turned off globally.

        https://github.com/torvalds/linux/bl...ents/qla.h#L12

        There are few examples in the Linux kernel
        #pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
        this has only been required 10 times across the complete kernel. This has a fairly good success rate of being 1000 to 1 yes over 10000 faults were found when that was first turned on only 10 were in fact errors.

        Best practise is really to leave as many compiler error detections on as you can and tag out the areas that those features don't work.

        Linus will accept patches that disable invalid warnings from being displayed he will be demanding you have confirmation from the compiler developer before submit the patch or he will have your head.

        Comment


        • #34
          Well considering most of Linus' legendary rants have been about code quality this is at the very least tonally consistent with the guy.

          I also wouldn't be the least bit surprised if this is primarily aimed at graphics driver devs as they are pretty infamous for their nonchalant attitude towards code quality and even serious warnings thrown up by compilers.
          "Why should I want to make anything up? Life's bad enough as it is without wanting to invent any more of it."

          Comment


          • #35
            It's surprising it hasn't been a case until now. OpenSSL has been doing this for ages.

            Comment


            • #36
              well... well.. linus is looking out for errors on his branch for a quite sometime and probably notoiced or better irked with simple or medium issues which can be fixed before sending patches.

              lets see how this will effect devs and the developnent process of kernel itself.

              Comment


              • #37
                That reference is from 3 years ago.

                Nowadays, all Mesa CI build jobs enable -Werror[0]. That means it's not possible to merge changes which cause compiler warnings in the most common build configurations.

                This is the best we can do. Enabling -Werror for all builds would leave some users with different compiler versions or unusual build configurations unable to build Mesa.

                [0] Some jobs still have to make exceptions for a small number of warnings. The goal is to keep shrinking and eventually eliminate those as the corresponding warnings are fixed.

                Comment


                • #38
                  Originally posted by Chewi View Post
                  It seems like a good idea on the face of it but compilers flag up more and more things as warnings with every release, meaning that trying to build older kernels with newer compilers won't work. Good thing it's configurable.
                  I think this part is key. Newer compilers necessarily evolve the warnings they emit. Putting -Werror by default is generally considered bad practice on a library cause you never know what a user is going to compile with. I understand Linus’ motivation but this is a real downside that his comments don’t seem to acknowledge.

                  others here mention the alternative that many libraries use, which is to enable Werror in all CI build bots. This way the patches never make it to review but still allow local/user builds even with warnings. Maybe that approach doesn’t work for the kernel due to the uniqueness of its development approach.

                  at least there is an opt out in kconfig, but seems that will be the first thing disabled in developer builds which might defeat the purpose of the enabled by default setting

                  Comment


                  • #39
                    Originally posted by Markopolo View Post
                    -Werror by default is generally considered bad practice on a library cause you never know what a user is going to compile with.
                    Except this is a part false statement. For security dependant libraries its in fact bad practice not to have -Werror features on by default and instead have option to disable it if required.

                    Originally posted by Markopolo View Post
                    Newer compilers necessarily evolve the warnings they emit.
                    NSA and many other guides cover this. Why on by default if there is new issue you want to error out and not just build silently so that someone in fact has to look at the problem and change something.

                    Failure in build stage that the developer has to look at that may cause the developer to see the problem is better than having the software deployed on million+ systems with a bug.

                    Originally posted by Markopolo View Post
                    at least there is an opt out in kconfig, but seems that will be the first thing disabled in developer builds which might defeat the purpose of the enabled by default setting
                    This also the problem. You missed that the reverse is the case as well were particular parties can get punished for lack of due care building a kernel without -Werror on and they have had to be making sure they have been turning it manually on.

                    To be regulation conforming in many countries -Werror features really do need to be on by default. Now if a developer decides to turn that off ouch.

                    Originally posted by Markopolo View Post
                    which is to enable Werror in all CI build bots.
                    Now there is a problem here where people will setup CI at some point and forget to enable -Werror as well. Off switch with stuff like this is a lot safer than the on switch route. If you are stuborn or need to build with -Werror off you still can.

                    Think about it what is a worse error -Werror being off in CI builds because someone forgot to turn it on resulting in badly coded patches getting mainline or -Werror being on in a individual developer build and it failing because they forgot to turn it off.

                    The reality here -Werror off by default is a far higher risk of large stuff ups.

                    Comment


                    • #40
                      Werror is OK for dev builds, but for stable releases it's totally nuts. Every time I update gcc stuff breaks because gcc adds new warnings and I have to patch out that shit.

                      Comment

                      Working...
                      X