heap profiler: hard fail if heap profiling is not supported in the current build config (component builds etc) |
||
Issue descriptionForking this from the discussion in crrev.com/2757123002 The heap profiler won't work in some build combinations, for instance: - Debug OR component builds on windows: because the shim is supported only in release+static - Perhaps component builds on other OS: depending on how the stack unwinder decides that a pointer is a valid return address. the fact that we don't support heap profiling in some cases is fine. IMHO our goal should be make sure that it works in production builds first. However, we should give clear signals (read: CHECK, LOG(FATAL) or similar) to developers when they are using a build config where the heap profiler won't work. Otherwise, it will be frustrating and infuriating for developers if: - they try to use heap profiling - that gives somehow broken results - they file a bug - our reply is "ah yeah, we know about that, just use non-component builds" Now, copy-pasting from the CL above: ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ primiano: But if it doesn't (or is just too painful to maintain) I am very happy if we just give a clear signal saying "nope, static build only". wez: That would equally apply to the pseudo-stack case, above, since its a question of whether we have working allocator shims, I believe. primiano: Ah, I was thinking more to the "can this unwinder cope with multiple .so(s)". but you make another good point. wez: Rather than an explicit check for component build here, I'd prefer to test a macro indicating whether or not we have working allocator shims, and have that set alongside the shimming code (or in the GN if necessary) appropriately to the build type. ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ ✂ wez@ temporarily assigning to you since you are touching code around there in crrev.com/2757123002 . Feel free to re-triage.
,
May 24 2017
,
May 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59c3060ac761de2221eea6febe7be5d223f0266a commit 59c3060ac761de2221eea6febe7be5d223f0266a Author: primiano <primiano@chromium.org> Date: Wed May 24 23:18:48 2017 memory-infra: LOG(FATAL) on unsupported heap profiler configs Adds a fatal error when trying to use the heap profiler in a configuration where it cannot work (e.g., Win debug or component builds, or anything that doesn't have the shim) BUG= 705461 Review-Url: https://codereview.chromium.org/2903733003 Cr-Commit-Position: refs/heads/master@{#474468} [modify] https://crrev.com/59c3060ac761de2221eea6febe7be5d223f0266a/base/trace_event/memory_dump_manager.cc
,
May 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e568d4d0e6a2039808505a21ea37c8cecbc32fe3 commit e568d4d0e6a2039808505a21ea37c8cecbc32fe3 Author: wez <wez@chromium.org> Date: Thu May 25 00:51:23 2017 Revert of memory-infra: LOG(FATAL) on unsupported heap profiler configs (patchset #3 id:40001 of https://codereview.chromium.org/2903733003/ ) Reason for revert: As noted in my comment on Patch Set #3, this disables heap-profiling on Windows, even in Static builds. Original issue's description: > memory-infra: LOG(FATAL) on unsupported heap profiler configs > > Adds a fatal error when trying to use the heap profiler in a > configuration where it cannot work (e.g., Win debug or component > builds, or anything that doesn't have the shim) > > BUG= 705461 > > Review-Url: https://codereview.chromium.org/2903733003 > Cr-Commit-Position: refs/heads/master@{#474468} > Committed: https://chromium.googlesource.com/chromium/src/+/59c3060ac761de2221eea6febe7be5d223f0266a TBR=erikchen@chromium.org,primiano@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 705461 Review-Url: https://codereview.chromium.org/2902343002 Cr-Commit-Position: refs/heads/master@{#474501} [modify] https://crrev.com/e568d4d0e6a2039808505a21ea37c8cecbc32fe3/base/trace_event/memory_dump_manager.cc
,
May 25 2017
From discussion w/ primiano: LOG(ERROR) from MDM when the flag is unsupported occurs too early to actually be displayed or logged, so in practice it's not very helpful.
Since it is possible to just ignore the flag and run normally, we think a butter-bar would be the best way to signal when --enable-heap-profiling is set in a build which cannot support it, akin to UX which warns when flags are used which impair stability/security (e.g. --no-sandbox).
This requires:
1. A [constexpr] helper function IsHeapProfilingSupported() in MemoryDumpManager, to indicate whether the Chrome binary actually supports the feature (i.e. allocator shims work, we can capture stack traces, etc).
2. Extend the ShowBadFlagsPrompt() with a new message, shown when a flag is specified on the command-line that an is-available function indicates cannot be supported (something like "The flag is not available in this build.").
For example:
static std::pair<const char*, bool> kUnsupportedFlags[] = {
{ switches::kenableHeapProfiling, IsHeapProfilingSupported() }
};
for (auto flag_pair : kUnsupportedFlags) {
if (cmd_line->HasSwitch(flag_pair.first) && !flag_pair.second) {
.. show a lovely alert ...
}
}
Note that this UI would not generally be seen by users, since this is primarily a developer-oriented flag.
,
May 25 2017
plan #5 LGTM, wez can you give a go to that?
,
May 25 2017
Re #6: Yup, will send you a patch.
,
Jun 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a990af190304be5bf38b120799c594df5a293518 commit a990af190304be5bf38b120799c594df5a293518 Author: Wez <wez@chromium.org> Date: Sun Jun 18 23:28:02 2017 Show a banner if an unimplemented command-line flag is used. Some command-line flags (e.g. --enable-heap-profiling) are only implemented in specific build configurations (e.g. Release+Static), and in all other binaries we want to warn the user that the flag will be ignored, if it is specified. This CL adds a generic "not implemented in this build" banner message and a special-case to show that message if an --enable heap-profiling option is specified that is not valid in the current build. Bug: 705461 Change-Id: Ie41be966ecf17935c52590826901b15cea7171f8 Reviewed-on: https://chromium-review.googlesource.com/517690 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Cr-Commit-Position: refs/heads/master@{#480321} [modify] https://crrev.com/a990af190304be5bf38b120799c594df5a293518/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/a990af190304be5bf38b120799c594df5a293518/base/trace_event/memory_dump_manager.h [modify] https://crrev.com/a990af190304be5bf38b120799c594df5a293518/chrome/app/generated_resources.grd [modify] https://crrev.com/a990af190304be5bf38b120799c594df5a293518/chrome/browser/ui/startup/bad_flags_prompt.cc
,
Jun 19 2017
,
Jun 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b8f6e49e9026fac27d7e07f873c31af61dd57b3 commit 9b8f6e49e9026fac27d7e07f873c31af61dd57b3 Author: Wez <wez@chromium.org> Date: Tue Jun 20 23:33:25 2017 Fix handling of heap-profiling=task-profiler mode. The patch which added the butter-bar for unimplemented modes broke the task-profiler case for the heap-profiling flag. Bug: 705461 Change-Id: Ib293ce8aa8d4c2f763c7b9ba414923381f2d9870 Reviewed-on: https://chromium-review.googlesource.com/540983 Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#481023} [modify] https://crrev.com/9b8f6e49e9026fac27d7e07f873c31af61dd57b3/base/trace_event/memory_dump_manager.cc |
||
►
Sign in to add a comment |
||
Comment 1 by w...@chromium.org
, Apr 14 2017