Simplify tracer generation

RESOLVED FIXED in Q3 11 - Serrano

Status

defect
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: treilly, Assigned: treilly)

Tracking

unspecified
Q3 11 - Serrano
Dependency tree / graph

Details

Attachments

(2 attachments, 5 obsolete attachments)

Make it one step.
Not a good idea necessarily.  Will try to elaborate later.
All I meant to say was that the dependency set for the tracer generators is quite a lot larger than the dependency set for the builtins script, and the tracer generators need to run every time you touch a header file (pretty much).  Right now there's no "make" style dependency checking there, the script just runs (it's been optimized to make that affordable).
Posted patch Cleanup trace gen (obsolete) — Splinter Review
#1 don't use ../utils/asc.jar, the player builds don't look there anymore
#2 don't build exactgc.abc unless we need to
#3 don't install new tracer headers unless they changed
#4 move tracer headers to generated directory
Attachment #509094 - Flags: review?(lhansen)
Comment on attachment 509094 [details] [diff] [review]
Cleanup trace gen

Looks OK to me.
Attachment #509094 - Flags: review?(lhansen) → review+
changeset: 5867:bf37ebd94bf7
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 630219 - Prep work for generating tracer in the player
1 don't use ../utils/asc.jar, the player builds don't look there anymore
2 don't build exactgc.abc unless we need to
3 don't install new tracer headers unless they changed
4 move tracer headers to generated directory

http://hg.mozilla.org/tamarin-redux/rev/bf37ebd94bf7
changeset: 5868:92abd6089079
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 630219: use of filecmp requires import of filecmp.  (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/92abd6089079
Doh thanks Felix
Attachment #509491 - Flags: superreview?(lhansen)
Attachment #509491 - Flags: review?(stejohns)
Comment on attachment 509491 [details] [diff] [review]
use absolute paths for generated includes

I'm not really wild about this as a precedent -- using #includes that assume a certain folder layout smells like a recipe for future pain. R+ on the assumption that this is a temporary band-aid to get us moving forward.
Attachment #509491 - Flags: review?(stejohns) → review+
Summary: Fold builtin-tracers.py into builtin.py → Simplify tracer generation
Idea is to make it really easy for player to use so this so it doesn't turn into a hairball like our native gen builtins.

Asking for feedback instead of review as I'm not gonna bother submitting until I get something working nicely in the player.

Highlights:

1) no need to include avmplus-interlock.h everywhere, its included from avmplus.h now
2) boiled 3 gen files into 2, one .h (interlock) and one .cpp (methods)
3) invoke via simple python module interface player can use mimic

I'm not gonna fold this into builtin.py, I wish there was one script to generate all the generated code but builtin.py is slow and lacks dependency mojo.

native code gen is fubared in the player and we want to keep gc tracer gen separate for now.
Attachment #509094 - Attachment is obsolete: true
Attachment #509095 - Attachment is obsolete: true
Attachment #509491 - Attachment is obsolete: true
Attachment #509567 - Flags: review?
Attachment #509567 - Flags: feedback?(lhansen)
Attachment #509491 - Flags: superreview?(lhansen)
Note that I also added exactgc.abc again we need to keep things simple for the player.
Attachment #509567 - Attachment is obsolete: true
Attachment #509570 - Flags: feedback?(lhansen)
Attachment #509567 - Flags: review?
Attachment #509567 - Flags: feedback?(lhansen)
Status: NEW → ASSIGNED
> I'm not really wild about this as a precedent -- using #includes that assume a
> certain folder layout smells like a recipe for future pain. R+ on the
> assumption that this is a temporary band-aid to get us moving forward.

Really lets see:

History of absolute include paths causing pain: nil
History of updating projects include paths causing pain: EPIC
> History of absolute include paths causing pain: nil
> History of updating projects include paths causing pain: EPIC

That's unlikely to be true, and a dirty fix should not be employed to avoid a one-time pain.  Beyond that I have no opinion either way.
Attachment #509570 - Flags: feedback?(lhansen) → feedback+
(In reply to comment #15)
> > History of absolute include paths causing pain: nil
> > History of updating projects include paths causing pain: EPIC
> 
> That's unlikely to be true, and a dirty fix should not be employed to avoid a
> one-time pain.  Beyond that I have no opinion either way.

Maybe you'd like to fix nanojit relative includes then?  The one time pain of hand editting 100 project files shouldn't be bad.   Oh and add the generated directory while you are at it.    Hopefully gyp will save the day but with the current situation relative includes are a massive time saver.

Personally I think our policy should be to require tamarin clients to have exactly one include path for tamarin and we should use relative includes intra-tamarin.   With our one header per module system this is easily managable.   To be of the opinion that all our clients include search paths should include core, nanojit, MMgc, VMPI, platform/*, extensions, pcre, and vmbase is asinine IMHO.   We should strive to make interfacing with tamarin as simple as possible on all fronts, include's and API.   If we just had one tamarin.h file at the toplevel directory and removed all the avmplus search paths from our clients I think that would be an improvement.

Sometimes its nice to what you're getting with relative includes than suffer the consequences of a massive include search path that history has shown fragile to duplicate name issues.    Ever wonder why we need avmplusHashtable.h instead of just Hashtable.h?
Please excuse snarkiness, I think we're making much ado about nothing and my snow blower broke so I had to shovel my football field length driveway by hand.  Big time crankasaurus ;-)
No worries on snarkiness.

Frankly, the one think I want out of this is a simple way to regenerate all the tracers; AFAIK I currently have to re-run exactgen.abc with a bunch of funky args. Can I have a single no-brainer script to re-do all of core + shell ?
(In reply to comment #18)
> Frankly, the one think I want out of this is a simple way to regenerate all the
> tracers; AFAIK I currently have to re-run exactgen.abc with a bunch of funky
> args. Can I have a single no-brainer script to re-do all of core + shell ?

I've been running core/builtin-tracers.py and shell/shell_toplevel-tracers.py.  Not a single script, but hopefully two scripts is good enough.  See docs/mmgc/exactgc-cookbook.html.
We could easily add a toplevel script to do both, consider it done.
Doh -- guess I should (re) RTFM.
Okay so the solution I went with is to follow convention and generate the builtin tracers into the player build dirs so no relative includes are needed.
Depends on: 632500
Duplicate of this bug: 632500
Also added a toplevel "generate.py" script SJ asked for.
Attachment #509570 - Attachment is obsolete: true
Attachment #511793 - Flags: review?(lhansen)
Comment on attachment 511793 [details] [diff] [review]
New python module to facilatate 3rd party usage

As a general rule I think it's a particularly bad idea to name files that are meant to be included into other files ".cpp" - it only causes confusion and has no benefits that I can think of.  This is especially so because I insist that my IDE file list contain all files in the projects so that I can search them effectively; when a file is called .cpp there's additional work involved in making sure it does not get compiled as a standalone file.  (Not a lot of work, but even so.)

I suggest you instead use ".h", or if you must have something different from ".h", use ".icc" ("include CC").  However .icc latter is not understood automatically by many IDE editors and is bad for that reason.

I'm not really qualified to review the Python code but it looks plausible to me.
Attachment #511793 - Flags: review?(lhansen) → review+
BTW there appears to be some junk in this patch: utils/exactgc.abc and ./generate.py.
I think the right thing is to have these .cpp's compiled separately and not be included at all, it was just done that way as a shortcut to avoid project file pain I suspect.   Or was there a reason for the include?

Also the native glue generator uses .cpp so I thought we should be consistent.
(In reply to comment #27)
> I think the right thing is to have these .cpp's compiled separately and not be
> included at all, it was just done that way as a shortcut to avoid project file
> pain I suspect.   Or was there a reason for the include?

My concern was probably that the prefixes in the generated files are not the same, thus it's a pain to generate complete cpp's without more command line switches.  (The generated file for the shell should probably start by including avmshell.h, not avmplus.h, IIRC.)

> Also the native glue generator uses .cpp so I thought we should be consistent.

But that's a separately compiled file, no?  It generates both builtin.h and builtin.cpp.
builtin.cpp isn't compiled separately its included into AbcData.cpp, probably for the prefix reason you mention.     I hadn't considered that issue.   I'm fine with changing it back, although I wish all the generated files meant to be included had there own suffix, is icc a convention or something you made up?   I found ".hh" is used in the idl world, I'll poke around a little and see if I can find something that will be different and yet still appear as C++ code that isn't meant to be compiled to IDEs.
XXX.icc comes from a coding standard somewhere (a book IIRC), same place the XXX-inlines.h convention comes from.
XCode understands .hh and thinks .icc is some color profile file.   See any problems with .hh, presumably VisualStudio will treat it as C++ since is a IDL convention.
how about builtin.h and builtin-impl.h.  Or, add the necessary boilerplate so that builtin.cpp is a toplevel copmilation unit, and remove the #include of it.
changeset: 5919:351eec29f199
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 630219 - Simplify tracer generation (r=lhansen)

Replace 3 trace files with 2, ie avmplus-tracer.h and avmplus-tracer.hh.

avmplus-tracers.h includes in avmplus.h so interlock includes don't
need to be included anymore downstream.

.hh - new generated include suffix convention, won't be compiled by
IDE by default and is treated as C++ source.

Updated xcode but not visual studio or eclipse.

No make building of these files yet.

New pytyon module to make building exact tracer code fool-proof for
3rd parties.

http://hg.mozilla.org/tamarin-redux/rev/351eec29f199
Posted patch Update docsSplinter Review
Attachment #512177 - Flags: review?(lhansen)
Attachment #512177 - Flags: review?(lhansen) → review+
+1 for builtin.hh, builtin-impl.hh

If Xcode and VS are happy, that's probably all we need -- might be worth checking Eclipse too to ensure it doesn't barf. Also ping Rick re: gyp.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.