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]"

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: njn)

Tracking

(Blocks 1 bug)

Trunk
mozilla20
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

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)?
Attachment #666018 - Attachment description: list of warnings that this triggers, for a single .c file → list of 364 warnings that we spam for for a single .c file that includes nsError.h
(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.
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.
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.
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.
Assignee: nobody → n.nethercote
What still requires C support? Can't we just convert it to C++?
There is still heaps of C code, including third-party stuff that's hard to modify.
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 :(
(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.
(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?
(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.
(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...
This version uses |static| instead of |extern|, as glandium suggested, and
trace-malloc compiles.
Attachment #696246 - Flags: review?(ayg)
Attachment #696231 - Attachment is obsolete: true
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.
Attachment #696246 - Flags: review?(ayg) → review?(ehsan)
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? :-)
Attachment #696246 - Flags: review?(ehsan) → review-
> > +		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 on attachment 696246 [details] [diff] [review]
Make |nsresult| a typedef for |uint32_t| in C code.

Oh, right, sorry my bad.
Attachment #696246 - Flags: review- → review+
> ::: 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.
  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?" :)
> 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.
Then gtk2xtbin.h was broken by the patch because all nsresult values became zero in C code.
> 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.
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.
Attachment #696246 - Attachment is obsolete: true
(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"?
> 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.
Attachment #697060 - Flags: feedback?(n.nethercote)
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 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"?
Attachment #697060 - Flags: feedback?(n.nethercote) → feedback+
Attachment #697062 - Attachment is obsolete: true
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.
Attachment #697335 - Flags: review?(mh+mozilla)
Attachment #697335 - Flags: review?(ehsan)
Attachment #697335 - Flags: review+
Attachment #696215 - Attachment is obsolete: true
Attachment #696933 - Attachment is obsolete: true
Attachment #697060 - Attachment is obsolete: true
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?
Attachment #697335 - Flags: review?(mh+mozilla) → review+
(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
Status: NEW → ASSIGNED
Flags: in-testsuite-
QA Contact: VYV03354
https://hg.mozilla.org/mozilla-central/rev/06a834b786cf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.