Skip to content

Commit

Permalink
Fix GN thread sanitizer bug.
Browse files Browse the repository at this point in the history
Tsan reported that Scope::GetValueWithScope was marking a value as "used"
on the same Scope from multiple threads. These sets were all to true so we
haven't noticed any stability problems, but we should not be trying to
write to scopes across threads.

What happens is that a template is created by BUILDCONFIG or a .gni file and
called from multiple BUILD.gn files in multiple threads. The scope stored by
the template was not marked const, so the derived scopes on each thread were
referencing this root closure as mutable and marking variables used in it.

This solution is to mark the Scope const. The derived scopes will reference
the parent scope as a const and no changes to the shared scope will occur.

Change-Id: I0a5026bb2f32b476babab63f479a5d377df410e8
Reviewed-on: https://chromium-review.googlesource.com/890230
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533408}
  • Loading branch information
Brett Wilson authored and Commit Bot committed Jan 31, 2018
1 parent c9c268a commit 9a22cda
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion tools/gn/template.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,15 @@ class Template : public base::RefCountedThreadSafe<Template> {
Template();
~Template();

std::unique_ptr<Scope> closure_;
// It's important that this Scope is const. A template can be referenced by
// the root BUILDCONFIG file and then duplicated to all threads. Therefore,
// this scope must be usable from multiple threads at the same time.
//
// When executing a template, a new scope will be created as a child of this
// one, which will reference it as mutable or not according to the mutability
// of this value.
std::unique_ptr<const Scope> closure_;

const FunctionCallNode* definition_;
};

Expand Down

0 comments on commit 9a22cda

Please sign in to comment.