Closed Bug 909576 Opened 12 years ago Closed 5 years ago

Gecko doesn't include js/RequiredDefines.h everywhere it should

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: n.nethercote, Unassigned)

Details

(Whiteboard: [js:t])

js/RequiredDefines.h contains this comment: "Embedders should add this file to the start of the command line via -include or a similar mechanism, or SpiderMonkey public headers may not work correctly." Gecko currently violates this -- only .cpp files under js/src/ include it (via js-confdefs.h, which is force-included via |-include|). But any Gecko .cpp file that includes a public JS header should include it. The violation currently doesn't cause problems because js/RequiredDefines.h only defines __STDC_LIMIT_MACROS, and mozilla-config.h.in also does that. But if we add anything substantial to js/RequiredDefines.h, problems occur (e.g. see bug 909171 comment 8). AFAICT, the easiest way to do this is to include js/RequiredDefines.h from mozilla-config.h.
Could we avoid the whole -include requirement and just do this ourselves manually (that is ensure that every JS .h transitively begins with #include RequiredDefines.h. I expect this would be easy to check in check-style. I was thinking this would be preferable to #including RequiredDefines.h in every .cpp in Gecko esp. when we start considering sticking more #includes in RequiredDefines (as we were discussing in bug 909171).
That doesn't work. If Gecko (say) includes one of our headers after <stdint.h> is (transitively) included, __STDC_LIMIT_MACROS won't take effect, and the limit macros won't be exposed. Why is requiring people to smack in the #include first thing using -include or similar not good enough for this?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2) > That doesn't work. If Gecko (say) includes one of our headers after > <stdint.h> is (transitively) included, __STDC_LIMIT_MACROS won't take > effect, and the limit macros won't be exposed. Is there anything other than __STDC_LIMIT_MACROS that has this special magic? Because just requiring -D __STDC_LIMIT_MACROS sounds more direct. We can #ifndef __STDC_LIMIT_MACROS # error "You forgot to define __STDC_LIMIT_MACROS" #endif to catch people forgetting to do this. > Why is requiring people to smack in the #include first thing using -include > or similar not good enough for this? -include is weird because it doesn't appear in the code; it involves build gunk. Also, as I said in comment 1: if we -include js/RequiredDefines.h in every Gecko .cpp, then it will be too broad to be the right place to stick "stuff #included in every JS translation unit, like jstypes.h/jsversion.h/etc", we'll need a different header for that narrower purpose.
Having to rebuild everything in mozilla-central because you changed something that applies just to the JS engine really sucks. Much of Gecko does not even depend on JS, let alone things like mozglue, our third party media libraries, sqlite, etc.
> Is there anything other than __STDC_LIMIT_MACROS that has this special > magic? Because just requiring -D __STDC_LIMIT_MACROS sounds more direct. > We can > #ifndef __STDC_LIMIT_MACROS > # error "You forgot to define __STDC_LIMIT_MACROS" > #endif > to catch people forgetting to do this. So the instructions for embedders would change from "you must -include RequiredDefines.h" to "you must -D __STDC_LIMIT_MACROS"... which is a bit weird. Taking a step back... jstypes.h is the closest thing we have to a "header which is always included by any code that depends on SpiderMonkey headers". In fact, because jstypes.h defines JS_PUBLIC_API, I can't see how it's possible for such code to not transitively include jstypes.h. So I was going to suggest folding RequiredDefines.h into jstypes... but again, ensuring that it's always included before <stdint.h> is tricky...
(In reply to Nicholas Nethercote [:njn] from comment #5) More pragmatically, we could have: // RequiredDefines.h #define __STDC_LIMIT_MACROS #include <stdint.h> #ifndef UINT32_MAX # error "<stdint.h> included before JS header without defining __STDC_LIMIT_MACROS first." "Either #define __STDC_LIMIT_MACROS above the #include <stdint.h> or compile with " "__STDC_LIMIT_MACROS defined globally with a compiler flag." #endif So in the normal case it Just Works and in the abnormal case the embedder is told how to fix their code. Any downside to this?
By the current rules, <stdint.h> should always come before js/RequiredDefines.h or any SM header that includes js/RequiredDefines.h. So the #error would always trigger. We could have a rule "all SM headers must include RequiredDefines.h first". That'd solve it, albeit brutally.
(In reply to Nicholas Nethercote [:njn] from comment #7) It'd be annoying to refactor for sure, but then we'd have a nice header that we knew always precedes other SM headers both internally and externally w/o these -include build issues. From there, we could fold in jstypes.h and jsversion.h and have 1 instead of 3, which is all I'm really after :)
(In reply to Luke Wagner [:luke] from comment #3) > Is there anything other than __STDC_LIMIT_MACROS that has this special > magic? Because just requiring -D __STDC_LIMIT_MACROS sounds more direct. I had patches in bug 730805 that would have added __STDC_CONSTANT_MACROS and __STDC_FORMAT_MACROS as well. The very point of the header was to be able to consolidate all these things in one place, rather than expand a growing list of them. > We can > #ifndef __STDC_LIMIT_MACROS > # error "You forgot to define __STDC_LIMIT_MACROS" > #endif > to catch people forgetting to do this. That doesn't really solve the problem -- it'll make people think that they need to #define it before any JS header, but doing so won't actually make it work *unless* no previous header includes <stdint.h>. Your extra-proposed hackaround of including, then testing for UINT32_MAX being defined would avoid that problem. But then you run into the issue that b2g's commandline has an -include in it, that transitively drags in <stdint.h>. It's also worth noting that Gecko already has a globally -include'd file. That makes at least three across the tree. I don't think your crusade to erase -include is going to work. > -include is weird because it doesn't appear in the code; it involves build > gunk. Sure, in an ideal world. But this is Gecko and SpiderMonkey. I don't think it's going to work. > Also, as I said in comment 1: if we -include js/RequiredDefines.h in > every Gecko .cpp, then it will be too broad to be the right place to stick > "stuff #included in every JS translation unit, like > jstypes.h/jsversion.h/etc", we'll need a different header for that narrower > purpose. I don't think we should have an everything-header, partly because it induces precisely the problem njn's trying to mitigate. But if we were to have one, yes, it would need to differ from js/RequiredDefines.h.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9) Comment 3 is so two days ago; comment 6 + 7 is the current proposal.
> But then you run into the issue that b2g's commandline > has an -include in it, that transitively drags in <stdint.h>. Urk, that makes things more difficult. I think that would even defeat the brutal "every SM header includes RequiredDefines.h first" approach, because -include headers always precede #include headers. BTW, how do we ensure that the -include of RequiredDefines.h occurs before the -include of the b2g file? Is it just the order they're presented on the command line? (Can you point me to the relevant part of the code?)
Oops, I missed that relevant part of Waldo's comment. Ack, this is why -include and build gunk is bad. For the b2g code, though, it seems like we could still #define __STDC_LIMIT_MACROS before the offending <stdint.h> #include or, if nothing else -D __STDC_LIMIT_MACROS.
(In reply to Nicholas Nethercote [:njn] from comment #11) > BTW, how do we ensure that the -include of RequiredDefines.h occurs before > the -include of the b2g file? Is it just the order they're presented on the > command line? It's the order in the command line. It's possible that js/RequiredDefines.h gets included earlier because of its include via js-confdefs.h, which might or might not precede that -include. I'm really not sure offhand. The issue, as I recall, manifested in Gecko code -- it wasn't js/RequiredDefines.h that was at fault, particularly, as best as I recall, it was that mozilla-config.h wasn't included early enough. > (Can you point me to the relevant part of the code?) See the patch in bug 812218. Really we should just fix b2g -- reordering the user-provided CXXFLAGS to the end of the command line as that patch in bug 812218 did. It foundered on the Windows PGO rocks, only, at the time. A couple months ago I tried it again, and it foundered on other, different rocks (not sure if additional rocks, or different rocks entirely). I imagine that problem is fairly easily solved, if someone can find the time to investigate it. (In reply to Luke Wagner [:luke] from comment #12) > For the b2g code, though, it seems like we could still #define > __STDC_LIMIT_MACROS before the offending <stdint.h> #include The include was (I think) a system header, so I don't think modification's possible. > or, if nothing else -D __STDC_LIMIT_MACROS. I *think* this applies after user-specified CXXFLAGS, so you'd have to hack it into some global spot, or something. But it's been awhile. Really, fixing CXXFLAGS include ordering so this isn't a problem is what should be done.
Product: Core → Firefox Build System

No action in 7 years, closing.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.