|
|
Created:
9 years, 6 months ago by tzik Modified:
9 years, 6 months ago CC:
chromium-reviews, brettw-cc_chromium.org Visibility:
Public. |
DescriptionAvoiding duplication of singular iterator
BUG=65151
TEST='MRUCacheTest.*'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89848
Patch Set 1 #
Total comments: 1
Messages
Total messages: 5 (0 generated)
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.
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.
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
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.
Change committed as 89848 |