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

Issue 620018 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Jun 2016
Cc:
█ sanjay@google.com
sebmarchand@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

leveldb's InternalFilterPolicy::CreateFilter is infinitely recursive, fails on LTCG with C4717

Project Member Reported by brucedaw...@chromium.org, Jun 14 2016 
When building official builds of some leveldb targets, with gyp or gn, warning C4717 is triggered. gyp repro steps are:

> set GYP_DEFINES=branding=Chrome buildtype=Official target_arch=x64
> python build\gyp_chromium
> ninja -C out\Release_x64 leveldb_corruption_test.exe

third_party\leveldatabase\src\db\dbformat.cc(115) : warning C4717: 'leveldb::InternalFilterPolicy::CreateFilter': recursive on all control paths, function will cause runtime stack overflow

Other affected build targets include:
- env_chromium_unittests.exe
- leveldb_table_test.exe
- leveldb_library.dll
- dump_file_system.exe

gn build settings to trigger this are:
  is_chrome_branded = true
  is_debug = false
  is_official_build = true
  symbol_level = 1
  target_cpu = "x64"

On gyp this only generates a warning, but on gn it is considered a fatal error.

This isn't blocking the official builders so they must not be building these targets.

The warning is assumed to be correct, which suggests that this function must never get called in these targets.

Comment 1 by brucedaw...@chromium.org, Jun 14 2016

Cc: sanjay@google.com
sanjay@ the leveldb::InternalFilterPolicy::CreateFilter function was added by change 85584d4 in April 2012. The Link Time Code Generation build says that it goes infinitely recursive, and it is generally correct about such things. LTCG builds have visibility into all source files simultaneously so they can make broad claims like this.

Any thoughts? I'm going to suppress the warning to unblock me but it seems like the underlying bug needs investigating at some point.

Comment 2 by sanjay@google.com, Jun 14 2016 

I am pretty sure this is a false positive.  InternalFilterPolicy is a
wrapper around another FilterPolicy.  Perhaps the warning is showing up
because Chromium doesn't actually have a real FilterPolicy of its own that
is disovered during LTCG.  But in that case, the filter policy object
passed into leveldb will be null and the wrapper won't be created/used.

Comment 3 by brucedaw...@chromium.org, Jun 14 2016 

Thanks for the analysis. Since we aren't seeing any reports from the field it seems that the code must be unreachable. I'll submit my CL to suppress the warning.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 15 2016 

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

commit d3a998e1c84c2162c79852454ab8944177fccaa7
Author: brucedawson <brucedawson@chromium.org>
Date: Wed Jun 15 19:45:59 2016

Suppress leveldb recursion warning on official builds

VS 2015 issues this warning on official builds of some targets:

warning C4717: 'leveldb::InternalFilterPolicy::CreateFilter': recursive on all control paths

However this not actually a problem. Suppressing the warning allows all
targets to build in official gn builds.

The problem was solved on gyp builds by disabling WarnAsError on
official builds. This fix seems cleaner.

BUG= 620018 

Review-Url: https://codereview.chromium.org/2067003002
Cr-Commit-Position: refs/heads/master@{#399992}

[modify] https://crrev.com/d3a998e1c84c2162c79852454ab8944177fccaa7/third_party/leveldatabase/BUILD.gn

Comment 5 by brucedaw...@chromium.org, Jun 16 2016

Status: Fixed (was: Untriaged)
Consensus seems to be that a code fix is not needed. If we were hitting crashes caused by this then they would be obvious. So, closing as fixed.
►

Sign in to add a comment