Ineffective use of debug::Alias() |
|||
Issue descriptionbase::debug::Alias only retains the local variable whose address is passed to it. If that local variable is a pointer then there is no guarantee that the memory which it points to will end up in the crash dump. crbug.com/756544 documents one instance of this. This bug will list more. Other mistakes including passing the address of something that is not a local variable, including the address of a class member, the value of a local variable, or the result of a function call. None of these will have the desired effect on the optimizer. Here are the probably-wrong examples of base::debug::Alias that I found. A separate search for WTF::debug::Alias could be fruitful. Histogram::Factory::CreateRanges base::debug::Alias(&ranges); - only pointer will be retained, and variable goes out of scope immediately after making the effect ephemeral HistogramSnapshotManager::PrepareSamples base::debug::Alias(&ranges_ptr); - only pointer will be retained base::debug::Alias(&histogram_name); - only pointer will be retained ActiveVerifier::StartTracking base::debug::Alias(&creation_stack_); - not a local variable void MemoryAblationExperiment::TouchMemory(size_t offset) { base::debug::Alias(memory_.get()); - no local variable to retain ChromeBrowserStateIOData::~ChromeBrowserStateIOData base::debug::Alias(app_context_cache); - only array of pointers will be retained base::debug::Alias(app_context_vtable_cache); base::debug::Alias(media_context_cache); base::debug::Alias(media_context_vtable_cache); bool StartupBrowserCreator::ProcessCmdLineImpl( base::debug::Alias(&DEBUG_profile_0); - only pointer will be retained base::debug::Alias(&DEBUG_profile_1); - only pointer will be retained base::debug::Alias(&DEBUG_it_begin); - only iterator (probably pointer) will be retained base::debug::Alias(&DEBUG_it_end); - only iterator (probably pointer) will be retained void ResourceDispatcherHostImpl::CompleteTransfer( base::debug::Alias(pending_loader); - pointer ProfileIOData::~ProfileIOData base::debug::Alias(app_context_cache); base::debug::Alias(app_context_vtable_cache); Channel::MessagePtr WaitForBrokerMessage(PlatformHandle platform_handle, BrokerMessageType expected_type) { base::debug::Alias(message.get()); - no local variable to retain int wmain(int argc, wchar_t* argv[]) base::debug::Alias(crashpad_info); - not passing the address of the local, so nothing will be retained, and local is a pointer so retaining it will have minimal value
,
Aug 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/263423ff606d241556e0cadd05c2bfd21f0fbebb commit 263423ff606d241556e0cadd05c2bfd21f0fbebb Author: Bruce Dawson <brucedawson@chromium.org> Date: Fri Aug 18 03:32:26 2017 Document effective use of base::debug::Alias Many uses of Alias(&var) end up being ineffective because var is a pointer and the crash dump mostly only records the contents of the thread stacks - therefore we end up with a pointer but not the memory that it points to. This change documents this issue and gives examples of effective use of base::debug::Alias. BUG= 756544 , 756589 Change-Id: I6095d5018e72c159bbe907c6294d7a48cf28fd82 Reviewed-on: https://chromium-review.googlesource.com/619112 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#495449} [modify] https://crrev.com/263423ff606d241556e0cadd05c2bfd21f0fbebb/base/debug/alias.h [modify] https://crrev.com/263423ff606d241556e0cadd05c2bfd21f0fbebb/third_party/WebKit/Source/platform/wtf/debug/Alias.h
,
Nov 22 2017
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/68ce7e280d14411b79408d5629da3be98beec426 commit 68ce7e280d14411b79408d5629da3be98beec426 Author: Bruce Dawson <brucedawson@chromium.org> Date: Tue Nov 28 02:27:44 2017 Fix use of base::debug::Alias in scoped_handle.cc base::debug::Alias can only reliably trick the compiler into retaining the values of local variables, not class member variables. Therefore it is important to copy class member variables to locals before aliasing them. Bug: 756589 Change-Id: I5c67f386c2a6ad192af74fd9aefbc9103b355774 Reviewed-on: https://chromium-review.googlesource.com/792297 Reviewed-by: Robert Liao <robliao@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#519531} [modify] https://crrev.com/68ce7e280d14411b79408d5629da3be98beec426/base/win/scoped_handle.cc
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/93e1b8dd39fcca3adc4b193d6f4f4e4b23853db6 commit 93e1b8dd39fcca3adc4b193d6f4f4e4b23853db6 Author: Bruce Dawson <brucedawson@chromium.org> Date: Tue Nov 28 21:53:22 2017 Add comments describing unusual use of Alias The base::debug::Alias function is normally used to 'fool' the compiler into retaining an otherwise discardable local variable. In this case base::debug::Alias is used in a completely different way which initially seems wrong, but is actually reasonable. A comment will avoid confusion when auditing uses of Alias. The initial discussion of using base::debug::Alias is here: https://codereview.chromium.org/2810833002#msg7 Bug: 756589 Change-Id: Ib00a1444c217680a69178a8bbf6c710179126e86 Reviewed-on: https://chromium-review.googlesource.com/794298 Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#519860} [modify] https://crrev.com/93e1b8dd39fcca3adc4b193d6f4f4e4b23853db6/chrome/browser/experiments/memory_ablation_experiment.cc
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0961a80cf723e2a1cf4bd6be92246b19b095d3bc commit 0961a80cf723e2a1cf4bd6be92246b19b095d3bc Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Nov 29 02:12:04 2017 Remove incorrect use of base::debug::Alias While investigating 738182 an Alias(op) call was added, but op is a pointer to non-stack memory, not the address of a local, so this has no effect. And 738182 is fixed now anyway. The Alias call was added in crrev.com/c/552837. Bug: 738182, 756589 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Change-Id: I6c4a138927610176cada7835d36f03613dabac9b Reviewed-on: https://chromium-review.googlesource.com/792510 Reviewed-by: vmpstr <vmpstr@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#519963} [modify] https://crrev.com/0961a80cf723e2a1cf4bd6be92246b19b095d3bc/cc/paint/paint_op_buffer.h
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d34c7e5025d4b4521f428ba30690d07b9df82a28 commit d34c7e5025d4b4521f428ba30690d07b9df82a28 Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Nov 29 03:19:31 2017 Remove incorrect uses of base::debug::Alias Calling base::debug::Alias tricks the compiler into retaining the local variable whose address is passed. However if that local variable is a pointer it does not retain the value pointed to. This change removes an incorrect use of base::debug::Alias to avoid confused expectations. Bug: 756589 Change-Id: I9b92ea8aac18fbfaff58d6782c335d1faf703188 Reviewed-on: https://chromium-review.googlesource.com/792007 Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Reviewed-by: Brian White <bcwhite@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Cr-Commit-Position: refs/heads/master@{#519984} [modify] https://crrev.com/d34c7e5025d4b4521f428ba30690d07b9df82a28/base/metrics/histogram_snapshot_manager.cc
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e46e77dc0dc346d81871d71ad7707fbde18eafea commit e46e77dc0dc346d81871d71ad7707fbde18eafea Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Nov 29 05:26:11 2017 Delete ineffective base::debug::Alias call base::debug::Alias is defined to force the compiler to retain local variables by fooling the optimizer into thinking there is a pointer to them stored elsewhere. However passing message.get() to Alias has no effect on the optimizer. It is harmless but misleading. Bug: 756589 Change-Id: Ia10afc5319772c3f6a40bd27c096fe7f37511d98 Reviewed-on: https://chromium-review.googlesource.com/794634 Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#520017} [modify] https://crrev.com/e46e77dc0dc346d81871d71ad7707fbde18eafea/mojo/edk/system/broker_win.cc
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ce53addebe44e8b97f6b077792023de40c9fdb4 commit 2ce53addebe44e8b97f6b077792023de40c9fdb4 Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Nov 29 21:19:09 2017 Remove ineffective use of base::debug::Alias from FontCache Passing the address of pointer variables to Alias has very minimal value because it causes the pointer to be retained but not what it points at. These ineffective uses should be removed to avoid confusion or incorrect expectations. Bug: 756544 , 756589 Change-Id: I77d91a3db5cb41d972d0330ce7034be9bee34a37 Reviewed-on: https://chromium-review.googlesource.com/793896 Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#520246} [modify] https://crrev.com/2ce53addebe44e8b97f6b077792023de40c9fdb4/third_party/WebKit/Source/platform/fonts/FontCache.cpp
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/36a2856c6906fe7ebd3aca67972cc1644ac2ce57 commit 36a2856c6906fe7ebd3aca67972cc1644ac2ce57 Author: Bruce Dawson <brucedawson@chromium.org> Date: Thu Nov 30 21:25:51 2017 Remove non-functional base::debug::Alias call base::debug::Alias is normally used to force the compiler to retain local variables by making it think that some external data structure contains a pointer to them. Passing an arbitrary pointer to Alias has no effect on the optimizer. Removing this use of Alias will help minimize confusion. Bug: 756589 Change-Id: Iab183c96748f555e1b364a0ea714e5b84c0b6829 Reviewed-on: https://chromium-review.googlesource.com/794553 Reviewed-by: Randy Smith <rdsmith@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#520692} [modify] https://crrev.com/36a2856c6906fe7ebd3aca67972cc1644ac2ce57/content/browser/loader/resource_dispatcher_host_impl.cc
,
Nov 30 2017
This is done. I improved the comment block to encourage correct usage and did an audit of all uses of base::debug::Alias. I fixed one set of incorrect uses and removed some uses that had zero value, and a few that had marginal value. I had originally claimed that passing the address of a local pointer to Alias was useless because you would only retain the pointer, not what it points to, but there are some cases where this is sufficient: sometimes it is useful to know the relationship between two pointers, or whether a pointer is valid versus NULL, or if it is a vtable pointer then it may be intrinsically meaningful. There was also one use where Alias was used to force the optimizer to not discard writes to memory that was never subsequently read from - a unique but valid use. More bad uses may creep in but the removal of bad examples and the improving of the documentation should slow this down. |
|||
►
Sign in to add a comment |
|||
Comment 1 by brucedaw...@chromium.org
, Aug 17 2017