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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(8 files)

      No description provided.
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 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 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+
Attached file warn-no-context.txt
(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 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 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 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 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.
Attachment #8913452 - Flags: review?(cpearce) → review?(kyle)
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 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+
Status: NEW → ASSIGNED
(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)
> 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)
(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 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+
It appears that one of them is missing an r+, but if you look at it on mozreview, it is there.
Keywords: checkin-needed
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
Depends on: 1406492
You need to log in before you can comment on or make changes to this bug.