795433 - nsresult enum list in nsError.h produces hundreds of build warnings like "nsError.h:130:19: warning: ISO C restricts enumerator values to range of ‘int’ [-pedantic]"

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: list of 364 warnings that we spam for for a single .c file that includes nsError.h

I get hundreds of build warnings from nsError.h now, of the form:
{
../../dist/include/nsError.h:130:19: warning: ISO C restricts enumerator values to range of ‘int’ [-pedantic]
../../dist/include/nsError.h:132:30: warning: ISO C restricts enumerator values to range of ‘int’ [-pedantic]
}

I think (?) these warnings are only triggered when we have a .c file (not .cpp) that #includes nsError.h.

Not sure what the cleanest way to fix these are.  Maybe it's worth just disabling the GCC "pedantic" warnings for this file (or this chunk of the file)?

Comment 1

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Not sure what the cleanest way to fix these are.  Maybe it's worth just
> disabling the GCC "pedantic" warnings for this file (or this chunk of the
> file)?

I think that's the right thing to do, unless we have evidence that gcc actually generates bad code here.

Comment 2

[Daniel Holbert [:dholbert]]

I don't think there's any concern that GCC generates bad code here; I think it's warning that other compilers would be within their rights to do so.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Unfortunately, it does look like C11 has this restriction -- 6.7.2.2(2) in the WG14 N1570 draft.  If we want our code to be standards-compatible, I guess we need to disable nsresult-as-enum for C code.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: First attempt, works with clang but not GCC.

In this version I made nsresult a typedef for |uint32_t| in C code, and
defined all the constants as |static const nsresult| in nsError.h.  It
compiled on clang but failed on GCC with "initializer element is not constant"
due to the arithmetic in the definitions of the constants.

Comment 5

[Masatoshi Kimura [:emk]]

What still requires C support? Can't we just convert it to C++?

Comment 6

[Nicholas Nethercote [inactive]]

There is still heaps of C code, including third-party stuff that's hard to modify.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Make |nsresult| a typedef for |uint32_t| in C code.

This also didn't work.  trace-malloc builds failed because the constants are
only defined in libxul.so.  I think we might just need to define these
constants as macros :(

Comment 8

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Created attachment 696231 [details] [diff] [review]
> Make |nsresult| a typedef for |uint32_t| in C code.
> 
> This also didn't work.  trace-malloc builds failed because the constants are
> only defined in libxul.so.  I think we might just need to define these
> constants as macros :(

Try static const instead of const.

Comment 9

[Masatoshi Kimura [:emk]]

(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Nicholas Nethercote [:njn] from comment #7)
> > Created attachment 696231 [details] [diff] [review]
> > Make |nsresult| a typedef for |uint32_t| in C code.
> > 
> > This also didn't work.  trace-malloc builds failed because the constants are
> > only defined in libxul.so.  I think we might just need to define these
> > constants as macros :(
> 
> Try static const instead of const.

Is |extern static const| possible?

Comment 10

[Mike Hommey [:glandium]]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> > Try static const instead of const.
> 
> Is |extern static const| possible?

No, and that's the point. That's precisely because the const are not static that Nicholas has build failures.

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to Mike Hommey [:glandium] from comment #10)
> (In reply to Masatoshi Kimura [:emk] from comment #9)
> > > Try static const instead of const.
> > 
> > Is |extern static const| possible?
> 
> No, and that's the point. That's precisely because the const are not static
> that Nicholas has build failures.

|static const| is already tried in the previous patch. Unlike C++, C doesn't allow variables in initializers even if the variables are const...

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Make |nsresult| a typedef for |uint32_t| in C code.

This version uses |static| instead of |extern|, as glandium suggested, and
trace-malloc compiles.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 696246 [details] [diff] [review]
Make |nsresult| a typedef for |uint32_t| in C code.

I don't think I understand what's going on here well enough to review, sorry.  I'm a bit hazy on stuff like exactly what "static const" means.

Comment 14

[(no longer active)]

Comment on attachment 696246 [details] [diff] [review]
Make |nsresult| a typedef for |uint32_t| in C code.

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

I wanted to say r=me with only the nsError.h bits, but minusing to give Nick a chance to tell me what I was missing about the rest of the hunks.

::: xpcom/base/Makefile.in
@@ +38,5 @@
>  		nsStackWalk.cpp \
>  		nsMemoryReporterManager.cpp \
>  		ClearOnShutdown.cpp \
>  		VisualEventTracer.cpp \
> +		nsError.cpp \

Not sure why you're renaming the file here.

::: xpcom/base/nsErrorAsserts.cpp
@@ +5,5 @@
>  // nsError.h, so let's put this in its own .cpp file instead of in the .h.
>  MOZ_STATIC_ASSERT(sizeof(nsresult) == sizeof(uint32_t),
>                    "nsresult must be 32 bits");
> +
> +#if !defined(__cplusplus)

How can this ever be true? :-)

Comment 15

[Nicholas Nethercote [inactive]]

> > +		nsError.cpp \
> 
> Not sure why you're renaming the file here.

That file used to hold only an assertion, but now it holds more general stuff, so I made the name more general.

Comment 16

[(no longer active)]

Comment on attachment 696246 [details] [diff] [review]
Make |nsresult| a typedef for |uint32_t| in C code.

Oh, right, sorry my bad.

Comment 17

[Nicholas Nethercote [inactive]]

> ::: xpcom/base/nsErrorAsserts.cpp
> @@ +5,5 @@
> >  // nsError.h, so let's put this in its own .cpp file instead of in the .h.
> >  MOZ_STATIC_ASSERT(sizeof(nsresult) == sizeof(uint32_t),
> >                    "nsresult must be 32 bits");
> > +
> > +#if !defined(__cplusplus)
> 
> How can this ever be true? :-)

Good question.  The code is dead.  Which means that we never define these variables.  And now I don't understand how this code works.

glandium, can you explain?  AIUI, by making the variables |static|, a C file that imports nsError.h gets its own local copies of the variables.  But that C file doesn't see a definition.  Huh?  Somehow the C++ definitions must be used when the C file is linked.  Or something.

Comment 18

[Masatoshi Kimura [:emk]]

  static const nsresult key;
just means
  static const nsresult key = 0;
which is obviously wrong if someone actually see the definition.

I asked "What still requires C support?" :)

Comment 19

[Nicholas Nethercote [inactive]]

> I asked "What still requires C support?" :)

Any C file that #includes nscore.h.  xpt_xdr.c is the first one I hit, though the #include can be removed without problem in that case.  widget/gtkxtbin/gtk2xtbin.h is the next one I hit, and the #include _cannot_ be removed in that case.

Comment 20

[Masatoshi Kimura [:emk]]

Then gtk2xtbin.h was broken by the patch because all nsresult values became zero in C code.

Comment 21

[Nicholas Nethercote [inactive]]

> xpt_xdr.c is the first one I hit,
> though the #include can be removed without problem in that case.

Oh, it turns out this causes link failures.

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: Make |nsresult| a typedef for |uint32_t| in C code.

This version does |static const nsresult key = val;| which compiles.  But
there's a subtle problem, which is that some of the values depend on other
values, and therefore we're depending on the order of static initializers,
which is a recipe for woe.

I'm running out of ideas.  We could just use a #pragra to disable the warning.
Or we could change nsresult's encoding to fit within an int, but I suspect
that'll be very difficult.

Comment 23

[Masatoshi Kimura [:emk]]

(In reply to Nicholas Nethercote [:njn] from comment #22)
> Created attachment 696933 [details] [diff] [review]
> Make |nsresult| a typedef for |uint32_t| in C code.
> 
> This version does |static const nsresult key = val;| which compiles.

Does it really compile on GCC without "initializer element is not constant"?

Comment 24

[Nicholas Nethercote [inactive]]

> Does it really compile on GCC without "initializer element is not constant"?

Oh, I only tried clang.  It probably doesn't work on GCC.  But it's not safe anyway, due to the static initializer issue.

Comment 25

[Masatoshi Kimura [:emk]]

Attachment: Use #define for nsresult values in C code

Comment 26

[Masatoshi Kimura [:emk]]

Comment on attachment 697060 [details] [diff] [review]
Use #define for nsresult values in C code

This patch passed TryServer:
https://tbpl.mozilla.org/?tree=Try&rev=3050c7f8f8e3

Comment 27

[Masatoshi Kimura [:emk]]

Attachment: Generated ErrorListCDefines.h for reference

Comment 28

[Nicholas Nethercote [inactive]]

Comment on attachment 697060 [details] [diff] [review]
Use #define for nsresult values in C code

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

Cool!  Thanks for doing this.

::: xpcom/base/Makefile.in
@@ +171,4 @@
>  		sed -n 's/.*ERROR(\([A-Z_0-9]*\).*/#define \1 nsresult::\1/p' < $< > $@
>  
> +ErrorListCDefines.h: ErrorList.h Makefile
> +		sed 's/.*ERROR(\([A-Z_0-9]*\),\( *\)\(.*\))[^)]*/#define \1 \2(\3)/' < $< > $@

This produces lines like this:

  #define NS_ERROR_BASE (0xC1F30000)

An explicit uint32_t cast might be a good idea, e.g.:

  #define NS_ERROR_BASE ((uint32_t)(0xC1F30000))

Or perhaps an nsresult cast instead:

  #define NS_ERROR_BASE ((nsresult)(0xC1F30000))

@@ +173,5 @@
> +ErrorListCDefines.h: ErrorList.h Makefile
> +		sed 's/.*ERROR(\([A-Z_0-9]*\),\( *\)\(.*\))[^)]*/#define \1 \2(\3)/' < $< > $@
> +
> +GARBAGE += \
> +	ErrorListDefines.h \

Perhaps this should now be called ErrorListCppDefines.h or ErrorListCxxDefines.h?

::: xpcom/base/nsError.h
@@ +142,5 @@
>    #include "ErrorListDefines.h"
>  #endif
> +#else /* defined(__cplusplus) */
> +  /* C enum can't have a value outside the range of 'int'.
> +   * C const can't initialize with a variable even if it is const.

Maybe change "variable" to "expression involving other variables"?

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: Use #define for nsresult values in C code. feedback=njn

Resolved feedback comments.
Try result:
https://tbpl.mozilla.org/?tree=Try&rev=1fd32e86c337

Comment 30

[Masatoshi Kimura [:emk]]

Attachment: Generated ErrorListCDefines.h

Comment 31

[(no longer active)]

Comment on attachment 697335 [details] [diff] [review]
Use #define for nsresult values in C code. feedback=njn

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

r=me, but I'd like glandium look over the build-system bits as well.

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 697335 [details] [diff] [review]
Use #define for nsresult values in C code. feedback=njn

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

::: xpcom/base/Makefile.in
@@ +171,4 @@
>  		sed -n 's/.*ERROR(\([A-Z_0-9]*\).*/#define \1 nsresult::\1/p' < $< > $@
>  
> +ErrorListCDefines.h: ErrorList.h Makefile
> +		sed 's/.*ERROR(\([A-Z_0-9]*\),\( *\)\(.*\))[^)]*/#define \1 \2((nsresult)(\3))/' < $< > $@

Why not use -n s///p like for ErrorListCxxDefines.h?

Comment 33

[Masatoshi Kimura [:emk]]

(In reply to Mike Hommey [:glandium] from comment #32)
> Why not use -n s///p like for ErrorListCxxDefines.h?

Because non-replaced lines such as |#define MODULE ...| are needed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/06a834b786cf

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06a834b786cf