1047176 - Fix warnings as errors build failures introduced by DeadlockDetector changes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

Somehow the DeadlockDetector changes in bug 1027921 are causing WAE build failures on win32 in certverifier. The errors seem to be coming from switching to nsClassHashtable:

We can see WAE is enabled:
    ...\dist\include\nsHashKeys.h(289) : error C2220: warning treated as error - no 'object' file generated

=============================================
This triggers an error:
    ...\dist\include\nsHashKeys.h(289) : warning C4365: 'return' : conversion from 'int32_t' to 'PLDHashNumber', signed/unsigned mismatch

=============================================

As well as this:
    ...\dist\include\nsTHashtable.h(390) : warning C4640: 'sOps' : construction of local static object is not thread-safe

        ...\dist\include\nsTHashtable.h(383) : while compiling class template member function 'void nsTHashtable<EntryType>::Init(uint32_t)'

        with

        [

            EntryType=nsBaseHashtableET<nsPtrHashKey<const mozilla::BlockingResourceBase::DeadlockDetectorEntry>,nsAutoPtr<mozilla::DeadlockDetector<mozilla::BlockingResourceBase::DeadlockDetectorEntry>::OrderingEntry>>

        ]

=============================================

As well as this:
        ...\dist\include\mozilla/HashFunctions.h(268) : see reference to function template instantiation 'uint32_t mozilla::AddToHash<T>(uint32_t,A)' being compiled

        with

        [

            T=char,

            A=char

        ]

        ...\dist\include\mozilla/HashFunctions.h(295) : see reference to function template instantiation 'uint32_t mozilla::detail::HashUntilZero<char>(const T *)' being compiled

        with

        [

            T=char

        ]
    ...\build\obj-firefox\dist\include\mozilla/HashFunctions.h(166) : warning C4365: 'argument' : conversion from 'char' to 'uint32_t', signed/unsigned mismatch

=============================================

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\dist\include\mozilla/HashFunctions.h(279) : see reference to function template instantiation 'uint32_t mozilla::AddToHash<T>(uint32_t,A)' being compiled

        with

        [

            T=char,

            A=char

        ]

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\dist\include\mozilla/HashFunctions.h(301) : see reference to function template instantiation 'uint32_t mozilla::detail::HashKnownLength<char>(const T *,size_t)' being compiled

        with

        [

            T=char

        ]

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\dist\include\mozilla/HashFunctions.h(166) : warning C4365: 'argument' : conversion from 'char' to 'uint32_t', signed/unsigned mismatch

Comment 1

[Eric Rahm [:erahm]]

Plan of attack:

  * warning C4640: 'sOps' : construction of local static object
    Just disable it in the certverifier moz.build, there's no way to "fix" it.

  * warning C4365: 'return' : conversion from 'int32_t' to 'PLDHashNumber', signed/unsigned mismatch
    Fix by using PTR_TO_UINT32

  * warning C4365: 'argument' : conversion from 'char' to 'uint32_t', signed/unsigned mismatch
    Fix by casting |const char*| to |const unsigned char*| in both locations

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 1: Disable warning C4640 in certverifier

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 2: Use NS_PTR_TO_UINT32 in nsISupportsHashKey

This patch swithches nsISupportsHashKey over to using NS_PTR_TO_UINT32 to satisfy the signed-unsigned warning.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Part 3: Cast to unsigned char in HashString

This patch casts the char* param in HashString to an unsigned char* to satisfy the signed-unsigned warning.

Comment 5

[Eric Rahm [:erahm]]

Note patches 2 & 3 would be unnecessary if we also disable warning C4365 in patch 1.

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8466341 [details] [diff] [review]
Part 1: Disable warning C4640 in certverifier

David, please see comment 5. If that's ok I'll ditch the other two patches and update this one.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8466343 [details] [diff] [review]
Part 2: Use NS_PTR_TO_UINT32 in nsISupportsHashKey

Review of attachment 8466343 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsHashKeys.h
@@ +285,5 @@
>  
>    static KeyTypePointer KeyToPointer(KeyType aKey) { return aKey; }
>    static PLDHashNumber HashKey(KeyTypePointer aKey)
>    {
> +    return NS_PTR_TO_UINT32(aKey) >> 2;

PLDHashNumber is a uint32_t if I understand correctly, so this should be done anyway.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8466341 [details] [diff] [review]
Part 1: Disable warning C4640 in certverifier

Review of attachment 8466341 [details] [diff] [review]:
-----------------------------------------------------------------

I would prefer to disable as few warnings as possible. I think what's going on here is CertVerifier.cpp is including something in security/manager/ssl/src that includes something from somewhere else with stricter warning flags than we use in security/manager/ssl/src. If/when we get around to fixing warnings in security/manager, we'll fix it then.

Comment 9

[Eric Rahm [:erahm]]

Attachment: Part 2: Use NS_PTR_TO_UINT32 in nsISupportsHashKey and nsPtrHashKey

This updates two of the hash key helpers.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8466610 [details] [diff] [review]
Part 2: Use NS_PTR_TO_UINT32 in nsISupportsHashKey and nsPtrHashKey

Review of attachment 8466610 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsHashKeys.h
@@ +285,5 @@
>  
>    static KeyTypePointer KeyToPointer(KeyType aKey) { return aKey; }
>    static PLDHashNumber HashKey(KeyTypePointer aKey)
>    {
> +    return NS_PTR_TO_UINT32(aKey) >> 2;

I see a couple more uses of this idiom in nsHashKeys and one instance of this in DeadlockDetector.h.  Could you please fix those instances in this patch as well?

Bonus points for filing and/or fixing the followup bug to address other uses of NS_PTR_TO_INT32(...) >> X in the tree.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8466344 [details] [diff] [review]
Part 3: Cast to unsigned char in HashString

Review of attachment 8466344 [details] [diff] [review]:
-----------------------------------------------------------------

I think the right place to fix this is in the AddToHash overload the compiler is actually complaining about.  Something like:

template<typename A>
MOZ_WARN_UNUSED_RESULT inline uint32_t
AddToHash(uint32_t aHash, A aA)
{
  return detail::AddU32ToHash(aHash, static_cast<MakeUnsigned<A>::Type>(aA));
}

but probably with some more smarts to deal with 64-bit |A| types, so that any errors we might have gotten from the implicit cast previously are not silenced by the explicit cast we now use.

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8466344 [details] [diff] [review]
Part 3: Cast to unsigned char in HashString

Review of attachment 8466344 [details] [diff] [review]:
-----------------------------------------------------------------

Filed bug 1048391 as a follow up for Nathan's request, we both agree it's worthwhile but shouldn't block landing the DeadlockDetector improvements. Nathan are you ok with the simple cast here for now?

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8466344 [details] [diff] [review]
Part 3: Cast to unsigned char in HashString

Review of attachment 8466344 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, thanks!

Comment 14

[Eric Rahm [:erahm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/817524a9ebdd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/151a16abe0bd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb28e68b76fa

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/817524a9ebdd
https://hg.mozilla.org/mozilla-central/rev/151a16abe0bd
https://hg.mozilla.org/mozilla-central/rev/eb28e68b76fa