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

()

RESOLVED FIXED
6 years ago
6 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)

(Reporter)

Description

6 years ago
Created attachment 666018 [details]
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)?
(Reporter)

Updated

6 years ago
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

Comment 1

6 years ago
(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.
(Reporter)

Comment 2

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 696215 [details] [diff] [review]
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → n.nethercote
What still requires C support? Can't we just convert it to C++?
(Assignee)

Comment 6

6 years ago
There is still heaps of C code, including third-party stuff that's hard to modify.
(Assignee)

Comment 7

6 years ago
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 :(
(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...
(Assignee)

Comment 12

6 years ago
Created attachment 696246 [details] [diff] [review]
Make |nsresult| a typedef for |uint32_t| in C code.

This version uses |static| instead of |extern|, as glandium suggested, and
trace-malloc compiles.
Attachment #696246 - Flags: review?(ayg)
(Assignee)

Updated

6 years ago
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 14

6 years ago
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-
(Assignee)

Comment 15

6 years ago
> > +		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

6 years ago
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+
(Assignee)

Comment 17

6 years ago
> ::: 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?" :)
(Assignee)

Comment 19

6 years ago
> 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.
(Assignee)

Comment 21

6 years ago
> 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.
(Assignee)

Comment 22

6 years ago
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.  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.
(Assignee)

Updated

6 years ago
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"?
(Assignee)

Comment 24

6 years ago
> 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.
Created attachment 697060 [details] [diff] [review]
Use #define for nsresult values in C code
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
Created attachment 697062 [details]
Generated ErrorListCDefines.h for reference
(Assignee)

Comment 28

6 years ago
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+
Created attachment 697335 [details] [diff] [review]
Use #define for nsresult values in C code. feedback=njn

Resolved feedback comments.
Try result:
https://tbpl.mozilla.org/?tree=Try&rev=1fd32e86c337
Attachment #697335 - Flags: review?(ehsan)
Created attachment 697337 [details]
Generated ErrorListCDefines.h
Attachment #697062 - Attachment is obsolete: true

Comment 31

6 years ago
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+
(Assignee)

Updated

6 years ago
Attachment #696215 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #696933 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.