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

Issue 817738 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Windows jumbo link failure in chrome/browser related to StrCat/StrCatW

Project Member Reported by grt@chromium.org, Mar 1 2018

Issue description

chrome/browser/webshare/share_service_impl.cc uses base::StrCat, and it looks like Shlwapi.h is #defining StrCat to StrCatW:

browser.lib(browser_jumbo_111.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl base::StrCatW(class base::span<class base::BasicStringPiece<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > > const >)" (__imp_?StrCatW@base@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V?$span@$$CBV?$BasicStringPiece@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@base@@@1@@Z) referenced in function "private: static bool __cdecl ShareServiceImpl::ReplacePlaceholders(class base::BasicStringPiece<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >,class base::BasicStringPiece<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >,class base::BasicStringPiece<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >,class GURL const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > *)" (?ReplacePlaceholders@ShareServiceImpl@@CA_NV?$BasicStringPiece@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@base@@00ABVGURL@@PAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z)
./chrome.dll : fatal error LNK1120: 1 unresolved externals

I'm not sure why the fyi builder isn't choking on this. Maybe it's specific to is_chrome_branded = true?

Thanks for taking a look.
 

Comment 1 by brat...@opera.com, Mar 1 2018

Cc: brucedaw...@chromium.org
Summary: Windows jumbo link failure in chrome/browser related to StrCat/StrCatW (was: Jumbo link failure in chrome/browser)
Ah, thanks for debugging it. I also don't know why the fyi builder is fine. I can't reproduce it but I can do some stabs in the dark.

(i.e. #undef StrCat after including shlwapi.h which seems to be included twice in chrome/browser)

Comment 2 by brat...@opera.com, Mar 1 2018

grt, are you able to check if https://chromium-review.googlesource.com/c/chromium/src/+/942822 helps you?

Comment 3 by grt@chromium.org, Mar 1 2018

Hmm, now link.exe keeps crashing. Lemme try a clean build.

Comment 4 by grt@chromium.org, Mar 1 2018

Nope, I'm getting the same error.

Comment 5 by brat...@opera.com, Mar 1 2018

I tried looking for any shlwapi.h includes in header files, but didn't see any, and then there were only the two includes above in chrome/browser.

Do you have source that I can't see, and can you check that for something similar?

I'm still doing test compiles (takes a while) and will return when/if I have anything more.


Comment 6 by grt@chromium.org, Mar 1 2018

Is there an easy way to see what sources are getting pulled into building (in my case) browser_jumbo_111.obj?

Comment 7 by brat...@opera.com, Mar 1 2018

The wrapper that picks the sources are in out/something/gen/chrome/browser/browser_jumbo_111.cc. Judging from the high number, you have a goma build and it's only 8 files (at most) per jumbo chunk.

When I debug those, I usually create a copy of that jumbo file and experiment with.

Comment 8 by brat...@opera.com, Mar 2 2018

Cc: j...@opera.com

Comment 9 by brat...@opera.com, Mar 2 2018

Cc: brettw@chromium.org
There are other (the inverse) problems with StrCat as well, where the base version is suddenly named StrCatW and the code looking for it looks for StrCat.

@brettw, since StrCat is still new (landed 2.5 months ago), and it collides with a Windows macro from a header that is included in many files, I would like to rename it. What do you think about StrConcat? StrJoin? StrMerge?


I hate to suggest it, but the way that we have dealt with this in the past (initially accidentally, but more recently intentionally, if with deep misgivings) is to ensure that these macros are defined everywhere.

That's because the alternative of making sure that they are defined nowhere is impractical, and consistency is the key. So, a likely fix is to add the define to base\win\windows_types.h, along with the other 23 tragic #defines.

Renaming the function would also work.

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 6 2018

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

commit 9edce6da807276a2b077bef9881fb181321df6fe
Author: Daniel Bratell <bratell@opera.com>
Date: Tue Mar 06 16:43:48 2018

Resolve conflict between base::StrCat and Win32 StrCat

StrCat is a Win32 API function and it uses a macro to redirect
StrCat calls to StrCatW. That macro will be available when
shlwapi.h has been included but not otherwise so mentions of
StrCat will sometimes refer to StrCat, and sometimes to StrCatW.

This patch makes it always be StrCatW as in many other functions.

Alternatives are to rename base::StrCat or undef StrCat whenever
shlwapi.h is included.

Bug:  817738 
Change-Id: I6fca88d7514867ef15e63c8498ddac6a5e9f43dc
Reviewed-on: https://chromium-review.googlesource.com/950923
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#541122}
[modify] https://crrev.com/9edce6da807276a2b077bef9881fb181321df6fe/base/strings/strcat.h
[modify] https://crrev.com/9edce6da807276a2b077bef9881fb181321df6fe/base/win/windows_types.h

Comment 12 by brat...@opera.com, Mar 6 2018

Status: Fixed (was: Assigned)
grt, I think this will fix your problem. Please let me know if it doesn't.

Comment 13 by grt@chromium.org, Mar 6 2018

👍 Thanks!
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 11

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

commit 3518f36e766dfe35c347cd21f7530c5e8545e01b
Author: Danil Chapovalov <danilchap@chromium.org>
Date: Sat Aug 11 16:13:43 2018

Ensure StrCat means StrCat

undefine StrCat from StrCatW after including windows headers that redefine it
Add preprocessor checks StrCat is not defined to something else.
Add presubmit check headers that include shlwapi.h that redefines StrCat are wrapped

Bug:  chromium:856536 ,  chromium:817738 
Change-Id: Ief9303cbf6fa4f55f671861e49fb1fc747ed59aa
Reviewed-on: https://chromium-review.googlesource.com/1117180
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Danil Chapovalov <danilchap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582449}
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/PRESUBMIT.py
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/PRESUBMIT_test.py
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/BUILD.gn
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/files/file_enumerator_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/strings/strcat.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/test/test_file_util_win.cc
[add] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/atl.h
[add] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/propvarutil.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/registry.cc
[add] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/shlwapi.h
[add] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/sphelper.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/win_util.cc
[add] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/windows_defines.inc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/windows_types.h
[add] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/base/win/windows_undefines.inc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/app/main_dll_loader_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/conflicts/uninstall_application_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/google/google_update_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/google/google_update_win_unittest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/importer/ie_importer_browsertest_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/shell_integration_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/speech/tts_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/ui/pdf/adobe_reader_info_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/browser/win/automation_controller.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/chrome_cleaner/http/error_utils.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/installer/util/copy_tree_work_item.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/installer/util/create_reg_key_work_item.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/installer/util/delete_reg_key_work_item.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/installer/util/google_update_settings_unittest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/installer/util/install_util.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/installer/util/l10n_string_util.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/chrome/installer/util/move_tree_work_item.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/components/policy/core/common/policy_loader_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/components/update_client/background_downloader_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/content/browser/accessibility/browser_accessibility_com_win.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/content/browser/accessibility/browser_accessibility_win.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/content/browser/accessibility/cross_platform_accessibility_browsertest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/content/browser/renderer_host/legacy_render_widget_host_win.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/remoting/host/pairing_registry_delegate_win_unittest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/remoting/host/win/chromoting_module.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/remoting/host/win/rdp_client_unittest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/remoting/host/win/rdp_client_window.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/remoting/host/win/rdp_desktop_session.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/rlz/test/rlz_test_helpers.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/sandbox/win/sandbox_poc/main_ui_window.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/sandbox/win/src/restricted_token_unittest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/sandbox/win/src/sid_unittest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/sandbox/win/tests/validation_tests/suite.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/services/device/generic_sensor/platform_sensor_and_provider_unittest_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/tools/win/CreateTempFilesPerfEvaluation/CreateTempFilesPerfEval.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/tools/win/ShowGlobals/ShowGlobals.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/ui/accessibility/platform/ax_platform_node_win.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/ui/accessibility/platform/ax_platform_node_win_unittest.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/ui/accessibility/platform/ax_platform_relation_win.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/ui/base/clipboard/clipboard_util_win.cc
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/ui/base/ime/win/tsf_event_router.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/ui/base/win/accessibility_misc_utils.h
[modify] https://crrev.com/3518f36e766dfe35c347cd21f7530c5e8545e01b/ui/base/win/atl_module.h

Sign in to add a comment