ARC migration: scoped_nsobject_unittest{,_arc}.mm tests success failure depends on the linking order |
||
Issue descriptionWith https://codereview.chromium.org/2219323002 I introduced an intermediate source_set when linking executable on iOS. This reordered the file on the command-line when running the linker which cause failure in scoped_nsobject_unittest{,_arc}.mm on 32-bit architecture for fat binaries (but could have been any combination, pure luck). [ RUN ] ScopedNSObjectTest.ScopedNSObject ../../base/mac/scoped_nsobject_unittest.mm:37: Failure Value of: [p1 retainCount] Actual: 3 Expected: 2u Which is: 2 [ FAILED ] ScopedNSObjectTest.ScopedNSObject (0 ms) After investigation, it is due to the ARC migration. The call "p3 = p1;" invokes the base::scoped_nsobject<NSObject>::operator =(const base::scoped_nsobject<NSObject>&) operator that ends up calling base::ScopedTypeRef<NSObject>::get() method. Since this is a templated method, the linker only keep one of the multiple instantiations. As the same binary links both scoped_nsobject_unittest{,_arc}.o objects files, there are at least two implementation of that method, one compiled without ARC and one compiled with ARC. Without the intermediate source_set, the non-ARC version is selected, with the source_set, the ARC version is selected. The non-ARC version does not contains any call to retain/release while the ARC version does call objc_retainAutoreleaseAndReturn. Those are functionally equivalent the retain/release calls are balanced, but the ARC version cause the refcount to be increased until the current autorelease pool is released. Putting the "p3 = p1;" in a separate block with an explicit auto-release pool drain fix the test in all configurations.
,
Aug 12 2016
,
Aug 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40865bdc7372f779689422a0c36cf8073d050366 commit 40865bdc7372f779689422a0c36cf8073d050366 Author: sdefresne <sdefresne@chromium.org> Date: Fri Aug 12 21:30:37 2016 Always enable intermediate source_set. The _use_intermediate_source_set variable was there to workaround a bug in scoped_nsobject_unittest{,_arc}.mm that was exhibited by the introduction of an intermediate source_set. Remove the variable and consider that it is always set to "true". BUG= 637065 Review-Url: https://codereview.chromium.org/2245513002 Cr-Commit-Position: refs/heads/master@{#411779} [modify] https://crrev.com/40865bdc7372f779689422a0c36cf8073d050366/build/config/ios/rules.gni
,
Aug 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8bcd27585d5c063b9c19aba05ffe310781181402 commit 8bcd27585d5c063b9c19aba05ffe310781181402 Author: justincohen <justincohen@chromium.org> Date: Sat Aug 13 03:59:36 2016 Revert of Always enable intermediate source_set. (patchset #2 id:20001 of https://codereview.chromium.org/2245513002/ ) Reason for revert: This is breaking earl grey tests downstream. I wonder why upstream eg tests aren't on the cq? Before: cmd LC_ID_DYLIB cmdsize 64 name @rpath/EarlGrey.framework/EarlGrey (offset 24) After: cmd LC_ID_DYLIB cmdsize 72 name obj/ios/third_party/earl_grey/x64/EarlGrey (offset 24) Original issue's description: > Always enable intermediate source_set. > > The _use_intermediate_source_set variable was there to workaround a > bug in scoped_nsobject_unittest{,_arc}.mm that was exhibited by the > introduction of an intermediate source_set. Remove the variable and > consider that it is always set to "true". > > BUG= 637065 > > Committed: https://crrev.com/40865bdc7372f779689422a0c36cf8073d050366 > Cr-Commit-Position: refs/heads/master@{#411779} TBR=dpranke@chromium.org,sdefresne@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 637065 Review-Url: https://codereview.chromium.org/2245903002 Cr-Commit-Position: refs/heads/master@{#411873} [modify] https://crrev.com/8bcd27585d5c063b9c19aba05ffe310781181402/build/config/ios/rules.gni
,
Sep 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c6c0b40fcf2fd87ca23e90f6bd352267b9fd080f commit c6c0b40fcf2fd87ca23e90f6bd352267b9fd080f Author: sdefresne <sdefresne@chromium.org> Date: Tue Sep 06 16:34:44 2016 Fix ios_framework_bundle target when using intermediate source sets. Split the config that add the header map to the include dirs and pass the -install_name flag to the linker as the former is only used while compiling the source file and the latter is only used by the link step. BUG= 637065 Review-Url: https://codereview.chromium.org/2314053002 Cr-Commit-Position: refs/heads/master@{#416639} [modify] https://crrev.com/c6c0b40fcf2fd87ca23e90f6bd352267b9fd080f/build/config/ios/rules.gni
,
Sep 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0cbc7aa6773e0085993968788af9cc4efafc4b81 commit 0cbc7aa6773e0085993968788af9cc4efafc4b81 Author: sdefresne <sdefresne@chromium.org> Date: Wed Sep 07 11:08:43 2016 Enable use of intermediate source set by iOS templates. Enable use of intermediate source set by ios_app_bundle and ios_framework_bundle templates as this should reduce false dependencies. BUG= 637065 Review-Url: https://codereview.chromium.org/2317703002 Cr-Commit-Position: refs/heads/master@{#416906} [modify] https://crrev.com/0cbc7aa6773e0085993968788af9cc4efafc4b81/build/config/ios/rules.gni
,
Sep 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5475013e3ff37f8882f7bf58911b655ce9ab2f04 commit 5475013e3ff37f8882f7bf58911b655ce9ab2f04 Author: sdefresne <sdefresne@chromium.org> Date: Thu Sep 08 16:51:45 2016 Remove _use_intermediate_source_set variable. As the variable is now true (it's end state), it can be removed and the corresponding legacy code. BUG= 637065 Review-Url: https://codereview.chromium.org/2316683002 Cr-Commit-Position: refs/heads/master@{#417316} [modify] https://crrev.com/5475013e3ff37f8882f7bf58911b655ce9ab2f04/build/config/ios/rules.gni |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Aug 12 2016