Avoiding duplication of singular iterator

Avoiding duplication of singular iterator


BUG=65151
TEST='MRUCacheTest.*'


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89848

[tzik, 2011-06-16 08:55:18]

Hi.

I'm trying to build and run chrome in _GLIBCXX_DEBUG mode.
In this mode, the library disallow us to duplicate invalid iterator, even if we
won't use it.
(I feel it too strict.)

At the line of this source, index_[key] is a uninitialized iterator.
So while we search the key to insert a valid iterator,
a duplication of uninitialize iterator can be done.

This patch is a work around of that.

http://codereview.chromium.org/7187005/diff/1/base/memory/mru_cache.h
File base/memory/mru_cache.h (right):

http://codereview.chromium.org/7187005/diff/1/base/memory/mru_cache.h#newcode94
base/memory/mru_cache.h:94: index_.insert(std::make_pair(key,
ordering_.begin()));
Previous 10 lines guarantee the absence of occupant.

[willchan no longer on Chromium, 2011-06-16 10:24:34]

I don't fully understand. I'm sending my LGTM now because the change is
innocuous and shuts up the error. But for my personal benefit, I'd like to
understand what the warning is about.

index_[key] is not an iterator. It returns a non-const reference to the value
type associated with that key. In the future, can you add the exact error in
this changelists?

On 2011/06/16 08:55:18, tzik wrote:
> Hi.
> 
> I'm trying to build and run chrome in _GLIBCXX_DEBUG mode.
> In this mode, the library disallow us to duplicate invalid iterator, even if
we
> won't use it.
> (I feel it too strict.)
> 
> At the line of this source, index_[key] is a uninitialized iterator.
> So while we search the key to insert a valid iterator,
> a duplication of uninitialize iterator can be done.
> 
> This patch is a work around of that.
> 
> http://codereview.chromium.org/7187005/diff/1/base/memory/mru_cache.h
> File base/memory/mru_cache.h (right):
> 
>
http://codereview.chromium.org/7187005/diff/1/base/memory/mru_cache.h#newcode94
> base/memory/mru_cache.h:94: index_.insert(std::make_pair(key,
> ordering_.begin()));
> Previous 10 lines guarantee the absence of occupant.

[tzik, 2011-06-21 02:34:31]

As detail of the error, I built unit_tests under following configuration,
ran it for 'RenderWidgetHostTest.Resize' and got error message like below.

In current implementation of glibc (quoted in bottom of this message),
map::operator[]() tries to find a key,
 make and inserts a default value (if needed) and returns the value as
reference.
The copy from default value to key-value pair is disallowed in this build mode.

I think it suggest that:
 - Iterator is DefaultConstructible but not fully Assignable,
   so we should not put it in container.
 - Or, iterator is Assignable but not DefaultConstructible,
   and all iterator should be valid anytime.

From former view point, the assertion claims not to store iterator to container.
From latter, not to use operator()[], which requires DefaultConstructible type
as data_type.

----
$ cat ~/.gyp/include.gypi
{
  'target_defaults': {
    'defines+': [
      '_GLIBCXX_DEBUG',
    ],
    'cflags_cc!': [
      '-fno-rtti',
    ],
    'cflags_cc+': [
      '-frtti',
    ],
  },
}

$ ./unit_tests --gtest_filter='RenderWidgetHostTest.Resize'
Note: Google Test filter = RenderWidgetHostTest.Resize
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from RenderWidgetHostTest
[ RUN      ] RenderWidgetHostTest.Resize
/usr/include/c++/4.4/debug/safe_iterator.h:126:error: attempt to copy-
    construct an iterator from a singular iterator.

Objects involved in the operation:
iterator "this" @ 0x0x7fff978f7f28 {
type =
N11__gnu_debug14_Safe_iteratorINSt6__norm14_List_iteratorISt4pairIP16RenderWidgetHostP12BackingStoreEEENSt7__debug4listIS8_SaIS8_EEEEE
(mutable iterator);
  state = singular;
}
iterator "other" @ 0x0x7fff978f7f50 {
type =
N11__gnu_debug14_Safe_iteratorINSt6__norm14_List_iteratorISt4pairIP16RenderWidgetHostP12BackingStoreEEENSt7__debug4listIS8_SaIS8_EEEEE
(mutable iterator);
  state = singular;
}
[27456:27456:0620/164640:272879463713:ERROR:process_util_posix.cc(113)] Received
signal 6
        base::debug::StackTrace::StackTrace() [0x2cd56f6]
        base::(anonymous namespace)::StackDumpSignalHandler() [0x2d3769a]
        0x7fa77b92eaf0
        0x7fa77b92ea75
        0x7fa77b9325c0
        0x7fa77c17d63d
        __gnu_debug::_Safe_iterator<>::_Safe_iterator() [0x46f3a14]
        std::pair<>::pair() [0x46f6a89]
        std::__norm::map<>::operator[]() [0x46f5648]
        base::MRUCacheBase<>::Put() [0x46f3ef6]
        (anonymous namespace)::CreateBackingStore() [0x46f2ebb]
        BackingStoreManager::PrepareBackingStore() [0x46f31bb]
        RenderWidgetHost::PaintBackingStoreRect() [0x475d3a5]
        RenderWidgetHost::OnMsgUpdateRect() [0x475c747]
        RenderWidgetHostTest_Resize_Test::TestBody() [0x118c81e]
        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x26e9629]
        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x26e58e6]
        testing::Test::Run() [0x26d9f70]
        testing::TestInfo::Run() [0x26da794]
        testing::TestCase::Run() [0x26daf21]
        testing::internal::UnitTestImpl::RunAllTests() [0x26dff1f]
        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x26ea8fb]
        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x26e670c]
        testing::UnitTest::Run() [0x26de874]
        base::TestSuite::Run() [0x4a63239]
        main [0x2769226]
        0x7fa77b919c4d
        0x4e9399

[willchan no longer on Chromium, 2011-06-21 10:13:02]

On 2011/06/21 02:34:31, tzik wrote:
> As detail of the error, I built unit_tests under following configuration,
> ran it for 'RenderWidgetHostTest.Resize' and got error message like below.
> 
> In current implementation of glibc (quoted in bottom of this message),
> map::operator[]() tries to find a key,
>  make and inserts a default value (if needed) and returns the value as
> reference.
> The copy from default value to key-value pair is disallowed in this build
mode.
> 
> I think it suggest that:
>  - Iterator is DefaultConstructible but not fully Assignable,
>    so we should not put it in container.
>  - Or, iterator is Assignable but not DefaultConstructible,
>    and all iterator should be valid anytime.
> 
> From former view point, the assertion claims not to store iterator to
container.
> From latter, not to use operator()[], which requires DefaultConstructible type
> as data_type.
> 
> ----
> $ cat ~/.gyp/include.gypi
> {
>   'target_defaults': {
>     'defines+': [
>       '_GLIBCXX_DEBUG',
>     ],
>     'cflags_cc!': [
>       '-fno-rtti',
>     ],
>     'cflags_cc+': [
>       '-frtti',
>     ],
>   },
> }
> 
> $ ./unit_tests --gtest_filter='RenderWidgetHostTest.Resize'
> Note: Google Test filter = RenderWidgetHostTest.Resize
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from RenderWidgetHostTest
> [ RUN      ] RenderWidgetHostTest.Resize
> /usr/include/c++/4.4/debug/safe_iterator.h:126:error: attempt to copy-
>     construct an iterator from a singular iterator.
> 
> Objects involved in the operation:
> iterator "this" @ 0x0x7fff978f7f28 {
> type =
>
N11__gnu_debug14_Safe_iteratorINSt6__norm14_List_iteratorISt4pairIP16RenderWidgetHostP12BackingStoreEEENSt7__debug4listIS8_SaIS8_EEEEE
> (mutable iterator);
>   state = singular;
> }
> iterator "other" @ 0x0x7fff978f7f50 {
> type =
>
N11__gnu_debug14_Safe_iteratorINSt6__norm14_List_iteratorISt4pairIP16RenderWidgetHostP12BackingStoreEEENSt7__debug4listIS8_SaIS8_EEEEE
> (mutable iterator);
>   state = singular;
> }
> [27456:27456:0620/164640:272879463713:ERROR:process_util_posix.cc(113)]
Received
> signal 6
>         base::debug::StackTrace::StackTrace() [0x2cd56f6]
>         base::(anonymous namespace)::StackDumpSignalHandler() [0x2d3769a]
>         0x7fa77b92eaf0
>         0x7fa77b92ea75
>         0x7fa77b9325c0
>         0x7fa77c17d63d
>         __gnu_debug::_Safe_iterator<>::_Safe_iterator() [0x46f3a14]
>         std::pair<>::pair() [0x46f6a89]
>         std::__norm::map<>::operator[]() [0x46f5648]
>         base::MRUCacheBase<>::Put() [0x46f3ef6]
>         (anonymous namespace)::CreateBackingStore() [0x46f2ebb]
>         BackingStoreManager::PrepareBackingStore() [0x46f31bb]
>         RenderWidgetHost::PaintBackingStoreRect() [0x475d3a5]
>         RenderWidgetHost::OnMsgUpdateRect() [0x475c747]
>         RenderWidgetHostTest_Resize_Test::TestBody() [0x118c81e]
>         testing::internal::HandleSehExceptionsInMethodIfSupported<>()
> [0x26e9629]
>         testing::internal::HandleExceptionsInMethodIfSupported<>() [0x26e58e6]
>         testing::Test::Run() [0x26d9f70]
>         testing::TestInfo::Run() [0x26da794]
>         testing::TestCase::Run() [0x26daf21]
>         testing::internal::UnitTestImpl::RunAllTests() [0x26dff1f]
>         testing::internal::HandleSehExceptionsInMethodIfSupported<>()
> [0x26ea8fb]
>         testing::internal::HandleExceptionsInMethodIfSupported<>() [0x26e670c]
>         testing::UnitTest::Run() [0x26de874]
>         base::TestSuite::Run() [0x4a63239]
>         main [0x2769226]
>         0x7fa77b919c4d
>         0x4e9399

I see, ok, this makes sense. Thanks for pasting the error.

[commit-bot: I haz the power, 2011-06-21 17:33:34]

Change committed as 89848