473188 - portable but not module specific assert macros

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

lets replace AvmAssert, GCAssert, and NanoAssert (and msg enabled variants) with VMPI_assert

(inspired by bug 473159)

Comment 1

[Lars T Hansen]

This does not make sense as long as (a) mmgc is independent of the rest of the VM and (b) the VMPI_ macros are notionally part of the vm, not of some lower-layer API layer (or the mmgc).  IMO we either need to dispense with the fiction that the two are independent, or we need to think up a strategy for a shared porting layer.  (I suspect doing both is right.)

Comment 2

[Edwin Smith]

I agree, lets dispense with the fiction that mmgc and the vm are independent, and work on a shared porting layer.  by the way, if this bug is blocking the wrong tracker, please help me find the right one.  I seem to recall a tracking bug for the porting layer but i couldn't find it with the obvious search terms.

Comment 3

[Edwin Smith]

incidentally if we do have a generic porting layer, then it could have an assert of its own, and MMgc and the VM could use it, regardless of MMgc's independence from the vm.

Comment 4

[Lars T Hansen]

Attachment: Preliminary patch

Moves the code for AvmAssert and friends into two new files, which for lack of a better place I have placed in VMPI.  They could live in core/, but given that the Player currently compiles MMgc separately from the rest of the VM, factoring seemed the right thing to do.

Makes the GC assert macros aliases for the now-common AvmAssert macros.

Comment 5

[Edwin Smith]

Comment on attachment 438384 [details] [diff] [review]
Preliminary patch

Cool.  with a tweak in nanojit.h to use avmplus::AvmAssertFail instead of NanoAssertFail, we also can eliminate AvmDebug.cpp/h.

Comment 6

[Lars T Hansen]

Attachment: Patch

Same as previous patch, but also changes project files, nanojit.h, and removes core/AvmDebug.{cpp,h}.

As a follow-on we'd want to consider changing GCAssert and so on to AvmAssert and so on, but that does not seem high priority right now.

Comment 7

[Steven Johnson]

Dumb question, why not name the moved functions VMPI_assert and such?

Comment 8

[Lars T Hansen]

The VMPI_ functions are for a platform interface (where the platform code should implement it).  VMPI_alloca is the exception but it's misnamed and we should fix that.  Anyway these macros are portable.

Comment 9

[Tommy Reilly]

Comment on attachment 438631 [details] [diff] [review]
Patch

The reality is that MMgc is used separately from the VM but in reality they are always compiled and linked in together.   Might behoove us to get rid of the notion of MMgc as a separate library and have all our clients linked against one project/library (avm).

Comment 10

[Edwin Smith]

Comment on attachment 438631 [details] [diff] [review]
Patch

looks, good, setting feedback flag for myself as a reminder to push the nanojit changes to nanojit-central along with a patch to nanojit/avmplus.h (for the lirasm utility).

Comment 11

[Lars T Hansen]

tamarin-redux changeset:   4411:5bab20b05ac8

Comment 12

[Edwin Smith]

Attachment: nanojit-central: Use AvmAssertFail instead of NanoAssertFail

This is for nanojit-central.  It modifies the definition of NanoAssert to call AvmAssertFail, which must be defined by the VM host.  For lirasm/tracemonkey AvmAssertFail goes in avmplus.h/cpp.

Ideally we need a better vm/nanojit api layer.  this patch takes a tiny step away from abstraction, but NanoAssert already relies on AvmLog, so for now this is tolerable.

Comment 13

[Andreas Gal :gal]

This will break TM in a trivial manner when we pull from n-c, but we can fix it then.

Comment 14

[Edwin Smith]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/7c682d3836f7
TR: http://hg.mozilla.org/tamarin-redux/rev/1bdc5515e0bf

(reopening solely for the sake of tracking the nanojit-central patch status)

Comment 15

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/2ffa3bdd61ee

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2ffa3bdd61ee