665819 - tracemonkey build fails when ENABLE_YARR_JIT disabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Paprocki]

Attachment: 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.

Comment 1

[David Mandelin [:dmandelin]]

(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?

Comment 2

[Andrew Paprocki]

Attachment: Add #if ENABLE_YARR_JIT to appropriate places.

Comment 3

[David Mandelin [: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.

Comment 4

[Andrew Paprocki]

> 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.

Comment 5

[David Mandelin [:dmandelin]]

(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.

Comment 6

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/691294843828