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

Issue 856536 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Aug 20
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

windows_types.h StrCat macro blocks absl::StrCat adoption in WebRTC

Project Member Reported by jonasolsson@chromium.org, Jun 26 2018

Issue description

Since chrome replaces StrCat with StrCatW in the Windows builds, we get build errors when trying to use absl::StrCat in WebRTC.

The macro is defined here:  https://cs.chromium.org/chromium/src/base/win/windows_types.h?rcl=5361078a2944c3b70c5092bebecbf042fb824fc0&l=251

Here's an example failure:
error: no member named 'StrCatW' in namespace 'absl'
    return absl::StrCat("[", min_, ",", max_, "]");
           ~~~~~~^
../..\base/win/windows_types.h(251,16):  note: expanded from macro 'StrCat'
#define StrCat StrCatW

The macro was added to fix this link failure: https://bugs.chromium.org/p/chromium/issues/detail?id=817738

Could that issue possibly be handled in some other way?
 
Cc: mbonadei@chromium.org kwiberg@chromium.org

Comment 2 by brat...@opera.com, Jun 26 2018

The best would be if absail didn't have anything named StrCat because as long as they have, they, or users of absail, will run into problems like this.

The second "typical" solution is to make sure that the macro is always defined, and that is the reason windows_types.h exists. If you include windows_types.h before including absail headers, then it will always be StrCatW for the compiler and it will be happy. On the other hand it can lead to link errors instead unless all StrCat users do the same.

It's also possible that you can get away with an #undef StrCat at a good place, but it can become a source of future problems and inconsistencies so it wouldn't be my first option. For the same reason I would prefer to not try to find a different solution to  bug 817738 .

(It's ok to curse the 30 year old decision to make all Win32 API functions macros, we all do it)

Cc: danilchap@chromium.org
bratell@ has summarized the issue very well. As the creator of windows_types.h I *hate* the fact that it #defines these names, but pragramatically it was the only solution that I could get working.

We could try removing StrCat from the list of macros that are defined and then see how many #undefs we need, but I'm not very hopeful. You could also try #undefing StrCat just in WebRTC. It is very hard to predict what will work.

I plan to do another push to remove Windows.h from more of Chromium's source code at some point and that might eventually make removing more macros more practical, but until then...

We talked to abseil team about this issue.
They haven't seen it before. 
Counter-suggestion was to undef StrCat everywhere instead of defining it everywhere (it is a deprecated api so shouldn't be used anyway).
That might be not the smallest, but seems cleanest long-term approach.

I've made a draft CL that does it (https://chromium-review.googlesource.com/c/chromium/src/+/1117180)
with an implicit suggestion to avoid including windows headers directly.

windows compile try bots seems fine. Does that mean it can work?


(I also have a plan B hack to allow absl::StrCat just in webrtc.
https://webrtc-review.googlesource.com/c/src/+/85680
but though webrtc is 1st user of abseil in chromium, it is not the only one, so it might be better to move towards solution that works for all chromium libraries)
Project Member

Comment 6 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

Status: Fixed (was: Unconfirmed)

Sign in to add a comment