941854 - Protect against two known bad patterns for unified builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

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 1

[(no longer active)]

Attachment: Protect against two known bad patterns for unified builds; r=wtc

Comment 2

[(no longer active)]

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!

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[(no longer active)]

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

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Benoit Girard (:BenWa)]

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.

Comment 7

[(no longer active)]

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

Comment 8

[Trevor Saunders (:tbsaunde)]

(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

Comment 9

[(no longer active)]

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

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[(no longer active)]

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

Comment 12

[(no longer active)]

Attachment: Protect against two known bad patterns for unified builds; r=gps

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[(no longer active)]

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

[Gregory Szorc [:gps]]

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)

Comment 16

[(no longer active)]

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

Comment 17

[(no longer active)]

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

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

Comment 18

[Matt Brubeck (:mbrubeck)]

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

Comment 19

[(no longer active)]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Phil Ringnalda (:philor)]

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

Comment 22

[Phil Ringnalda (:philor)]

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.

Comment 23

[Carsten Book [:Tomcat]]

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