Potential follow-up work wrt -Wimplicit-fallthrough for chromium's dependencies that build in the chromium_code config |
||||||||
Issue descriptionI'm adding -Wimplicit-fallthrough to the chromium_code config in issue 177475 / https://chromium-review.googlesource.com/#/c/chromium/src/+/895726 Several of Chromium's dependencies use this config (which is a good thing imho). I cleaned up all the code in src.git to build with the new warning, but I don't know if all dependencies want it, and I also want to enable the warning for chromium before all dependencies can build with it even if they all want it. So I'm going to disable the warning for the dependencies for now (namely: angle and angle's deps vulkan_suppport, spirv_tools, and glslang; v8; pdfium; cld3; breakpad). If any of these dependencies _does_ want to enable the warning, file a bug blocking this one and do the work there. You'll have to add a YOUR_DEP_FALLTHROUGH macro that looks like so: https://cs.chromium.org/chromium/src/base/compiler_specific.h?type=cs&q=fallthrough&sq=package:chromium&l=225 Or, if you want to be comprehensive and support many compilers, like so: https://cs.chromium.org/chromium/src/third_party/boringssl/src/crypto/internal.h?type=cs&q=openssl_fall&sq=package:chromium&l=175 It is important that you define this fallthrough macro in a project-internal header that clients of your library (like chromium) never include -- because clang's diagnostic checks if there's a macro expanding to [[clang::fallthrough]] defined, and if so it suggests the first macro expanding to it. So if your macro is defined in a public header, clang will suggest your macro instead of chromium's macro when compiling chromium code, which is confusing to devs. (see also https://boringssl-review.googlesource.com/c/boringssl/+/25464) After adding the macro, enable the warning for your project, and in each place where the compiler complains, check if the fallthrough is intentional. If so, add your macro, else add the missing break. (The bug linked to at the start has many examples.) No action needs to be taken; I'm filing this bug so I can refer to it. cc'ing a few folks fyi.
,
Jan 31 2018
,
Feb 1 2018
,
Feb 1 2018
Here's a list of chromium_code targets that suppress the warning: https://cs.chromium.org/search/?q=crbug.com%5C/807632&sq=package:chromium&type=cs Currently, angle + its deps, pdfium, v8. (I fixed breakpad and cld3.) Also, skia isn't chromium_code but it's the type of thing they might be interested in opting in to themselves, so +mtklein
,
Feb 1 2018
Yep, this does seem like a good thing to do in Skia. Is [[fallthrough]] generally supported now, or do you do compiler-specific things?
,
Feb 1 2018
Ah, I should have Googled before I asked... since C++17. I'll see about doing a Clang-only version in Skia too.
,
Feb 1 2018
See comment 0 starting at paragraph 5 :-) [[fallthrough]] is C++17 and available ~nowhere in chrome atm.
,
Feb 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/mini_chromium/+/3b953302848580cdf23b50402befc0ae09d03ff9 commit 3b953302848580cdf23b50402befc0ae09d03ff9 Author: Scott Graham <scottmg@chromium.org> Date: Sat Feb 03 02:49:29 2018 Add -Wimplicit-fallthrough when building on clang Bug: chromium:807632 Change-Id: I02904913dd32c67b578f591fd0244a7adca4907b Reviewed-on: https://chromium-review.googlesource.com/899847 Reviewed-by: Mark Mentovai <mark@chromium.org> [modify] https://crrev.com/3b953302848580cdf23b50402befc0ae09d03ff9/build/BUILD.gn [modify] https://crrev.com/3b953302848580cdf23b50402befc0ae09d03ff9/base/compiler_specific.h [modify] https://crrev.com/3b953302848580cdf23b50402befc0ae09d03ff9/build/common.gypi
,
Feb 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/crashpad/crashpad.git/+/a8ecdbc973d969a87aaa2efffb1668efb52b799d commit a8ecdbc973d969a87aaa2efffb1668efb52b799d Author: Scott Graham <scottmg@chromium.org> Date: Sat Feb 03 03:05:45 2018 Updates to support -Wimplicit-fallthrough https://chromium-review.googlesource.com/c/chromium/mini_chromium/+/899847 turns the warning on. This adds one annotation, and fixes one bug. Includes mini_chromium roll: .../mini_chromium$ git log 5fcfa43c1587b94132e24782579350cb8266b990..3b953302848580cdf23b50402befc0ae09d03ff9 --oneline 3b95330 (HEAD, origin/master, origin/HEAD) Add -Wimplicit-fallthrough when building on clang Bug: chromium:807632 Change-Id: I2f3ddca0228e52013844cb8d78d10cb359e851d0 Reviewed-on: https://chromium-review.googlesource.com/900317 Commit-Queue: Scott Graham <scottmg@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> [modify] https://crrev.com/a8ecdbc973d969a87aaa2efffb1668efb52b799d/DEPS [modify] https://crrev.com/a8ecdbc973d969a87aaa2efffb1668efb52b799d/handler/linux/exception_handler_server.cc [modify] https://crrev.com/a8ecdbc973d969a87aaa2efffb1668efb52b799d/minidump/minidump_context_writer.cc
,
Feb 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbd36495fc5ac6c3fc7eb59fa2eb667358336ced commit cbd36495fc5ac6c3fc7eb59fa2eb667358336ced Author: Peng Huang <penghuang@chromium.org> Date: Tue Feb 06 19:44:31 2018 Fix build errors with enable_vulkan=true for Linux Bug: 807632 Change-Id: I4ba225af77087aec951420e033c6e361f88f249d Reviewed-on: https://chromium-review.googlesource.com/905222 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Peng Huang <penghuang@chromium.org> Cr-Commit-Position: refs/heads/master@{#534756} [modify] https://crrev.com/cbd36495fc5ac6c3fc7eb59fa2eb667358336ced/third_party/SPIRV-Tools/BUILD.gn [modify] https://crrev.com/cbd36495fc5ac6c3fc7eb59fa2eb667358336ced/third_party/glslang/BUILD.gn
,
Feb 9 2018
,
Feb 9 2018
I looked at v8 a bit (enough to get unittests d8 mksnapshot to compiler in my chromium/linux checkout), see https://chromium-review.googlesource.com/c/v8/v8/+/911731 Not sure if it's worth pursuing that. I looked at angle and that seems definitely worth it, landing at https://chromium-review.googlesource.com/c/angle/angle/+/911529 I looked at pdfium at https://pdfium-review.googlesource.com/c/pdfium/+/26210 and it didn't find anything, but it was also easy to enable, so probably do want that.
,
Feb 9 2018
Just to confirm that the ANGLE fix was very appreciated and found a number of very real and embarrassing bugs.
,
Feb 15 2018
,
Feb 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/b66de58fc571d7d27e8316f2ff1853b3d8d440b4 commit b66de58fc571d7d27e8316f2ff1853b3d8d440b4 Author: Nico Weber <thakis@chromium.org> Date: Fri Feb 16 02:09:12 2018 Mark third-party code in ANGLE as no_chromium_code. That way, we don't have to explicitly disable Wimplicit-fallthrough for these targets, and when we add new warnings to chromium_code in the future, these targets won't need any special attention. Like https://chromium-review.googlesource.com/c/chromium/src/+/905222 Bug: 807632 Change-Id: I3a605dd3ef9ed7d8cfe9e36964c3433040bfc330 Reviewed-on: https://chromium-review.googlesource.com/922503 Reviewed-by: Jamie Madill <jmadill@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> [modify] https://crrev.com/b66de58fc571d7d27e8316f2ff1853b3d8d440b4/third_party/glslang/BUILD.gn [modify] https://crrev.com/b66de58fc571d7d27e8316f2ff1853b3d8d440b4/third_party/vulkan-validation-layers/BUILD.gn [modify] https://crrev.com/b66de58fc571d7d27e8316f2ff1853b3d8d440b4/third_party/spirv-tools/BUILD.gn
,
Feb 21 2018
The query in comment 4 now returns nothing (or would if it wasn't for issue 814337), so I'm calling this done. mtklein, you might still want to do this for skia, but it's now done for all chromium_code.
,
Feb 21 2018
Yep, just started looking into this for Skia. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by scottmg@chromium.org
, Jan 31 2018