New issue
Advanced search Search tips

Issue 792519 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocking:
issue 787920



Sign in to add a comment

chromium.clang ToTWin failing with error: comparison of integers of different signs

Project Member Reported by h...@chromium.org, Dec 6 2017

Issue description

From the first red build https://ci.chromium.org/buildbot/chromium.clang/ToTWin/582:


FAILED: obj/third_party/crashpad/crashpad/minidump/test_support/minidump_file_writer_test_util.obj 
../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showIncludes  @obj/third_party/crashpad/crashpad/minidump/test_support/minidump_file_writer_test_util.obj.rsp /c ../../third_party/crashpad/crashpad/minidump/test/minidump_file_writer_test_util.cc /Foobj/third_party/crashpad/crashpad/minidump/test_support/minidump_file_writer_test_util.obj /Fd"obj/third_party/crashpad/crashpad/minidump/test_support_cc.pdb"
In file included from ../../third_party/crashpad/crashpad/minidump/test/minidump_file_writer_test_util.cc:17:
../../third_party/googletest/src/googletest/include\gtest/gtest.h(1392,11):  error: comparison of integers of different signs: 'const unsigned long long' and 'const _MINIDUMP_TYPE' [-Werror,-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
../../third_party/googletest/src/googletest/include\gtest/gtest.h(1421,12):  note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<unsigned long long, _MINIDUMP_TYPE>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
../../third_party/crashpad/crashpad/minidump/test/minidump_file_writer_test_util.cc(55,3):  note: in instantiation of function template specialization 'testing::internal::EqHelper<false>::Compare<unsigned long long, _MINIDUMP_TYPE>' requested here
  EXPECT_EQ(header->Flags, MiniDumpNormal);
  ^
../../third_party/googletest/src/googletest/include\gtest/gtest.h(1924,63):  note: expanded from macro 'EXPECT_EQ'
                      EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \
                                                              ^
1 error generated.
 

Comment 1 by h...@chromium.org, Dec 6 2017

Status: Started (was: Assigned)
Suspect:

------------------------------------------------------------------------
r319875 | rsmith | 2017-12-05 19:00:51 -0800 (Tue, 05 Dec 2017) | 5 lines

Fix a bunch of wrong "tautological unsigned enum compare" diagnostics in C++.

An enumeration with a fixed underlying type can have any value in its
underlying type, not just those spanned by the values of its enumerators.

------------------------------------------------------------------------

Comment 2 by h...@chromium.org, Dec 6 2017

Confirmed it's r319875.



$ echo 'enum E { foo }; bool f(unsigned a, E b) { return a == b; }' | bin/clang -Wsign-compare -c -x c++ - -target i686-pc-win32
<stdin>:1:52: warning: comparison of integers of different signs: 'unsigned int' and 'E' [-Wsign-compare]
enum E { foo }; bool f(unsigned a, E b) { return a == b; }
                                                 ~ ^  ~
1 warning generated.

Comment 3 by h...@chromium.org, Dec 6 2017

In this case we probably want to fix crashpad, but there could be more..
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 6 2017

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

commit 39c8f83f7bdd333e8645cb53a5a570a8630abb36
Author: Hans Wennborg <hans@chromium.org>
Date: Wed Dec 06 22:29:44 2017

crashpad: Fix enum vs unsigned -Wsign-compare warnings

Recent Clang versions started taking into account that enums are signed
on Windows when emitting these warnings.

Bug:  792519 
Change-Id: I5029cde81ac6f1777ec2317d431d55589b814465
Reviewed-on: https://chromium-review.googlesource.com/811685
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522226}
[modify] https://crrev.com/39c8f83f7bdd333e8645cb53a5a570a8630abb36/third_party/crashpad/README.chromium
[modify] https://crrev.com/39c8f83f7bdd333e8645cb53a5a570a8630abb36/third_party/crashpad/crashpad/minidump/minidump_file_writer.cc
[modify] https://crrev.com/39c8f83f7bdd333e8645cb53a5a570a8630abb36/third_party/crashpad/crashpad/minidump/test/minidump_file_writer_test_util.cc

Comment 5 by h...@chromium.org, Dec 7 2017

Two more. I don't like the ICU one :-(


FAILED: obj/base/base_unittests/message_formatter_unittest.obj 
../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showIncludes  @obj/base/base_unittests/message_formatter_unittest.obj.rsp /c ../../base/i18n/message_formatter_unittest.cc /Foobj/base/base_unittests/message_formatter_unittest.obj /Fd"obj/base/base_unittests_cc.pdb"
In file included from ../../base/i18n/message_formatter_unittest.cc:16:
In file included from ../..\third_party/icu/source/i18n/unicode/datefmt.h:33:
../../third_party/icu/source/common\unicode/enumset.h(45,65):  error: comparison of integers of different signs: 'UDateFormatBooleanAttribute' and 'unsigned int' [-Werror,-Wsign-compare]
    inline UBool isValidEnum(T toCheck) const {  return (toCheck>=minValue&&toCheck<limitValue); }
                                                         ~~~~~~~^ ~~~~~~~~
../..\third_party/icu/source/i18n/unicode/datefmt.h(48,27):  note: in instantiation of member function 'icu_60::EnumSet<UDateFormatBooleanAttribute, 0, 4>::isValidEnum' requested here
template class U_I18N_API EnumSet<UDateFormatBooleanAttribute,
                          ^
In file included from ../../base/i18n/message_formatter_unittest.cc:16:
In file included from ../..\third_party/icu/source/i18n/unicode/datefmt.h:33:
../../third_party/icu/source/common\unicode/enumset.h(45,84):  error: comparison of integers of different signs: 'UDateFormatBooleanAttribute' and 'unsigned int' [-Werror,-Wsign-compare]
    inline UBool isValidEnum(T toCheck) const {  return (toCheck>=minValue&&toCheck<limitValue); }
                                                                            ~~~~~~~^~~~~~~~~~~
2 errors generated.






../..\base/logging.h(757,26):  error: comparison of integers of different signs: 'const cc::EffectNode::StableIdLabels' and 'const unsigned long long' [-Werror,-Wsign-compare]
DEFINE_CHECK_OP_IMPL(EQ, ==)
~~~~~~~~~~~~~~~~~~~~~~~~~^~~
../..\base/logging.h(746,33):  note: expanded from macro 'DEFINE_CHECK_OP_IMPL'
    if (ANALYZER_ASSUME_TRUE(v1 op v2))                                      \
                             ~~ ^  ~~
../..\base/logging.h(331,36):  note: expanded from macro 'ANALYZER_ASSUME_TRUE'
#define ANALYZER_ASSUME_TRUE(arg) (arg)
                                   ^~~
../../third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp(304,3):  note: in instantiation of function template specialization 'logging::CheckEQImpl<cc::EffectNode::StableIdLabels, unsigned long long>' requested here
  DCHECK_EQ(cc::EffectNode::INVALID_STABLE_ID, mask_isolation.stable_id);
  ^
../..\base/logging.h(932,31):  note: expanded from macro 'DCHECK_EQ'
#define DCHECK_EQ(val1, val2) DCHECK_OP(EQ, ==, val1, val2)
                              ^
../..\base/logging.h(886,18):  note: expanded from macro 'DCHECK_OP'
      ::logging::Check##name##Impl((val1), (val2),                     \
                 ^
<scratch space>(63,1):  note: expanded from here
CheckEQImpl
^
1 error generated.

Comment 6 by h...@chromium.org, Dec 7 2017

Okay, there are lots more. The good news is that, besides the ICU one above, we don't seem to need any third-party changes for this.

Comment 7 by h...@chromium.org, Dec 7 2017

Patch for ICU: https://chromium-review.googlesource.com/#/c/chromium/deps/icu/+/813495

All the Chromium ones: https://chromium-review.googlesource.com/#/c/chromium/src/+/813182

With this I can get through a full debug build.
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/crashpad/crashpad.git/+/914b0d6755282a42ad1cb114e62ab2ce1e321fd4

commit 914b0d6755282a42ad1cb114e62ab2ce1e321fd4
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Dec 07 04:44:47 2017

Fix enum vs unsigned -Wsign-compare warnings

Recent Clang versions started taking into account that enums are signed
on Windows when emitting these warnings.

Bug:  chromium:792519 
Change-Id: I08767fa1f5c8211e663769c7e76b13a1b7146f4f
Reviewed-on: https://chromium-review.googlesource.com/813497
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>

[modify] https://crrev.com/914b0d6755282a42ad1cb114e62ab2ce1e321fd4/minidump/minidump_file_writer.cc
[modify] https://crrev.com/914b0d6755282a42ad1cb114e62ab2ce1e321fd4/minidump/test/minidump_file_writer_test_util.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 7 2017

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

commit 4b99b0f31388e70d24bbdeecfb03df14b601ae04
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Dec 07 17:01:28 2017

Fix enum vs unsigned -Wsign-compare warnings

Recent Clang versions started taking into account that enums are signed
on Windows when emitting these warnings.

NOTRY=true

Bug:  792519 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I0fe2646985282b759e3e261a09fa0f1380c98e61
Reviewed-on: https://chromium-review.googlesource.com/813182
Commit-Queue: Hans Wennborg <hans@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522454}
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/base/trace_event/trace_config_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/base/win/shortcut_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/cc/resources/scoped_resource_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/chrome/browser/media/media_storage_id_salt_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/chrome/browser/sessions/persistent_tab_restore_service_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/chrome/browser/ui/views/status_icons/status_tray_state_changer_interactive_uitest_win.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/chrome/browser/win/chrome_elf_init_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/chrome_elf/blacklist/test/blacklist_test.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/components/cast_channel/cast_auth_util_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/components/subresource_filter/content/browser/content_ruleset_service_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/components/tracing/child/child_trace_message_filter_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/browser/loader/async_resource_handler_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/browser/loader/resource_dispatcher_host_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/browser/renderer_host/input/legacy_input_router_impl_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/child/dwrite_font_proxy/dwrite_font_proxy_win_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/renderer/input/input_event_filter_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/renderer/loader/resource_dispatcher_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/renderer/pepper/plugin_power_saver_helper_browsertest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/renderer/scheduler/resource_dispatch_throttler_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/renderer/service_worker/service_worker_dispatcher_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/content/renderer/service_worker/service_worker_provider_context_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/extensions/browser/renderer_startup_helper_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/extensions/renderer/api_activity_logger_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/media/renderers/video_renderer_impl_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/net/base/prioritized_dispatcher_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/net/nqe/network_quality_estimator_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/net/quic/core/crypto/crypto_server_test.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/third_party/WebKit/Source/core/exported/PrerenderingTest.cpp
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/third_party/WebKit/Source/platform/text/LocaleWinTest.cpp
[modify] https://crrev.com/4b99b0f31388e70d24bbdeecfb03df14b601ae04/ui/display/display_change_notifier_unittest.cc

Comment 10 by h...@chromium.org, Dec 7 2017

Just need to get the ICU patch landed now: https://chromium-review.googlesource.com/#/c/chromium/deps/icu/+/813495
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/icu.git/+/26f7d8ad2f7c6c902825a985146a1d9d68e783cb

commit 26f7d8ad2f7c6c902825a985146a1d9d68e783cb
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Dec 07 19:24:47 2017

Fix enum vs unsigned -Wsign-compare warning

Recent Clang versions started taking into account that enums are signed
on Windows when emitting these warnings.

Bug:  792519 
Change-Id: I32782dc34fc0480942038e4950266448e38d2eec
Reviewed-on: https://chromium-review.googlesource.com/813495
Reviewed-by: Jungshik Shin <jshin@chromium.org>

[modify] https://crrev.com/26f7d8ad2f7c6c902825a985146a1d9d68e783cb/README.chromium
[add] https://crrev.com/26f7d8ad2f7c6c902825a985146a1d9d68e783cb/patches/isvalidenum.patch
[modify] https://crrev.com/26f7d8ad2f7c6c902825a985146a1d9d68e783cb/source/common/unicode/enumset.h

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 7 2017

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

commit cc2e6f63afcdab16c2fe7aaa8611808d9448b4f4
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Dec 07 21:27:38 2017

Roll src/third_party/icu/ 741688ebf..26f7d8ad2 (1 commit)

https://chromium.googlesource.com/chromium/deps/icu.git/+log/741688ebf328..26f7d8ad2f7c

$ git log 741688ebf..26f7d8ad2 --date=short --no-merges --format='%ad %ae %s'
2017-12-07 hans Fix enum vs unsigned -Wsign-compare warning

Created with:
  roll-dep src/third_party/icu

TBR=jshin

Bug:  792519 
Change-Id: Ia77a0d7567c3ee213277ca77686c3051b2ecbba9
Reviewed-on: https://chromium-review.googlesource.com/815017
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522550}
[modify] https://crrev.com/cc2e6f63afcdab16c2fe7aaa8611808d9448b4f4/DEPS

Project Member

Comment 13 by bugdroid1@chromium.org, Dec 8 2017

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

commit 7760d4615a40126445a1fb3b7659fb375ba7f125
Author: Hans Wennborg <hans@chromium.org>
Date: Fri Dec 08 01:41:55 2017

Fix enum vs unsigned -Wsign-compare warnings

Recent Clang versions started taking into account that enums are signed
on Windows when emitting these warnings.

TBR=jochen

Bug:  792519 
Change-Id: If9fdc280b0d6a79105e9191a18214077565dcddb
Reviewed-on: https://chromium-review.googlesource.com/816045
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522673}
[modify] https://crrev.com/7760d4615a40126445a1fb3b7659fb375ba7f125/chrome/installer/util/l10n_string_util_unittest.cc
[modify] https://crrev.com/7760d4615a40126445a1fb3b7659fb375ba7f125/media/formats/mp4/box_reader_unittest.cc

Comment 14 by h...@chromium.org, Dec 12 2017

Status: Fixed (was: Started)
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 12 2017

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

commit f725b25ecda9dd10e3011079f03ad1128f02e190
Author: Hans Wennborg <hans@chromium.org>
Date: Tue Dec 12 19:02:15 2017

Fix enum vs int -Wsign-compare warnings

Recent Clang versions have started to take into account the signedness of enums
for the -Wsign-compare warning.

TBR=lpromero

Bug:  792519 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4e1ab7030fc46690943954229e13246a085136de
Reviewed-on: https://chromium-review.googlesource.com/823091
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523498}
[modify] https://crrev.com/f725b25ecda9dd10e3011079f03ad1128f02e190/ios/chrome/browser/ui/settings/dataplan_usage_collection_view_controller_unittest.mm
[modify] https://crrev.com/f725b25ecda9dd10e3011079f03ad1128f02e190/ios/chrome/browser/ui/settings/time_range_selector_collection_view_controller_unittest.mm

Sign in to add a comment