Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1836093004:
Fix base dependency in views_examples_with_content_exe (Closed)
|
Created:
4 years, 8 months ago by Will Harris Modified:
4 years, 8 months ago Reviewers:
sadrul CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix base dependency in views_examples_with_content_exe
This was causing a link error as base/allocator was linked twice
when building in static_library using VS2015 allocator shim.
https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile/logs/stdio
BUG=481611, 599223
Committed: https://crrev.com/14b98d2bde6a9cb8cd4fc770be8d182ed84eef7c
Cr-Commit-Position: refs/heads/master@{#384338}
Patch Set 1 #Patch Set 2 : content content #Patch Set 3 : fix the fix #
Total comments: 3
Messages
Total messages: 34 (13 generated)
Description was changed from ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as allocator was linked twice. BUG=481611 ========== to ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as allocator was linked twice when using VS2015 allocator shim https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611 ==========
wfh@chromium.org changed reviewers: + sadrul@chromium.org
PTAL
The CQ bit was checked by wfh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836093004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836093004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The build failure on linux_chromium_chromeos_compile_dbg_ng looks relevant. Mind taking a look? (also, do you need to update GN too?)
On 2016/03/30 14:39:29, sadrul wrote: > The build failure on linux_chromium_chromeos_compile_dbg_ng looks relevant. Mind > taking a look? (also, do you need to update GN too?) Yup saw that last night and was going to fix this morning. :)
The CQ bit was checked by wfh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836093004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836093004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as allocator was linked twice when using VS2015 allocator shim https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611 ========== to ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as allocator was linked twice when using VS2015 allocator shim https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611,599223 ==========
Description was changed from ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as allocator was linked twice when using VS2015 allocator shim https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611,599223 ========== to ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as base/allocator was linked twice when building in static_library using VS2015 allocator shim. https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611,599223 ==========
The CQ bit was checked by wfh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836093004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836093004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looks good now, but I'll do a full testing run with the vs2015 allocator shim in place before committing. PTAL.
lgtm
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836093004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836093004/40001
Message was sent while issue was closed.
Description was changed from ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as base/allocator was linked twice when building in static_library using VS2015 allocator shim. https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611,599223 ========== to ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as base/allocator was linked twice when building in static_library using VS2015 allocator shim. https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611,599223 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as base/allocator was linked twice when building in static_library using VS2015 allocator shim. https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611,599223 ========== to ========== Fix base dependency in views_examples_with_content_exe This was causing a link error as base/allocator was linked twice when building in static_library using VS2015 allocator shim. https://build.chromium.org/p/chromium/builders/Win/builds/41816/steps/compile... BUG=481611,599223 Committed: https://crrev.com/14b98d2bde6a9cb8cd4fc770be8d182ed84eef7c Cr-Commit-Position: refs/heads/master@{#384338} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/14b98d2bde6a9cb8cd4fc770be8d182ed84eef7c Cr-Commit-Position: refs/heads/master@{#384338}
Message was sent while issue was closed.
https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... File ui/views/examples/examples.gyp (right): https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... ui/views/examples/examples.gyp:174: '../../../base/base.gyp:base', Are there other places where this is being doing like this? Sorry, but this looks like a hack. Why should we link to base only in the shared build when the code already depends on it no matter what?
Message was sent while issue was closed.
+bruce our linker expert, what do you think - see also the bug which has all the errors https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... File ui/views/examples/examples.gyp (right): https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... ui/views/examples/examples.gyp:174: '../../../base/base.gyp:base', On 2016/03/31 18:46:14, tfarina wrote: > Are there other places where this is being doing like this? Sorry, but this > looks like a hack. Why should we link to base only in the shared build when the > code already depends on it no matter what? I agree it's quite mysterious, it doesn't happen in GN because GN passes individual obj files to the link stage rather than libs, and it doesn't happen at the almost-identical configuration "views_examples_exe" and "views_examples_lib" above. The problem comes when two copies of the allocator end up linking into the same executable. This happened once elsewhere in gles2_conform_support.gyp linking via a direct and indirect dependency. I think it has to do with link order.
Message was sent while issue was closed.
I'll take a look. I do hate unexplained fixes. On the other hand, it's getting more difficult to care about perfection in gyp...
Message was sent while issue was closed.
https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... File ui/views/examples/examples.gyp (right): https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... ui/views/examples/examples.gyp:174: '../../../base/base.gyp:base', On 2016/03/31 19:24:44, Will Harris wrote: > On 2016/03/31 18:46:14, tfarina wrote: > > Are there other places where this is being doing like this? Sorry, but this > > looks like a hack. Why should we link to base only in the shared build when > the > > code already depends on it no matter what? > > I agree it's quite mysterious, it doesn't happen in GN because GN passes > individual obj files to the link stage rather than libs, and it doesn't happen > at the almost-identical configuration "views_examples_exe" and > "views_examples_lib" above. > I think gn does that only if the "library" is a source_set. > The problem comes when two copies of the allocator end up linking into the same > executable. This seems to indicate that maybe the way the allocator is currently setup is not correctly. Otherwise this would have been happening for everything else. What is special about the allocator? I remember seeing some changes from Primiano to make it part of base. Maybe it has something to do with this? Last time I looked into this (with you) I saw we build tcmalloc from base and the gyp file is a mess and very complex. Not sure if you remember this discussion.
Message was sent while issue was closed.
On 2016/03/31 19:53:43, tfarina wrote: > https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... > File ui/views/examples/examples.gyp (right): > > https://codereview.chromium.org/1836093004/diff/40001/ui/views/examples/examp... > ui/views/examples/examples.gyp:174: '../../../base/base.gyp:base', > On 2016/03/31 19:24:44, Will Harris wrote: > > On 2016/03/31 18:46:14, tfarina wrote: > > > Are there other places where this is being doing like this? Sorry, but this > > > looks like a hack. Why should we link to base only in the shared build when > > the > > > code already depends on it no matter what? > > > > I agree it's quite mysterious, it doesn't happen in GN because GN passes > > individual obj files to the link stage rather than libs, and it doesn't happen > > at the almost-identical configuration "views_examples_exe" and > > "views_examples_lib" above. > > > I think gn does that only if the "library" is a source_set. > > > The problem comes when two copies of the allocator end up linking into the > same > > executable. > This seems to indicate that maybe the way the allocator is currently setup is > not correctly. Otherwise this would have been happening for everything else. > What is special about the allocator? the new VS2015 allocator shims override the built in functions in the ucrt rather than stripping them out of the crt (which we were previously doing) This works as long as there is only one instance of the overridden function definition, but fails if there are two because... *calls brucedawson*. > > I remember seeing some changes from Primiano to make it part of base. Maybe it > has something to do with this? Yes allocator is now a part of base but should be its own lib, that's what I see in the linked input anyway (base_allocator.lib) Perhaps it is accidentally also being linked into base.lib but I doubt it... > > Last time I looked into this (with you) I saw we build tcmalloc from base and > the gyp file is a mess and very complex. Not sure if you remember this > discussion. this issue only appears on windows so shouldn't be tcmalloc related. The CL where I am enabling the VS2015 shim is https://codereview.chromium.org/1414453017/ and was blocked on this fix, if you want to take a look at it.
Message was sent while issue was closed.
I understand why this CL works to enable the allocator shim CL. A quick explanation before I go to a meeting. Without this CL the link order (from the .rsp file) starts as: obj\ui\views\examples\views_examples_with_content_exe.examples_with_content_main_exe.obj obj\base\base.lib That initial .obj file generates no references to calloc/malloc/free/etc. so when base.lib is examined there is no reason to pull in allocator_shim_win.obj. As the other libraries are scanned there are, naturally, some references to calloc/malloc/free/etc. found. Those requests are stored away. Eventually the scanning gets to libucrt.lib, which satisfies those references. Then the linker starts again from the beginning of the list of libs (this is how most linkers work) to satisfy any remaining symbols. It finds a reference to _malloc_unchecked, which is satisfied by allocator_shim_win.obj, and now we have a conflict. This CL avoids the problem purely by moving base.lib later in the list - it becomes the fifth .lib file scanned, at which point lots of requests for calloc/malloc/free have accumulated, and it only takes one. So, the root cause is that base.lib is processed too early - before any allocation functions have been requested. Possible solutions: - Make sure that the exe's main .obj file references malloc or free - adding free(malloc(10)) does the trick - Move base.lib later in the list - last would be ideal, since libucrt.lib is (I think) implicitly right after the explicitly listed .libs. - Strip the problematic functions from libucrt.lib - Link allocator_shim_win.obj directly into all EXE and DLL files (all PE files) - Probably some other options
Message was sent while issue was closed.
Thanks for the detailed analysis, Bruce! 1. Since the VS2015 allocator shim is contained in one obj file that will either be successfully linked with the target, or clash with the ucrt symbols, I feel confident (especially given the extra runtime check calling IsAllocatorInitialized from ContentMainRunner::Initialize) that, assuming no link errors, the allocator shim will be present and working. 2. This, and gles2_conform_support.exe, seem to be the only two instances in the codebase where the link order and lack of calls to allocation routines cause this failure to happen, so it is rare. 3. The failures do not happen using GN, because of the way that GN uses source_set and specific .obj files (which mean the shim is always in place). Given GN is coming soon, gyp will be dead soon(!), and that the issue is well understood and documented in this CL, my request is that we leave this fix in place. If so, would you like me to leave a comment in the gyp file to this effect? WDYT? |
