Closed Bug 645878 Opened 13 years ago Closed 6 years ago

Tamarin Foundation Utilities

Categories

(Tamarin Graveyard :: Virtual Machine, enhancement)

Product:
Tamarin Graveyard
Component:
Virtual Machine
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: stan, Assigned: stan)

References

Details

Attachments

(2 files, 4 obsolete files)

Patch for review and, ultimately, submission
469.95 KB, patch
lhansen
: review+
Details | Diff | Splinter Review
Patch to earlier patch to make localtime threadsafe
2.61 KB, patch
lhansen
: review+
Details | Diff | Splinter Review 
This bug proposes to carve out out a "Foundation Utilities" component, similar to vmbase (or perhaps a refinement to it) so that a common infrastructure for threading, synchronization, etc. can be shared between the VM and clients of the VM.

It is duplicative and probably dangerous for Tamarin and its clients to have different platform implementations of threading and synchronization, so we need to find a supportable way to share these common foundation components.

Nothing currently prevents clients from just calling into vmbase or VMPI, but at present these are understood as components internal to the VM. There is an effort underway to make the VM interface narrower and more encapsulated, so if this is to be allowed the interface should be formalized and not ad-hoc.

Additionally, in order to facilitate better separation between client code that is VM-involved and GC-involved, this set of utilities should be more primitive than and independent of both the GC (and safe points) and of the VM. Specifically, we do not want to force VM or GC dependencies on code which does not need to have any.

The component would include:

   threads (pretty much as-is, but probably renamed)
   locks, monitors, condition variables (pretty much as-is, but probably renamed)
   thread local storage
   assert and static assert

Additionally, since the foundation will serve as a sort of inter-component substrate or vocabulary, it will be very useful to have a generic (non-VM) abstractions for strings and arguably, but far less urgently, vectors and hash maps.

It will probably also be necessary to abstract a “panic abort” protocol from the MMGC out-of-memory machinery.
The Windows Mobile implementation of VMPI_getPrivateResidentPageCount() currently calls back into MMgc to get the range of allocated pages, because the OS does not provide a means of getting at the address range.  That structure could be changed by providing VMPI calls with which MMgc notifies the VMPI layer of page allocations, for example, or possibly there are other workarounds, hooks into block allocators, etc - I've not looked into this in any detail yet.  (I think the WinMo implementation is the way it is partly for expedience.)  I only mention this as an example of how the VMPI layer is not strictly more primitive than MMgc at this point.   Obviously I'm all in favor of cleaning that up.
Isn't the winmo code being T'd up for blast off into the great big bit bucket in the sky?
(In reply to comment #2)
> Isn't the winmo code being T'd up for blast off into the great big bit bucket
> in the sky?

It is, but the point is where there is one tie back to a higher level there may be others...
Exposing all of VMPI as a supported client API is fairly easy (all we have to do is say it, and voila!--except for the (potential?) problem mentioned above), but it runs counter to the goal of a smaller API surface.

Should we consider splitting VMPI into the more general, shareable part (theads, TLS, etc.) and the part that is more specifically oriented toward MMGC and the VM (page allocation, executable memory, etc.)? That could moot the problem Lars mentions above. But it might also be slicing the bologna too thin.

Thoughts?

IMO, this is not the part of the API where surface area is really a problem. The greater issue is that clients can peer too deeply into the workings of the VM.

Regardless, I do consider it a requirement that this shared base be compilable and linkable without pulling in any of MMGC or the VM.
Unit testing...

We will want separable and comprehensive unit tests for this new component. Besides their obvious value as tests, it will also ensure that the component can be compiled and linked independently.

We've been looking at the google test framework for this (gtest). I've been very impressed. It's extremely easy to use and quite functional (suite subsetting, death tests, threading, etc.)

  http://code.google.com/p/googletest/wiki/FAQ

Thoughts?
IMO there's no real utility in slicing the VMPI layer into groups; all of it is needed to host the Player anyway.

With one possible exception: libc.  Currently we're grouping VMPI_ versions of libc with the rest of VMPI (eg, VMPI_memcpy).  The VMPI_ versions of libc came about because libc implementations have been varying in their quality; by always having our own libc names (normally just macros that map to the libc variants) we are able to replace individual functions cleanly, or provide functions where the local libc does not have those functions.  Windows is a little deficient in the vnsprintf department, for example (missing functions, different return values).
The VM has a unit testing framework called selftest that we use a bit.  Documentation is at the start of extensions/selftest.as.  It's lightweight and we control it so it's easy to extend it to meet our needs.  There are examples of its use in extensions/*.st.

Selftest might have some accidental dependencies on the VM (for example, it assumes MMgc's shared block manager has been created, but does not assume anything about a GC being available) but if it does those could likely be cleaned up without too much fuss.
(In reply to comment #6)
> With one possible exception: libc.  Currently we're grouping VMPI_ versions of
> libc with the rest of VMPI (eg, VMPI_memcpy).  The VMPI_ versions of libc came
> about because libc implementations have been varying in their quality; by
> always having our own libc names (normally just macros that map to the libc
> variants) we are able to replace individual functions cleanly, or provide
> functions where the local libc does not have those functions.

I would sure want to know more details about that. If you can't trust the quality of the RTL, why trust, say, the template implementation?

I get that some of the sprintf family differs wildly, but FWIW, templates cannot make up the difference in semantics and function signatures anyway. As for quality of implementation (again: details?) couldn't the linker just resolve to your new, improved one?

Anyway... yeah,  to the extent that this is a real issue (count me a skeptic), I'd gladly keep it internal to the VM. :)
I'd like to quickly summarize some of our "wants" for the sharable component:

 - Can be compiled, linked, tested and used independent of any higher-level modules, notably MMgc.
 - Makes few if any impositions on the runtime environment of users
 - provides for orderly aborts (so that plugins can "abort" independently of their containers)
 - no C++ exceptions
 - public header files segregated from internal headers and source in the source tree
 
In addition, we'd really like to promulgate the include pattern:

  #include <module/header.h>

And (finally?), we'd like to associate a C++ namespace with each module.

I suppose all of that is pretty uncontroversial except for segregated headers and the include pattern. We want to do those things in order to make the modular structure evident in the code, to minimize conflicts, to be able to enforce the API perimeter, to help enforce proper dependencies, and to make {.lib,header} packaging feasible.

Thoughts pro or con? Discuss!
And also we'd have to decide what to name this.

I hate to gratuitously move or rename things, so please talk me out of it!

opening bids:
 - mmfx (dunno what it stands for, but we used it before)
 - vmbase (just keep it the same and say it means something else)
 - kernel (something we've called it in internal discussions, but likely to conflict with things)
(In reply to comment #7)
> The VM has a unit testing framework called selftest that we use a bit. 
> Documentation is at the start of extensions/selftest.as.  It's lightweight and
> we control it so it's easy to extend it to meet our needs.  There are examples
> of its use in extensions/*.st.
> 
> Selftest might have some accidental dependencies on the VM (for example, it
> assumes MMgc's shared block manager has been created, but does not assume
> anything about a GC being available) but if it does those could likely be
> cleaned up without too much fuss.

Actually, selftest is pretty deeply entwined with avmcore and avmshell.

Without a lot of rework, it would not be able to test things independent of AVM.

What are the cons of introducing a mature third-party testing framework like gtest? Is it out of the question?
(In reply to comment #8)
> Anyway... yeah,  to the extent that this is a real issue (count me a skeptic),
> I'd gladly keep it internal to the VM. :)

It's definitely a real issue. MSVC and GCC have different warts on the RTL; wrapping them in this way ensures that we have a predictable interface. (I'll bet you a dollar that Clang will have enough differences from both of these that it will need massaging too.) All that said, I think that the wartiest of the warts were in now-defunct platforms (WinCE and Symbian).(In reply to comment #9)

> I'd like to quickly summarize some of our "wants" for the sharable component:

Those all sound reasonable to me.

> And also we'd have to decide what to name this.

Umm... how about "VMPI"?

> What are the cons of introducing a mature third-party testing framework like
> gtest? Is it out of the question?

I'm certainly open to that. What license is it under?
(In reply to comment #10)
> And also we'd have to decide what to name this.
> 
> I hate to gratuitously move or rename things, so please talk me out of it!
> 
> opening bids:
>  - mmfx (dunno what it stands for, but we used it before)
>  - vmbase (just keep it the same and say it means something else)
>  - kernel (something we've called it in internal discussions, but likely to
> conflict with things)

mmfx is an awkward but serviceable shorthand for "fixed memory manager", ie, malloc-like.  I don't know who tought of it, probably some combination of Tommy/BrentG/Antti/Ruchi.
not me on the "mmfx" thing. :)

Either way, something like 'vmbase' sounds more appropriate since names like "fixed memory" (mmfx) or "platform interface" (VMPI) don't really sound right.
tpr (tamarin portable runtime)
To reiterate my comments to Stan:

- All of the thread/synchronization classes defined in vmbase, including those implementing safepoints, have no dependencies on the VM or GC. They require only VMPI.

- The following features can be considered as extensions to the current vmbase synchronization constructs: OOM handling, exception support (auto-unlock and rethrow), safepoint-aware locking, deadlock detection. Of these, only safepoint-aware locking has landed in vmbase. The others are listed as future enhancements.

- The rules described in VMThread.h which govern the use of safepoint aware locking will be applicable to the player, as soon as any threads which share a safepoint context can content for a player lock.

- The vmbase atomics/memory barrier APIs are a useful (cross-platform), counterpart to the higher-level synchronization constructs.

- The VM's selftests have a collection of tests that stress both the vmbase and VMPI synchronization constructs. These could probably be transplanted into googletest.
(In reply to comment #16)

Thanks for the run-through yesterday. It was very helpful.

It's great to find that the thing you are looking for pretty much already exists.

About the only thing I'd change is to move atom.* elsewhere.

I will investigate the feasibility of converting the tests to googletest, although I don't feel that the testing framework question has reached anything like consensus yet.
(In reply to comment #9)

> I suppose all of that is pretty uncontroversial except for segregated headers
> and the include pattern. We want to do those things in order to make the
> modular structure evident in the code, to minimize conflicts, to be able to
> enforce the API perimeter, to help enforce proper dependencies, and to make
> {.lib,header} packaging feasible.

Here's what I was thinking (since so far nobody's objected):

 - clients include by writing #include <vmbase/VMThread.h>
 - compiler -I flags should be able to select in-situ set of headers for the vmbase module or a copy thereof.
 - With suitable -I flags, clients should be able to include public headers but not private headers
 - With suitable -I flags, clients should be able to select just vmbase and not other tamarin components so that attempts to include them would fail.

Unfortunately doing all of that leads to some funny looking directory structures. This isn't the only conceivable organization, but it fills the bill:

root/
  include/  -- LLVM does something like this
     vmbase/  -- So that you can specify this dir on the compiler's -I directive
        vmbase/  -- what, again? Yeah, because the module name's in the #include
           VMThread.h  -- etc.

OK, so this has issues: The apparent stutter in the file paths. That you have to (and get to) specify each module in the compile command. That's so that the compiler can enforce your intended dependencies. And, finally, well... we use so many inlines, etc. that there might not be many definitions that can even be left out of the public header files. They might all end up over there anyway, and that would defeat the point of separating public and private headers.

I want to keep this as simple as possible. Thoughts?
(In reply to comment #17)
> About the only thing I'd change is to move atom.* elsewhere.

I misspoke there.

Let me summarize the changes I envision (please talk me out of anything that seems unjustified):

 - public header organization such as described in comment #18.
 - moving atom-related stuff elsewhere: probably MMgc since that's the lowest-level thing that needs to grok atoms.
 - possible renaming from VMetc, but I really would rather not do that and would prefer a clever backronym for vmbase, etc.
 - possible introduction of googletest (which removes certain dependencies, but adds one too: googletest itself)
 - VMPI stays as is, with possible reorganization of public headers
 - possible renaming of AvmAssert, etc, but probably having that a name-preserving wrapper elsewhere.
 - add a static assert in the same file as AvmAssert (based on the one in MMgc)

I know it sounds a lot like "redecorating" (I'm probably more down on that sort of thing than most people!), but I'm trying to find enforceable boundaries and to make the names reflective of those boundaries while changing as little as possible.
> - public header organization such as described in comment #18.

I like the llvm include file conventions -- a toplevel include directory where public module interfaces go (thus, just one -I directive to the compiler commandline); subdirectories in there if you want to see heirarchy in your #include's, and module-private include files mixed in with the source code.

so, include/vmbase, but nothing deeper.  

It is not as granular as what you wrote (can't exclude the vm from the compilers commandline path) but does the extra nesting pay for itself?  how important is the use case of filtering compiler commandline directories?.  It could be that I'm missing the motivation for the more granular directory structure under /include.  (and even, if its needed, I'd propose something still flatter, with the convention that the deepest directory you put in the compiler path always has the word 'include' in it:
a) include-vmbase, include-somethingelse, and so-on).
b) vmbase/include, somethingelse/include, etc.

>  - moving atom-related stuff elsewhere: probably MMgc since that's
> the lowest-level thing that needs to grok atoms.

we put them in vmbase because MMgc can/is used without avmplus (directly from C++, and sometimes the avm isn't instantiated).  At the time, FEATURE_AVMPLUS was still in the flash code, maybe the requirement is gone now?

>  - possible introduction of googletest (which removes certain dependencies,
>    but adds one too: googletest itself)

shrug.  if it can do what the existing selftest stuff can do, great; that code can be migrated whenever it seems important to do it.  If not, or if googletest is slow/sucky/unportable, then forget it.  (I'm assuming its good, or you wouldn't have proposed it).  Apart from that, I dont have any opinion of googletest vs CUTE vs the other couple dozen options.  If you've investigated and picked that one, great.

I would ask that at least a handful of tests be migrated, or new ones added, so that future test-writing has examples to work from.

 - possible renaming of AvmAssert, etc, but probably having that a
name-preserving wrapper elsewhere.
 - add a static assert in the same file as AvmAssert (based on the one in MMgc)

The possible renamings of AvmAssert I'd propose are: assert (do we really need a different one, and if so, why not #undef the old one and replace it?), or VMPI_assert, just 'cuz.  then, static_assert() or VMPI_static_assert.  I don't think we really need module-specific asserts (MMgcAssert, AvmAssert, NanoAssert).

static asserts are motherhood and apple pie, great to adopt a convention.
> OK, so this has issues: The apparent stutter in the file paths. That you have
> to (and get to) specify each module in the compile command. That's so that the
> compiler can enforce your intended dependencies. And, finally, well... we use
> so many inlines, etc. that there might not be many definitions that can even be
> left out of the public header files. They might all end up over there anyway,
> and that would defeat the point of separating public and private headers.

> I want to keep this as simple as possible. Thoughts?

If you have a public API that's a class, its okay to have private members, and if they're inline functions, they can go in the .cpp file that uses them (if there's only one, which is more often than you'd think), or in a module-private include file.  The only inline functions that would go in the public include directory would be implementations of public api's, which seems fine.

I'll just channel Lars for a sec, but anyone using these IDE's should agree: it's paramount that we keep precompiled headers working in Xcode and VS20XX.  Moving away from one mongo header file is probably good, but keeping PCH's working means that groups of .cpp files still want to share a single PCH-able include file amongst themselves.  how do we manage that?

exmample: all the .cpp files in my module include the same stuff, so i want a single PCH file for these files.  should this grouping be internal to a module or should it show up in the module's api somehow (i admit i'm not being precise).
(In reply to comment #21)
> ...it's paramount that we keep precompiled headers working...

Standardizing the way we construct the pch file would be useful. I would propose that we have an uber-include file in both the public header directory and the private one. that way clients of a module can make their pch file easily depend on the module's public uber-include file. Our current ad-hoc approach only leads to confusion (perhaps more so in the player, the vm is better organized in this regard).
(In reply to comment #20)
> >  - moving atom-related stuff elsewhere: probably MMgc since that's
> > the lowest-level thing that needs to grok atoms.
> 
> we put them in vmbase because MMgc can/is used without avmplus (directly from
> C++, and sometimes the avm isn't instantiated).  At the time, FEATURE_AVMPLUS
> was still in the flash code, maybe the requirement is gone now?

We would like people to be able to use vmbase without necessarily using MMgc or the avm.
(In reply to comment #21)
> I'll just channel Lars for a sec, but anyone using these IDE's should agree:
> it's paramount that we keep precompiled headers working in Xcode and VS20XX. 
> Moving away from one mongo header file is probably good, but keeping PCH's
> working means that groups of .cpp files still want to share a single PCH-able
> include file amongst themselves.  how do we manage that?

We definitely need to keep precompiled headers working. But we should not force unnecessary dependencies on clients either. Fortunately there's no tension between these goals: The PCH used to compile the AVM could include the AVM's private headers, but the PCH used to compile the player need only include the public headers from the avm.
(In reply to comment #20)
> The possible renamings of AvmAssert I'd propose are: assert (do we really need
> a different one, and if so, why not #undef the old one and replace it?), or
> VMPI_assert, just 'cuz.  then, static_assert() or VMPI_static_assert.  I don't
> think we really need module-specific asserts (MMgcAssert, AvmAssert,
> NanoAssert).
> 
> static asserts are motherhood and apple pie, great to adopt a convention.

OK, I'll run with that: I'll define them as assert and static_assert. If there are conflicts, people can just undefine one or the other. It is likely to conflict with the standard assert.h header, but as you say people can always undef one or the other.

I'm not sure you'd want the code churn of changing all the AvmAssert()s to assert()s, though.
(In reply to comment #25)
> (In reply to comment #20)
> > The possible renamings of AvmAssert I'd propose are: assert

+1, also slightly off topic but the llvm guys have a nice idiom for asserts that contain messages:

assert(somecond && "some useful string")
Where I am now with this (in my local sandbox):

- added gtest 1.5.0
- built vmbase and vmpi together as a lib (xcode only) with no reference to MMgc or avmcore
- moved atom.* from vmcore to MMgc (the lowest-level thing that needs to know about that)
- split PosixPortUtils.cpp so that there's a separate PosixMMgcPortUtils.cpp to break the link back to MMgc.
- adapted all the threading tests to use gtest (they adapted straightforwardly and it really is simple)

So far that's all built using the xcode project, now I need to make it work with configure.py and the manifest.mk files.

The way that the makefiles currently work, a directory is a "thing" (lib, executable and but all things share the same includes.

I want to change that as follows:
  - split VMPI into the part that does not depend on MMGC and the part that does not. That will mean a new dir named something, perhaps VMPI/MMgcport
  - build VMPI and vmbase without avmplus/mmgc includes

That still leaves these stray includes from VMPI into platform and actually I'm sorta thinking that these headers ought to be moved to VMPI.

#if AVMSYSTEM_WIN32
  #include "win32/win32-platform.h"
#elif AVMSYSTEM_UNIX
  #include "unix/unix-platform.h"
#elif AVMSYSTEM_MAC
  #include "mac/mac-platform.h"
#elif AVMSYSTEM_SYMBIAN
  #include "symbian/symbian-platform.h"
#endif

Looking for feedback and sanity check.
Assignee: nobody → stan
(In reply to comment #27)
> 
> So far that's all built using the xcode project, now I need to make it work
> with configure.py and the manifest.mk files.
> 

Be aware of the on-going effort to introduce gyp; see Bug 637228
(In reply to comment #28)
> Be aware of the on-going effort to introduce gyp; see Bug 637228

Thanks for the bug ref. There is a parallel gyp effort in the Player, too. There will probably be some overlap but I think that we may end up with sharable config files. Shame not to, but it shouldn't be difficult to converge them later if need be.

I'm wondering if silence is consent. I was mischievously thinking about proposing to rename all the top directories just to see if anybody complained. :)  But I won't.

So, I'll go ahead and split VMPI into VMPI and VMPIgc (and corresponding headers). The first will have no dependencies on MMgc; the second will depend on avmplus.h.
I've had a slightly better idea for naming, I think: VMPI already has "VM" in its name; it seems to relate to the VM and GC already. Perhaps we should come up with a new name for the platform-interface services that are more generic than that (but what?).
portable runtime has been used before (nspr, apr netscape and apache respectively)
It appears to me that a proper solution here would involve some renaming and reassignment of namespaces. In the spirit of making the minimum possible changes, I'm thinking along these lines:

- things in platform/ go into namespace vmbase rather than avmplus
- things in VMPI/ do not appear to be in any namespaces, but probably should be? (though in a sense redundant with the VMPI_ prefix)
- my current best idea about splitting VMPI is VMIP (generic) and (AVMPI) linked to AVM. (This seems of a piece with vmbase).  It probably means changing the names of some (but not many) functions to begin AVMPI.

I'm probably over-thinking this, but I don't want to go too far out on a limb.
(In reply to comment #20)
> The possible renamings of AvmAssert I'd propose are: assert (do we really need
> a different one, and if so, why not #undef the old one and replace it?), 

"Because somebody somewhere is going to include the system's assert.h and that
will redefine it" might be a pretty good reason. I've hit it already. I still
like the idea, though.
(In reply to comment #33)
> (In reply to comment #20)
> > The possible renamings of AvmAssert I'd propose are: assert
> "Because somebody somewhere is going to include the system's assert.h...

I think edwin was suggesting that we just use assert.h, there isn't any functional benefit to having our own implementation and it should be a complete nop in release builds so it won't be adding any RTL dependencies we don't already have
(In reply to comment #34)
> I think edwin was suggesting that we just use assert.h, there isn't any
> functional benefit to having our own implementation and it should be a complete
> nop in release builds so it won't be adding any RTL dependencies we don't
> already have

I like that a lot.

Note that "AVMPLUS_CUSTOM_ASSERTION_HANDLER" does not fit into the new world order and is not yet an exposed feature. Barring objections, I will yank it and define AvmAssert in terms of assert.
I am experiencing some tangle between avmcore and vmpi on Windows in the surprisingly involved function VMPI_getDaylightSavingsTA(), which makes calls back into (ahem!) locally-declared avmplus functions:

namespace avmplus
{
    int WeekDay(double t);
    double MakeDate(double day, double time);

I'm tempted to move this to the "AVMPI" side of things and punt, but first I'm wondering why this doesn't just use localtime and tm_isdst like the other ports do.

If I do punt should I just move that one function, or all the time/date set of functions?
(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #20)
> > > The possible renamings of AvmAssert I'd propose are: assert
> > "Because somebody somewhere is going to include the system's assert.h...
> 
> I think edwin was suggesting that we just use assert.h, there isn't any
> functional benefit to having our own implementation and it should be a
> complete nop in release builds so it won't be adding any RTL dependencies 
> we don't already have

There is a functional benefit to having our own, namely, our own can reliably redirect failure information onto telemetry channels.  Also, of course, we have multiple variants (AvmAssertMsg and so on) where assert only has one form.  Sometimes assert can be used to have the same effect (I tend to like

  assert(!"This is a message")

myself, but it's not always so easy).
> sharable config files. Shame not to, but it shouldn't be difficult to converge
> them later if need be.

Not having them shared would be an Epic Fail. It's a must-have requirement.

> There is a functional benefit to having our own, namely, our own can reliably
> redirect failure information onto telemetry channels.

+1 -- the standard assert() is woefully underspecified and leaves us at the mercy of bad C runtimes. A custom name for assert costs of little and adds vast flexibility.
Attached patch Patch for review (obsolete) — — Splinter Review 
This patch does not include updates to the xcode and visual studio projects but builds on Mac/Win/Linux using configure.py and gmake.

Because gtest is too large for a patch, I'm uploading everything except gtest in this patch.  You can see the whole thing here:

   http://asteam.macromedia.com/hg/users/stan/sandbox (changeset 224c3965c64a)

Notes:

- vmbase and VMPI is independent of the feature configuration system. It builds into the vmbase lib. I've got a tiny hack in there to enable safepoints; but I will fix that pending advice on that.

- vmbase/unittest contains gtest unit tests for threading primitives that are straightforwardly adapted from the equivalent tests in extensions/ They have not (yet) been removed from extensions/

builds the gtest unit tests to <builddir>/gtest_vmbase

I went with the maximal theory of using standard assert.h and adding a backfill for the upcoming standard static_assert, but I note that there are recent comments about that.

I'm new to the patch process: expect noob issues.
(In reply to comment #37)
> There is a functional benefit to having our own, namely, our own can reliably
> redirect failure information onto telemetry channels. 

We can define it to use the standard assert until we decide for it to implement that and then change it later when of if that actually comes up.

> Also, of course, we have
> multiple variants (AvmAssertMsg and so on) where assert only has one form. 
> Sometimes assert can be used to have the same effect (I tend to like
> 
>   assert(!"This is a message")
> 
> myself, but it's not always so easy).

Actually it is always that easy:

    assert(p != NULL);
    assert((p != NULL) && "foo pointer is null");
    assert(!"bad parameter");
(In reply to comment #38)
> Not having them [gyp config files] shared would be an Epic Fail. It's a must-have requirement.

Absolutely. We will find a way to share them.

> > There is a functional benefit to having our own [assert], namely, our own can reliably
> > redirect failure information onto telemetry channels.
> 
> +1 -- the standard assert() is woefully underspecified and leaves us at the
> mercy of bad C runtimes. A custom name for assert costs of little and adds vast
> flexibility.

As long as we bottleneck the inclusion of assert.h (and the definition of "assert") we retain that flexibility and defend ourselves from hypothetical bad runtimes at little cost. As a rule, we are better off using standard notations and mechanisms whenever possible.

If "assert" aborts with a message in debug builds (only!) and leaves you in the debugger if you're debugging, it is a win. There is no more to want.
(In reply to comment #41)
> As long as we bottleneck the inclusion of assert.h (and the definition of
> "assert") we retain that flexibility and defend ourselves from hypothetical bad
> runtimes at little cost. 

That I can agree with.

> If "assert" aborts with a message in debug builds (only!) and leaves you in the
> debugger if you're debugging, it is a win. There is no more to want.

Not sure I agree; it's quite reasonable that we may want to modify the behavior in some runtimes (eg, to log the assertion message somewhere. log a traceback, etc)... but as you say, as long as we bottleneck the includsion of assert.h we can always redefine assert as we wish.
Attachment #528136 - Flags: superreview?(edwsmith)
Attachment #528136 - Flags: review?(lhansen)
Review ping?
(In reply to comment #39)
> Created attachment 528136 [details] [diff] [review] [review]
> Patch for review

One potentially substantive change aligns the Windows implementation of VMPI_getDaylightSavingsTA with the other platforms. The existing implementation (on Windows only) involved a call back into avmplus logic that I needed to resolve. I speculate that this is a vestige from the Win95 era when the localtime() API might have been less dependable.
Huge patch so I'll just record issues as I find them.

Major:

Definition of static_assert in MMgc/PageMap.h not acceptable.

I'm concerned that adding the AVM atom representation to the MMgc directory is misguided, it's true that MMgc needs to know but atom is not a type exported by MMgc - it's a VM type that must be known also to MMgc.  Especially disconcerting is that the definitions are still in the avmplus namespace.

Reindentation in CodegenLIR.cpp (many places) does not belong in this patch.

The division between AVMPI and VMPI seems arbitrary, I still don't understand why this can't all be "VMPI".  It's not like you can compile the player without MMgc and AVM.  Making the page memory protection code "MMgc" code (the oddly named PosixMMgcPortUtils) seems particularly inappropriate, MMgc has nothing to do with it, the code is using the standard GCHeap client interface.

Minor:

When we created VMPI we specifically decided not to use the name "AVMPI" so I'm not particularly pleased to see it come back (though ISTR that I argued in favor of it at the time).  If Ed can live with it I can live with it.

Spurious intentation mangling on GC.cpp:1040.

Spurious edit on Lir.h:1646.

No opinion:

Concurrency unit tests not reviewed.
Attachment #528136 - Flags: review?(lhansen) → review-
(In reply to comment #45)

> Definition of static_assert in MMgc/PageMap.h not acceptable.

It follows the upcoming C++ standard and is implemented identically to the one in MMgc. Please elaborate.
(In reply to comment #46)
> (In reply to comment #45)
> 
> > Definition of static_assert in MMgc/PageMap.h not acceptable.
> 
> It follows the upcoming C++ standard and is implemented identically to the
> one in MMgc. Please elaborate.

I took his point to be that it does not belong in the PageMap.h file.

(In other words, why is it defined there instead of MMgc/StaticAssert.h ?)
(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #45)
> > 
> > > Definition of static_assert in MMgc/PageMap.h not acceptable.
> > 
> > It follows the upcoming C++ standard and is implemented identically to the
> > one in MMgc. Please elaborate.
> 
> I took his point to be that it does not belong in the PageMap.h file.
> 
> (In other words, why is it defined there instead of MMgc/StaticAssert.h ?)

Mmm, ouch. Editing hash. Sorry. No idea how that got there!
(In reply to comment #45)

> The division between AVMPI and VMPI seems arbitrary, I still don't
> understand why this can't all be "VMPI".  It's not like you can compile the
> player without MMgc and AVM.  Making the page memory protection code "MMgc"
> code (the oddly named PosixMMgcPortUtils) seems particularly inappropriate,
> MMgc has nothing to do with it, the code is using the standard GCHeap client
> interface.

The point is to be able to have components that benefit from the portability layer which don't necessarily bring in all of AVM.  There are a variety of reasons for this, but consider the problem of releasing Telemetry libraries.

Well, MMgc is the component name and that file is a pile of utils specific to porting that thing and which also, inconveniently, depend on its implementation in some cases. That was my reasoning, but I'll gladly take any suggestions for improving its name.

I tried to preflight my direction with AVMPI in earlier comments. In the spirit of incrementatlity, I thought that separating would be the first order of business and name rationalization might follow. But if we can settle names earlier that's even better.
(In reply to comment #45)
> 
> Spurious intentation mangling on GC.cpp:1040.
> 
> Spurious edit on Lir.h:1646.

etc.

Sorry, noob errors; I'm not used to a source control system that leaves all files "checked out" and I have not worked out good procedures to review my changes. There must be a better way than poring through a patch file. I'll ask around for some help how to do this better next time.
(In reply to comment #45)

> When we created VMPI we specifically decided not to use the name "AVMPI" so
> I'm not particularly pleased to see it come back (though ISTR that I argued
> in favor of it at the time).  If Ed can live with it I can live with it.

I can live with it, and having more cooks in the kitchen might not help here.  IIRC, VMPI was just a letter shorter with no loss, so shrug.

At this point, if there really needs to be two prefixes, then the distinction is important, and that suggests two names that are more distinct than just one letter.  If I suggested another name now, I'd just be choosing something to minimise churn (which subset is smaller? give that one the new name), so I won't suggest one.
(In reply to comment #50)
> (In reply to comment #45)
> > 
> > Spurious intentation mangling on GC.cpp:1040.
> > 
> > Spurious edit on Lir.h:1646.
> 
> etc.
> 
> Sorry, noob errors; I'm not used to a source control system that leaves all
> files "checked out" and I have not worked out good procedures to review my
> changes. There must be a better way than poring through a patch file. I'll
> ask around for some help how to do this better next time.

I've found some better tools (integrated Araxis merge with mercurial) and now I'll go back and audit all the changes.

The whitespace changes seem the be due to the merge ignoring whitespace diffs and picking the wrong set of changes (apparently whitespace changed in the trunk and I reverted it by accident).

I'll clear this up and resubmit.
Attached patch Updated patch for review (obsolete) — — Splinter Review 
This patch fixes the whitespace problems and the bogus pasted noted in the previous review.

It's been merged up to revision ebce4dbbd966 in tamarin-redux (latest as of now).  As before, I've excluded the gtest tree since it's too big for a patch, but you can see all the changes by comparing 29db10f13865 and ebce4dbbd966 in http://asteam.macromedia.com/hg/users/stan/sandbox
Attachment #528136 - Attachment is obsolete: true
Attachment #528136 - Flags: superreview?(edwsmith)
Attachment #532071 - Flags: review?(lhansen)
review ping?
There appear to be no comments above about how AVMPI code depends on MMgc than the now-dead WinMo code; some of those appear to be

 - MMgc::GCHeap::GetGCHeap()->DumpMemoryInfo();
 - GCHeap poison values
 - GCHeap::Alloc for allocating code memory on some platforms

I don't like it, because the split between VMPI and AVMPI is spurious, but I guess I'll go with it.

However, speaking as the MMgc module owner I will not take the AS3 atom definitions into MMgc, and IMO this patch cannot land until that has been resolved.  It is true that MMgc needs to know about them at present to decode atoms during exact scanning, but the definitions don't belong in MMgc.  I don't think that's been addressed in the comments since the last review.  So R- for that (and for that only, based on a cursory re-review of the patch as offered).
Attachment #532071 - Flags: review?(lhansen) → review-
(In reply to comment #55)

Thanks for digging through all of that!

> However, speaking as the MMgc module owner I will not take the AS3 atom
> definitions into MMgc, and IMO this patch cannot land until that has been
> resolved.  It is true that MMgc needs to know about them at present to
> decode atoms during exact scanning, but the definitions don't belong in
> MMgc.  I don't think that's been addressed in the comments since the last
> review.  So R- for that (and for that only, based on a cursory re-review of
> the patch as offered).

I feel that the scope of definition of Atom must be no bigger than the things that need to know about Atoms.  VMPI/VMBase should (must, actually) be independent of it. We need to be able to change the representation of Atom without affecting vmbase/vmpi/"kernel".

I hesitate to add yet another module when the distinction I've introduced between AVMPI and VMPI already seems so tenuous. (I will address that a bit in a subsequent comment; maybe I can make you feel better about that). But now that you mention it, yes, of course MMgc is the wrong place. It _could_ go into core, but that would be an undesirable reverse dependency from MMgc to core. That leaves AVMPI, which is certainly better than MMgc and no worse than vmbase.  Sound reasonable, assuming I can better justify the sense of AVMPI?
(In reply to comment #44)
> One potentially substantive change aligns the Windows implementation of
> VMPI_getDaylightSavingsTA with the other platforms. The existing
> implementation (on Windows only) involved a call back into avmplus logic
> that I needed to resolve. I speculate that this is a vestige from the Win95
> era when the localtime() API might have been less dependable.

Abundance of caution: I would be interested in thoughts on this. AFIK it is the only change that isn't purely structural.
SInce my latest pull/merge, my project file changes are completely wrecked, but the gmake/manifest.mk files are in working order. (Funny how that works!)

I don't know the usual way to proceed here. Do those projects need to be fixed, or is the gmake system the only one I'm responsible for? If the project files need to be updated, I need some guidance/assistance for fixing/creating new projects for the visual studio changes. It seems I need new VS projects for the gtest, for the vmbase/kernel lib, and possibly also for the vmbase unit tests. That's quite a few project files (and targets) and I'm sure that there is a certain art to this that I should be aware of before I attempt it.

Soon enough gyp will solve all of that. Yay!
(In reply to comment #56)
> [The definition of Atom] _could_  go into core, but that would be an
> undesirable reverse dependency from MMgc to core.

But it does represent reality accurately: that the definition of Atom is properly the core's problem but nevertheless MMgc must know about it.

Anyway, my current plan is to move it to AVMPI.
(In reply to comment #56)
> I hesitate to add yet another module when the distinction I've introduced
> between AVMPI and VMPI already seems so tenuous. (I will address that a bit
> in a subsequent comment; maybe I can make you feel better about that).

While the separation between VMPI and AVMPI was driven by by mechanical considerations (separability), the point of separating it was to make certain generally-useful things generally available, without introducing a necessary dependency on the VM and MMgc.

What emerged was:

VMPI - OS abstraction of broad utility
  - malloc/free-level memory abstraction
  - threading and synchronization support (VMPI_getVMPageSize is needed in windows to get the guard page size).
  - time functions
  - logging functions (which should move to an instrumentation module eventually?)
  - debugging-related functions

AVMPI - Things more narrowly-targetted to VM/GC-specific services, whose implementations may include MMgc/vmmplus headers and call their functions.
  - MMU-level memory-management services
  - Executable memory
  - Stack frame analysis
  - MMgc/VM-specific instrumentation (AVMPI_isMemoryProfilingEnabled, etc).
  - Atom (I believe we concluded :) )

Regarding dependencies back from AVMPI into MMgc, for instance AVMPI_allocateCodeMemory depends on the GCHeap.

Arguably, if we take a minimal view of vmbase/vmpi, we might exclude time and logging functions. If you'd rather those move to AVMPI, I believe that could be done. I hesitate to create yet a third place!

The division turns out to be reasonably rational, and if you think that moving certain categories makes it more rational, I can attempt that (there may be dependencies I haven't discovered, though). For instance, moving the time functions to AVMPI would allow me to restore the previous Windows implementation of VMPI_getDaylightSavingsTA, reducing the risk). But I think the current division represents a reasonable balance of general functionality versus more specific MMgc and VM support. support.
One more loose end: As of now, I have vmbase and vmpi independent of feature configuration ifdefs except for VMCFG_SAFEPOINTS. I'm trying to figure out what to do about that, and I'm wondering whether there's any reason not to just always have that on. That is: remove the ifdefs and just have the safepoints code in all the time.
(In reply to comment #59)
> (In reply to comment #56)
> > [The definition of Atom] _could_  go into core, but that would be an
> > undesirable reverse dependency from MMgc to core.

IMO the atom definitions belong in core, because that's where atom originates and sees its main uses.

> But it does represent reality accurately: that the definition of Atom is
> properly the core's problem but nevertheless MMgc must know about it.

Precisely.

> Anyway, my current plan is to move it to AVMPI.

Why can't it go in core?  Are there technical problems?  The "undesirable reverse dependency" seems to me primarily a problem of principle, and maybe a problem of standalone unit testing, not a problem of practice - modules have interdependencies / cyclic dependencies as a matter of routine.  The principle of layered design is a good one, but breaks down often enough that we need to be able to cope with it without moving code to places where it does not belong just to uphold the fiction of the layers.

(As long as atom's not in MMgc I probably don't care too much; I would however advise you to find a reviewer other than me for a change that puts it into anything but core since I'm likely to be skeptical and inclined to put up a fight.)
(In reply to comment #62)
> (In reply to comment #59)
> > (In reply to comment #56)
> > > [The definition of Atom] _could_  go into core, but that would be an
> > > undesirable reverse dependency from MMgc to core.
> 
> IMO the atom definitions belong in core, because that's where atom
> originates and sees its main uses.
> 
> > But it does represent reality accurately: that the definition of Atom is
> > properly the core's problem but nevertheless MMgc must know about it.
> 
> Precisely.
> 
> > Anyway, my current plan is to move it to AVMPI.
> 
> Why can't it go in core?  Are there technical problems?

None whatsoever; I was just waiting for guidance on that point. :)
Attached patch Patch for review and, ultimately, submission (obsolete) — — Splinter Review 
Patch intended for final review and submission.

This patch file excludes the gtest directories since they are too large; the total patch including those files can be created by differencing:

  revisions 5ad00a8fab63 and a3213c39b785
  in http://asteam.macromedia.com/hg/users/stan/sandbox

This patch also does not include updates to the project files; I will work on those while the review is in process and submit a separate patch or, if preferred, an update to this one.
Attachment #532071 - Attachment is obsolete: true
Attachment #539286 - Flags: review?(lhansen)
Attached patch Patch for review and, ultimately, submission (obsolete) — — Splinter Review 
Patch intended for final review and submission.

This patch file excludes the gtest directories since they are too large; the total patch including those files can be created by differencing:

  revisions 5ad00a8fab63 and a3213c39b785
  in http://asteam.macromedia.com/hg/users/stan/sandbox

This updated patch include updates to the project files for VisualStudio, XCode, and Eclipse.
Attachment #539286 - Attachment is obsolete: true
Attachment #539286 - Flags: review?(lhansen)
Attachment #539395 - Flags: review?(lhansen)
(In reply to comment #65)
>the total patch including those files can be created by differencing:
>   revisions 5ad00a8fab63 and a3213c39b785
>   in http://asteam.macromedia.com/hg/users/stan/sandbox

Oops.  Copy/paste fail.  Make that:
The total patch including gtest files can be created by differencing:
   revisions 48caaeeb3dc5 and b237aec76bd6
   in http://asteam.macromedia.com/hg/users/stan/sandbox
Attachment #539395 - Attachment is obsolete: true
Attachment #539395 - Flags: review?(lhansen)
Please hold off on the review a bit. It seems that fixing the project files and synching up to head messed up the Windows builds.
Patch intended for final review and submission.

This patch file excludes the gtest directories since they are too large; the total patch including those files can be created by differencing:

  revisions 4ad8ee76a335 and 2529571bfdfa
  in http://asteam.macromedia.com/hg/users/stan/sandbox

This (re)updated patch includes updates to the project files for VisualStudio, XCode, and Eclipse and corrects the Windows build issues that somehow snuck at the last merge.
Attachment #539570 - Flags: review?(lhansen)
Blocks: 664771
Stan: I probably won't be able to do this review today, but will almost certainly get to it on Monday.
(Review notes)

MMgc: OK.

VMPI:
- Some TODOs left in VMPI.h (which is fine with me since I don't necessarily
  agree with the gist of those comments :-)
- WinPortUtils.cpp, UpdateTimeZoneInfo uses VMPI_lockAcquire/lockRelease,
  isn't there a block-scoped macro that could/should be used instead?  (style.)
- WinPortUtils.cpp, VMPI_getDaylightSavingsTA has been rewritten in a big way.
  This method is notoriously difficult to get right.  I don't know why I should
  belive that the rewritten version is correct / equivalent to the old version,
  need to look into that further.

Core: OK.

nanojit: OK, though see next item.

platform:
- system-selection.h: The removal/inlining of the nanojit CPU detection
  here appears unmotivated, since presumably that CPU detection worked
  just fine.  What's accomplished?
- mac/mac-platform.h: The motivation for these changes is unclear, especially
  now that carbon and PPC is no longer supported.  Probably just missing
  cleanup?

shell: OK

vmbase: OK.
I'm in favor of R+, but I will await feedback on the motivation for the rewrite of VMPI_getDaylightSavingsTA.
> platform:
> - system-selection.h: The removal/inlining of the nanojit CPU detection
>   here appears unmotivated, since presumably that CPU detection worked
>   just fine.  What's accomplished?

Based on an IM conversation with Stan, I beleive the goal was avoiding a dependency on nanojit.h.  I'll let him provide detail.
(In reply to comment #70)
> (Review notes)
> VMPI:
> - Some TODOs left in VMPI.h (which is fine with me since I don't necessarily
>   agree with the gist of those comments :-)

Our requirement is for the foundation to be entirely separable from the rest of AVM; strictly a layer beneath it (and other things). These are a few remaining entanglements to be addressed (somehow) in a subsequent run at this.

> - WinPortUtils.cpp, UpdateTimeZoneInfo uses VMPI_lockAcquire/lockRelease,
>   isn't there a block-scoped macro that could/should be used instead? 
> (style.)

Scoped locks require full-fledged monitors which require initialization. Spinlocks don't require any initialization (except to be zero). Also, I'm not particularly happy with VMPI calling "up" into vmbase.

> - WinPortUtils.cpp, VMPI_getDaylightSavingsTA has been rewritten in a big
> way.
>   This method is notoriously difficult to get right.  I don't know why I
> should
>   belive that the rewritten version is correct / equivalent to the old
> version,
>   need to look into that further.

Well, yes, it's a big change but the net effect of it is a massive simplification and to rely on the system implementation the exactly same way as the other platforms do. I mentioned this in comment 44 in a previous review:

  "The existing implementation (on Windows only) involved a call back into avmplus logic that I needed to resolve. I speculate that this is a vestige from the Win95 era when the localtime() API might have been less dependable."

The greater mystery is why the Windows implementation of localtime() was deemed inadequate. I strongly suspect this code to be a historical vestige. You can argue that _not_ using localtime() risks causing a skew between the host OS/browser's concept of the time and the VM's.

Anyway, the solutions that seem feasible are:
 - take this change and demystify the code (my preference, obviously)
 - duplicate a bunch of timezone code from AVM into WinPortUtils (yuck).
 - move all the time-related functionality to AVMPI, which can call into AVM core if it must.

> 
> Core: OK.
> 
> nanojit: OK, though see next item.
> 
> platform:
> - system-selection.h: The removal/inlining of the nanojit CPU detection
>   here appears unmotivated, since presumably that CPU detection worked
>   just fine.  What's accomplished?

As Edwin commented below.

> - mac/mac-platform.h: The motivation for these changes is unclear, especially
>   now that carbon and PPC is no longer supported.  Probably just missing
>   cleanup?

CoreServices/CoreServices.h is needed for this file and it was being satisfied in a roundabout way from a file that is no longer being included.

The commented-out include of <assert.h> can now be deleted. I missed that.

> 
> shell: OK
> 
> vmbase: OK.
(In reply to comment #73)
> The greater mystery is why the Windows implementation of localtime() was
> deemed inadequate. I strongly suspect this code to be a historical vestige.
> You can argue that _not_ using localtime() risks causing a skew between the
> host OS/browser's concept of the time and the VM's.

I was able to trace that code back to P4 CL 123147 (6/11/04), when it was originally added in essentially its present form.

At that time, the Mac implementation was in terms of OS-9 PQIs and the Windows implementation avoided libc functions and went directly to the Win32 APIs as well. That may have been well motivated at the time; it is surely less so now.

When I compare to WebKit's JavaScriptCore implementation (DataPrototype.cpp), it relies on WTF's GregorianDateTime (DateMath.cpp) which uses localtime* (in calculateUTCOffset()) to compute the difference between the system's idea of the hour and minute (including its notion of DST) and the non-dst-adjusted local time (where our implementations simply check tm_isdst, which seems more straightforward.

* In fact, WebKit prefers the localtime_s and localtime_r variants where available, which we should do as well (on all platforms) because localtime is not thread-safe.

I will make those changes right now!
(In reply to comment #74)
> In fact, WebKit prefers the localtime_s and localtime_r variants where
> available, which we should do as well (on all platforms) because localtime
> is not thread-safe.
> 
> I will make those changes right now!

I just noticed that gmtime in PosixPortUtils.cpp should be gmtime_r also. I'll post an incremental patch to the patch shortly.
In responding to review comments, I realized that the Posix code (and now the Windows code once I aped the Posix implementation there) had a thread-safety issue.

This patch is an increment to the earlier patch. A cumulative patch can be made by differencing revisions 3a6c9599459b and 2529571bfdfa in
    http://asteam.macromedia.com/hg/users/stan/sandbox
Attachment #540680 - Flags: review?(lhansen)
Attachment #540680 - Flags: review?(lhansen) → review+
Attachment #539570 - Flags: review?(lhansen) → review+
Collapsed into a single changeset and pushed to TR as 6408:fc85aeb22f5e

(Leaving to Stan to close this bug if we're done)
(In reply to comment #77)
> Collapsed into a single changeset and pushed to TR as 6408:fc85aeb22f5e

Correction: TR 6408:c86f37feef86
Thanks, Steven!
(In reply to comment #78)
> (In reply to comment #77)
> > Collapsed into a single changeset and pushed to TR as 6408:fc85aeb22f5e
> 
> Correction: TR 6408:c86f37feef86

Correction: TR 6409:c86f37feef86
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 666613
argh.

at least one of our third party libraries defines static_assert for itself, and we only seem to encounter this on AIR Android.

(The definition in question is in third_party/ustl/ustl-1.2/typet.h)

I am currently planning to work-around this by conditionally-defining our own static_assert.  I am unhappy about the situation.
(reopening ticket so that I have a place to post my patch for review.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note 1: the inclusion of the third_party's definition comes after that of VMAssert.h's definition, so it is not sufficient to surround the definition in VMAssert.h with a ifndef guard.

Note 2: I don't think the third_party definition of static_assert is even compatible with ours (it seems like it is at first blush, since it takes the same number and type of parameters, but then when you actually try it out the compiler blows up at how we are using it within MMGC_STATIC_ASSERT definition).

My plan is still to conditionally-define one of the static asserts; the difference is now I'm putting the condition around the third_party definition, because of the above two issues.

I see that solely as a short-term hack, though.  I think the right approach is to just name it VMPI_static_assert, which should be sufficiently robust protection against future occurrences of this sort of problem.  (I suspect the rule should be that anything defined in the interface-header files should belong to a clearly delineated namespace, most likely via a prefix in the name, because of issues like this.)
(In reply to comment #73)
> (In reply to comment #70)
> > - mac/mac-platform.h: The motivation for these changes is unclear, especially
> >   now that carbon and PPC is no longer supported.  Probably just missing
> >   cleanup?
> 
> CoreServices/CoreServices.h is needed for this file and it was being
> satisfied in a roundabout way from a file that is no longer being included.

I don't think we can unconditionally include CoreServices.h.  Doing so causes the AIR_IOS builds to break.  The include file chain ends up failing like so:

  /Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator4.2.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/WebServicesCore.framework/Headers/WSMethodInvocation.h:759: error: 'CFXMLTreeRef' has not been declared

Reverting this portion of the patch:

-#if defined(AVMPLUS_MAC_CARBON) || defined(AVMPLUS_PPC)
-    #include <CoreServices/CoreServices.h>   // for MakeDataExecutable
-#endif
+#include <CoreServices/CoreServices.h>   // for MakeDataExecutable
+

makes the problem go away (though I do not yet know if it causes problems on the other builds).

Stan, can you clarify whether keeping the #ifdef guard would actually break things for you?  And if so, can you figure out a minimal #ifdef guard that would work (since, as I said at the outset, I do not think unconditional inclusion will work).
An aside: After spending three days adding the refactor .cpp files to the various project files, I suspect that some of the names could be revised to follow a more regular convention.

For example, where does AVMPI_freeCodeMemory live?  Well, its either in:

  MMgcPortWin.cpp, or
  PosixMMgcPortUtils.cpp

Say what?  Does the OS go on the front or the end?

(But such revision should wait until after we have gyp; the last three days have been painful.)
(In reply to comment #84)

> Reverting this portion of the patch:
> 
> -#if defined(AVMPLUS_MAC_CARBON) || defined(AVMPLUS_PPC)
> -    #include <CoreServices/CoreServices.h>   // for MakeDataExecutable
> -#endif
> +#include <CoreServices/CoreServices.h>   // for MakeDataExecutable
> +
...
> Stan, can you clarify whether keeping the #ifdef guard would actually break
> things for you? 

That guard is OK. I must have assumed it was vestigial. Apparently not.
(In reply to comment #86)
> (In reply to comment #84)
> 
> > Reverting this portion of the patch:
> > 
> > -#if defined(AVMPLUS_MAC_CARBON) || defined(AVMPLUS_PPC)
> > -    #include <CoreServices/CoreServices.h>   // for MakeDataExecutable
> > -#endif
> > +#include <CoreServices/CoreServices.h>   // for MakeDataExecutable
> > +
> ...
> > Stan, can you clarify whether keeping the #ifdef guard would actually break
> > things for you? 
> 
> That guard is OK. I must have assumed it was vestigial. Apparently not.

(There are other changes coming down the pipeline; I'll wait to apply them until after I actually get a full sandbox run to completion successfully.)
(In reply to comment #81)
> at least one of our third party libraries defines static_assert for itself,
> and we only seem to encounter this on AIR Android.
> 
> (The definition in question is in third_party/ustl/ustl-1.2/typet.h)
> 
> I am currently planning to work-around this by conditionally-defining our
> own static_assert.  I am unhappy about the situation.

I'd hope we can manage to retain static_assert without renaming, as discussed in comment 20 and 25.

Seems ustl doesn't have a way NOT to define static_assert, but it may be feasible to put #undef static_assert before the includes for ustl. There's a fair chance that there is a good bottleneck in our headers somewhere.
(In reply to comment #88)
> (In reply to comment #81)
> > at least one of our third party libraries defines static_assert for itself,
> > and we only seem to encounter this on AIR Android.
> > 
> > (The definition in question is in third_party/ustl/ustl-1.2/typet.h)
> > 
> > I am currently planning to work-around this by conditionally-defining our
> > own static_assert.  I am unhappy about the situation.
> 
> I'd hope we can manage to retain static_assert without renaming, as
> discussed in comment 20 and 25.
> 
> Seems ustl doesn't have a way NOT to define static_assert, but it may be
> feasible to put #undef static_assert before the includes for ustl. There's a
> fair chance that there is a good bottleneck in our headers somewhere.

And then redefine our own static_assert after that?  See note 2 of comment 83; ustl's static_assert appears incompatible with our own use of the form.

The work-around I ended up employing was to modify ustl's header file so that it only defines static_assert if its not already defined.

(I don't find comment 20 and comment 25 terribly convincing as an argument for using the generic name, but I also don't care terribly much about the problem either way; its not a current blocking issue for the merge.)
(In reply to comment #73)
> (In reply to comment #70)
> > - mac/mac-platform.h: The motivation for these changes is unclear, especially
> >   now that carbon and PPC is no longer supported.  Probably just missing
> >   cleanup?
> 
> CoreServices/CoreServices.h is needed for this file and it was being
> satisfied in a roundabout way from a file that is no longer being included.
> 
> The commented-out include of <assert.h> can now be deleted. I missed that.

Apparently our iOS target was depending on that (Debug-conditionalized) include of <assert.h>.  Or at least, I was asked to put it back.

(It could also be that it would suffice to put an #include of <VMAssert.h> ahead of <VMPI.h> in just the right places, but I did not want to block the iOS Debug build to investigate that approach.)

I guess I will keep taking notes here about the fallout from the change until I think its all landed.  (The fallout, that is.)
The Player smoke tests have broken due to the change to VMPI_getDaylightSavingsTA.

Filed followup in Bug 668442.
changeset: 6439:e7ccea930c76
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 645878: Fix windows 32-bit builds (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/e7ccea930c76
Tangential remark: I know Stan mentioned from the start that gtest was part of this.  My question: Do we really need to put the gtest source code into the tamarin tree?  Is there another option available?  (E.g. could we put it into a separate repo and then utilize hgsub extension to make it a subrepository?)
Flags: flashplayer-qrb+
Depends on: 670924
changeset: 6452:a81addf0b027
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 668442: rollback slice of patch for Bug 645878 to match FP code state (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/a81addf0b027
changeset: 6461:0064647d010a
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 668442: rollback slice of patch for Bug 645878 to match FP code state (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/0064647d010a
changeset:   6690:c66d2f1e0877
user:        Edwin Smith <edwsmith@adobe.com>
date:        Mon Oct 31 09:29:44 2011 -0400
summary:     Bug 645878 - Tamarin Foundation Utilities

http://hg.mozilla.org/tamarin-redux/rev/c66d2f1e0877
Tamarin is a dead project now. Mass WONTFIX.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 6 years ago
Resolution: --- → WONTFIX
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: