649600 - Unable to build avmshell with --enable-aot

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Alok Manchanda]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.204 Safari/534.16
Build Identifier: 

It's not possible to build avmshell with AOT support right now because:
1) AOT directories are not being recursed and are not part of include path
2) AOT libraries are not linked with avmshell
3) Invoking AOTStubs.py does not use the right path (missing one "..") 

Reproducible: Always

Steps to Reproduce:
Try to build avmshell with --enable-aot

Comment 1

[Alok Manchanda]

Attachment: Proposed fix

Comment 2

[Alok Manchanda]

If the fix looks OK, would you please check it in for me? Thanks!

Comment 3

[Tamarin Bot]

changeset: 6193:8442451d32dc
user:      Alok Manchanda <almancha>
summary:   Bug 649600 - Unable to build avmshell with --enable-aot (r=almancha)

http://hg.mozilla.org/tamarin-redux/rev/8442451d32dc

Comment 4

[Tamarin Bot]

changeset: 6386:53e8f502a08c
user:      Steven Johnson <[e-mail]>
summary:    Bug 649600 - Unable to build avmshell with --enable-aot (r=almancha), previous commit was nuked in a merge by mistake

http://hg.mozilla.org/tamarin-redux/rev/53e8f502a08c

Comment 6

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

Comment on attachment 525647 [details] [diff] [review]
Proposed fix

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

::: aot/manifest.mk
@@ +1057,3 @@
>  	@echo "Generating AOT stubs..."
>  	@mkdir -p aot
> +	@cd aot && python ../$(topsrcdir)/aot/AOTStubs.py --numstubheaders $(numstubs) --picklefile $(topsrcdir)/aot/aotstubs.pickle

The "../$(topsrcdir)" makes my spidey-sense tingle.

Introducing a dependence on $(topsrcdir) be a relative path is probably a bad idea; I'm pretty sure it was originally an absolute path, and we only turned it into a relative path in order to work-around platforms like Windows where the absolute paths often had spaces embedded in them.

Is there anything better we can do here?

Comment 7

[Alok Manchanda]

Yes, I certainly think so. After your comment I actually looked at what's going on here and it's:
1) cd aot
2) python ../$(topsrcdir)/aot/AOTStubs.py
I'm guessing that #2 could be replaced by python AOTStubs.py?

-alok.

(In reply to comment #6)
> Comment on attachment 525647 [details] [diff] [review] [review]
> Proposed fix
> 
> Review of attachment 525647 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: aot/manifest.mk
> @@ +1057,3 @@
> >  	@echo "Generating AOT stubs..."
> >  	@mkdir -p aot
> > +	@cd aot && python ../$(topsrcdir)/aot/AOTStubs.py --numstubheaders $(numstubs) --picklefile $(topsrcdir)/aot/aotstubs.pickle
> 
> The "../$(topsrcdir)" makes my spidey-sense tingle.
> 
> Introducing a dependence on $(topsrcdir) be a relative path is probably a
> bad idea; I'm pretty sure it was originally an absolute path, and we only
> turned it into a relative path in order to work-around platforms like
> Windows where the absolute paths often had spaces embedded in them.
> 
> Is there anything better we can do here?

Comment 8

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

(In reply to comment #7)
> Yes, I certainly think so. After your comment I actually looked at what's
> going on here and it's:
> 1) cd aot
> 2) python ../$(topsrcdir)/aot/AOTStubs.py
> I'm guessing that #2 could be replaced by python AOTStubs.py?

That's close but still broken, because in general:

  1. The xplat system is going to be run in a working directory, call it $(objdir), that is distinct from $(topsrcdir).
  2. So $(topsrcdir)/aot/ and $(objdir)/aot/ will be different
  3. And I assume that AOTStubs.py is generating code into the current working directory.  (Otherwise why would it "cd aot" at all; having said that, I haven't looked at the script to verify this assumption.)

Here's a different idea that came to me this morning that is very close to what you propose:

>  	@mkdir -p aot
> -	@cd aot && python ../$(topsrcdir)/aot/AOTStubs.py --numstubheaders $(numstubs) --picklefile $(topsrcdir)/aot/aotstubs.pickle
> +     @cp -f $(topsrcdir)/aot/AOTStubs.py aot/AOTStubs.py
> +     @cd aot && python AOTStubs.py --numstubheaders $(numstubs) --picklefile $(topsrcdir)/aot/aotstubs.pickle

This way, the usage of $(topsrcdir) is resolved in a context where the current working directory has not been changed.  I assume here that the extra copy of the script is trivial overhead in space and time.

(I threw a '-f' into the cp options because you made me worry about the scenario where someone *does* have $(objdir) == $(topsrcdir), but the '-f' may be totally unnecessary.  Probably worth testing that scenario in any case.)

Comment 9

[Alok Manchanda]

(In reply to comment #8)

Yes, that makes sense Felix.

Comment 10

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

Of course, I haven't tested my suggestion.

If there are any helper Python scripts in aot/, then my idea will break.

Also, there's still the reference to $(topsrcdir) in the --picklefile option.

How was that working before, when you changed the reference to the script to the "../$(topsrcdir)" but did not change the --picklefile parameter?

Comment 11

[Alok Manchanda]

(In reply to comment #10)
> How was that working before, when you changed the reference to the script to
> the "../$(topsrcdir)" but did not change the --picklefile parameter?

Oh no, my bad. The picklefile part wasn't working. I missed a message stating "No stub ordering file found" in the output. It's an optional parameter.

Comment 12

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

(In reply to comment #11)
> (In reply to comment #10)
> > How was that working before, when you changed the reference to the script to
> > the "../$(topsrcdir)" but did not change the --picklefile parameter?
> 
> Oh no, my bad. The picklefile part wasn't working. I missed a message
> stating "No stub ordering file found" in the output. It's an optional
> parameter.

Hmm.  So that means that the solution I proposed above (of first making a local copy of the script before running it) would have to be generalized to make copies of the script and all other resources, like the pickle file.

Another approach would be to generalize the script so that it did not depend on what directory it ran out of.  (For example, have it take a parameter that indicates where the files should be generated into; if the parameter is not supplied, then use the current directory as the inferred value for the parameter, for backwards compatibility.)

I'm pretty sure many of the other Python scripts in our infrastructure try to take this approach.

Comment 13

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

(reopening as alok said in comment 11 that the picklefile part of his fix was broken.)

Comment 14

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

Alok: assuming that this gets reassigned back to you, please review the case-sensitivity issues for the pickle file, since it seems like the python script got the sed -e s/aot/AOT treatment in the manifest.mk, but the pickle file did not get similar treatment.

Comment 15

[Andre Kruetzfeldt]

Was fixed in TR and case-sensitivity to be noted in Manifest. Closing bug.