Last Comment Bug 665819 - tracemonkey build fails when ENABLE_YARR_JIT disabled
: tracemonkey build fails when ENABLE_YARR_JIT disabled
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla8
Assigned To: Andrew Paprocki
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-20 18:50 PDT by Andrew Paprocki
Modified: 2011-07-12 03:53 PDT (History)
3 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add #if ENABLE_YARR_JIT to appropriate places. (1.47 KB, patch)
2011-06-20 18:50 PDT, Andrew Paprocki
no flags Details | Diff | Review
Add #if ENABLE_YARR_JIT to appropriate places. (2.57 KB, patch)
2011-06-21 13:07 PDT, Andrew Paprocki
dmandelin: review+
Details | Diff | Review

Description Andrew Paprocki 2011-06-20 18:50:57 PDT
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.
Comment 1 David Mandelin [:dmandelin] 2011-06-21 11:24:58 PDT
(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 2011-06-21 13:07:14 PDT
Created attachment 540851 [details] [diff] [review]
Add #if ENABLE_YARR_JIT to appropriate places.
Comment 3 David Mandelin [:dmandelin] 2011-06-21 13:42:37 PDT
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 2011-06-22 17:20:01 PDT
> 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] 2011-06-23 11:00:08 PDT
(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) 2011-07-12 03:53:44 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/691294843828

Note You need to log in before you can comment on or make changes to this bug.