(no longer active) Assignee
	Description • 11 years ago

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.

	(no longer active) Assignee
	Comment 2 • 11 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

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.

	(no longer active) Assignee
	Comment 4 • 11 years ago

(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.

	Wan-Teh Chang
	Comment 5 • 11 years ago

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.

	Benoit Girard (:BenWa)
	Comment 6 • 11 years ago

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.

	(no longer active) Assignee
	Comment 7 • 11 years ago

(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.

	Trevor Saunders (:tbsaunde)
	Comment 8 • 11 years ago

(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

	(no longer active) Assignee
	Comment 9 • 11 years ago

(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...

	Wan-Teh Chang
	Comment 10 • 11 years ago

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.

	(no longer active) Assignee
	Comment 11 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

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....

	(no longer active) Assignee
	Comment 14 • 11 years ago

(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!

	Gregory Szorc [:gps]
	Comment 15 • 11 years ago

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)

	(no longer active) Assignee
	Comment 16 • 11 years ago

(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.  :-)

	(no longer active) Assignee
	Comment 17 • 11 years ago

Landed this for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5a9bb4cd6c

I will address gps' comment as a follow-up.

	Matt Brubeck (:mbrubeck)
	Comment 18 • 11 years ago

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

	(no longer active) Assignee
	Comment 19 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cde86e42d829

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 11 years ago

> 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.

	Phil Ringnalda (:philor)
	Comment 21 • 11 years ago

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

	Phil Ringnalda (:philor)
	Comment 22 • 11 years ago

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.

	Carsten Book [:Tomcat]
	Comment 23 • 11 years ago

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