750694 - halfmoon does not compile on windows

Status: RESOLVED FIXED

Description

[Brent Baker]

Compiling halfmoon enabled shell on windows and windows64 fails with the following error:

nanojit\Allocator.h(126) : error C2512: 'halfmoon::TypeKey' : no appropriate default constructor available
build/rules.mk:62: recipe for target `halfmoon/hm-instrgraph.obj' failed

This happens either with the current tip of tamarin and using the --enable-halfmoon switch, or using the initial checkin of the halfmoon code (rev 7316:a6cded0538e7) and adding -DAVMFEATURE_HALFMOON=1 to the Makefile

Comment 1

[Tamarin Bot]

changeset: 7385:7324cae3a421
user:      Brent Baker <[e-mail]>
summary:   Bug 750694: do not try to compile halfmoon on windows in the deep phase of the build as it currently doesn't compile and is causing the build to fail (r+brbaker)

http://hg.mozilla.org/tamarin-redux/rev/7324cae3a421

Comment 2

[Brent Baker]

Bump, this needs to be fixed so that we can start building and testing the halfmoon code in the sandbox and in the main tamarin-redux Jenkins builders.

Comment 3

[William Maddox]

Created attachment 648188 [details] [diff] [review]
Fixes for win32 and linux32 builds

Work in progress -- builds ok in local builds of 32-bit Windows, Linux, MacOSX.

Comment 4

[William Maddox]

Created attachment 648596 [details] [diff] [review]
Build fixes for Win32, Linux, and ARM-architecture platforms

This is a bit unpolished and some follow-up is warranted, but I'd like to get this going in automated builds before messing around with it anymore.

Comment 5

[Brent Baker]

(In reply to William Maddox from comment #4)
> Created attachment 648596 [details] [diff] [review]
> Build fixes for Win32, Linux, and ARM-architecture platforms
> 
> This is a bit unpolished and some follow-up is warranted, but I'd like to
> get this going in automated builds before messing around with it anymore.

Patch is really close to compiling release and debug-debugger on all platforms. These are the current holdouts (Android and WindowsRT):

** Android: debug-debugger build **
cc1plus: warnings being treated as errors
../halfmoon/hm-typeanalyzer.cpp: In function 'bool halfmoon::isCastCall(halfmoon::Lattice*, const halfmoon::Type*, uint32_t, const halfmoon::Type**)':
../halfmoon/hm-typeanalyzer.cpp:627: error: logical '&&' with non-zero constant will always evaluate as true
cc1plus: warnings being treated as errors
../halfmoon/hm-check.cpp: In member function 'bool halfmoon::TypeChecker::do_default(halfmoon::Instr*)':
../halfmoon/hm-check.cpp:245: error: logical '&&' with non-zero constant will always evaluate as true

** WindowsRT **
../halfmoon/hm-deoptimizer.cpp(522) : error C2220: warning treated as error - no 'object' file generated
../halfmoon/hm-deoptimizer.cpp(522) : warning C4244: 'return' : conversion from 'nanojit::NIns' to 'uint8_t', possible loss of data
../build/rules.mk:62: recipe for target `halfmoon/hm-deoptimizer.obj' failed

Comment 6

[Tom Rodriguez]

In NativeARM.cpp, there isn't really a bug but the optimizer isn't able to figure it out.  dm may be uninitialzed if !qwordPrecision and !ARM_VFP and there's an assert that guards against case, so that path should never be reached.  Initializing it seems ok to fix the warning but I think you should drop the comment.  Any reason not to simply delete the FIXME stuff in Deopt-CL.cpp?  Otherwise it looks like.  Even without the changes it looks ok.

Comment 7

[Tom Rodriguez]

(In reply to Brent Baker from comment #5)
> (In reply to William Maddox from comment #4)
> > Created attachment 648596 [details] [diff] [review]
> > Build fixes for Win32, Linux, and ARM-architecture platforms
> > 
> > This is a bit unpolished and some follow-up is warranted, but I'd like to
> > get this going in automated builds before messing around with it anymore.
> 
> Patch is really close to compiling release and debug-debugger on all
> platforms. These are the current holdouts (Android and WindowsRT):
> 
> ** Android: debug-debugger build **
> cc1plus: warnings being treated as errors
> ../halfmoon/hm-typeanalyzer.cpp: In function 'bool
> halfmoon::isCastCall(halfmoon::Lattice*, const halfmoon::Type*, uint32_t,
> const halfmoon::Type**)':
> ../halfmoon/hm-typeanalyzer.cpp:627: error: logical '&&' with non-zero
> constant will always evaluate as true

This is trying to null test a GCMember which is an object.  Converting it to != NULL would probably make the message go away.

> cc1plus: warnings being treated as errors
> ../halfmoon/hm-check.cpp: In member function 'bool
> halfmoon::TypeChecker::do_default(halfmoon::Instr*)':
> ../halfmoon/hm-check.cpp:245: error: logical '&&' with non-zero constant
> will always evaluate as true

This is trying to null check a reference, so it probably needs to be &use != NULL.

> 
> ** WindowsRT **
> ../halfmoon/hm-deoptimizer.cpp(522) : error C2220: warning treated as error
> - no 'object' file generated
> ../halfmoon/hm-deoptimizer.cpp(522) : warning C4244: 'return' : conversion
> from 'nanojit::NIns' to 'uint8_t', possible loss of data

This should just be cast to uint8_t for the moment.  There's a big abuse of NIns going here as noted in the comments.

  // Pointer to next byte to be written.
  // TODO: Fix abuse of NIns*.
  NIns* mdins_;

I can give these a try in the shelf builder

> ../build/rules.mk:62: recipe for target `halfmoon/hm-deoptimizer.obj' failed

Comment 8

[Tom Rodriguez]

Created attachment 648780 [details] [diff] [review]
android and windowsrt fixes

The android build is done with these fixes but windows rt is still working.  it should be done soon.

Comment 9

[Tom Rodriguez]

(In reply to Tom Rodriguez from comment #8)
> Created attachment 648780 [details] [diff] [review]
> android and windowsrt fixes
> 
> The android build is done with these fixes but windows rt is still working. 
> it should be done soon.

Actually I suspect it didn't build it right since it will require some configuration changes to build halfmoon by default, which I don't have.  So the patch should be right but it will require confirmation.

Comment 10

[Brent Baker]

(In reply to Tom Rodriguez from comment #9)
> Actually I suspect it didn't build it right since it will require some
> configuration changes to build halfmoon by default, which I don't have.  So
> the patch should be right but it will require confirmation.

I tweaked the sandbox for you change and resubmitted the build. It looks good but I have to wait for the WIndowsRT build to finish another build to know for certain that it is compiling cleanly. I'll inform Dan Schaffer about the change that I made so that he can revert it once your build is complete (and later commit the change once the code fix is checked in)

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 648780 [details] [diff] [review]
android and windowsrt fixes

(swipe; I think even I can vouch for these changes.)

Comment 12

[Brent Baker]

(In reply to Brent Baker from comment #10)
> I tweaked the sandbox for you change and resubmitted the build. It looks
> good but I have to wait for the WIndowsRT build to finish another build to
> know for certain that it is compiling cleanly. I'll inform Dan Schaffer
> about the change that I made so that he can revert it once your build is
> complete (and later commit the change once the code fix is checked in)

Update: Halfmoon compilation of release and debug-debugger has completed on all supported platforms with the latest code changes.

Comment 13

[Tom Rodriguez]

Fixed as CL 1096510.

Comment 15

[Tamarin Bot]

changeset: 7523:5b15da1e8285
user:      Tom Rodriguez <[e-mail]>
summary:   750694: halfmoon does not compile on windows (p=wmaddox, r=fklockii, r=throdrig)

http://hg.mozilla.org/tamarin-redux/rev/5b15da1e8285

Comment 16

[Dan Schaffer]

I enabled compiling halfmoon in jenkins for all shell platforms ( debug and release).  I added running release halfmoon acceptance tests for desktop platforms.  They all pass but I assume this is due to halfmoon falling back to interp.