Fix warnings as errors build failures introduced by DeadlockDetector changes

RESOLVED FIXED in mozilla34

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla34
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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
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
Created attachment 8466341 [details] [diff] [review]
Part 1: Disable warning C4640 in certverifier
Created attachment 8466343 [details] [diff] [review]
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.
Created attachment 8466344 [details] [diff] [review]
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.
Note patches 2 & 3 would be unnecessary if we also disable warning C4365 in patch 1.
Status: NEW → ASSIGNED
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.
Attachment #8466341 - Flags: review?(dkeeler)
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 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.
Attachment #8466341 - Flags: review?(dkeeler) → review+
Attachment #8466343 - Flags: review?(nfroyd)
Attachment #8466344 - Flags: review?(nfroyd)
Created attachment 8466610 [details] [diff] [review]
Part 2: Use NS_PTR_TO_UINT32 in nsISupportsHashKey and nsPtrHashKey

This updates two of the hash key helpers.
Attachment #8466610 - Flags: review?(nfroyd)
Attachment #8466343 - Attachment is obsolete: true
Attachment #8466343 - Flags: review?(nfroyd)
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.
Attachment #8466610 - Flags: review?(nfroyd) → review+
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.
Attachment #8466344 - Flags: review?(nfroyd)
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?
Attachment #8466344 - Flags: review?(nfroyd)
Comment on attachment 8466344 [details] [diff] [review]
Part 3: Cast to unsigned char in HashString

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

Yup, thanks!
Attachment #8466344 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/817524a9ebdd
https://hg.mozilla.org/mozilla-central/rev/151a16abe0bd
https://hg.mozilla.org/mozilla-central/rev/eb28e68b76fa
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.