clang/win warnings fixit – with prices!

[Nico Weber]

Hi,

if you never build chrome on Windows, you can stop reading now.

On Windows, many of clang's useful warnings are currently disabled. We're holding a fixit from now (Monday ~morning in Japan / Australia) until Friday, July 10, 5pm Pacific to rectify this. If you fix the most warnings you'll win an electronic animal that lives in your fridge and talks to you when your fridge is open (one of these: https://www.youtube.com/watch?v=jCyQEmOVG_8).

So if you wanted to play with clang/win, you now have the opportunity to do this – and you get a chance to win a useful household appliance too!

If you're interested:

1. Pick a warning flag we currently disable from http://code.google.com/p/chromium/issues/list?can=2&q=blocking%3A504657+status%3Aavailable

2. Assign the bug to yourself, so that no duplicate work is being done

3. Build chrome with clang and the warning enabled locally (run `python tools\clang\scripts\update.py` once, add clang=1 to your GYP_DEFINES, and regyp – that's all. If you want colored warning messages from clang, also install ansicon before running gyp)

4. Fix all instances of the warning. (If the warning is in third-party code, it should probably be disabled for the third-party code it fires on.)

5. Check in fix

6. Go to 1.

For warnings I'm pretty sure will be useful, I said things like "good warning" on their bug.

You get 1 point for every warning you fix. If a warning requires changes in multiple repos (say, blink and chromium) you get 1 point for every repo you need to modify.

The person with the most points wins. You need at least 3 points to be eligible.

Happy hunting,

Nico

[Rouslan Solomakhin]

Kawaii!!!!

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Matt Giuca]

I'm trying to build with Clang now, and I am getting errors that 'precompile.h' is not found (full error below).

Windows 7. GYP_DEFINES with "target_arch=x64 component=shared_library clang=1". (Have tried various combinations of shared_library and fastbuild.)

It builds about 300 files, then starts hitting a few of these precompile.h errors and gives up.

(Another issue was that I couldn't do a 32-bit build because some "__try" keyword was not supported.)

A GN build seems to be going better so I'll stick with that (but another colleague said that GN on Windows builds but crashes on Chrome startup).

FAILED: ninja -t msvc -e environment.x64 -- "..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo /showIncludes /FC @obj\base\base_static.base_switches.obj.rsp /c ..\..\base\base_switches.cc /Foobj\base\base_static.base_switches.obj /Fdobj\base\base_static.cc.pdb

In file included from <built-in>:324:

<command line>(78,10) :  fatal error: 'precompile.h' file not found

#include "precompile.h"

         ^

1 error generated.

[Nico Weber]

On Mon, Jun 29, 2015 at 8:29 PM, Matt Giuca <mgi...@chromium.org> wrote:

I'm trying to build with Clang now, and I am getting errors that 'precompile.h' is not found (full error below).

Windows 7. GYP_DEFINES with "target_arch=x64 component=shared_library clang=1". (Have tried various combinations of shared_library and fastbuild.)

It builds about 300 files, then starts hitting a few of these precompile.h errors and gives up.

(Another issue was that I couldn't do a 32-bit build because some "__try" keyword was not supported.)

(This doesn't stop the build from completing successfully. From https://groups.google.com/a/chromium.org/d/msg/chromium-dev/h20a68y8Q_Q/n1kaW2vLQNIJ : "clang-cl can't compile __try in 32-bit yet, and it falls back to MSVC's compiler when it sees these statements. While this makes the build noisy, your build will still succeed")

 

A GN build seems to be going better so I'll stick with that (but another colleague said that GN on Windows builds but crashes on Chrome startup).

FAILED: ninja -t msvc -e environment.x64 -- "..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo /showIncludes /FC @obj\base\base_static.base_switches.obj.rsp /c ..\..\base\base_switches.cc /Foobj\base\base_static.base_switches.obj /Fdobj\base\base_static.cc.pdb

In file included from <built-in>:324:

<command line>(78,10) :  fatal error: 'precompile.h' file not found

#include "precompile.h"

         ^

1 error generated.

How do you set your GYP_DEFINES? Note that on Windows you need to write

> set GYP_DEFINES=target_arch=x64 component=shared_library clang=1

_without quotes_ after the =. What's the output of `echo %GYP_DEFINES%`? build/common.gypi tries to set chromium_win_pch if OS=="win" and clang==1, so it kind of looks like the gyp defines are maybe not getting through?

(Here's a bot that builds with the gyp defines you mentioned, and it's green: http://build.chromium.org/p/chromium.fyi/builders/CrWinClang64(dll) )

[Matt Giuca]

Hmm, I'm using the git-bash shell included in depot_tools (not cmd), so things might be a bit different. I'm setting the defines in ~/.gyp/include.gypi.

Just tried to go through using cmd. I opened cmd, ran the exact "set GYP_DEFINES" command you specified, gclient runhooks, and run ninja from the cmd command line. Same error.

I doubt it is that GYP_DEFINES are not getting through, because before I changed GYP_DEFINES it was building fine (with MSVC), and now it is trying to use clang.

GN is still working (but I haven't finished a build yet ... hit some actual compiler warnings after disabling a -Wno flag!)

[Matt Giuca]

Continuing this discussion (my build issue) in http://crbug.com/505711. Sorry for hijacking this thread!

[Matt Giuca]

Encountered a problem with GN which others on this quest may find important. (If you aren't building with GN this may bite you without realising!)

I hit a warning in a third-party module that I had to disable in its build file. First thing I tried was just adding the -Wno flag to this module's cflags:

--- a/third_party/qcms/BUILD.gn

+++ b/third_party/qcms/BUILD.gn

@@ -40,5 +40,11 @@ source_set("qcms") {

       "/wd4056",  # Overflow in floating-point constant arithmetic (INFINITY).

       "/wd4756",  # Overflow in constant arithmetic (INFINITY).

     ]

+

+    if (is_clang) {

+      cflags += [

+        "-Wno-sometimes-uninitialized",

+      ]

+    }

   }

 }

Unfortunately, this *did not* disable the warning. Thanks to +Sam McNally (very helpful), I found "gn desc <outdir> third_party/qcms" which shows the full gn debug dump. The problem is that -Wno-sometimes-uninitialized was being added right at the start, before /W3, which is in the global config and turns all warnings back on!

So basically, it will not be possible to disable any specific warnings in GN files for a particular module.

The workaround we found was to create a separate "config", so my new patch looks like this:

--- a/third_party/qcms/BUILD.gn

+++ b/third_party/qcms/BUILD.gn

@@ -6,6 +6,14 @@ config("qcms_config") {

   include_dirs = [ "src" ]

 }

+config("qcms_private_config") {

+  if (is_win && is_clang) {

+    cflags = [

+      "-Wno-sometimes-uninitialized",

+    ]

+  }

+}

+

 source_set("qcms") {

   sources = [

     "src/chain.c",

@@ -23,6 +31,7 @@ source_set("qcms") {

   configs -= [ "//build/config/compiler:chromium_code" ]

   configs += [ "//build/config/compiler:no_chromium_code" ]

+  configs += [ ":qcms_private_config" ]

   public_configs = [ ":qcms_config" ]

   if (current_cpu == "x86" || current_cpu == "x64") {

This inserts the -Wno flag at the end after /W3 (in roughly the same spot as all the other -Wnos), so it works.

Feels like a bit of a hack though. Is there any easier way to fix this? (I assume removing /W3 on Clang is out of the question?)

[Nico Weber]

That's (almost) the best you can do. You can make the config local to the source_set – see e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg/BUILD.gn&l=147 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx/BUILD.gn&l=218 for other examples. (Found via "wno- file:third_party.*\.gn" on cs.chromium.org which admittedly finds way more examples with cflags, which only help if the warnings aren't re-added by -Wall).

Nice work tracking this down, and sorry about not mentioning this in the OP.

[Matt Giuca]

Thanks for the help with those things, Nico!

Now I'm curious about what the policy should be for third_party libraries that have been copied (not DEPS'd) into the Chromium repo. Should we be disabling the warning on those, or actually fixing it in the code and adding a note to README.chromium?

I had a look in third_party/qcms/README.chromium and it seems that a lot of random changes have been made locally in the Chromium repo, including "fix warnings", so maybe I should just fix the code and add a note there instead of disabling the warning? Same for protobuf.

[Nico Weber]

On Mon, Jun 29, 2015 at 10:00 PM, Matt Giuca <mgi...@chromium.org> wrote:

Thanks for the help with those things, Nico!

Now I'm curious about what the policy should be for third_party libraries that have been copied (not DEPS'd) into the Chromium repo. Should we be disabling the warning on those, or actually fixing it in the code and adding a note to README.chromium?

I had a look in third_party/qcms/README.chromium and it seems that a lot of random changes have been made locally in the Chromium repo, including "fix warnings", so maybe I should just fix the code and add a note there instead of disabling the warning? Same for protobuf.

"It depends" :-)

In general, disabling the warning is the right thing to do for third-party code. But if a) it's a real, possibly real bug or b) the warning fires only once or twice and it looks like the third-party project tries to build with this warning enabled, you can fix it and contribute the fix upstream as well. In many cases, when I try to do that, I notice that the warning has already been fixed upstream – in these cases I usually cherry-pick the commit that fixed them. (And if the list of cherry-picks becomes too long, I sometimes try to update the dep to a newer upstream revision instead. Personally, I think having lots of local patches to a third-party dep is a bad idea, as it makes updating the dep difficult.)

Fixing it locally without upstreaming the patch is almost always the wrong thing though.

[Peter Kasting]

On Mon, Jun 29, 2015 at 10:00 PM, Matt Giuca <mgi...@chromium.org> wrote:

I had a look in third_party/qcms/README.chromium and it seems that a lot of random changes have been made locally in the Chromium repo, including "fix warnings", so maybe I should just fix the code and add a note there instead of disabling the warning? Same for protobuf.

When I've encountered these in the past, I've always fixed in Chromium's copy and then tried my best to upstream the fix as well.  Pulling a new version with the upstream fix can be harder for some targets, so I don't always make it that far, but at least that way the warning should be fixed if we pull a new future copy.

PK 

[Matt Giuca]

My approach so far has been to sent a patch upstream and make a suppression locally (for the entire third-party library). If/when the upstream merges the patch, I'll cherry-pick the patch locally and remove the suppression.

(I guess Peter's approach would be essentially the same advice, but assume the patch is merged upstream and cherry-pick the fix immediately.)

[Peter Kasting]

On Tue, Jun 30, 2015 at 1:31 AM, Matt Giuca <mgi...@chromium.org> wrote:

My approach so far has been to sent a patch upstream and make a suppression locally (for the entire third-party library). If/when the upstream merges the patch, I'll cherry-pick the patch locally and remove the suppression.

(I guess Peter's approach would be essentially the same advice, but assume the patch is merged upstream and cherry-pick the fix immediately.)

Yeah.  I think the difference in the two approaches is mostly that if we suppress locally, it's easy to forget to remove that suppression later, especially if upstream fixes the warning in a different way or at a different time than via the included patch.  That could probably be ameliorated by adding commentary to the README.chromium file.

PK 

[Nico Weber]

This is now over, 25 of the 28 disabled warnings got enabled. (And 2 of the remaining 3 are in process.)

Here are three examples of buggy code that was found and fixed as part of this fixit (even though we run our code under various static analyzers such as MSVC's /analyze already). Can you see the bug? Author and reviewer of these snippets couldn't:

1.

    bool Win32Window::setPosition(int x, int y) {

      if (mX == x && mY == mY)

        return true;

      // ...

    }

2.

    if (!IsService(&w8_service->mov_r10_rcx_mov_eax) ||

        w8_service->mov_1 != kMov1 || w8_service->mov_1 != kMov1 ||

        w8_service->mov_1 != kMov1) {

      return false;

    }

3.

    if (security == onc::wifi::kSecurityNone) {

      *authentication = kAuthenticationOpen;

      *encryption = kEncryptionNone;

    } else if (security == onc::wifi::kWEP_PSK) {

      *authentication = kAuthenticationOpen;

      *encryption = kEncryptionWEP;

      *key_type = kKeyTypeNetwork;

    }

In addition to this, well over 50 unused variables and member variables were deleted.

The winner of the household appliance is *drummroll*…sammc – congrats! Many thanks to the other participants mgiuca dcheng benwells scottmg as well :-)

On Sun, Jun 28, 2015 at 7:39 PM, Nico Weber <tha...@chromium.org> wrote:

[Thiago Farina]

On Mon, Jul 13, 2015 at 7:54 PM, Nico Weber <tha...@chromium.org> wrote:

This is now over, 25 of the 28 disabled warnings got enabled. (And 2 of the remaining 3 are in process.)

Here are three examples of buggy code that was found and fixed as part of this fixit (even though we run our code under various static analyzers such as MSVC's /analyze already). Can you see the bug? Author and reviewer of these snippets couldn't:

1.

    bool Win32Window::setPosition(int x, int y) {

      if (mX == x && mY == mY)

Here it is comparing mY to mY, which is always true. He may have wanted to compare mY to y.

 

        return true;

      // ...

    }

2.

    if (!IsService(&w8_service->mov_r10_rcx_mov_eax) ||

        w8_service->mov_1 != kMov1 || w8_service->mov_1 != kMov1 ||

        w8_service->mov_1 != kMov1) {

Here it appears to be doing the same check trice (w8_service->mov_1 != kMov1), not sure if that is the bug though.

Congrats to the participants!

--

Thiago Farina

[Matt Giuca]

Thanks for organizing this Nico! I'm looking forward to the sweet sweet error messages on Clang rather than the often indecipherable nonsense that comes out of MSVC!

Congrats Sam! Not only did he fix a huge amount of warnings himself, but he also rallied the other Sydney participants into action! Deservedly the winner.

(Sent again from the right address.)

--