New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 595867 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

DrMemory update process is unreliable

Project Member Reported by brucedaw...@chromium.org, Mar 17 2016

Issue description

When DrMemory is run for the first time, perhaps like this:

valgrind\chrome_tests.bat -t content_browsertests --tool drmemory_light --build-dir=out\Release --gtest_filter=ManifestBrowserTest.DummyManifest

then the DrMemory unpacker is run. If the DrMemory version sha is changed and gclient runhooks is run (to download the new packed version of the tools) then it is reasonable to assume that the unpacker would run again.

It will not. That is, it will not if the command is run in the same command prompt.

chrome_tests.bat only runs the DrMemory unpacker if %DRMEMORY_COMMAND% is not set. When it runs the unpacker it sets DRMEMORY_COMMAND. Since there is no 'setlocal' command in the batch file the DRMEMORY_COMMAND environment variable is propagated to the outer command shell. That means that subsequent invocations of chrome_tests.bat will not unpack new versions, unless the machine has rebooted or a new command prompt is used.

I'm not sure what the intent was. There seems to be no way of detecting a new version so the only safe thing to do is to run the unpack step every time. This is quite quick so this is my recommendation.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38e785b06f3942adad154fddd1b83d8a0f299a5e

commit 38e785b06f3942adad154fddd1b83d8a0f299a5e
Author: Robert Liao <robliao@chromium.org>
Date: Fri Mar 18 01:51:20 2016

Make DrMemory update process reliable

The DrMemory update process only runs when it thinks that it needs to
but its heuristic is not accurate. A better thing to do is to always
update.

This change also adds setlocal so that environment variables don't
leak outside. This may also help with making this script more robust
in the future, once it has been in place for long enough for all
machines affected by the environment variable leakage to have rebooted.

BUG=595867
R=scottmg@chromium.org

Review URL: https://codereview.chromium.org/1807013004 .

Cr-Commit-Position: refs/heads/master@{#381870}

[modify] https://crrev.com/38e785b06f3942adad154fddd1b83d8a0f299a5e/tools/valgrind/chrome_tests.bat

Owner: brucedaw...@chromium.org
Status: Started (was: Untriaged)
I suspect that the original intent was to run the unpack step every time.  I generally use chrome_tests.sh and not the .bat file and it unpacks every time.

Comment 4 by timurrrr@google.com, Mar 21 2016

Right, the original intention was to unpack every time.
I mean, it would be nice to optimize if possible, but IIRC there were many other reasons that made it easier to just unpack every time to have a clean state of a directory with the DrM binary.

The DRMEMORY_COMMAND env however was originally intended for DrM development, as a simple way to use a custom build instead of the auto-unpacked one.
chrome_tests.bat was not unpacking every time due to the lack of the 'setlocal' directive which caused DRMEMORY_COMMAND to leak out to the calling command prompt. That was fixed with crrev.com/1807013004.

Having used DrMemory a few dozen times last week I have to say that I found unpacking every time annoying when running local tests. The ~650 lines of output is distracting enough to interfere with seeing the actual output of DrMemory. I think it is worth considering having some way of easily explicitly suppressing the unpacking to avoid this noise when it isn't needed.

I am not a regular user of DrMemory so I represent the naive user who doesn't have lots of DrMemory experience.

Could we have the top-level DEPS action for DrMemory unpack the archive (first deleting the existing one), eliminating the need to unpack at runtime?
Having the DEPS action unpack the runtime seems to make sense, especially if the downloading only happens when the version changes.

It would be important to make this resistant to failure - that is, if the unpacking is interrupted and "gclient runhooks" is run again then the unpacking should occur even if no new download is required.

The VS toolchain handles this by storing the current toolchain in a directory whose name is the hash that was used to download the toolchain. This allows for efficient switching of toolchains and ensures that old and new toolchains won't be confused.

Owner: timurrrr@chromium.org
Status: Assigned (was: Started)
Assigning to a project owner.
Owner: bruening@chromium.org
I don't work on Dr.Memory anymore

Sign in to add a comment