Closed Bug 632857 Opened 13 years ago Closed 12 years ago

xplat build: add rules for generating tracers.

Categories

(Tamarin Graveyard :: Tools, defect)

Product:
Tamarin Graveyard
Component:
Tools
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: pnkfelix, Assigned: pnkfelix)

References

Details

Attachments

(1 file, 6 obsolete files)

patch P v7: generated tracers before anything if AVM defined
4.21 KB, patch
Details | Diff | Splinter Review 
This is a followup to Bug 632086: the knowledge I gained fixing the builtin and shell_toplevel make rules should be applicable to adding similar rules for builtin-tracers and shell_toplevel-tracers.
OS: Mac OS X → All
Hardware: x86 → All
It is probably best to wait until the dust settles on Bug 630219 before starting on this task; the target location of the tracer files is likely to affect the structure of related makefile rules.
Flags: flashplayer-qrb+
Target Milestone: --- → Future
In progress.

Ideally any patch for this would have the property that successive calls to make would invoke the tracer-generating script but do *nothing* else.  Initial experiments with this patch indicate that it may converge on that state, but not immediately, which is a little disturbing.
(I posted the patch mostly because it might solve the problem for me, and thus may solve the problem for others who use xplat.  For me, its all about making Emacs compilation mode work seamlessly.)
(In reply to Felix S Klock II from comment #2)
> Created attachment 561454 [details] [diff] [review] [diff] [details] [review]
> patch P v1.  Might not be completely baked.

This patch wasn't completely baked.

But I now think there is a simple way to fix it.  The key appears to be to use "order-only prerequisites" in the Makefile:

  http://www.gnu.org/software/make/manual/make.html#Prerequisite-Types

as illustrated here:

  http://stackoverflow.com/questions/2121620/parallel-building-with-gnumake-and-prerequisites
The feature offered by an order-only prereq is that it forces the rule to run *without* forcing the target to be rebuilt.  This is important for the tracer-generation script, because the script is designed to be run unconditionally and *avoid* updating the output files if they have been unchanged.

(I still need to double-check that the rest of the build process works properly when the output files *are* changed by the tracer-generation script.  But this has been promising so far...)
Attachment #561454 - Attachment is obsolete: true
*anything* in particular includes MMgc/AVMPI/VMPI

I'm still not sure this is 100% right (I think I see evidence that the build is progressng to preprocess the first input before its done generating the tracers) but it seems closer, and I might be able to get it to wait until the tracers are generated if I add a dependence on the generated files in between the .ii target and the .PHONY core-tracer prereq.

Anyway, its good enough for me to use for now.
Attachment #569015 - Attachment is obsolete: true
Two fixes:

1. A typo in my definition of MMgc_PREPROCESSED was the cause of the "progressng to preprocess the first input before its done generating the tracers" that I noted in comment 6

2. Added order dependence from shell-tracers onto core-tracers to avoid write-write conflict on util/exactgc.abc
Attachment #569197 - Attachment is obsolete: true
(In reply to Felix S Klock II from comment #7)
> Created attachment 569923 [details] [diff] [review] [diff] [details] [review]
> patch P v4: generated tracers before *anything*

Oh, and one more note on the patch itself:
The "./" discrepancy between the line
	cd $(topsrcdir)/core; python builtin.py
and
	cd $(topsrcdir)/core; python ./builtin-tracers.py
may seem odd.

But its necessary in the latter case; otherwise its import of utils.exactgc fails.  (The other script does not import any helper scripts from the TR checkout, since it is just a wrapper around an invocation of asc.jar.)
(In reply to Felix S Klock II from comment #7)
> Created attachment 569923 [details] [diff] [review] [diff] [details] [review]
> patch P v4: generated tracers before *anything*

Buildbot failure; need to put AVM into the environment:

make -j2

true "Preprocessing other-licenses/zlib/adler32"
true "Preprocessing other-licenses/zlib/compress"
true "Preprocessing other-licenses/zlib/crc32"
true "Preprocessing other-licenses/zlib/deflate"
true "Preprocessing other-licenses/zlib/gzio"
true "Preprocessing other-licenses/zlib/infback"
true "Preprocessing other-licenses/zlib/inffast"
true "Preprocessing other-licenses/zlib/inflate"
true "Preprocessing other-licenses/zlib/inftrees"
true "Preprocessing other-licenses/zlib/trees"
true "Preprocessing other-licenses/zlib/uncompr"
true "Preprocessing other-licenses/zlib/zutil"
cd ../core; python ./builtin-tracers.py
true "Compiling other-licenses/zlib/adler32"
ERROR: AVM environment variable must point to avm executable
true "Compiling other-licenses/zlib/compress"
make: *** [core-tracers] Error 1
make: *** Waiting for unfinished jobs....
build failed return value 2
avmshell is missing, build failed
(Or would people prefer that the Makefile conditionally check if AVM is set before attempting to run the tracer generation script, with the idea that we would like to be able to sort-of bootstrap the system from a clean checkout without any avmshell installed?)
Discussing with brbaker:
  http://asteam.corp.adobe.com/irc/log/ttlogger/tamarin/20111028
convinced me that the approach outlined in comment 10 is the way to go.

I have it set up to issue a message saying that its skipping the tracer generation step; this seems like a reasonable compromise to me (though perhaps I should have used the 'true <msg>' pattern that I've been using elsewhere so that it will respect 'make -s')
Assignee: nobody → fklockii
Attachment #569923 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(In reply to Felix S Klock II from comment #11)
> Created attachment 570205 [details] [diff] [review] [diff] [details] [review]
> patch P v5: generated tracers before anything if AVM defined

This one seems like it should work, but the buildbot failed on win64, with the message that it couldn't open the file win64setjmp.obj after the step where it assembled win64setjmp.asm

This seems like it is unrelated.  But in the log file, the command to assemble win64setjmp did appear right before the 'Skipping shell-tracers since AVM unset' message.  That might be coincidence, or it might be a clue that my patch is in fact related to this failure.  I won't try to land this until I resolve that mystery.
(rebased; still haven't resolved the win64 issue mentioned above.)
Comment on attachment 570205 [details] [diff] [review]
patch P v5: generated tracers before anything if AVM defined

(obsoleted by v6)
Attachment #570205 - Attachment is obsolete: true
(In reply to Felix S Klock II from comment #12)
> This one seems like it should work, but the buildbot failed on win64, with
> the message that it couldn't open the file win64setjmp.obj after the step
> where it assembled win64setjmp.asm
> 
> This seems like it is unrelated.  But in the log file, the command to
> assemble win64setjmp did appear right before the 'Skipping shell-tracers
> since AVM unset' message.  That might be coincidence, or it might be a clue
> that my patch is in fact related to this failure.  I won't try to land this
> until I resolve that mystery.

Finally solved the mystery: the make rule for building .obj from .asm via $(MASM) does not construct the target directory, so the $(MASM) invocation can fail.

My hypothesis for why this worked before is that we've been getting lucky in that the other rules to build .obj from .c/.cpp via $(CC) were getting run first and building the target directory before make got around to applying the one case where $(MASM) needed to be invoked.

But with the changes associated with this patch, the order of invocations is changed in some manner so that we end up trying to $(MASM) rule before the $(CC) rule (probably because all the .c/.cpp code needs the builtin-tracers to get run first, and so all those rules that were running early before are now delayed).
(In reply to Felix S Klock II from comment #15)
> Finally solved the mystery: the make rule for building .obj from .asm via
> $(MASM) does not construct the target directory, so the $(MASM) invocation
> can fail.

Filed off as subtask Bug 715158
Depends on: 715158
(rebased, otherwise unchanged.)

With Bug 715158 fixed, it looks like this passes the buildbot.  I'll wait to land it until after the next (and imminent) TR -> FRmain merge, though.
Attachment #572424 - Attachment is obsolete: true
changeset: 7184:0d46338d1354
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 632857: generate gc tracers on every xplat build when AVM defined (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/0d46338d1354
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 724695
I'm not convinced that i got this right; i'm pretty sure I'm seeing evidence of build wackiness.  e.g. sometimes first invocation of "make -jK" for some K breaks, but then a subsequent invocation of "make -jK" builds cleanly.

I *am* pretty sure that the only people affected by this are those with the AVM environment variable set during an xplat build (which is hopefully the same as the set of people deliberately trying to use the feature added by this ticket).  So its not super-high priority to fix, but I should still attempt to do so.

I won't re-open this ticket until I have a way to demonstrate the problem consistently and repeatably.
Depends on: 748344
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: