836438 - nsresult isn't unsigned in C++, with compilers that we haven't determined to support C++11 typed enums or enum classes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

nsresult is defined like so:

#if defined(__cplusplus)
#if defined(MOZ_HAVE_CXX11_STRONG_ENUMS)
  typedef enum class tag_nsresult : uint32_t
#elif defined(MOZ_HAVE_CXX11_ENUM_TYPE)
  /* Need underlying type for workaround of Microsoft compiler (Bug 794734) */
  typedef enum tag_nsresult : uint32_t
#else
  /* no strong enums */
  typedef enum tag_nsresult
#endif
  {
    #undef ERROR
    #define ERROR(key, val) key = val
    #include "ErrorList.h"
    #undef ERROR
  } nsresult;

#if defined(MOZ_HAVE_CXX11_STRONG_ENUMS)
  /* We're using enum classes, so we need #define's to put the constants in
   * global scope for compatibility with old code. */
  #include "ErrorListCxxDefines.h"
#endif
#else /* defined(__cplusplus) */
  /* C enum can't have a value outside the range of 'int'.
   * C const can't initialize with an expression involving other variables
   * even if it is const. So we have to fall back to bad old #defines. */
  typedef uint32_t nsresult;
  #include "ErrorListCDefines.h"
#endif /* defined(__cplusplus) */

In the defined(__cplusplus) but !defined(MOZ_HAVE_CXX11_STRONG_ENUMS) and !defined(MOZ_HAVE_CXX11_ENUM_TYPE) case, nsresult will be an enum, which will be represented by a signed type.  We have in-tree code that depends upon nsresult being an unsigned type, so this is no good.  If we can't change the underlying type of an enum to be the right unsigned type, we can't make nsresult an enum.

So when we're C++ without typed enum/enum class support, we should do what the !defined(__cplusplus) fallback does.

I compiled long enough to hit a file that used this, but not all the way through -- it'll go through try before landing, to be safe.

Comment 1

[(no longer active)]

Comment on attachment 708256 [details] [diff] [review]
Patch

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

Thanks!

::: xpcom/base/nsError.h
@@ +6,4 @@
>  #ifndef nsError_h__
>  #define nsError_h__
>  
> +#include "mozilla/Assertions.h"

Please remove this...

@@ +156,5 @@
>    #include "ErrorListCDefines.h"
> +#endif
> +
> +MOZ_STATIC_ASSERT(((nsresult)0) < ((nsresult)-1),
> +                  "nsresult must be an unsigned type");

... and move this to http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsErrorAsserts.cpp.

@@ +158,5 @@
> +
> +MOZ_STATIC_ASSERT(((nsresult)0) < ((nsresult)-1),
> +                  "nsresult must be an unsigned type");
> +MOZ_STATIC_ASSERT(sizeof(nsresult) == sizeof(uint32_t),
> +                  "nsresult must have uint32_t representation");

And remove this too.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Actually working patch for compilers not supporting typed enums

So I made the changes requested (and added nsErrorAssertsC.c to test the C case) and tried.  And it turns out that the C nsresult #defines as they exist now are broken -- they depend on the SUCCESS/FAILURE/SUCCESS_OR_FAILURE macros in nsError.h that get promptly #undef'd after the list of #defines is #include'd.

Fixing this without not-undefing those macros (and I don't think we want those names polluting the world) appears non-trivial.  But it's possible to implement a C++ fix easily, so I did that.  The C brokenness is not all that obvious to the casual user, so I made it much more obvious.  This is not a functional regression, so I don't think we should hold this up waiting on a hypothetical C fix when nobody's complained in all the time since bug 795433 landed.  :-)  I'll file a followup to actually implement it (which is Someone Else's Problem) if you agree.

In passing I decided to untangle the pointless #include snarl between nscore.h and nsError.h, since it was so trivial to do.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 708413 [details] [diff] [review]
Actually working patch for compilers not supporting typed enums

Or, no.  NS_* is broken only if it uses SUCCESS or FAILURE.  New patch needed.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch

Whoever ends up being the first to use one of the SUCCESS/FAILURE-using constants in C is going to blow up the minefield, and anyone using one of the others won't know they're just getting lucky.  Fun times.  And same as before.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 708418 [details] [diff] [review]
Patch

ipc/chromium/src/chrome/common/ipc_message_utils.h is doing some template-fu using nsresult as a parameter, depending on it not to alias some number of other types.  With this change it seems to alias size_t on B2g.  Go directly to jail.  Do not pass Go, do not collect $200.

I promise not to post a patch here until it fully passes try.
I promise not to post a patch here until it fully passes try.
I promise not to post a patch here until it fully passes try.

(...that doesn't have the same punitive force when I can Ctrl+C.)

Comment 6

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Fixing this without not-undefing those macros (and I don't think we want
> those names polluting the world) appears non-trivial.  But it's possible to
> implement a C++ fix easily, so I did that.  The C brokenness is not all that
> obvious to the casual user, so I made it much more obvious.  This is not a
> functional regression, so I don't think we should hold this up waiting on a
> hypothetical C fix when nobody's complained in all the time since bug 795433
> landed.  :-)  I'll file a followup to actually implement it (which is
> Someone Else's Problem) if you agree.

Sure.

Also, admire your patience!  :-)

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Shave the IPC::ParamTraits<P> yak

This and the next patch haven't quite gone fully green, but the combination of this patch in one try run (with some not-working-everywhere bits atop it) and the next patch in a second one tell me this and the next are good to go.

https://tbpl.mozilla.org/?tree=Try&rev=dbbb6a022847
https://tbpl.mozilla.org/?tree=Try&rev=126170df7e20

I wrote basically the next patch I'll upload, only to hit a compile error in IPC code on B2g (that uses gcc 4.4 and gets nsresult-not-enum).  The problem was a templatized ParamTraits class that had explicit specializations for both uint32_t and nsresult.

  template <class P> struct ParamTraits {};
  template <> struct ParamTraits<uint32_t> {};

  // ERROR: multiple definitions of ParamTraits<unsigned int> here if
  //        unsigned int underlies both types
  template <>  struct ParamTraits<nsresult> {};

ParamTraits had the above specializations and thus crashed and burned.  The current workaround is to #if one of every set of maybe-conflicting types -- extremely obscure, just-so conditions.  Doing the same with nsresult as well (when it actually conflicted!) would be even worse, as I'd have to add in conditions based around the TypedEnum.h internal #defines.  Too much spooky interaction at a distance for me!

The relatively simple solution (at least if you've half-digested the IsSupported class used in implementing mozilla::CheckedInt) is to put handling for differently-named but potentially-the-same types in multiple explicitly-specialized traits classes.  Carefully chain those together via inheritance, and you can get a single traits class that appears to have any set of types you want, without needing to do your own conflict resolution.  See the comment in the patch for details.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Final patch for the nsresult issue

Having the initializers not have the same type as nsresult broke some C++ code we have, that used templates, that expected NS_OK and friends to be exactly nsresult.  But it occurs to me that const nsresult will work just fine for this in C++, if not in C, so do that.

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 708522 [details] [diff] [review]
Final patch for the nsresult issue

>+   * nsresult to the correct unsigned type, and fall back to using #defines for
>+   * all error constants.
Is this comment still true?

Comment 10

[(no longer active)]

Comment on attachment 708521 [details] [diff] [review]
Shave the IPC::ParamTraits<P> yak

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

wow!  This makes my head spin, but the basic idea is simple and well documented, and I don't know of any better solution.  So, r=me if it works.

Comment 11

[(no longer active)]

Comment on attachment 708522 [details] [diff] [review]
Final patch for the nsresult issue

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

::: xpcom/base/nsError.h
@@ +158,5 @@
> +  /*
> +   * C doesn't have any way to fix the type underlying an enum, and enum
> +   * initializers can't have values outside the range of 'int'.  So typedef
> +   * nsresult to the correct unsigned type, and fall back to using #defines for
> +   * all error constants.

Please fix this comment.

::: xpcom/base/nscore.h
@@ -325,4 @@
>  typedef uint32_t nsrefcnt;
>  #endif
>  
> -#include "nsError.h"

Hmm, why is this necessary?

Comment 12

[Jeff Walden [:Waldo]]

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #11)
> Please fix this comment.

That comment's still accurate -- it's in the not-C++ section where we still do use #defines.  Unless I'm really missing something, but I don't think I am...

> ::: xpcom/base/nscore.h
> @@ -325,4 @@
> >  typedef uint32_t nsrefcnt;
> >  #endif
> >  
> > -#include "nsError.h"
> 
> Hmm, why is this necessary?

It's not.  But this is the second such #include in nscore.h, and nsError.h is idempotent, so it's pointless and/or confusing.

Comment 13

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> (In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #11)
> > Please fix this comment.
> 
> That comment's still accurate -- it's in the not-C++ section where we still
> do use #defines.  Unless I'm really missing something, but I don't think I
> am...

Oh, right, sorry.  My bad.

> > ::: xpcom/base/nscore.h
> > @@ -325,4 @@
> > >  typedef uint32_t nsrefcnt;
> > >  #endif
> > >  
> > > -#include "nsError.h"
> > 
> > Hmm, why is this necessary?
> 
> It's not.  But this is the second such #include in nscore.h, and nsError.h
> is idempotent, so it's pointless and/or confusing.

Heh, nice catch!

Comment 14

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14550ec92519
https://hg.mozilla.org/integration/mozilla-inbound/rev/091c6ec19444

Comment 15

[Jeff Walden [:Waldo]]

Filed bug 836923 on making the nsresult constants work in C (although I did also suggest just converting that C code to C++, too :-) ).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14550ec92519
https://hg.mozilla.org/mozilla-central/rev/091c6ec19444