Hi parser and wasm folks, could you check it the bugs found are real bugs or false positives?

Parser:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_sanitizer_coverage_rel/builds/10451/steps/Check/logs/AllThePushbacks
https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_sanitizer_coverage_rel/builds/10451/steps/Check/logs/Bookmarks

WASM:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_sanitizer_coverage_rel/builds/10451/steps/Check/logs/instantiate-module-ba..

Hmm, in oder to get the wasm link work you need to copy the second dot

I will take a look at the wasm issue.

I think the WASM issue is about to be nuked by a change I have in flight that ties that object's lifetime to a heap-allocated wrapper.

What I'd like to know: Is the issue real? Was it good that it's found? (because then it's bad that our normal asan/lsan bot didn't find it)

Parser: https://cs.chromium.org/chromium/src/v8/test/cctest/parsing/test-scanner.cc?q=make_scanner&sq=package:chromium&l=20&dr=CSs

I had the clever idea that return a unique_pointer from make_scanner + declaring stuff as auto would declare a unique_pointer and hence make those Scanner objects auto-deleted at the end of the block. Apparently that doesn't work. Not sure if my misunderstanding of C++ (likely), or if there's some compiler options that modify the behaviour in subtle ways.

vogelheim: I don't get why that wouldn't work.

Replacing unique_ptr<Scanner> with auto shouldn't change anything.

Returning a unique_ptr should be just fine, too, and use move semantics. If you somehow mess that up, it should result in a null pointer but not a memory leak.

Drive-by:
  std::unique_ptr<Scanner> scanner(new Scanner(new UnicodeCache()));

Scanner takes a UnicodeCache which is probably never deleted. If Scanner owns it then it should have destructor taking care of it.

Dear world, please ignore #9. Offline discussion revealed a better explanation.

Pls relay it online too, actually I don't get either what's up w/ UnicodeCache.

Also the actual leak report doesn't seem to be online any more, so I.. I guess I have an excuse for guessing now?

Recent link:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_sanitizer_coverage_rel/builds/10944

Please discuss fixing the leaks in the subbug  issue 662388 . This is about figuring out why GN and GYP find different stuff.

#13: #11 reveals the same, better explanation.

I assumed Scanner owns the unicode cache, but it doesn't and hence won't delete it.

That, of course, _is_ _the_ _whole_ _point_ of passing in a UnicodeCache*. We have (in V8; not in the tests) one instance of this, which contains & caches potentially expensive Unicode-related stuff, and all Scanners share that one instance. That's why it needs to be passed in, and that's why Scanner can't delete it.


So the Scanner + UnicodeCache part is fine. But the test needs to be fixed.


-----
"Also the actual leak report doesn't seem to be online any more..." Right, but some of the later links work.

Ah, confused w/ comment numbering. Agree w/ comment 11.

machenbach: Just trying to figure out whether the leaks are real or not (i.e., which version is correct). :) And we concluded the leaks are real.

CC gn and sanitizer folks. Any idea why we might be finding actual memory leaks with our old gyp config and not with GN? I'll work on putting together a simple repro.

An overview of the flag differences can be found here:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_sanitizer_coverage_rel/builds/11018/steps/compare%20build%20flags%20%28fyi%29/logs/stdio

It seems to be independent of the coverage stuff. It also repros when I remove it.

Does the bug reproduce locally? Is there a specific binary for which the leaks are reported only if it's built with GYP but not with GN?

According to the logs this affects all our binaries (in this case d8 and cctest). I'll work on a simple step by step repro.

Here is a simple repro as promised:

# Get V8:
fetch v8
cd v8

# Random revision where problem is present:
git checkout 0dcc7a0e209f10f88735cb51c4ae0ec32ba183ba
gclient sync

# Build with gn:
gn gen out/test --args="is_asan=true is_lsan=true is_debug=false target_cpu=\"x64\" use_goma=true"
ninja -C out/test -j1000 unittests

# Build with gyp:
GYP_CHROMIUM_NO_ACTION=0 GYP_DEFINES="asan=1 lsan=1 target_arch=x64 use_goma=1" gclient runhooks
ninja -C out/Release -j1000 unittests

# Run and compare:
out/Release/unittests --random-seed=254731550 --gtest_filter=ValueSerializerTestWithWasm.DecodeWasmModule --gtest_random_seed=254731550 --gtest_print_time=0 --nohard-abort --nodead-code-elimination --nofold-constants --invoke-weak-callbacks --omit-quit
-> detects leaks

out/test/unittests --random-seed=254731550 --gtest_filter=ValueSerializerTestWithWasm.DecodeWasmModule --gtest_random_seed=254731550 --gtest_print_time=0 --nohard-abort --nodead-code-elimination --nofold-constants --invoke-weak-callbacks --omit-quit
-> no leaks

It's very important that this gets fixed asap!

I just recently found that the GYP bot found a wide range of memory leaks in V8 that the GN bot did not find. Some of these issues are serious memory leaks that are already being shipped.

Please make sure that the GN sanitizer builds work as expected!

Do we have an example leak that is in production and should have been found in Chrome? If this also affects Chromium, then it's not just us setting up the gn lsan build wrong. Then it'd also affect chromium lsan checking.

But I assume we couldn't build chromium with gyp anymore for a showcase anyways :(

Making this available as I won't look at it the next 2-3 weeks.

I have no particular ideas why you'd see leaks w/ gyp but not gn. I'd guess you need to look at differences in the command lines.

It is certainly not possible to do GYP builds w/ chromium at this point, unless you wanted to work off of an older branch to do so.

# I manually analyzed more (bisected through the build and link flags...). Repro:

# Context is repro from comment 21
# Used v8_use_snapshot=false or v8_use_snapshot=0 to make reasoning easier
# Compile with gn, then:
rm out/test/unittests
ninja -C out/test -j1 -v -d keeprsp unittests
# Doesn't find leaks...
# Manually execute shown link command, but remove -Wl,-u_sanitizer_options_link_helper
# -> Now it finds the leaks!

Switching it off doesn't compile because a host executable used during compilation has leaks:
https://codereview.chromium.org/2593613002/

Are those false positives due to not using the option or are those real leaks?

Update after chat with glider:
- Seems like V8 lacks setting detect_leaks=1 in the dynamic asan options when running the tests.
- Since the GN switch, chromium's default asan options are linked into the executables
- Detecting leaks is off by default and needs to be set by the test environment

Furthermore, some host executables seem to be leaky in chromium, but it also doesn't really matter.

Will come up with a fix CL.

CL: https://codereview.chromium.org/2592663004/

The CL now detects a bunch of latent leaks. Will land it once all those are fixed. Findings are in sub  issue 662388 .