Protect against two known bad patterns for unified builds

RESOLVED FIXED in mozilla28

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla28
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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!
Attachment #8336348 - Flags: review?(wtc)
Depends on: 941866
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.
Depends on: 940215
(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...
Component: NSPR → Build Config
Product: NSPR → Core
Version: other → Trunk
Attachment #8336348 - Attachment is obsolete: true
Attachment #8336348 - Flags: review?(wtc)
Attachment #8337304 - Flags: review?(gps)
Attachment #8337304 - Flags: review?(mh+mozilla)
Depends on: 942489
Depends on: 942486
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!
Attachment #8337304 - Flags: review?(mh+mozilla)
Attachment #8337304 - Flags: review?(gps)
Attachment #8337304 - Flags: review+
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)
Attachment #8337304 - Flags: review+
(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.  :-)
Flags: needinfo?(gps)
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
Depends on: 943208
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 943554
Flags: needinfo?(gps)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.