1432378 - MacroAssembler::makeFrameDescriptor and MacroAssembler::call(TrampolinePtr) are defined in MacroAssembler-inl.h but used from SharedICHelpers.h, rather than only from .cpp/-inl.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jeff Walden [:Waldo]]

In certain build configurations of SpiderMonkey -- namely, apply a patch that sets |FILES_PER_UNIFIED = 1| -- an #include of jit/SharedICHelpers.h will define inline functions that *use* MacroAssembler::{makeFrameDescriptor,call(TrampolinePtr)} without jit/MacroAssembler-inl.h being #include'd to define it.  In GCC this triggers used-before-defined warnings that turn into errors with -Werror.

This requires the usual fix -- move these offending definitions to a SharedICHelpers-inl.h header.  Bit messier because of the need to do this for N different architectures 'cause it's JIT, but the basic idea still applies.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Jeff Walden [:Waldo]]

For an example of this on treeherder, see https://treeherder.mozilla.org/logviewer.html#?job_id=157661875&repo=try&lineNumber=1977 (at least for as long as the job's artifacts are preserved).  I'm not sure it's necessary to upload them here for permanent preservation, nor to bundle up a patchset that triggers the error (not sure if *only* the change mentioned in comment 0 is required, or if some of my current patchwork would also have to be included as well -- I only tested with portions of both in place).

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8944643 [details] [diff] [review]
Patch

Review of attachment 8944643 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for using "hg cp" before removing lines from the -inl.h files.
And yes, this is the proper way to fix this issue, even if this creates more files.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/289c8202444a
Fix used-but-not-defined errors for MacroAssembler::makeFrameDescriptor and MacroAssembler::call(TrampolinePtr) by moving the uses-from-inlines-in-.h headers to be in a new *-inl.h header, then #include-ing it in files that need those functions.  r=nbp

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/289c8202444a

Comment 6

[Dragan Mladjenovic]

Attachment: mips_fix.patch

Tiny fix that fixes build on mips after previous patch.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8946195 [details] [diff] [review]
mips_fix.patch

Review of attachment 8946195 [details] [diff] [review]:
-----------------------------------------------------------------

.h files are not supposed to include the -inl.h files.

Comment 8

[Dragan Mladjenovic]

Attachment: mips_fix2.patch

Sorry for the late replay.

I kinda missed that this issue has been already closed at the time of may posting.
This new fix pulls the needed definitions back from *-inl.h. Is it necessary for me to open a new issue for this patch?

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8946582 [details] [diff] [review]
mips_fix2.patch

Review of attachment 8946582 [details] [diff] [review]:
-----------------------------------------------------------------

If you want to open a new bug, this would be ideal, in which case mark the new bug as a blocking this bug.