js-confdefs.h should not export symbols like "HAVE_UNISTD_H"
Categories
(Core :: JavaScript Engine, task, P3)
Tracking
()
People
(Reporter: ptomato, Assigned: ptomato)
References
(Blocks 1 open bug)
Details
Attachments
(15 files, 1 obsolete file)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
2.00 KB,
text/plain
|
Details |
Since the fix for bug 1553938 where we include js-confdefs.h (in order to get #defines that affect the layout of public structures) we also bring in a bunch of other #defines exported by Autoconf such as HAVE_UNISTD_H. These can conflict with #defines that embedders will have in their own config.h if they use Autoconf. (At least, they should be #defined to the same value on the same system, but the compiler will complain about them being redefined)
Is there any possibility of filtering the #defines exported in js-confdefs.h?
Assignee | ||
Updated•5 years ago
|
Comment 1•5 years ago
|
||
This is hard to fix and keep it fixed. Possibilities:
-
This issue is something autoconf has coped with in the past, and there's a standard way to do it (wishful thinking, maybe)
-
Take this whole header private and stop having public structs that depend on #defines (lots of work, so pretty unlikely)
-
Rename these macros to have a
JS_
prefix (problematic: we did this once withDEBUG
so now we have bothDEBUG
andJS_DEBUG
; also note that SM sharesDEBUG
with Firefox, I guess on purpose.)
A low priority for us. We'd take a patch if there's a solid plan.
Assignee | ||
Comment 2•5 years ago
|
||
Unfortunately, the standard way that Autoconf copes with it is their guideline to "never install config.h", but that ship has sailed :-/
This is a fairly high priority for me because it means that embedders basically can't use Autoconf.
I think maybe the best solution, given the constraints you mentioned, is to add HAVE_UNISTD_H (and possibly some other Autoconf-defined ones pre-emptively) to _NON_GLOBAL_ACDEFINES here (https://searchfox.org/mozilla-central/source/build/autoconf/config.status.m4#152) which filters them out of js-confdefs.h.
However, a much more sustainable solution would be to include only defines in js-confdefs.h that do affect the layout of public structs, instead of including them all and removing them one by one as they cause problems.
Comment 3•5 years ago
|
||
Ugh, messy.
It's tempting to add a build step that does a grep -v HAVE_
. The only other one that looked bad at a glance was STDC_HEADERS. But there are probably others. And the HAVE_ thing isn't perfect, though the only one I see offhand is HAVE_64BIT_BUILD which will be used to determine the type of JitExceptionHandler on Windows.
Comment 4•5 years ago
|
||
Would it work to change the resolution of bug 1553938, and say that anything that affects the structure layout must show up in js-config.h instead?
Then we'd want the SM(pkg) job to fail if we mess it up.
Assignee | ||
Comment 5•5 years ago
|
||
Yes, that would also work. I'm not sure how we'd get the SM(pkg) job to fail though, since I'm not sure how it would be possible to catch the original mismatch that prompted bug 1553938 in an automated way.
Assignee | ||
Comment 6•5 years ago
|
||
In ESR68, HAVE_64BIT_BUILD
is used in jsfriendapi.h and mfbt/HashTable.h, and HAVE_STRNDUP
is used in mfbt/mozalloc.h.
I like sfink's suggestion best, but without some automation to catch regressions, we'd just end up in the same situation as bug 1553938 again, so I'm probably more inclined to go the _NON_GLOBAL_ACDEFINES
route.
Unless ... these defines would be included used by other parts of Firefox? I'm hoping that's not the case.
Assignee | ||
Comment 7•5 years ago
|
||
Sadly, the defines seem to be used, so we'll have to revert bug 1553938 and go with the other solution.
I've done a bit more research on which defines appear where in the esr68 branch. I've grepped all the symbols that are used in an #if
or #ifdef
in mfbt/*.h
, js/public/*.h
, or js/src/*.h
, skipping ones that are defined by the compiler or defined in another one of these header files themselves. That's not to say that all these symbols affect the layout of structs, but they do affect the contents of header files that embedders (who after the revert wouldn't be including js-confdefs.h
anymore) would see.
These symbols affect header files, are defined by configure in js-confdefs.h
, and also appear in js-config.h
, so are accessible to embedders:
JS_DEBUG
JS_GC_SMALL_CHUNK_SIZE
JS_GC_ZEAL
JS_HAS_CTYPES
JS_NUNBOX32
JS_PUNBOX64
These symbols affect header files, are defined by configure in js-confdefs.h
, but don't appear in js-config.h
, so embedders don't have them:
ENABLE_TYPED_OBJECTS
ENABLE_WASM_CRANELIFT
ENABLE_WASM_GC
EXPOSE_INTL_API
FUZZING
HAVE_64BIT_BUILD
HAVE_STRNDUP
HAVE_THREAD_TLS_KEYWORD
HAVE_VISIBILITY_ATTRIBUTE
JS_64BIT
JS_BUILD_BINAST
JS_CODEGEN_X64
JS_CODEGEN_X86
JS_JITSPEW
JS_OOM_BREAKPOINT
JS_STANDALONE
JS_TRACE_LOGGING
JS_UNALIGNED_PRIVATE_VALUES
MOZ_ASAN
MOZ_CLANG_PLUGIN
MOZ_DEBUG
MOZ_DEV_EDITION
MOZ_HAS_MOZGLUE
MOZ_UBSAN
MOZ_VALGRIND
MOZILLA_INTERNAL_API
MOZILLA_OFFICIAL
NIGHTLY_BUILD
STATIC_JS_API
XP_MACOSX
XP_UNIX
XP_WIN
These symbols affect header files, but don't seem to be defined anywhere, so the checks could likely be removed:
ENABLE_SHARED_ARRAY_BUFFER
JS_OLD_GETTER_SETTER_METHODS
JS_OOM_DO_BACKTRACES
JS_USE_CUSTOM_ALLOCATOR
- possibly this is supposed to be defined by the embedder? Does supplying a custom allocator even work?LZ4_DLL_EXPORT
- this is probably vestigial, from the vendored lz4.h header, should probably be left alone to avoid diverging from lz4LZ4_PUBLISH_STATIC_FUNCTIONS
- dittoMOZ_HAVE_BITSCAN64
- this is only checked in order to#undef
it if it's defined, maybe it can be removed?XGILL_PLUGIN
- I assume this is defined when compiling with the plugin?
Additionally, there are a few symbols that appear in js-config.h
but don't affect anything, maybe they can be removed?
JS_NO_JSVAL_JSID_STRUCT_TYPES
JS_THREADSAFE
My plan is to ensure that all the symbols in the second group above are defined in js-config.h
. That's a lot of symbols and they're not namespaced very well, for example HAVE_STRNDUP
will still conflict with embedders if they use AC_CHECK_FUNCS([strndup])
in their build files, so maybe I should start with only the ones that actually affect struct layouts.
sfink, your opinion appreciated!
Assignee | ||
Comment 8•5 years ago
|
||
This macro is not defined anywhere and has no effect in the end whether
it's defined or not.
Assignee | ||
Comment 9•5 years ago
|
||
This macro is never defined, and the functions that it hides are missing
function definitions anyway.
Depends on D51767
Assignee | ||
Comment 10•5 years ago
|
||
This macro isn't defined anywhere and doesn't seem to do anything. It
affects the oom-backtraces property of the build configuration object in
the testing functions, but since the macro is never defined, it seems to
be always set to false anyway, so just hardcode it.
Depends on D51768
Assignee | ||
Comment 11•5 years ago
|
||
This macro seems to have been introduced in
https://bugzilla.mozilla.org/attachment.cgi?id=772469&action=diff
but there doesn't seem to be any mention of it anywhere else. Currently
it's only checked in order to #undef it if it's defined.
Depends on D51769
Assignee | ||
Comment 12•5 years ago
|
||
This macro is defined when building the Rust bindings, but it doesn't
actually affect anything.
Depends on D51770
Assignee | ||
Comment 13•5 years ago
|
||
This macro is never defined and has no effect anywhere, JS_THREADSAFE
builds seem to have been removed long ago.
Depends on D51771
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
Attached a first wave of patches to remove macros that seem to be obsolete.
Here's what I've found out in the meantime about what the defines in the second group affect:
These affect struct layout in public headers:
ENABLE_WASM_CRANELIFT
(ContextOptions)ENABLE_WASM_GC
(ContextOptions)FUZZING
(ContextOptions)JS_64BIT
-> (shadow::String viaJS_BITS_PER_WORD
)JS_OOM_BREAKPOINT
(AutoEnterOOMUnsafeRegion)
These don't affect struct layout, but do affect which function prototypes are visible:
ENABLE_TYPED_OBJECTS
EXPOSE_INTL_API
(renamed toENABLE_INTL_API
in mozilla-central)JS_BUILD_BINAST
JS_JITSPEW
MOZ_DEBUG
MOZ_HAS_MOZGLUE
/MOZILLA_INTERNAL_API
(viaMOZ_HAS_JSRUST
)NIGHTLY_BUILD
These affect something else important that's publicly visible:
HAVE_64BIT_BUILD
(affects static assert in mfbt/HashTable.h)HAVE_THREAD_TLS_KEYWORD
(affects how thread local storage is implemented)HAVE_VISIBILITY_ATTRIBUTE
(affects definition ofMOZ_EXPORT
andJS_PUBLIC_API
)JS_CODEGEN_X64
/JS_CODEGEN_X86
(affectsJS_SWEPT_CODE_PATTERN
)JS_STANDALONE
(affects definition ofMFBT_API
andMFBT_DATA
)JS_TRACE_LOGGING
(if not defined then some APIs are inline no-ops so probably will conflict if the library was built with it defined)JS_UNALIGNED_PRIVATE_VALUES
(esr68 only; affects behaviour of Value)
These are probably OK to ignore and don't have to go in js-config.h:
MOZ_ASAN
/MOZ_UBSAN
/MOZ_VALGRIND
(although maybe embedders should define them when running asan/ubsan/valgrind on their own code?)MOZ_CLANG_PLUGIN
MOZ_DEV_EDITION
MOZILLA_OFFICIAL
(only affects whetherMOZ_DBG
is available)STATIC_JS_API
(affects definition ofJS_PUBLIC_API
etc but embedders probably shouldn't use this)
Not sure about XP_MACOSX
/XP_UNIX
/XP_WIN
, they seem to be definitely needed for stuff, but also... they seem to get defined somehow anyway?
Assignee | ||
Comment 15•5 years ago
|
||
In order to not depend on the preprocessor macros that JS was built with
being defined in public headers, use sizeof(void*) in this static assert
to determine whether pointers are 8 bytes or not.
Depends on D51772
Assignee | ||
Comment 16•5 years ago
|
||
We should have the same public API available whenever possible, and make
it a no-op or make it throw immediately if JS was built without support
for it, instead of showing or hiding the API in header files using
configure macros. Otherwise embedders can easily get mismatches between
a library with functionality and header files without it, or vice versa.
Depends on D52123
Assignee | ||
Comment 17•5 years ago
|
||
If only relying on JS_64BIT to determine whether the system is 64-bits,
then the result will be incorrect when the header is installed as a
public header for use by embedders, and since JS_BITS_PER_WORD affects
the layout of structs in header files, things will go badly wrong.
This uses two other ways of determining pointer width, hopefully
cross-platform enough. SIZEOF_POINTER is a GCC-ism and probably
works in Clang as well. UINTPTR_MAX is hopefully sufficiently
cross-platform as a last resort.
Depends on D52124
Assignee | ||
Comment 18•5 years ago
|
||
These are configure macros that start with JS_ and have an effect on the
public API declared in JSAPI header files, so they should be included in
the installed js-config.h header file.
Depends on D52458
Assignee | ||
Comment 19•5 years ago
|
||
Previously, if SpiderMonkey embedders linked to a copy of libmozjs built
with --enable-cranelift, --enable-wasm-gc, or --enable-fuzzing, then the
size of the ContextOptions data structure declared in the header file
would be different than the size of ContextOptions in the library,
likely leading to crashes. This makes all members of ContextOptions
independent of preprocessor macros. Any options not compiled into
SpiderMonkey will still be no-ops.
Depends on D52459
Assignee | ||
Comment 20•5 years ago
|
||
Whether ENABLE_INTL_API and ENABLE_TYPED_OBJECTS are defined, affects
the behaviour of JS_FOR_PROTOTYPES for the prototypes of Intl and
TypedObject. Therefore, these macros have to be available to embedders.
Rename them to JS_HAS_INTL_API and JS_HAS_TYPED_OBJECTS (in line with
the existing JS_HAS_CTYPES) everywhere they are used, and add them to
js-config.h.in.
Depends on D52460
Assignee | ||
Comment 21•5 years ago
|
||
It turns out that we don't actually want to install js-confdefs.h
because it contains macro definitions that can conflict when embedders
include JSAPI headers in their Autotools projects.
Depends on D52461
Assignee | ||
Comment 22•5 years ago
|
||
This should specifically prevent bug 1553938 from happening in the
future. Unfortunately it won't prevent other similar bugs from
happening.
Depends on D52462
Assignee | ||
Comment 23•5 years ago
|
||
OK, I think this is the best we can do for now. Any thoughts on how to programmatically enforce
The only patches out of this series that would really need to be uplifted to esr68 are:
- "Determine 64-bit in public JS header without configure macro", to ensure
JS_BITS_PER_WORD
is defined; - "Include JS_FOO macros that influence public API in js-config.h", taking care to include
JS_UNALIGNED_PRIVATE_VALUES
andJS_CODEGEN_FOO
in there even though those aren't needed on central; - "Revert bug 1553938".
It's debatable whether to uplift "Make ENABLE_INTL_API and ENABLE_TYPED_OBJECTS into js-config macros", they do break the behaviour of JS_FOR_PROTOTYPES
for embedders, but I'd say for the amount of churn that patch introduces (and the amount of tedium to backport it since ENABLE_INTL_API
is EXPOSE_INTL_API
on esr68) that breakage is probably minor enough and can be worked around.
Comment 24•5 years ago
|
||
Comment 25•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fee367f20825
https://hg.mozilla.org/mozilla-central/rev/cee3ca35e416
https://hg.mozilla.org/mozilla-central/rev/bedd96446c98
https://hg.mozilla.org/mozilla-central/rev/b98e3a2058c0
https://hg.mozilla.org/mozilla-central/rev/41cd25e887f0
https://hg.mozilla.org/mozilla-central/rev/35fb1e29016b
Assignee | ||
Comment 26•5 years ago
|
||
That was only half the patches, reopening.
Updated•5 years ago
|
Assignee | ||
Comment 27•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 28•5 years ago
|
||
Comment on attachment 9107685 [details]
Bug 1590907 - Determine 64-bit in public JS header without configure macro. r?jwalden
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: These patches remove preprocessor dependence from public SpiderMonkey header files. This is intended fix the recurring problem of embedders using installed SpiderMonkey header files and not having the same preprocessor macros defined.
I'm requesting uplift for as few patches as possible, operating under the assumption that uplifts should only fix serious bugs on ESR68, but below are potential reasons why some others of the remaining patches might be considered for uplift:
- "Remove obsolete macro" patches - these don't fix anything, they are harmless cleanups of unused code.
- "Stop configure macros from masking function prototypes in public JS headers" - this is fairly invasive, and embedders wouldn't have been able to use these APIs before anyway because they wouldn't have been visible in the public headers, so I think it's OK to leave it be.
- "Remove preprocessor dependence from size of ContextOptions" - this still leaves a potential bug for embedders on ESR68, but it seems unlikely that embedders are using SpiderMonkey builds with the affected options.
- "Make ENABLE_INTL_API and ENABLE_TYPED_OBJECTS into js-config macros" - this also still leaves a potential bug for embedders on ESR68, but it only affects the behaviour of JS_FOR_EACH_PROTOTYPE which is quite minor, and since the ENABLE_INTL_API option is called EXPOSE_INTL_API on mozilla-esr68 it would be quite tedious to backport, so I think it's minor enough to leave it be.
- "Add safeguard when JS_BITS_PER_WORD affects struct layout" - this doesn't fix anything, it just adds an extra compile-time safeguard in case someone forgets to include js-config.h.
- User impact if declined: Embedders would need to build SpiderMonkey with these or similar patches and the patches would need to be carried by downstream Linux distributions.
- Fix Landed on Version: 72
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): If these patches were to cause any regressions, they'd most likely be obvious compile-time errors, or far less likely obvious crashes such as the one in bug 1553938.
- String or UUID changes made by this patch: None
Assignee | ||
Updated•5 years ago
|
Comment 29•5 years ago
|
||
Comment 30•5 years ago
|
||
Failed to land, please rebase:
On Tue, November 26, 2019, 2:37 AM GMT+2, by ccoroiu@mozilla.com.
Revisions: D52124 diff 197869 ← D52458 diff 197870 ← D52459 diff 197871 ← D52460 diff 197872 ← D52461 diff 197873 ← D52462 diff 197874 ← D52463 diff 197875
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpXW9Y8d js/src/vm/OffThreadScriptCompilation.cpp Hunk #1 FAILED at 203. 1 out of 1 hunk FAILED -- saving rejects to file js/src/vm/OffThreadScriptCompilation.cpp.rej js/src/jsapi.h Hunk #4 FAILED at 1943. 1 out of 4 hunks FAILED -- saving rejects to file js/src/jsapi.h.rej js/src/jsapi.cpp Hunk #3 FAILED at 3660. Hunk #4 FAILED at 4584. 2 out of 4 hunks FAILED -- saving rejects to file js/src/jsapi.cpp.rej js/public/OffThreadScriptCompilation.h Hunk #1 FAILED at 131. 1 out of 1 hunk FAILED -- saving rejects to file js/public/OffThreadScriptCompilation.h.rej abort: patch command failed: exited with status 256
Comment 32•5 years ago
|
||
Comment 33•5 years ago
|
||
Backed out 7 changesets (Bug 1590907) for build bustages (hazard) at jsfriendapi.cpp.
https://hg.mozilla.org/integration/autoland/rev/98fd7a2227453a52213c0345605958f29a2a7f7b
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=278150981&repo=autoland&lineNumber=4171
Assignee | ||
Comment 34•5 years ago
|
||
Apparently some APIs changed between the time I rebased it and the time it landed... I've rebased all the patches again and fixed the breakage. Here's a try run with the three jobs that failed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb99bca14aed596d918454e92b54ee0db351a723
Comment 35•5 years ago
|
||
Comment 36•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2f5288e7e77e
https://hg.mozilla.org/mozilla-central/rev/1fcd2dd27766
https://hg.mozilla.org/mozilla-central/rev/2e8022ea6b75
https://hg.mozilla.org/mozilla-central/rev/965e57bb64a3
https://hg.mozilla.org/mozilla-central/rev/bfdd5f34f2f4
https://hg.mozilla.org/mozilla-central/rev/06611450ed60
https://hg.mozilla.org/mozilla-central/rev/454ac96ed988
Assignee | ||
Comment 37•5 years ago
|
||
Here's a backport of one patch to ESR68, which needs to include an extra option.
The other patches requested for backport I think should apply cleanly.
Assignee | ||
Updated•5 years ago
|
Comment 38•5 years ago
|
||
Comment on attachment 9107685 [details]
Bug 1590907 - Determine 64-bit in public JS header without configure macro. r?jwalden
Various SpiderMonkey build fixes to make life better for embedders. No affect on Mozilla-produced builds. Approved for 68.4esr.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 39•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr68/rev/3d4060cdea68
https://hg.mozilla.org/releases/mozilla-esr68/rev/11af3f9251b5
https://hg.mozilla.org/releases/mozilla-esr68/rev/085c965a7830
https://hg.mozilla.org/releases/mozilla-esr68/rev/f79bfc535dd9
Does this bug still need to be open?
Assignee | ||
Comment 40•5 years ago
|
||
No need to be open, I think it's finally resolved! thanks all
Updated•5 years ago
|
Description
•