One such pattern is a file that #defines PL_ARENA_CONST_ALIGN_MASK and then #includes plarena.h, the other is a file which wants to force NSPR logging. Those files should always be compiled separately. This patches catches a whole bunch of these problems in our code right now, which I will fix separately.

Comment on attachment 8336348 [details] [diff] [review] Protect against two known bad patterns for unified builds; r=wtc Wan-Teh, we need this patch very urgently in mozilla-central. It is Mozilla specific and will not affect other consumers of NSPR. I'd really appreciate if you could please expedite this review. Thanks!

Hmm. Can you not instead check that PL_ARENA_CONST_ALIGN_MASK and the force log macro are not defined after including the .cpp in the unified file, similar to what we do with windows.h in binding unified files? Then you don't have to change NSPR here.

(In reply to comment #3) > Hmm. Can you not instead check that PL_ARENA_CONST_ALIGN_MASK and the force > log macro are not defined after including the .cpp in the unified file, similar > to what we do with windows.h in binding unified files? Then you don't have to > change NSPR here. That is possible but a bit more fragile... I'll do that if this patch doesn't get anywhere but I hope that we can get this into the tree very soon.

Hi Ehsan, I don't know how to review this patch because I don't know what a unified build is. Could you please explain it? It is strange to add checks in NSPR header files to detect problems in Mozilla files that will break a particular way to build Mozilla. There must be a better solution.

Yes ideally we wouldn't need to modify nspr and even could avoid this entirely but we're working around a particularly bad pattern in nspr where we have files with include guards but how they get processed depends on some having FORCE_PR_LOG defined. Say you have this: #include "prlog.h" ...<In another file in the same compilation unit> // Let's add runtime logging for my feature #define FORCE_PR_LOG #include "prlog.h" // <-- This wont enable logging because of the include guards We're just trying to catch some cases where this mistake might be made.

(In reply to Wan-Teh Chang from comment #5) > Hi Ehsan, > > I don't know how to review this patch because I don't know > what a unified build is. Could you please explain it? A unified build is basically creating a file like Unified.cpp and inside it #including your original source files, for example: #include "Source1.cpp" #include "Source2.cpp" // etc. Benoit gave a great description of the problem. > It is strange to add checks in NSPR header files to > detect problems in Mozilla files that will break a particular > way to build Mozilla. There must be a better solution. There isn't really. :( I mean, ideally we would keep this as a change to the NSPR headers inside our tree without upstreaming it, but we can't maintain patches to NSPR in our tree, so this is pretty much the only clean way for us to do this. As I mentioned before, this should have no impact on other consumers of NSPR.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4) > (In reply to comment #3) > > Hmm. Can you not instead check that PL_ARENA_CONST_ALIGN_MASK and the force > > log macro are not defined after including the .cpp in the unified file, similar > > to what we do with windows.h in binding unified files? Then you don't have to > > change NSPR here. > > That is possible but a bit more fragile... I'll do that if this patch > doesn't get anywhere but I hope that we can get this into the tree very soon. gcc and clang both support #pragma GCC poison, which you can use to poison macros, so that's another option though I'm not sure if msvc has something similar. In any case we should probably change the bindings thing to use it when possible since it gives you a trace to the file that pulled in windows.h

(In reply to comment #8) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #4) > > (In reply to comment #3) > > > Hmm. Can you not instead check that PL_ARENA_CONST_ALIGN_MASK and the force > > > log macro are not defined after including the .cpp in the unified file, similar > > > to what we do with windows.h in binding unified files? Then you don't have to > > > change NSPR here. > > > > That is possible but a bit more fragile... I'll do that if this patch > > doesn't get anywhere but I hope that we can get this into the tree very soon. > > gcc and clang both support #pragma GCC poison, which you can use to poison > macros, so that's another option though I'm not sure if msvc has something > similar. > > In any case we should probably change the bindings thing to use it when > possible since it gives you a trace to the file that pulled in windows.h I don't know of any such option unfortunately...

Benoit, Ehsan: thank you for the explanation. I think we should set up a system to maintain private NSPR patches in the Mozilla source tree. I did that once before, in the mozilla/nsprpub/patches directory, but that directory got removed by the NSPR import script because the NSPR import script overwrites everything under mozilla/nsprpub. One simple solution is to maintain the private NSPR patches in the mozilla/nspr-patches directory. Another solution is to modify the NSPR import script to preserve the mozilla/nsprpub/patches subdirectory.

Hmm, actually I came up with a better fix based on comment 3 outside of NSPR which catches more problems...

Comment on attachment 8337304 [details] [diff] [review] Protect against two known bad patterns for unified builds; r=gps Shouldn't unified_checks default to True? Seems like it should be a hard-fail for any case of unified sources....

(In reply to Boris Zbarsky [:bz] from comment #13) > Comment on attachment 8337304 [details] [diff] [review] > Protect against two known bad patterns for unified builds; r=gps > > Shouldn't unified_checks default to True? Seems like it should be a > hard-fail for any case of unified sources.... We can do that in another bug, I didn't want to fix up the potential DOM bindings/IPDL bustage here just yet, in the interest of landing this ASAP. Speaking of which, gps/glandium: this patch is extremely urgent for us to land, since people keep writing patches with known badness for unified builds. I'd really appreciate if you could prioritize this review. Thanks!

Comment on attachment 8337304 [details] [diff] [review] Protect against two known bad patterns for unified builds; r=gps Review of attachment 8337304 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +665,5 @@ > + '#ifdef FORCE_PR_LOG\n' > + '#error "%(cppfile)s forces NSPR logging, ' > + 'so it cannot be built in unified mode."\n' > + '#undef FORCE_PR_LOG\n' > + '#endif') Maybe this string should be extracted into a module-level variable? UNIFIED_CHECKS_INCLUDE_TEMPLATE = ''' BLAH BLAH BLAH ''' (triple ''' or """ preserve newlines and don't require escaping for nearly everything)

(In reply to comment #15) > Comment on attachment 8337304 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=8337304 > Protect against two known bad patterns for unified builds; r=gps > > Review of attachment 8337304 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: python/mozbuild/mozbuild/backend/recursivemake.py > @@ +665,5 @@ > > + '#ifdef FORCE_PR_LOG\n' > > + '#error "%(cppfile)s forces NSPR logging, ' > > + 'so it cannot be built in unified mode."\n' > > + '#undef FORCE_PR_LOG\n' > > + '#endif') > > Maybe this string should be extracted into a module-level variable? > > UNIFIED_CHECKS_INCLUDE_TEMPLATE = ''' > BLAH BLAH BLAH > ''' > > (triple ''' or """ preserve newlines and don't require escaping for nearly > everything) Can you please clarify what you mean? I am not sure if I know enough python to grok this. :-)

Landed this for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5a9bb4cd6c I will address gps' comment as a follow-up.

Sorry, backed out because of build errors: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d65b8146dbd #error : "FTPChannelChild.cpp forces NSPR logging, so it cannot be built in unified mode."

> We can do that in another bug, I didn't want to fix up the potential DOM bindings/IPDL > bustage here just yet Please do file that followup, and fix it? If DOM bindings or IPDL do either of these things I will be _very_ surprised; it would mean they include some header that defines these, which headers are Not Supposed To Do.

It's not just FTPChannelChild.cpp, it's in the makefile so it's the whole of ftp, all because in April 1999 valeski wanted it force-enabled on Windows release builds, so http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Fnetwerk%2Fprotocol%2Fftp%2Fsrc%2FAttic%2Fmakefile.win&rev=&cvsroot=%2Fcvsroot

Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e17a2ffab6 to stop the burning (if I did it right) and filed bug 943208 to stop the logging.

https://hg.mozilla.org/mozilla-central/rev/cde86e42d829 https://hg.mozilla.org/mozilla-central/rev/e6e17a2ffab6