Jumbo build breakage in v8 |
|||||
Issue descriptionThe jumbo build recently broke when using the following GN args: enable_nacl = false target_cpu = "x86" remove_webcore_debug_symbols = true symbol_level = 2 is_win_fastlink = false use_goma = true goma_dir = "C:\src\goma" use_jumbo_build = true is_clang = true is_component_build = true win_console_app = true use_lld=true use_ghash=true The error is: In file included from gen/v8/v8_base_jumbo_32.cc:6: In file included from .\../../v8/src/identity-map.cc:5: In file included from ../../v8\src/identity-map.h:9: In file included from ../../v8\src/handles.h:13: In file included from ../../v8\src/checks.h:10: ../../v8\src/globals.h(937,3): error: expected identifier CONST, // declared via 'const' declarations (last lexical) ^ C:\src\chrome\src\third_party\depot_tools\win_toolchain\vs_files\1180cb75833ea365097e279efb2d5d7a42dee4b0\win_sdk\bin\..\..\win_sdk\Include\10.0.15063.0\shared\minwindef.h(153,29): note: expanded from macro 'CONST' #define CONST const ^ The problematic arguments are apparently use_goma and use_jumbo_build. Apparently using Goma changes the jumbo merge limit and it explains why this isn't failing on the continuous builders.
,
May 10 2018
How about switching to kConstantStyle for the enums (https://google.github.io/styleguide/cppguide.html#Enumerator_Names)?
,
May 10 2018
Yep, that should work as long as there's no other enum named kConst in any of the header v8 files, I'll give it a shot.
,
May 14 2018
,
May 14 2018
Maybe switch to an enum class if kConst is too common a name?
,
May 14 2018
I've tried to switch to an enum class but it doesn't work (error is similar), so switching to kConst might be our best hope. Note that this is not failing right now, it looks like some things change in v8 and it's building fine now, but this might not last... (we might just be lucky on how the source files get merged). Renaming to kConst might still be worth it, not urgent but this might prevent future failures.
,
May 17 2018
This is happening again... I've looked at renaming CONST (and the variable in this enum) to kConst but this looks like a pretty big change (the enum values are used in a lot of files!). +hablich@ before doing a change like this I'd like to make sure that your team is ok with this?
,
May 17 2018
Another option (not recommending it necessarily, but something to consider): Create an undef_windows.h header which #undef's annoying macros like CONST, then #include it in any files that #include windows.h - I only see five such files in v8/src, but there are possibly other header files that pull in windows.h transitively (I don't have access to a windows machine to test this).
,
May 17 2018
Sebastien pointed out that such a file does exist. I have found the #undef technique to be impractical in many cases in Chrome, although it very much depends on the context. Here's v8's undef file: https://cs.chromium.org/chromium/src/v8/src/base/win32-headers.h?q=win32-headers.h&sq=package:chromium&g=0&l=68
,
May 17 2018
Thanks for the heads-up Seb. I added Yang and machenbach who should be able to better assess the situation.
,
May 17 2018
Note that I've started working on a fix, see https://chromium-review.googlesource.com/c/v8/v8/+/1064197 (still a WIP, the unittests don't compile yet and there's still some issues but it's *almost* done).
,
May 17 2018
I've made more progress on my CL (https://chromium-review.googlesource.com/c/v8/v8/+/1064197) but I'm still getting some failures on some of the V8 tests, I'll take another look later but if someone from V8 has any idea on why these tests might fail that'd help me :) ("Parser failed on: '({let});' with error: Unexpected token '}'").
,
May 18 2018
Why do you use goma and jumbo together? I thought these are orthogonal features...
,
May 18 2018
General answer, not specific to v8: jumbo and goma solve the same problem in different ways. There is an intersection which is probably faster than either of them alone but it all comes down to tuning and what you value most. Hard to measure too since goma performance depends on its caches so if you just run your own tuning/config you will get cold caches and slower builds.
,
May 18 2018
Well, the instructions don't say that you shouldn't use both at the same time (https://chromium.googlesource.com/chromium/src/+/master/docs/jumbo.md) , we just reduce the jumbo merge limit in this case (see https://cs.chromium.org/chromium/src/build/config/jumbo.gni?l=38&rcl=06d3b4517b9c527cf598ed3c6e2fb08456629acd) This is something that could/would affect the non-goma build anyway, so it's probably worth fixing it before it breaks it?
,
May 18 2018
Agreed. The CONST macro together with the CONST enum is a time bomb that could go off anytime, particularly in jumbo builds. I've added "get Microsoft to use fewer macros" to my time machine bucket list.
,
May 22 2018
Ping? I've a fix almost ready for this (https://chromium-review.googlesource.com/c/v8/v8/+/1064197) , but I'd like to be sure that you're ok with this change before finishing it.
,
May 22 2018
Whom do you mean by "you"? For the change it would be best to find a v8 code owner (which I'm rather not). Maybe just post it to v8-dev google group describing the problem that you want to solve?
,
May 22 2018
By "you" I meant whoever is in a position to approve this kind of change :), posting to v8-dev@ is a good suggestion, I'll do this! Thanks!
,
May 23 2018
Update, my fix seems to be working (fix the problem locally, and my CL passed the CQ dry run), it's up for review now: https://chromium-review.googlesource.com/c/v8/v8/+/1064197
,
May 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/df2419039cd591134f3666329204ea9b2fd12084 commit df2419039cd591134f3666329204ea9b2fd12084 Author: Sebastien Marchand <sebmarchand@chromium.org> Date: Tue May 29 14:36:48 2018 Fix a Jumbo breakage see crbug.com/841460 , we recently hit some build issues when using Goma + jumbo builds because of a conflict on the definition of CONST, v8 defines it in globals.h and including windows.h also defines it. It should be possible to fix this by adding a bunch of #undef CONST but it seems a little bit hacky and might not always work (this could only fix the problem temporary if the jumbo merge limit changes and cause some include files to get included in a different order). Renaming the v8 definition of CONST to kConst, this follows the style guide guidelines: "there is no reason to change old code to use constant-style names, unless the old names are actually causing a compile-time problem" (https://google.github.io/styleguide/cppguide.html#Enumerator_Names) I also had to turn the PropertyConstness enum into an enum class to avoid some conflicts (both PropertyConstness and VariableMode define kConst). Bug: chromium:841460 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I2b70b9095374e88a5ae364cc557b39f20a3ab60f Reviewed-on: https://chromium-review.googlesource.com/1064197 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Reviewed-by: Leszek Swirski <leszeks@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Sigurd Schneider <sigurds@chromium.org> Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org> Cr-Commit-Position: refs/heads/master@{#53413} [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/ast/ast.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/ast/scopes.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/ast/variables.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/ast/variables.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/compiler/access-info.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/compiler/js-native-context-specialization.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/contexts.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/globals.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/ic/accessor-assembler.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/ic/handler-configuration-inl.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/ic/ic.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/interpreter/bytecode-generator.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/lookup.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/map-updater.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/map-updater.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/objects.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/objects/map-inl.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/objects/map.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/objects/scope-info.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/parsing/expression-scope-reparenter.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/parsing/parser-base.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/parsing/parser.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/parsing/parser.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/parsing/pattern-rewriter.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/parsing/preparser.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/property-details.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/property.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/src/property.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/test/cctest/heap/test-heap.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/test/cctest/scope-test-helper.h [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/test/cctest/test-code-stub-assembler.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/test/cctest/test-field-type-tracking.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/test/cctest/test-parsing.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/test/cctest/test-transitions.cc [modify] https://crrev.com/df2419039cd591134f3666329204ea9b2fd12084/test/cctest/test-unboxed-doubles.cc
,
May 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/38ae84f430d4415cd3c29b18a251aaa6b8640f20 commit 38ae84f430d4415cd3c29b18a251aaa6b8640f20 Author: Junliang Yan <jyan@ca.ibm.com> Date: Wed May 30 14:42:28 2018 Fix GCC 5.4 compilation error GCC 5.4 complains type mismatch as follows: ../../v8/src/contexts.cc:302:34: error: format '%hhu' expects argument of type 'int', but argument 3 has type 'v8::internal::VariableMode' [-Werror=format=] Bug: chromium:841460 Change-Id: Id90c1211f459309e477a8ad5658cecdf9cc10938 Reviewed-on: https://chromium-review.googlesource.com/1077051 Reviewed-by: Sigurd Schneider <sigurds@chromium.org> Commit-Queue: Junliang Yan <jyan@ca.ibm.com> Cr-Commit-Position: refs/heads/master@{#53443} [modify] https://crrev.com/38ae84f430d4415cd3c29b18a251aaa6b8640f20/src/contexts.cc
,
May 31 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sebmarchand@chromium.org
, May 9 2018