Closed Bug 788242 Opened 12 years ago Closed 12 years ago

Implement and make use of void versions of NS_ENSURE_* macros

Categories

(Core :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: koosha.khajeh, Assigned: koosha.khajeh)

References

Details

Attachments

(2 files, 2 obsolete files)

I tried to compile the source both with gcc and clang but failed both times. My gcc version is gcc (Debian 4.4.5-8) 4.4.5 on Debian GNU/Linux Squeeze i686. I tried a couple of times after pulling the latest changes but didn't succeed.

Here is the output error:

make[5]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make -C config libs
make[5]: Entering directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/config'
/home/koosha/firefox-src/obj-i686-pc-linux-gnu/config/nsinstall -R -m 755 nsinstall ../dist/host/bin
make[5]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/config'
make -C build libs
make[5]: Entering directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/build'
make -C unix libs
make[6]: Entering directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix'
make -C elfhack libs
/home/koosha/firefox-src/obj-i686-pc-linux-gnu/config/nsinstall -D ../../dist/sdk/bin
make[7]: Entering directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix/elfhack'
# Will either crash or return exit code 1 if elfhack is broken
LD_PRELOAD=/home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix/elfhack/test-array.so /home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix/elfhack/dummy
Bus error
make[7]: *** [libs] Error 135
make[7]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix/elfhack'
make[6]: *** [libs] Error 2
make[6]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/build'
make[4]: *** [libs_tier_base] Error 2
make[4]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make[3]: *** [tier_base] Error 2
make[3]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/home/koosha/firefox-src'
make: *** [build] Error 2
Can you attach the /home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix/elfhack/test-array.so file?
Attached file test-array.so
Sure.
The file is truncated. Try removing it and rebuild.
(In reply to Mike Hommey [:glandium] from comment #3)
> The file is truncated. Try removing it and rebuild.

No only this file, but the whole obj directory I removed and nothing changed. My system used to have some problems with libraries in /usr/lib. Could you please tell me what libraries are required to compile this module (or others if possible) so that I could double check and retry.
I checked the linked, reinstalled all the libraries, removed the obj dir and finally tried again but got this error:

make[7]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content/xslt/tests/buster'
make -C tests/mochitest libs
make[7]: Entering directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content/xslt/tests/mochitest'
/home/koosha/firefox-src/obj-i686-pc-linux-gnu/config/nsinstall -R "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug319374.xhtml" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug440974.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug427060.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug468208.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug453441.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug511487.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug551412.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug551654.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug566629.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug566629.xhtml" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug603159.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug616774.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_bug667315.html" "/home/koosha/firefox-src/content/xslt/tests/mochitest/test_exslt_regex.html" ../../../../_tests/testing/mochitest/tests/content/xslt/tests/mochitest
make[7]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content/xslt/tests/mochitest'
make[6]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content/xslt'
make[5]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content'
make[4]: *** [libs_tier_platform] Error 2
make[4]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make[3]: *** [tier_platform] Error 2
make[3]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/home/koosha/firefox-src'
make: *** [build] Error 2
(In reply to Koosha Khajeh Moogahi [:koosha] from comment #6)
> I checked the linked,

link*
There's not enough context to your pasted log to know what's wrong. Please try again with make -C /home/koosha/firefox-src/obj-i686-pc-linux-gnu without any -j option.
home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2405:25: error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2405:25: error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2422:29: error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2422:29: error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2439:31: error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2439:31: error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98

make[6]: *** [nsHTMLDocument.o] Error 1
make[6]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content/html/document/src'
make[5]: *** [src_libs] Error 2
make[5]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content/html/document'
make[4]: *** [document_libs] Error 2
make[4]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content/html'
make[3]: *** [html_libs] Error 2
make[3]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu/content'
make[2]: *** [libs_tier_platform] Error 2
make[2]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make[1]: *** [tier_platform] Error 2
make[1]: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'
make: *** [default] Error 2
make: Leaving directory `/home/koosha/firefox-src/obj-i686-pc-linux-gnu'

Also, when I copy the first arg for the second one to these macros, a new error shows up:

/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2405: error: return-statement with a value, in function returning 'void'
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2422: error: return-statement with a value, in function returning 'void'
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2439: error: return-statement with a value, in function returning 'void'

which is expected. Since the function(s) (other functions that have similar macros called in their body are affected as well) have 'void' as return value, but the macros don't return void.
Attached patch patch - v1 (obsolete) — Splinter Review
Not sure if this is the right solution, but it does resolve the compilation error.
Assignee: nobody → koosha.khajeh
Status: NEW → ASSIGNED
Attachment #660380 - Flags: review?(mh+mozilla)
Depends on: 564461
If NS_ENSURE_SUCCESS(rv, ); is causing the problem, 
changing those to
if (NS_FAILED(rv)) {
  return;
}

should be ok. (We won't get warnings on debug builds, but that should be ok in this case.)
(In reply to Olli Pettay [:smaug] from comment #11)
> If NS_ENSURE_SUCCESS(rv, ); is causing the problem, 
> changing those to
> if (NS_FAILED(rv)) {
>   return;
> }
> 
> should be ok. (We won't get warnings on debug builds, but that should be ok
> in this case.)

So you mean that I should update my patch?
(In reply to Koosha Khajeh Moogahi [:koosha] from comment #12)
> So you mean that I should update my patch?

Yes
Attachment #660380 - Flags: review?(mh+mozilla)
Attached patch patch - v2 (obsolete) — Splinter Review
Attachment #660380 - Attachment is obsolete: true
Attachment #660510 - Flags: review?(mh+mozilla)
Severity: normal → major
Hardware: x86 → All
I just disable the warning with clang, but I OK with changing the code. In which case I will revert the clang change.
See Also: → 790290
or should I say, not commit it :-)
Comment on attachment 660510 [details] [diff] [review]
patch - v2

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

I'm not a peer on these parts of the code.
Attachment #660510 - Flags: review?(mh+mozilla) → review?(ehsan)
So, first questions first, does gcc have a flag which we can use to ask it to shut down this warning?  Having to change the code to work around gcc stupidity sort of sucks.  :(
Also, Koosha, do you have --enable-warnings-as-errors in your mozconfig?
(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> Also, Koosha, do you have --enable-warnings-as-errors in your mozconfig?

I did have it. But, I disabled it and experienced no change at all.

Note the word "error" in

error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98

Seems gcc treats it as an error not a warning.
BTW, what version of the Firefox source code have you been trying to build? I've been building Firefox source code with the Debian squeeze gcc for a while and up to version 17, with no such error.
(In reply to comment #20)
> (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> > Also, Koosha, do you have --enable-warnings-as-errors in your mozconfig?
> 
> I did have it. But, I disabled it and experienced no change at all.
> 
> Note the word "error" in
> 
> error: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are
> undefined in ISO C90 and ISO C++98
> 
> Seems gcc treats it as an error not a warning.

Hmm, can you try compiling a simple program such as:

#define foo(x, y)
foo(x, )

And see if your gcc screams when it sees that? :-)
(In reply to Mike Hommey [:glandium] from comment #21)
> BTW, what version of the Firefox source code have you been trying to build?
> I've been building Firefox source code with the Debian squeeze gcc for a
> while and up to version 17, with no such error.

Refer to bug description.
(In reply to Ehsan Akhgari [:ehsan] from comment #22)
> (In reply to comment #20)
> Hmm, can you try compiling a simple program such as:
> 
> #define foo(x, y)
> foo(x, )
> 
> And see if your gcc screams when it sees that? :-)

I tried. A simple two-argument macro without a body works fine when called M(x, ).
FWIW, xpcom/tests/TestObserverArray.cpp has the same empty macro argument issue.

There's no good way to disable it from GCC's command line.
(In reply to Koosha Khajeh Moogahi [:koosha] from comment #24)
> (In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > (In reply to comment #20)
> > Hmm, can you try compiling a simple program such as:
> > 
> > #define foo(x, y)
> > foo(x, )
> > 
> > And see if your gcc screams when it sees that? :-)
> 
> I tried. A simple two-argument macro without a body works fine when called
> M(x, ).

So, one of the command line arguments that building firefox passes to gcc is causing this.  Can you go to objdir/content/html/document/src, rm nsHTMLDocument.o, make nsHTMLDocument.o, record the full command line and paste it here?

Also, it would be good if you can test and see adding which command line that we use when building firefox will result in the above simple program failing to compile...
nsHTMLDocument.cpp
c++ -o nsHTMLDocument.o -c -I../../../../dist/stl_wrappers -I../../../../dist/system_wrappers -include /home/koosha/firefox-src/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -D_IMPL_NS_LAYOUT  -I/home/koosha/firefox-src/content/html/document/src -I. -I../../../../dist/include  -I/home/koosha/firefox-src/obj-i686-pc-linux-gnu/dist/include/nspr -I/home/koosha/firefox-src/obj-i686-pc-linux-gnu/dist/include/nss     -I/home/koosha/firefox-src/content/html/document/src/../../../base/src -I/home/koosha/firefox-src/content/html/document/src/../../../events/src -I/home/koosha/firefox-src/content/html/document/src/../../content/src -I/home/koosha/firefox-src/layout/style -I/home/koosha/firefox-src/dom/base -I/home/koosha/firefox-src/xpcom/io -I/home/koosha/firefox-src/caps/include -I/home/koosha/firefox-src/xpcom/ds   -fPIC  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fomit-frame-pointer    -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -MD -MF .deps/nsHTMLDocument.o.pp /home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2408:25: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2408:25: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2425:29: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2425:29: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2442:31: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/koosha/firefox-src/content/html/document/src/nsHTMLDocument.cpp:2442:31: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98


Compiled successfully with the error turned into a warning!
But, I think I would look nicer if we don't get this ugly warning.
... it* would look ...
(In reply to comment #28)
> But, I think I would look nicer if we don't get this ugly warning.

Agreed.  The proper way to fix that would be to create alternate versions of these macros for void (such as NS_ENSURE_TRUE_VOID(cond)) functions which take only one parameter and just return if the condition check fails, and then replace the warning instances with the new macros.  Are you willing to write that patch?  I'd be happy to review it.  :-)
Sure, I'll do that. Hence, changing the bug title and its importance.
Severity: major → enhancement
OS: Linux → All
Summary: Can't compile Firefox from the trunk source code on Linux with GCC → Implement and make use of void versions of NS_* macros
Attachment #660510 - Flags: review?(ehsan)
Attached patch patch - v3Splinter Review
Attachment #660510 - Attachment is obsolete: true
Attachment #661171 - Flags: review?(ehsan)
Summary: Implement and make use of void versions of NS_* macros → Implement and make use of void versions of NS_ENSURE_* macros
bsmedberg already hates these macros, I bet he won't like this! :)
But there are also people who like NS_ENSURE_* macros ;
Comment on attachment 661171 [details] [diff] [review]
patch - v3

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

This looks good to me.  Asking review from bsmedberg since Ted indicated that he may not like this.  Note that we're not really making the existing issue of using these macros any worse, we're just making them build on more platforms.  :-)
Attachment #661171 - Flags: review?(ehsan)
Attachment #661171 - Flags: review?(benjamin)
Attachment #661171 - Flags: review+
Component: Build Config → General
Product: Firefox → Core
Is bsmedberg around? How long are we supposed to wait?
See Also: → 609210
Comment on attachment 661171 [details] [diff] [review]
patch - v3

I do very much despise these macros, but you're right that we're not making anything worse here.
Attachment #661171 - Flags: review?(benjamin) → review+
Probably bug 609210 should be marked as a duplicate of this one.
Thanks for your patch, Koosha!  It should get merged to mozilla-central within a few hours.
Cool, finally some progress, thanks :)

The comm-central version of this is bug 716278.
Blocks: 716278
See Also: 609210
https://hg.mozilla.org/mozilla-central/rev/ce2ac7b4015b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: