The default bug view has changed. See this FAQ.

tracemonkey build fails when ENABLE_YARR_JIT disabled

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Andrew Paprocki, Assigned: Andrew Paprocki)

Tracking

Trunk
mozilla8
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 540655 [details] [diff] [review]
Add #if ENABLE_YARR_JIT to appropriate places.

Current tracemonkey tip fails to build if all JIT (specifically ENABLE_YARR_JIT) is disabled. Linking the shell failed unless ENABLE_ASSEMBLER was removed from the #if list in OSAllocatorPosix.cpp. I'm not sure if anything different is desired, but it builds fine now with these changes.
(In reply to comment #0)
> Created attachment 540655 [details] [diff] [review] [review]
> Add #if ENABLE_YARR_JIT to appropriate places.

Patch looks sensible. Would you mind reposting with 8 lines of context (e.g., "hg diff -p -U 8", you can also set this in .hgrc) to make it easier to review?
(Assignee)

Comment 2

6 years ago
Created attachment 540851 [details] [diff] [review]
Add #if ENABLE_YARR_JIT to appropriate places.
Attachment #540655 - Attachment is obsolete: true
Attachment #540851 - Flags: review?(dmandelin)
Comment on attachment 540851 [details] [diff] [review]
Add #if ENABLE_YARR_JIT to appropriate places.

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

::: js/src/jsregexpinlines.h
@@ +492,2 @@
>      codeBlock.setFallBack(true);
> +#endif

Please merge this #if block with the one just above.

::: js/src/yarr/OSAllocatorPosix.cpp
@@ +28,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "assembler/wtf/Platform.h"
>  
> +#if WTF_OS_UNIX && !WTF_OS_SYMBIAN

This change surprised me, so I want to make sure it's right. Why is it needed? Specifically, where is this file getting included and what is it needed for other than the assembler?

If it is needed, I think the same change should be made to OSAllocatorWin.cpp.
Attachment #540851 - Flags: review?(dmandelin)
(Assignee)

Comment 4

6 years ago
> Please merge this #if block with the one just above.

This should not be in that block because it needs to be called even when JS_METHODJIT is not defined. If you look back around line 367, isFallBack() is checked when just ENABLE_YARR_JIT.

> This change surprised me, so I want to make sure it's right.

Without removing the ENABLE_ASSEMBLER, the link fails like this:

YarrInterpreter.o: In function `WTF::PageAllocation::allocate(unsigned long, WTF::OSAllocator::Usage, bool, bool)':
js/src/yarr/PageAllocation.h:104: undefined reference to `WTF::OSAllocator::reserveAndCommit(unsigned long, WTF::OSAllocator::Usage, bool, bool)'

Simply removing the ENABLE_ASSEMBLER satisfies the symbol and the link succeeds.
(In reply to comment #4)

Thanks for the clarifications.

> > Please merge this #if block with the one just above.
> 
> This should not be in that block because it needs to be called even when
> JS_METHODJIT is not defined. If you look back around line 367, isFallBack()
> is checked when just ENABLE_YARR_JIT.

OK. I had missed the fact that the #ifs weren't testing exactly the same thing. Something still seems a bit weird here, but it's my fault for not coming up with a clear definition of exactly how the enabling flags are supposed to be related, so I'm not going to ask you to sort it out right now--let's just get it working. (But feel free if you want!)

> > This change surprised me, so I want to make sure it's right.
> 
> Without removing the ENABLE_ASSEMBLER, the link fails like this:
> 
> YarrInterpreter.o: In function `WTF::PageAllocation::allocate(unsigned long,
> WTF::OSAllocator::Usage, bool, bool)':
> js/src/yarr/PageAllocation.h:104: undefined reference to
> `WTF::OSAllocator::reserveAndCommit(unsigned long, WTF::OSAllocator::Usage,
> bool, bool)'
> 
> Simply removing the ENABLE_ASSEMBLER satisfies the symbol and the link
> succeeds.

OK, I think is for the bump allocator used by the interpreter, so that makes sense.
Attachment #540851 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Assignee: general → andrew
Pushed:
http://hg.mozilla.org/mozilla-central/rev/691294843828
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.