Albatross needs V8 DCHECK implementation to observe a feature flag |
|||
Issue descriptionAfter a bit of discussion, it looks like the best way forward is to add an API. This API may either set the lethality of DCHECKs, or delegate to the embedder to decide same. The DEBUG build flag also sets the default state of several V8 flags, some of which enable extra validation code, which in turn generally CHECKs unconditionally. These flags also need to be modulated somehow by the embedders feature flag.
,
Sep 7 2017
I had a chat with Jakob, who warned me that there's a fair amount of code conditionally under #if(def) DEBUG and this code typically assumes DCHECK===CHECK. On a quick search, I find some of that (example Scope::ResolveTo), but it shouldn't be a big deal to clean up by moving to DCHECK consistently. There are also cases of extra checks that are enabled by #if(def) DEBUG, but I still think we're going to get 99% there by diverting DCHECKs and setting the DEBUG_BOOL-initialized flags.
,
Sep 11 2017
,
Sep 13 2017
So far I have https://chromium-review.googlesource.com/c/v8/v8/+/661054 to allow making DCHECK non-fatal, and https://chromium-review.googlesource.com/c/v8/v8/+/665351 to turn CHECKs inside #ifdef DEBUG into DCHECKs. What remains is the flags (and the Chrome-side work): C:\src\v8\v8>git grep DEBUG_BOOL src\flag-definitions.h src/flag-definitions.h:DEFINE_BOOL(turbo_verify, DEBUG_BOOL, "verify TurboFan graphs at each phase") src/flag-definitions.h:DEFINE_BOOL(verify_csa, DEBUG_BOOL, src/flag-definitions.h:DEFINE_BOOL(turbo_verify_allocation, DEBUG_BOOL, src/flag-definitions.h:DEFINE_BOOL(debug_code, DEBUG_BOOL, One way to deal with the flags is simply to stomp them from Chrome with V8::SetFlagsFromString. This is probably the most expedient, though it ties the embedder to the implementation detail of V8's debug config. WDYT?
,
Sep 13 2017
I think that the cleanest solution would be to have a build system defined symbol that can factor into the #definition of DEBUG_BOOL; but lacking that, SetFlagsFromString is an acceptable workaround. I think it's preferable over introducing yet another API function to turn off those flags.
,
Sep 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e commit 70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e Author: Sigurdur Asgeirsson <siggi@chromium.org> Date: Thu Sep 14 15:08:27 2017 Convert CHECK under #if(def) DEBUG to DCHECK. Bug: chromium:763010 Change-Id: Iafed5a0e8087f415cd2c11a0b1326c04bd01ef80 Reviewed-on: https://chromium-review.googlesource.com/665351 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Reviewed-by: Andreas Haas <ahaas@chromium.org> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org> Cr-Commit-Position: refs/heads/master@{#48018} [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/arm64/simulator-arm64.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/ast/scopes.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/compiler/jump-threading.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/compiler/node.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/compiler/simplified-lowering.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/compiler/state-values-utils.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/compiler/verifier.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/d8.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/debug/debug-evaluate.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/deoptimizer.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/handles-inl.h [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/heap/heap.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/heap/spaces.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/interpreter/bytecode-array-builder.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/mips/assembler-mips.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/mips/macro-assembler-mips.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/mips64/assembler-mips64.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/mips64/macro-assembler-mips64.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/objects-debug.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/objects-inl.h [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/runtime/runtime-object.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/snapshot/deserializer.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/wasm/wasm-interpreter.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/wasm/wasm-objects.cc [modify] https://crrev.com/70372dfc64dd1d24dd0c388cfc87d7ad0bb9248e/src/x64/macro-assembler-x64.cc
,
Sep 14 2017
Looks like RenderProcessImpl::RenderProcessImpl is where flag-stomping code would go: https://cs.chromium.org/chromium/src/content/renderer/render_process_impl.cc?dr=C&l=104
,
Sep 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/a787c3f9e12d8f392f56d7116dbf1fac09b12123 commit a787c3f9e12d8f392f56d7116dbf1fac09b12123 Author: Sigurdur Asgeirsson <siggi@chromium.org> Date: Fri Sep 15 11:48:16 2017 Allow overriding DCHECK handling and make it non-fatal. Bug: chromium:763010 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I7d479f8abb16ffd7ffc19d3a6b58da01f5feddd0 Reviewed-on: https://chromium-review.googlesource.com/661054 Reviewed-by: Camillo Bruni <cbruni@chromium.org> Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org> Cr-Commit-Position: refs/heads/master@{#48038} [modify] https://crrev.com/a787c3f9e12d8f392f56d7116dbf1fac09b12123/include/v8.h [modify] https://crrev.com/a787c3f9e12d8f392f56d7116dbf1fac09b12123/src/api.cc [modify] https://crrev.com/a787c3f9e12d8f392f56d7116dbf1fac09b12123/src/base/logging.cc [modify] https://crrev.com/a787c3f9e12d8f392f56d7116dbf1fac09b12123/src/base/logging.h [modify] https://crrev.com/a787c3f9e12d8f392f56d7116dbf1fac09b12123/test/cctest/test-api.cc [modify] https://crrev.com/a787c3f9e12d8f392f56d7116dbf1fac09b12123/test/unittests/base/logging-unittest.cc
,
Sep 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad25d87127f7f5288f9ac5b186a77266998ab8f1 commit ad25d87127f7f5288f9ac5b186a77266998ab8f1 Author: Sigurdur Asgeirsson <siggi@chromium.org> Date: Wed Sep 20 19:10:29 2017 Give kSyzyAsanDCheckIsFatalFeature unique linkage. This is exploding in CheckFeatureIdentity on second use of the constexpr, as presumably the Feature object is instantiated per call. Bug: 763010 Change-Id: I330690fcf052a66ed3287f3a3134866025803a01 Reviewed-on: https://chromium-review.googlesource.com/674096 Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#503224} [modify] https://crrev.com/ad25d87127f7f5288f9ac5b186a77266998ab8f1/base/feature_list.cc [modify] https://crrev.com/ad25d87127f7f5288f9ac5b186a77266998ab8f1/base/feature_list.h
,
Sep 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/379c51e423f16cb23822cb9d24020f7b31152a42 commit 379c51e423f16cb23822cb9d24020f7b31152a42 Author: Sigurdur Asgeirsson <siggi@chromium.org> Date: Thu Sep 21 12:52:45 2017 Make V8s DCHECK observe the IsFatal feature flag in SyzyASAN builds. Bug: 763010 Change-Id: I379e5cc8ecd7de45b6c8dbea2aa5b3216dc4550d Reviewed-on: https://chromium-review.googlesource.com/673267 Reviewed-by: Wez <wez@chromium.org> Reviewed-by: Dimitri Glazkov <dglazkov@chromium.org> Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org> Cr-Commit-Position: refs/heads/master@{#503427} [modify] https://crrev.com/379c51e423f16cb23822cb9d24020f7b31152a42/content/renderer/render_process_impl.cc
,
Sep 22 2017
This is all done, as far as I can see. |
|||
►
Sign in to add a comment |
|||
Comment 1 by siggi@chromium.org
, Sep 7 2017