Closed
Bug 1404147
Opened 7 years ago
Closed 7 years ago
Address non gfx/ related signed comparison warnings exposed by MinGW build
Categories
(Core :: General, enhancement)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
References
Details
Attachments
(8 files)
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
cpearce
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jld
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
Bug 1404147 Convert tid_t to integer on all platforms to resolve signed/unsigned comparison warnings
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jimm
:
review+
|
Details |
215.22 KB,
text/plain
|
Details |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=679109fd04f72e78b2fbf1e8c0ba0c3d962f865b
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
Tom, can you please attach a file containing the warnings themselves? It's hard to evaluate these patches without knowing where the problematic comparisons are.
Flags: needinfo?(tom)
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8913460 [details] Bug 1404147 Cast constants to the type they are compared to https://reviewboard.mozilla.org/r/184786/#review190280
Attachment #8913460 -
Flags: review?(bugs) → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8913461 [details] Bug 1404147 Resolve inconsistency between GetLastError (DWORD - unsigned) and HRESULT (signed) https://reviewboard.mozilla.org/r/184788/#review190282 Looks like there is similar cast in http://searchfox.org/mozilla-central/rev/f2b181af9497af258d96213f2f0cfccc08740a86/ipc/chromium/src/third_party/libevent/arc4random.c#157
Attachment #8913461 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #31) > Tom, can you please attach a file containing the warnings themselves? It's > hard to evaluate these patches without knowing where the problematic > comparisons are. Kind of. There are a lot of warnings, so I tend to do a lot of grepping and manual copy/pasting to filter them myself. I've attached a doc that contains: > grep "warning: " all-log.txt | grep -v "security/nss" | grep -v "nsprpub/" > warn-no-context.txt The Try run whose build log I used is this one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cf168528a59ba7adcfcb760ac326cc76f58894a&selectedJob=133906172 (I tend to search for things like 'sign-compare]' to find the specific warning instances. Between the two of them you can hopefully narrow things down. The patch I flagged you on specifically were the comparisons in platform.cpp. Going forward I'll try and be more explicit about the individual warnings covered in a patch and put them in the bug description.
Flags: needinfo?(tom)
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8913463 [details] Bug 1404147 Make string index and lengths unsigned to resolve signed/unsigned comparison warning https://reviewboard.mozilla.org/r/184792/#review190334
Attachment #8913463 -
Flags: review?(jmathies) → review+
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8913462 [details] Bug 1404147 Convert tid_t to integer on all platforms to resolve signed/unsigned comparison warnings https://reviewboard.mozilla.org/r/184790/#review190392 Thank you for attaching the warnings, that's helpful. Unfortunately I'm not happy with this patch as is; see below. ::: tools/profiler/core/platform.h:104 (Diff revision 7) > class Thread { > public: > #if defined(GP_OS_windows) > typedef DWORD tid_t; > #else > - typedef ::pid_t tid_t; > + typedef unsigned int tid_t; I think the right way to do this patch is to (a) leave `Thread::tid_t` unchanged, (b) change ThreadInfo::ThreadId()'s return type to Thread::tid_t, and (c) fix up the other type signatures to use `Thread::tid_t` instead of `int`.
Attachment #8913462 -
Flags: review?(n.nethercote) → review-
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8913458 [details] Bug 1404147 Fixed Signed/Unsigned comparison of HRESULT https://reviewboard.mozilla.org/r/184782/#review190494 FYI This file was just rename to dom/media/fake-cdm/cdm-test-output-protection.h.
Attachment #8913458 -
Flags: review?(cpearce) → review+
Comment 38•7 years ago
|
||
mozreview-review |
Comment on attachment 8913452 [details] Bug 1404147 Fixed Unsigned/Signed comparison in WindowsGamepad.cpp https://reviewboard.mozilla.org/r/184776/#review190496 I think Kyle should review this, not me.
Updated•7 years ago
|
Attachment #8913452 -
Flags: review?(cpearce) → review?(kyle)
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8913452 [details] Bug 1404147 Fixed Unsigned/Signed comparison in WindowsGamepad.cpp https://reviewboard.mozilla.org/r/184776/#review190668
Attachment #8913452 -
Flags: review?(kyle) → review+
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8913459 [details] Bug 1404147 Fix signed/unsigned comparison of uints https://reviewboard.mozilla.org/r/184784/#review190674
Attachment #8913459 -
Flags: review?(jld) → review+
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 41•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #36) > Comment on attachment 8913462 [details] > Bug 1404147 Convert from signed -> unsigned in Profiler > > https://reviewboard.mozilla.org/r/184790/#review190392 > > Thank you for attaching the warnings, that's helpful. Unfortunately I'm not > happy with this patch as is; see below. > > ::: tools/profiler/core/platform.h:104 > (Diff revision 7) > > class Thread { > > public: > > #if defined(GP_OS_windows) > > typedef DWORD tid_t; > > #else > > - typedef ::pid_t tid_t; > > + typedef unsigned int tid_t; > > I think the right way to do this patch is to (a) leave `Thread::tid_t` > unchanged, (b) change ThreadInfo::ThreadId()'s return type to Thread::tid_t, > and (c) fix up the other type signatures to use `Thread::tid_t` instead of > `int`. Sure thing. I got pretty far in doing this, but hit a snag: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fb9d7f5f84fbbcca5d52e670a92a3815db0a648 public/GeckoProfiler.h needs to include core/platform.h to get the Thread::tid_t definition, but it appears that the public/ directory can not (or does not) include anything from core/ I'm not sure the the correct way to resolve this is, do you have any thoughts?
Flags: needinfo?(n.nethercote)
Comment 42•7 years ago
|
||
> public/GeckoProfiler.h needs to include core/platform.h to get the > Thread::tid_t definition, but it appears that the public/ directory can not > (or does not) include anything from core/ I'm not sure the the correct way > to resolve this is, do you have any thoughts? Oh yeah, GeckoProfiler.h shouldn't include platform.h. And I'm the process of reducing what's in GeckoProfiler.h for bug 1403868, so I don't want to move `Thread` into it. So now I suggest getting rid of Thread::tid_t and just using `int` everywhere for thread IDs. On Windows we'll need to convert the `unsigned int` the system gives us to `int`, but that shouldn't be a problem. Apologies for leading you to a dead end with the previous suggestion.
Flags: needinfo?(n.nethercote)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 57•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #42) > > public/GeckoProfiler.h needs to include core/platform.h to get the > > Thread::tid_t definition, but it appears that the public/ directory can not > > (or does not) include anything from core/ I'm not sure the the correct way > > to resolve this is, do you have any thoughts? > > Oh yeah, GeckoProfiler.h shouldn't include platform.h. And I'm the process > of reducing what's in GeckoProfiler.h for bug 1403868, so I don't want to > move `Thread` into it. > > So now I suggest getting rid of Thread::tid_t and just using `int` > everywhere for thread IDs. On Windows we'll need to convert the `unsigned > int` the system gives us to `int`, but that shouldn't be a problem. > Apologies for leading you to a dead end with the previous suggestion. Okay, think I got it correct now. MinGw Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e01ec77ac4725d275a249a99fc4998cd6a7e7c86&selectedJob=134672804 Vanilla Debug Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1650b92f15e60b565c3a3f05e8221d100642472
Comment 58•7 years ago
|
||
mozreview-review |
Comment on attachment 8913462 [details] Bug 1404147 Convert tid_t to integer on all platforms to resolve signed/unsigned comparison warnings https://reviewboard.mozilla.org/r/184790/#review191278 Great, thanks! ::: tools/profiler/core/platform-win32.cpp:48 (Diff revision 9) > -/* static */ Thread::tid_t > +/* static */ int > Thread::GetCurrentId() > { > - return GetCurrentThreadId(); > + DWORD threadId = GetCurrentThreadId(); > + MOZ_ASSERT(threadId <= INT32_MAX, "native thread ID is > INT32_MAX"); > + return (int)threadId; I suggest `return int(threadId)`.
Attachment #8913462 -
Flags: review?(n.nethercote) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 60•7 years ago
|
||
It appears that one of them is missing an r+, but if you look at it on mozreview, it is there.
Keywords: checkin-needed
Comment 61•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/720ab0ee359a Fixed Unsigned/Signed comparison in WindowsGamepad.cpp r=qdot https://hg.mozilla.org/integration/autoland/rev/cc50c0ad627c Fixed Signed/Unsigned comparison of HRESULT r=cpearce https://hg.mozilla.org/integration/autoland/rev/f4cb27775f62 Fix signed/unsigned comparison of uints r=jld https://hg.mozilla.org/integration/autoland/rev/c5b45a4eddac Cast constants to the type they are compared to r=smaug https://hg.mozilla.org/integration/autoland/rev/2b8802e0b8af Resolve inconsistency between GetLastError (DWORD - unsigned) and HRESULT (signed) r=smaug https://hg.mozilla.org/integration/autoland/rev/18793ef56f56 Make string index and lengths unsigned to resolve signed/unsigned comparison warning r=jimm https://hg.mozilla.org/integration/autoland/rev/6d4ac28f61bf Convert tid_t to integer on all platforms to resolve signed/unsigned comparison warnings r=njn
Keywords: checkin-needed
Comment 62•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/720ab0ee359a https://hg.mozilla.org/mozilla-central/rev/cc50c0ad627c https://hg.mozilla.org/mozilla-central/rev/f4cb27775f62 https://hg.mozilla.org/mozilla-central/rev/c5b45a4eddac https://hg.mozilla.org/mozilla-central/rev/2b8802e0b8af https://hg.mozilla.org/mozilla-central/rev/18793ef56f56 https://hg.mozilla.org/mozilla-central/rev/6d4ac28f61bf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•