New issue
Advanced search Search tips

Issue 841460 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner: ----
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

Jumbo build breakage in v8

Project Member Reported by sebmarchand@chromium.org, May 9 2018

Issue description

The 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.
 
Cc: brat...@opera.com
+bratell@ FYI 

The problem is because of the CONST entry in this enum : https://cs.chromium.org/chromium/src/v8/src/globals.h?l=937&rcl=9c5f77a1382ee3825f708aba48f5675fb55c84e1 , it conflicts with the definition that we get from windows.h

Not sure of what's the best solution here, renaming the enum value would fix it but seems hacky, this enum in v8 should probably an 'enum class' but this doesn't fix the problem anyway.

Comment 2 by grt@chromium.org, May 10 2018

How about switching to kConstantStyle for the enums (https://google.github.io/styleguide/cppguide.html#Enumerator_Names)?
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.

Comment 4 by brat...@opera.com, May 14 2018

Cc: most...@vewd.com

Comment 5 by most...@vewd.com, May 14 2018

Maybe switch to an enum class if kConst is too common a name?
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.
Cc: hablich@chromium.org
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? 

Comment 8 by most...@vewd.com, 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).
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
Cc: yangguo@chromium.org machenb...@chromium.org
Thanks for the heads-up Seb. I added Yang and machenbach who should be able to better assess the situation.
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).
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 '}'").

Why do you use goma and jumbo together? I thought these are orthogonal features...

Comment 14 by brat...@opera.com, 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.



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?
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.
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.
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?
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!
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
Project Member

Comment 21 by bugdroid1@chromium.org, 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

Project Member

Comment 22 by bugdroid1@chromium.org, 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

Status: Fixed (was: Untriaged)

Sign in to add a comment