Skip to content

Commit 8953e49

Browse files
omerktzV8 LUCI CQ
omerktz
authored and
V8 LUCI CQ
committed
[flags] Fix missing flag implications
crrev.com/c/5537448 changed flag implication applications such that it happens once after all flags are set. Unfortunately the timing of applying implications was too late and some implications were already needed before (specifically v8_flags.soft_abort in [1]). We fix this issue by moving the implication application up to before any of the flags are read. V8::Initialize also included some hard-coded implications. To avoid conflicts between these and regular implications, all the hard-coded implications are replaced equivalent regular flag implications. This CL also allows non-bool flags to appear multiple times as long as they have the same value. The variants scripts are also updated to not expect that such cases crash. [1] https://source.chromium.org/chromium/chromium/src/+/main:v8/src/init/v8.cc;drc=a678f03a05eb69165d7c3d7eaea9b4358508eef0;l=242 Change-Id: I4e535cc0c1ce9a9195686d195b15e2b52eb3616f Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5557771 Reviewed-by: Alexander Schulze <[email protected]> Commit-Queue: Omer Katz <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Michael Lippautz <[email protected]> Cr-Commit-Position: refs/heads/main@{#94142}
1 parent 7065698 commit 8953e49

7 files changed

Lines changed: 176 additions & 127 deletions

File tree

‎src/flags/flag-definitions.h‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,23 @@
7777
changed |= TriggerImplication(!v8_flags.whenflag, "!" #whenflag, \
7878
&v8_flags.thenflag, #thenflag, value, false);
7979

80+
#define DEFINE_NEG_VALUE_VALUE_IMPLICATION(whenflag, whenvalue, thenflag, \
81+
thenvalue) \
82+
changed |= \
83+
TriggerImplication(v8_flags.whenflag != whenvalue, #whenflag, \
84+
&v8_flags.thenflag, #thenflag, thenvalue, false);
85+
86+
#define DEFINE_MIN_VALUE_IMPLICATION(flag, min_value) \
87+
changed |= TriggerImplication(v8_flags.flag < min_value, #flag, \
88+
&v8_flags.flag, #flag, min_value, false);
89+
90+
#define DEFINE_DISABLE_FLAG_IMPLICATION(whenflag, thenflag) \
91+
if (v8_flags.whenflag && v8_flags.thenflag) { \
92+
PrintF(stderr, "Warning: disabling flag --" #thenflag \
93+
" due to conflicting flags\n"); \
94+
} \
95+
DEFINE_VALUE_IMPLICATION(whenflag, thenflag, false)
96+
8097
// We apply a generic macro to the flags.
8198
#elif defined(FLAG_MODE_APPLY)
8299

@@ -114,6 +131,18 @@
114131
#ifndef DEFINE_NEG_VALUE_IMPLICATION
115132
#define DEFINE_NEG_VALUE_IMPLICATION(whenflag, thenflag, value)
116133
#endif
134+
#ifndef DEFINE_NEG_VALUE_VALUE_IMPLICATION
135+
#define DEFINE_NEG_VALUE_VALUE_IMPLICATION(whenflag, whenvalue, thenflag, \
136+
thenvalue)
137+
#endif
138+
139+
#ifndef DEFINE_MIN_VALUE_IMPLICATION
140+
#define DEFINE_MIN_VALUE_IMPLICATION(flag, min_value)
141+
#endif
142+
143+
#ifndef DEFINE_DISABLE_FLAG_IMPLICATION
144+
#define DEFINE_DISABLE_FLAG_IMPLICATION(whenflag, thenflag)
145+
#endif
117146

118147
#ifndef DEBUG_BOOL
119148
#error DEBUG_BOOL must be defined at this point.
@@ -1141,6 +1170,46 @@ DEFINE_SLOW_TRACING_BOOL(trace_representation, false,
11411170
DEFINE_BOOL(
11421171
trace_turbo_stack_accesses, false,
11431172
"trace stack load/store counters for optimized code in run-time (x64 only)")
1173+
1174+
// When fuzzing and concurrent compilation is enabled, disable Turbofan
1175+
// tracing flags since reading/printing heap state is not thread-safe and
1176+
// leads to false positives on TSAN bots.
1177+
// TODO(chromium:1205289): Teach relevant fuzzers to not pass TF tracing
1178+
// flags instead, and remove this section.
1179+
DEFINE_BOOL(fuzzing_and_concurrent_recompilation, true,
1180+
"fuzzing && concurrent_recompilation")
1181+
DEFINE_NEG_NEG_IMPLICATION(fuzzing, fuzzing_and_concurrent_recompilation)
1182+
DEFINE_NEG_NEG_IMPLICATION(concurrent_recompilation,
1183+
fuzzing_and_concurrent_recompilation)
1184+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1185+
trace_turbo)
1186+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1187+
trace_turbo_graph)
1188+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1189+
trace_turbo_scheduled)
1190+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1191+
trace_turbo_reduction)
1192+
#ifdef V8_ENABLE_SLOW_TRACING
1193+
// If expensive tracing is disabled via a build flag, the following flags
1194+
// cannot be disabled (because they are already).
1195+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1196+
trace_turbo_trimming)
1197+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1198+
trace_turbo_jt)
1199+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1200+
trace_turbo_ceq)
1201+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1202+
trace_turbo_loop)
1203+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1204+
trace_turbo_alloc)
1205+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1206+
trace_all_uses)
1207+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1208+
trace_representation)
1209+
#endif
1210+
DEFINE_DISABLE_FLAG_IMPLICATION(fuzzing_and_concurrent_recompilation,
1211+
trace_turbo_stack_accesses)
1212+
11441213
DEFINE_BOOL(turbo_verify, DEBUG_BOOL, "verify TurboFan graphs at each phase")
11451214
DEFINE_STRING(turbo_verify_machine_graph, nullptr,
11461215
"verify TurboFan machine graph before instruction selection")
@@ -1416,6 +1485,24 @@ DEFINE_BOOL(wasm_to_js_generic_wrapper, true,
14161485
"allow use of the generic wasm-to-js wrapper instead of "
14171486
"per-signature wrappers")
14181487
DEFINE_BOOL(expose_wasm, true, "expose wasm interface to JavaScript")
1488+
// Do not expose wasm in jitless mode.
1489+
//
1490+
// Even in interpreter-only mode, wasm currently still creates executable
1491+
// memory at runtime. Unexpose wasm until this changes.
1492+
// The correctness fuzzers are a special case: many of their test cases are
1493+
// built by fetching a random property from the the global object, and thus
1494+
// the global object layout must not change between configs. That is why we
1495+
// continue exposing wasm on correctness fuzzers even in jitless mode.
1496+
// TODO(jgruber): Remove this once / if wasm can run without executable
1497+
// memory.
1498+
DEFINE_BOOL(jitless_and_not_correctness_fuzzer_suppressions, true,
1499+
"jitless && !correctness_fuzzer_suppressions")
1500+
DEFINE_NEG_NEG_IMPLICATION(jitless,
1501+
jitless_and_not_correctness_fuzzer_suppressions)
1502+
DEFINE_NEG_IMPLICATION(correctness_fuzzer_suppressions,
1503+
jitless_and_not_correctness_fuzzer_suppressions)
1504+
DEFINE_DISABLE_FLAG_IMPLICATION(jitless_and_not_correctness_fuzzer_suppressions,
1505+
expose_wasm)
14191506
DEFINE_INT(wasm_num_compilation_tasks, 128,
14201507
"maximum number of parallel compilation tasks for wasm")
14211508
DEFINE_VALUE_IMPLICATION(single_threaded, wasm_num_compilation_tasks, 0)
@@ -1719,6 +1806,8 @@ DEFINE_SIZE_T(max_semi_space_size, 0,
17191806
"max size of a semi-space (in MBytes), the new space consists of "
17201807
"two semi-spaces")
17211808
DEFINE_INT(semi_space_growth_factor, 2, "factor by which to grow the new space")
1809+
// Set minimum semi space growth factor
1810+
DEFINE_MIN_VALUE_IMPLICATION(semi_space_growth_factor, 2)
17221811
DEFINE_SIZE_T(max_old_space_size, 0, "max size of the old space (in Mbytes)")
17231812
DEFINE_SIZE_T(
17241813
max_heap_size, 0,
@@ -1927,6 +2016,9 @@ DEFINE_BOOL(stress_compaction, false,
19272016
DEFINE_BOOL(stress_compaction_random, false,
19282017
"Stress GC compaction by selecting random percent of pages as "
19292018
"evacuation candidates. Overrides stress_compaction.")
2019+
DEFINE_IMPLICATION(stress_compaction, force_marking_deque_overflows)
2020+
DEFINE_IMPLICATION(stress_compaction, gc_global)
2021+
DEFINE_VALUE_IMPLICATION(stress_compaction, max_semi_space_size, (size_t)1)
19302022
DEFINE_BOOL(flush_baseline_code, false,
19312023
"flush of baseline code when it has not been executed recently")
19322024
DEFINE_BOOL(flush_bytecode, true,
@@ -2992,6 +3084,13 @@ DEFINE_NEG_IMPLICATION(predictable, parallel_compile_tasks_for_lazy)
29923084
DEFINE_NEG_IMPLICATION(predictable, maglev_deopt_data_on_background)
29933085
DEFINE_NEG_IMPLICATION(predictable, maglev_build_code_on_background)
29943086
#endif // V8_ENABLE_MAGLEV
3087+
// Avoid random seeds in predictable mode.
3088+
DEFINE_BOOL(predictable_and_random_seed_is_0, true,
3089+
"predictable && (random_seed == 0)")
3090+
DEFINE_NEG_NEG_IMPLICATION(predictable, predictable_and_random_seed_is_0)
3091+
DEFINE_NEG_VALUE_VALUE_IMPLICATION(random_seed, 0,
3092+
predictable_and_random_seed_is_0, false)
3093+
DEFINE_VALUE_IMPLICATION(predictable_and_random_seed_is_0, random_seed, 12347)
29953094

29963095
DEFINE_BOOL(predictable_gc_schedule, false,
29973096
"Predictable garbage collection schedule. Fixes heap growing, "
@@ -3060,6 +3159,35 @@ DEFINE_IMPLICATION(verify_predictable, predictable)
30603159
DEFINE_INT(dump_allocations_digest_at_alloc, -1,
30613160
"dump allocations digest each n-th allocation")
30623161

3162+
#define LOG_FLAGS(V) \
3163+
V(log_code) \
3164+
V(log_code_disassemble) \
3165+
V(log_deopt) \
3166+
V(log_feedback_vector) \
3167+
V(log_function_events) \
3168+
V(log_ic) \
3169+
V(log_maps) \
3170+
V(log_source_code) \
3171+
V(log_source_position) \
3172+
V(log_timer_events) \
3173+
V(prof) \
3174+
V(prof_cpp)
3175+
3176+
#define SET_IMPLICATIONS(V) \
3177+
DEFINE_IMPLICATION(log_all, V) \
3178+
DEFINE_IMPLICATION(V, log)
3179+
3180+
LOG_FLAGS(SET_IMPLICATIONS)
3181+
3182+
#undef SET_IMPLICATIONS
3183+
#undef LOG_FLAGS
3184+
3185+
DEFINE_IMPLICATION(log_all, log)
3186+
DEFINE_IMPLICATION(perf_prof, log)
3187+
DEFINE_IMPLICATION(perf_basic_prof, log)
3188+
DEFINE_IMPLICATION(ll_prof, log)
3189+
DEFINE_IMPLICATION(gdbjit, log)
3190+
30633191
// Cleanup...
30643192
#undef FLAG_FULL
30653193
#undef FLAG_READONLY
@@ -3075,10 +3203,14 @@ DEFINE_INT(dump_allocations_digest_at_alloc, -1,
30753203
#undef DEFINE_IMPLICATION
30763204
#undef DEFINE_WEAK_IMPLICATION
30773205
#undef DEFINE_NEG_IMPLICATION
3206+
#undef DEFINE_NEG_IMPLICATION_WITH_WARNING
30783207
#undef DEFINE_WEAK_NEG_IMPLICATION
30793208
#undef DEFINE_NEG_NEG_IMPLICATION
30803209
#undef DEFINE_NEG_VALUE_IMPLICATION
3210+
#undef DEFINE_NEG_VALUE_VALUE_IMPLICATION
30813211
#undef DEFINE_VALUE_IMPLICATION
3212+
#undef DEFINE_MIN_VALUE_IMPLICATION
3213+
#undef DEFINE_DISABLE_FLAG_IMPLICATION
30823214
#undef DEFINE_WEAK_VALUE_IMPLICATION
30833215
#undef DEFINE_GENERIC_IMPLICATION
30843216
#undef DEFINE_ALIAS_BOOL

‎src/flags/flags-impl.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
#ifndef V8_FLAGS_FLAGS_IMPL_H_
66
#define V8_FLAGS_FLAGS_IMPL_H_
77

8+
#include <unordered_set>
9+
810
#include "src/base/macros.h"
911
#include "src/base/optional.h"
1012
#include "src/base/vector.h"
@@ -92,9 +94,12 @@ struct Flag {
9294
#ifdef DEBUG
9395
bool ImpliedBy(const void* ptr) const {
9496
const Flag* current = this->implied_by_ptr_;
97+
std::unordered_set<const Flag*> visited_flags;
9598
while (current != nullptr) {
99+
visited_flags.insert(current);
96100
if (current->PointsTo(ptr)) return true;
97101
current = current->implied_by_ptr_;
102+
if (visited_flags.contains(current)) break;
98103
}
99104
return false;
100105
}

‎src/flags/flags.cc‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -155,13 +155,11 @@ bool Flag::CheckFlagChange(SetBy new_set_by, bool change_flag,
155155
// changes. So specifying the same flag with the same value multiple times
156156
// is allowed.
157157
// For other flags, we disallow specifying them explicitly or in the
158-
// presence of an implication even if the value is the same.
158+
// presence of an implication if the value is not the same.
159159
// This is to simplify the rules describing conflicts in variants.py: A
160-
// repeated non-boolean flag is considered an error independently of its
161-
// value.
160+
// repeated non-boolean flag is considered an error.
162161
bool is_bool_flag = type_ == TYPE_MAYBE_BOOL || type_ == TYPE_BOOL;
163162
bool check_implications = change_flag;
164-
bool check_command_line_flags = change_flag || !is_bool_flag;
165163
switch (set_by_) {
166164
case SetBy::kDefault:
167165
break;
@@ -182,7 +180,7 @@ bool Flag::CheckFlagChange(SetBy new_set_by, bool change_flag,
182180
}
183181
break;
184182
case SetBy::kCommandLine:
185-
if (new_set_by == SetBy::kImplication && check_command_line_flags) {
183+
if (new_set_by == SetBy::kImplication && check_implications) {
186184
// Exit instead of abort for certain testing situations.
187185
if (v8_flags.exit_on_contradictory_flags) base::OS::ExitProcess(0);
188186
if (is_bool_flag) {
@@ -194,8 +192,7 @@ bool Flag::CheckFlagChange(SetBy new_set_by, bool change_flag,
194192
<< FlagName{implied_by}
195193
<< " but also specified explicitly";
196194
}
197-
} else if (new_set_by == SetBy::kCommandLine &&
198-
check_command_line_flags) {
195+
} else if (new_set_by == SetBy::kCommandLine && check_implications) {
199196
// Exit instead of abort for certain testing situations.
200197
if (v8_flags.exit_on_contradictory_flags) base::OS::ExitProcess(0);
201198
if (is_bool_flag) {
@@ -495,7 +492,9 @@ uint32_t ComputeFlagListHash() {
495492
flag.PointsTo(&v8_flags.memory_reducer) ||
496493
flag.PointsTo(&v8_flags.cppheap_concurrent_marking) ||
497494
flag.PointsTo(&v8_flags.cppheap_incremental_marking) ||
498-
flag.PointsTo(&v8_flags.single_threaded_gc)) {
495+
flag.PointsTo(&v8_flags.single_threaded_gc) ||
496+
flag.PointsTo(&v8_flags.fuzzing_and_concurrent_recompilation) ||
497+
flag.PointsTo(&v8_flags.predictable_and_random_seed_is_0)) {
499498
#ifdef DEBUG
500499
if (flag.ImpliedBy(&v8_flags.predictable)) {
501500
flags_ignored_because_of_predictable.insert(flag.name());

‎src/heap/heap.cc‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5113,10 +5113,6 @@ void Heap::ConfigureHeap(const v8::ResourceConstraints& constraints,
51135113
GlobalMemorySizeFromV8Size(min_old_generation_size_);
51145114
}
51155115

5116-
if (v8_flags.semi_space_growth_factor < 2) {
5117-
v8_flags.semi_space_growth_factor = 2;
5118-
}
5119-
51205116
initial_max_old_generation_size_ = max_old_generation_size();
51215117
ResetOldGenerationAndGlobalAllocationLimit();
51225118

0 commit comments

Comments
 (0)