Last Comment Bug 788242 - Implement and make use of void versions of NS_ENSURE_* macros
: Implement and make use of void versions of NS_ENSURE_* macros
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla18
Assigned To: Koosha KM
:
Mentors:
: 609210 790290 (view as bug list)
Depends on: 564461
Blocks: 716278
  Show dependency treegraph
 
Reported: 2012-09-04 12:48 PDT by Koosha KM
Modified: 2012-09-18 06:08 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test-array.so (904 bytes, application/octet-stream)
2012-09-04 12:59 PDT, Koosha KM
no flags Details
patch - v1 (9.16 KB, patch)
2012-09-12 03:29 PDT, Koosha KM
no flags Details | Diff | Splinter Review
patch - v2 (4.50 KB, patch)
2012-09-12 10:48 PDT, Koosha KM
no flags Details | Diff | Splinter Review
patch - v3 (33.29 KB, patch)
2012-09-14 03:08 PDT, Koosha KM
ehsan: review+
benjamin: review+
Details | Diff | Splinter Review

Description Koosha KM 2012-09-04 12:48:59 PDT
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
Comment 1 Mike Hommey [:glandium] 2012-09-04 12:54:11 PDT
Can you attach the /home/koosha/firefox-src/obj-i686-pc-linux-gnu/build/unix/elfhack/test-array.so file?
Comment 2 Koosha KM 2012-09-04 12:59:02 PDT
Created attachment 658193 [details]
test-array.so

Sure.
Comment 3 Mike Hommey [:glandium] 2012-09-04 14:15:45 PDT
The file is truncated. Try removing it and rebuild.
Comment 4 Koosha KM 2012-09-04 22:23:34 PDT
(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.
Comment 6 Koosha KM 2012-09-05 01:40:09 PDT
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
Comment 7 Koosha KM 2012-09-05 01:41:23 PDT
(In reply to Koosha Khajeh Moogahi [:koosha] from comment #6)
> I checked the linked,

link*
Comment 8 Mike Hommey [:glandium] 2012-09-05 02:05:04 PDT
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.
Comment 9 Koosha KM 2012-09-07 03:16:30 PDT
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.
Comment 10 Koosha KM 2012-09-12 03:29:05 PDT
Created attachment 660380 [details] [diff] [review]
patch - v1

Not sure if this is the right solution, but it does resolve the compilation error.
Comment 11 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-12 03:36:23 PDT
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.)
Comment 12 Koosha KM 2012-09-12 03:41:42 PDT
(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?
Comment 13 Mike Hommey [:glandium] 2012-09-12 04:42:17 PDT
(In reply to Koosha Khajeh Moogahi [:koosha] from comment #12)
> So you mean that I should update my patch?

Yes
Comment 14 Koosha KM 2012-09-12 10:48:07 PDT
Created attachment 660510 [details] [diff] [review]
patch - v2
Comment 15 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-09-12 11:34:05 PDT
I just disable the warning with clang, but I OK with changing the code. In which case I will revert the clang change.
Comment 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-09-12 11:34:54 PDT
or should I say, not commit it :-)
Comment 17 Mike Hommey [:glandium] 2012-09-12 14:18:20 PDT
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.
Comment 18 :Ehsan Akhgari 2012-09-12 15:28:20 PDT
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.  :(
Comment 19 :Ehsan Akhgari 2012-09-12 15:31:08 PDT
Also, Koosha, do you have --enable-warnings-as-errors in your mozconfig?
Comment 20 Koosha KM 2012-09-12 15:37:18 PDT
(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.
Comment 21 Mike Hommey [:glandium] 2012-09-12 15:50:42 PDT
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.
Comment 22 :Ehsan Akhgari 2012-09-12 17:03:29 PDT
(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? :-)
Comment 23 Koosha KM 2012-09-12 21:59:15 PDT
(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.
Comment 24 Koosha KM 2012-09-12 22:00:44 PDT
(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, ).
Comment 25 Nathan Froyd [:froydnj] 2012-09-13 01:01:05 PDT
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.
Comment 26 :Ehsan Akhgari 2012-09-13 11:20:24 PDT
(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...
Comment 27 Koosha KM 2012-09-13 11:37:06 PDT
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!
Comment 28 Koosha KM 2012-09-13 11:39:24 PDT
But, I think I would look nicer if we don't get this ugly warning.
Comment 29 Koosha KM 2012-09-13 11:40:01 PDT
... it* would look ...
Comment 30 :Ehsan Akhgari 2012-09-13 14:56:38 PDT
(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.  :-)
Comment 31 Koosha KM 2012-09-14 01:04:01 PDT
Sure, I'll do that. Hence, changing the bug title and its importance.
Comment 32 Koosha KM 2012-09-14 03:08:11 PDT
Created attachment 661171 [details] [diff] [review]
patch - v3
Comment 33 Ted Mielczarek [:ted.mielczarek] 2012-09-14 03:23:24 PDT
bsmedberg already hates these macros, I bet he won't like this! :)
Comment 34 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-14 05:42:47 PDT
But there are also people who like NS_ENSURE_* macros ;
Comment 35 :Ehsan Akhgari 2012-09-14 10:38:54 PDT
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.  :-)
Comment 36 Koosha KM 2012-09-16 02:55:51 PDT
Is bsmedberg around? How long are we supposed to wait?
Comment 37 Benjamin Smedberg [:bsmedberg] 2012-09-17 12:33:10 PDT
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.
Comment 38 Koosha KM 2012-09-17 14:09:15 PDT
Probably bug 609210 should be marked as a duplicate of this one.
Comment 39 :Ehsan Akhgari 2012-09-17 14:36:12 PDT
*** Bug 609210 has been marked as a duplicate of this bug. ***
Comment 41 :Ehsan Akhgari 2012-09-17 14:39:16 PDT
Thanks for your patch, Koosha!  It should get merged to mozilla-central within a few hours.
Comment 42 :aceman 2012-09-17 14:52:19 PDT
Cool, finally some progress, thanks :)

The comm-central version of this is bug 716278.
Comment 43 Graeme McCutcheon [:graememcc] 2012-09-18 05:06:39 PDT
https://hg.mozilla.org/mozilla-central/rev/ce2ac7b4015b
Comment 44 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-09-18 06:08:24 PDT
*** Bug 790290 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.