Add `collect_validations_metadata` boolean flag to generated_file() targets. The default is false, unless the .gn flag experimental_collect_validations_metadata is set to true. When this flag is true, metadata walks will visit validation dependencies. This may result in runtime crashes due to the data race described in the associated bug. A future CL will ensure that walks that include validations are always performed after all targets have been resolved to fix the issue. Bug: 566346002 Change-Id: I12a8d913aa3895de8ec991b3a491c45e41b0cbfa Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27085 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: David Turner <digit@google.com>
diff --git a/docs/reference.md b/docs/reference.md index 0c7600c..6a03917 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -117,6 +117,7 @@ * [cflags_objcc: [string list] Flags passed to the Objective C++ compiler.](#var_cflags_objcc) * [check_includes: [boolean] Controls whether a target's files are checked.](#var_check_includes) * [check_includes_strict: [boolean] Controls whether strict include checking is enforced.](#var_check_includes_strict) + * [collect_validations_metadata: [bool] Collect metadata values from validations deps.](#var_collect_validations_metadata) * [complete_static_lib: [boolean] Links all deps into a static library.](#var_complete_static_lib) * [configs: [label list] Configs applying to this target or config.](#var_configs) * [contents: Contents to write to file.](#var_contents) @@ -558,6 +559,7 @@ cflags_c [--blame] cflags_cc [--blame] check_includes + collect_validations_metadata configs [--tree] (see below) data_keys defines [--blame] @@ -2162,6 +2164,9 @@ Collected metadata, if specified, will be returned in postorder of dependencies. See the example for details. + + By default, validations dependencies are never visited by metadata collection, + but setting `collect_validations_metadata = true` changes this behavior. ``` #### **Variables** @@ -2174,7 +2179,7 @@ output_extension, output_name, public, sources, testonly, visibility Generated file: contents, data_keys, rebase, walk_keys, output_conversion, - outputs + outputs, collect_validations_metadata ``` #### **Example (metadata collection)** @@ -5784,6 +5789,22 @@ ... } ``` +### <a name="var_collect_validations_metadata"></a>**collect_validations_metadata**: Collect metadata values from validations deps [Back to Top](#gn-reference) + +``` + A boolean flag for generated_file() targets. When true, a metadata walk + will visit validations dependencies (and their transitive dependencies, + including other validations ones) and collect metadata from them, unless + there are explicit barriers to prevent this. + + When false (the default) the metadata walk will ignore validations + deps, to avoid inserting unexpected results in the result. + + The default value can be changed by setting + 'experimental_collect_validations_metadata = true' in the .gn file, + but this feature is temporary and will be removed in the future. See + https://gn.issues.chromium.org/566346002 for details. +``` ### <a name="var_complete_static_lib"></a>**complete_static_lib**: [boolean] Links all deps into a static library. [Back to Top](#gn-reference) ```
diff --git a/misc/vim/syntax/gn.vim b/misc/vim/syntax/gn.vim index 2719560..5ca6a2e 100644 --- a/misc/vim/syntax/gn.vim +++ b/misc/vim/syntax/gn.vim
@@ -45,7 +45,7 @@ syn keyword gnVariable all_dependent_configs allow_circular_includes_from syn keyword gnVariable args arflags asmflags assert_no_deps syn keyword gnVariable cflags cflags_c cflags_cc cflags_objc cflags_objcc -syn keyword gnVariable check_includes complete_static_lib configs +syn keyword gnVariable check_includes collect_validations_metadata complete_static_lib configs syn keyword gnVariable data data_deps data_keys defines depfile deps syn keyword gnVariable framework_dirs frameworks include_dirs inputs ldflags syn keyword gnVariable lib_dirs libs output_extension output_name outputs
diff --git a/src/gn/command_desc.cc b/src/gn/command_desc.cc index fc6fa93..d1e004a 100644 --- a/src/gn/command_desc.cc +++ b/src/gn/command_desc.cc
@@ -305,6 +305,7 @@ {variables::kDataKeys, DefaultHandler}, {variables::kRebase, DefaultHandler}, {variables::kWalkKeys, DefaultHandler}, + {variables::kCollectValidationsMetadata, DefaultHandler}, {variables::kWeakFrameworks, DefaultHandler}, {variables::kWeakLibraries, DefaultHandler}, {variables::kWriteOutputConversion, DefaultHandler}, @@ -407,6 +408,7 @@ HandleProperty(variables::kLibDirs, handler_map, v, dict); HandleProperty(variables::kDataKeys, handler_map, v, dict); HandleProperty(variables::kRebase, handler_map, v, dict); + HandleProperty(variables::kCollectValidationsMetadata, handler_map, v, dict); HandleProperty(variables::kRustflags, handler_map, v, dict); HandleProperty(variables::kWalkKeys, handler_map, v, dict); HandleProperty(variables::kWeakFrameworks, handler_map, v, dict); @@ -514,6 +516,7 @@ cflags_c [--blame] cflags_cc [--blame] check_includes + collect_validations_metadata configs [--tree] (see below) data_keys defines [--blame]
diff --git a/src/gn/desc_builder.cc b/src/gn/desc_builder.cc index a9f46f3..e04af12 100644 --- a/src/gn/desc_builder.cc +++ b/src/gn/desc_builder.cc
@@ -584,6 +584,10 @@ keys.GetList().push_back(base::Value(k)); res->SetKey(variables::kWalkKeys, std::move(keys)); } + if (what(variables::kCollectValidationsMetadata)) { + res->SetKey(variables::kCollectValidationsMetadata, + base::Value(target_->collect_validations_metadata())); + } } if (what(variables::kDeps))
diff --git a/src/gn/functions_target.cc b/src/gn/functions_target.cc index c5e3e2e..d7edcbe 100644 --- a/src/gn/functions_target.cc +++ b/src/gn/functions_target.cc
@@ -987,12 +987,15 @@ Collected metadata, if specified, will be returned in postorder of dependencies. See the example for details. + By default, validations dependencies are never visited by metadata collection, + but setting `collect_validations_metadata = true` changes this behavior. + Variables )" DEPENDENT_CONFIG_VARS DEPS_VARS GENERAL_TARGET_VARS R"( Generated file: contents, data_keys, rebase, walk_keys, output_conversion, - outputs + outputs, collect_validations_metadata Example (metadata collection)
diff --git a/src/gn/generated_file_target_generator.cc b/src/gn/generated_file_target_generator.cc index 0a92122..a034562 100644 --- a/src/gn/generated_file_target_generator.cc +++ b/src/gn/generated_file_target_generator.cc
@@ -36,6 +36,10 @@ if (!FillContents()) return; + + if (!FillCollectValidationsMetadata()) + return; + if (!FillDataKeys()) return; @@ -81,6 +85,30 @@ return true; } +bool GeneratedFileTargetGenerator::FillCollectValidationsMetadata() { + std::string_view variable = variables::kCollectValidationsMetadata; + + bool flag_value = false; + + const Value* value = scope_->GetValue(variable, true); + if (value) { + if (!value->VerifyTypeIs(Value::BOOLEAN, err_)) + return false; + + if (!IsMetadataCollectionTarget(variable, value->origin())) + return false; + + flag_value = value->boolean_value(); + } else if (!contents_defined_) { + flag_value = target_->settings() + ->build_settings() + ->experimental_collect_validations_metadata(); + } + + target_->set_collect_validations_metadata(flag_value); + return true; +} + bool GeneratedFileTargetGenerator::FillOutputConversion() { const Value* value = scope_->GetValue(variables::kWriteOutputConversion, true);
diff --git a/src/gn/generated_file_target_generator.h b/src/gn/generated_file_target_generator.h index c240023..ba5db34 100644 --- a/src/gn/generated_file_target_generator.h +++ b/src/gn/generated_file_target_generator.h
@@ -27,6 +27,7 @@ bool FillGeneratedFileOutput(); bool FillOutputConversion(); bool FillContents(); + bool FillCollectValidationsMetadata(); bool FillDataKeys(); bool FillWalkKeys(); bool FillRebase();
diff --git a/src/gn/target.cc b/src/gn/target.cc index dc0d539..91ecb76 100644 --- a/src/gn/target.cc +++ b/src/gn/target.cc
@@ -1288,12 +1288,14 @@ MetadataWalker(const KeyList& data_keys, const KeyList& walk_keys, const SourceDir& rebase_dir, + bool collect_validations_metadata, std::vector<Value>* result, TargetSet& targets_walked, Err& err) : data_keys_(data_keys), walk_keys_(walk_keys), rebase_dir_(rebase_dir), + collect_validations_metadata_(collect_validations_metadata), result_(result), targets_walked_(targets_walked), err_(err) {} @@ -1339,11 +1341,13 @@ return false; } } - for (const auto& dep : target.validations()) { - // If we haven't walked this dep yet, go down into it. - if (targets_walked_.add(dep.ptr)) { - if (!Walk(*dep.ptr, false)) - return false; + if (collect_validations_metadata_) { + for (const auto& dep : target.validations()) { + // If we haven't walked this dep yet, go down into it. + if (targets_walked_.add(dep.ptr)) { + if (!Walk(*dep.ptr, false)) + return false; + } } } @@ -1379,7 +1383,7 @@ break; } } - if (!found_next) { + if (!found_next && collect_validations_metadata_) { for (const auto& dep : target.validations()) { // Match against the label with the toolchain. if (dep.label.Matches(next_label, toolchain_label)) { @@ -1418,6 +1422,7 @@ const KeyList& data_keys_; const KeyList& walk_keys_; const SourceDir& rebase_dir_; + bool collect_validations_metadata_; std::vector<Value>* result_; TargetSet& targets_walked_; Err& err_; @@ -1432,8 +1437,9 @@ std::vector<Value>* result, TargetSet* targets_walked, Err* err) const { - MetadataWalker walker(data_keys, walk_keys, rebase_dir, result, - *targets_walked, *err); + MetadataWalker walker(data_keys, walk_keys, rebase_dir, + collect_validations_metadata_, result, *targets_walked, + *err); return walker.Walk(*this, deps_only); }
diff --git a/src/gn/target.h b/src/gn/target.h index 4023062..4403b76 100644 --- a/src/gn/target.h +++ b/src/gn/target.h
@@ -472,6 +472,13 @@ user_friendly_location_ = location; } + bool collect_validations_metadata() const { + return collect_validations_metadata_; + } + void set_collect_validations_metadata(bool value) { + collect_validations_metadata_ = value; + } + // Computes and returns the outputs of this target expressed as SourceFiles. // // For binary target this depends on the tool for this target so the toolchain @@ -577,6 +584,7 @@ bool check_includes_ = true; bool check_includes_strict_ = false; bool complete_static_lib_ = false; + bool collect_validations_metadata_ = false; std::vector<std::string> data_; std::unique_ptr<BundleData> bundle_data_; OutputFile write_runtime_deps_output_;
diff --git a/src/gn/target_unittest.cc b/src/gn/target_unittest.cc index 7ed4055..5518761 100644 --- a/src/gn/target_unittest.cc +++ b/src/gn/target_unittest.cc
@@ -1545,10 +1545,10 @@ EXPECT_EQ("input.modulemap.pcm", output[0].value()) << output[0].value(); } -TEST(TargetTest, CollectMetadataWithValidation) { +TEST(TargetTest, CollectMetadataMustIgnoreValidationByDefault) { TestWithScope setup; - TestTarget a(setup, "//foo:a", Target::SOURCE_SET); + TestTarget a(setup, "//foo:a", Target::GENERATED_FILE); Value a_expected(nullptr, Value::LIST); a_expected.list_value().push_back(Value(nullptr, "foo")); a.metadata().contents().emplace("walk", a_expected); @@ -1569,6 +1569,35 @@ &err); EXPECT_SUCCESS(err); + std::vector<Value> expected = {Value(nullptr, "foo")}; + EXPECT_EQ(result, expected); +} + +TEST(TargetTest, CollectValidationsMetadataFlag) { + TestWithScope setup; + + TestTarget a(setup, "//foo:a", Target::GENERATED_FILE); + Value a_expected(nullptr, Value::LIST); + a_expected.list_value().push_back(Value(nullptr, "foo")); + a.metadata().contents().emplace("walk", a_expected); + + TestTarget b(setup, "//foo:b", Target::SOURCE_SET); + Value b_expected(nullptr, Value::LIST); + b_expected.list_value().push_back(Value(nullptr, "bar")); + b.metadata().contents().emplace("walk", b_expected); + + a.validations().push_back(LabelTargetPair(&b)); + a.set_collect_validations_metadata(true); + + std::vector<std::string> data_keys = {"walk"}; + std::vector<std::string> walk_keys; + std::vector<Value> result; + TargetSet targets; + Err err; + a.GetMetadata(data_keys, walk_keys, SourceDir(), false, &result, &targets, + &err); + EXPECT_SUCCESS(err); + std::vector<Value> expected = {Value(nullptr, "bar"), Value(nullptr, "foo")}; EXPECT_EQ(result, expected); }
diff --git a/src/gn/variables.cc b/src/gn/variables.cc index f3cf257..994322d 100644 --- a/src/gn/variables.cc +++ b/src/gn/variables.cc
@@ -948,6 +948,28 @@ } )"; +const char kCollectValidationsMetadata[] = "collect_validations_metadata"; +const char kCollectValidationsMetadata_HelpShort[] = + "collect_validations_metadata: [bool] Collect metadata values from " + "validations deps."; +const char kCollectValidationsMetadata_Help[] = + R"(collect_validations_metadata: Collect metadata values from validations deps + + A boolean flag for generated_file() targets. When true, a metadata walk + will visit validations dependencies (and their transitive dependencies, + including other validations ones) and collect metadata from them, unless + there are explicit barriers to prevent this. + + When false (the default) the metadata walk will ignore validations + deps, to avoid inserting unexpected results in the result. + + The default value can be changed by setting + 'experimental_collect_validations_metadata = true' in the .gn file, + but this feature is temporary and will be removed in the future. See + https://gn.issues.chromium.org/566346002 for details. + +)"; + const char kCompleteStaticLib[] = "complete_static_lib"; const char kCompleteStaticLib_HelpShort[] = "complete_static_lib: [boolean] Links all deps into a static library."; @@ -2604,6 +2626,7 @@ INSERT_VARIABLE(CflagsObjCC) INSERT_VARIABLE(CheckIncludes) INSERT_VARIABLE(CheckIncludesStrict) + INSERT_VARIABLE(CollectValidationsMetadata) INSERT_VARIABLE(CompleteStaticLib) INSERT_VARIABLE(Configs) INSERT_VARIABLE(Data)
diff --git a/src/gn/variables.h b/src/gn/variables.h index 7e39f47..97c21b3 100644 --- a/src/gn/variables.h +++ b/src/gn/variables.h
@@ -170,6 +170,10 @@ extern const char kCheckIncludesStrict_HelpShort[]; extern const char kCheckIncludesStrict_Help[]; +extern const char kCollectValidationsMetadata[]; +extern const char kCollectValidationsMetadata_HelpShort[]; +extern const char kCollectValidationsMetadata_Help[]; + extern const char kCompleteStaticLib[]; extern const char kCompleteStaticLib_HelpShort[]; extern const char kCompleteStaticLib_Help[];