Skip to content

Commit

Permalink
[BRP] Take a step towards enforcing raw_ptr/ref in Renderer code
Browse files Browse the repository at this point in the history
We intend to exclude only a handful specific sub-directories (for perf
reasons). This CL removes the broad exclusions from the rewriter and
the enforcing clang plugin.

There is a risk that by the time the plugin rolls into Chromium,
someone will add a raw pointer, which will cause the plugin
to error out and block the roll-out. Therefore, add those broad
exclusions into BUILD.gn temporarily -- they'll be removed after
the plugin rolls, once we confirm that all pointers in those
directories are properly rewritten.

In other words, this CL doesn't change the clang plugin enforcement
behavior.

Bug: 40064499, 331846123
Change-Id: Ia95e8df846d167521a5ac43cc63e556002702ee9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5600329
Auto-Submit: Bartek Nowierski <[email protected]>
Reviewed-by: Takuto Ikuta <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Bartek Nowierski <[email protected]>
Reviewed-by: Keishi Hattori <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1312344}
  • Loading branch information
bartekn-chromium authored and Chromium LUCI CQ committed Jun 8, 2024
1 parent 43802cc commit 49b1a45
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 41 deletions.
16 changes: 13 additions & 3 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -7172,11 +7172,21 @@ def _IsMiraclePtrDisallowed(input_api, affected_file):
if not _IsCPlusPlusFile(input_api, path):
return False

# Renderer code is generally allowed to use MiraclePtr.
# These directories, however, are specifically disallowed.
# Renderer-only code is generally allowed to use MiraclePtr. These
# directories, however, are specifically disallowed, for perf reasons.
if ("third_party/blink/renderer/core/" in path
or "third_party/blink/renderer/platform/heap/" in path
or "third_party/blink/renderer/platform/wtf/" in path):
or "third_party/blink/renderer/platform/wtf/" in path
or "third_party/blink/renderer/platform/fonts/" in path):
return True

# The below paths are an explicitly listed subset of Renderer-only code,
# because the plan is to Oilpanize it.
# TODO(crbug.com/330759291): Remove once Oilpanization is completed or
# abandoned.
if ("third_party/blink/renderer/core/paint/" in path
or "third_party/blink/renderer/platform/graphics/compositing/" in path
or "third_party/blink/renderer/platform/graphics/paint/" in path):
return True

# We assume that everything else may be used outside of Renderer processes.
Expand Down
12 changes: 12 additions & 0 deletions PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -4841,6 +4841,18 @@ def testDisallowedCases(self):
MockAffectedFile(
'test3/third_party/blink/renderer/platform/wtf/foo.cc',
['raw_ptr<int>']),
MockAffectedFile(
'test4/third_party/blink/renderer/platform/fonts/foo.h',
['raw_ptr<int>']),
MockAffectedFile(
'test5/third_party/blink/renderer/core/paint/foo.cc',
['raw_ptr<int>']),
MockAffectedFile(
'test6/third_party/blink/renderer/platform/graphics/compositing/foo.h',
['raw_ptr<int>']),
MockAffectedFile(
'test7/third_party/blink/renderer/platform/graphics/paint/foo.cc',
['raw_ptr<int>']),
]
mock_output_api = MockOutputApi()
errors = PRESUBMIT.CheckRawPtrUsage(mock_input_api, mock_output_api)
Expand Down
12 changes: 12 additions & 0 deletions build/config/clang/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ config("find_bad_constructs") {
"-Xclang",
"disable-check-raw-ptr-to-stack-allocated-error",

# TODO(https://crbug.com/40064499): Remove when clang plugin rolls with
# updated RawPtrManualPathsToIgnore.h and we confirm that these
# directories are fully rewritten.
"-Xclang",
"-plugin-arg-find-bad-constructs",
"-Xclang",
"raw-ptr-exclude-path=/renderer/",
"-Xclang",
"-plugin-arg-find-bad-constructs",
"-Xclang",
"raw-ptr-exclude-path=third_party/blink/public/web/",

# TODO(crbug.com/40944547): Remove when raw_ptr check has been
# enabled for the dawn repo.
"-Xclang",
Expand Down
28 changes: 9 additions & 19 deletions tools/clang/plugins/RawPtrManualPathsToIgnore.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,29 +70,19 @@ constexpr const char* const kRawPtrManualPathsToIgnore[] = {
// Exclude dir that should hold C headers.
"mojo/public/c/",

// Exclude code that only runs inside a renderer process - renderer
// processes are excluded for now from the MiraclePtr project scope,
// because they are sensitive to performance regressions (to a much higher
// degree than, say, the Browser process).
// Renderer-only code is generally allowed to use MiraclePtr. These
// directories, however, are specifically disallowed, for perf reasons.
//
// Note that some renderer-only directories are already excluded
// elsewhere - for example "v8/" is excluded in another part of this
// file.
//
// The common/ directories must be included in the rewrite as they contain
// code
// that is also used from the browser process.
// elsewhere - for example "v8/" is excluded, because it's in another
// repository.
//
// Also, note that isInThirdPartyLocation AST matcher in
// RewriteRawPtrFields.cpp explicitly includes third_party/blink
// (because it is in the same git repository as the rest of Chromium),
// but we go ahead and exclude most of it below (as Renderer-only code).
"/renderer/", // (e.g. //content/renderer/ or
// //components/visitedlink/renderer/
// or //third_party/blink/renderer)",
"third_party/blink/public/web/", // TODO: Consider renaming this directory
// to",
// public/renderer?",
// RewriteRawPtrFields.cpp explicitly allows third_party/blink
"third_party/blink/renderer/core/",
"third_party/blink/renderer/platform/heap/",
"third_party/blink/renderer/platform/wtf/",
"third_party/blink/renderer/platform/fonts/",
// The below paths are an explicitly listed subset of Renderer-only code,
// because the plan is to Oilpanize it.
// TODO(crbug.com/330759291): Remove once Oilpanization is completed or
Expand Down
29 changes: 10 additions & 19 deletions tools/clang/raw_ptr_plugin/RawPtrManualPathsToIgnore.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,29 +70,20 @@ constexpr const char* const kRawPtrManualPathsToIgnore[] = {
// Exclude dir that should hold C headers.
"mojo/public/c/",

// Exclude code that only runs inside a renderer process - renderer
// processes are excluded for now from the MiraclePtr project scope,
// because they are sensitive to performance regressions (to a much higher
// degree than, say, the Browser process).
// Renderer-only code is generally allowed to use MiraclePtr. These
// directories, however, are specifically disallowed, for perf reasons.
//
// Note that some renderer-only directories are already excluded
// elsewhere - for example "v8/" is excluded in another part of this
// file.
//
// The common/ directories must be included in the rewrite as they contain
// code
// that is also used from the browser process.
// elsewhere - for example "v8/" is excluded, because it's in another
// repository.
//
// Also, note that isInThirdPartyLocation AST matcher in
// RewriteRawPtrFields.cpp explicitly includes third_party/blink
// (because it is in the same git repository as the rest of Chromium),
// but we go ahead and exclude most of it below (as Renderer-only code).
"/renderer/", // (e.g. //content/renderer/ or
// //components/visitedlink/renderer/
// or //third_party/blink/renderer)",
"third_party/blink/public/web/", // TODO: Consider renaming this directory
// to",
// public/renderer?",
// RewriteRawPtrFields.cpp explicitly allows third_party/blink
"third_party/blink/renderer/core/",
"third_party/blink/renderer/platform/heap/",
"third_party/blink/renderer/platform/wtf/",
"third_party/blink/renderer/platform/fonts/",

// The below paths are an explicitly listed subset of Renderer-only code,
// because the plan is to Oilpanize it.
// TODO(crbug.com/330759291): Remove once Oilpanization is completed or
Expand Down

0 comments on commit 49b1a45

Please sign in to comment.