464643 - [redux] AMD64 longjmp hack needs reengineering on non-Windows platforms

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

There is a hack in place in ExceptionFrame::throwException() to handle longjmp values for AVMPLUS_AMD64; it uses a (large) fixed-size buffer to record exception information.  (It could appear that the problem is that longjmp can only pass int values, not pointer values.)  The size of the buffer is set at 64K elements.  When the program triggers more than 64K exceptions an assertion fails (in debug builds) and the VM halts.  Presumably in release mode it starts overwriting memory.

The mops test cases throw more exceptions than that, hence they fail on 64-bit mac builds.  There is a custom solution in place for WIN64 builds, apparently, so Windows is not affected but all other 64-bit platforms are.

Comment 1

[Lars T Hansen]

Apparently there is also a problem with the MOPS with -Dinterp on Windows.  Could be a different problem there, haven't investigated yet.

Comment 2

[Lars T Hansen]

The Windows problem is very likely something else, see https://bugzilla.mozilla.org/show_bug.cgi?id=466529.

Comment 3

[Lars T Hansen]

The patch for https://bugzilla.mozilla.org/show_bug.cgi?id=466691 adds a simple check that prevents crashes when the fixed-size buffer overflows in release builds - exiting instead.  (In debug builds the AvmAssert should stop it.)

Comment 4

[Edwin Smith]

From looking at the code, it doesn't look like we really need ExceptionFrame::lptr[].  we can store the exception in core->exceptionAddr, and just pass 1 to longjmp(), just test setjmp() for 0/1, and always load from exceptionAddr.  

codegenMIR needs to change to something like:

loadIns(MIR_ld, (uintptr)&core->exceptionAddr, InsConst(0))

Comment 5

[Lars T Hansen]

This is also in Watson Express (high priority) as #1926333.

Comment 6

[Lars T Hansen]

Summary of email discussion:

It sounds like this is really something that belongs in a porting layer.  Using our naming convention (VMPI_ prefix for porting layer code) we'd have something like this:

  // 32-bit systems

  #define VMPI_setjmp(loc,buf)     ::setjmp(buf)
  #define VMPI_longjmp(loc,buf,v)  ::longjmp(buf, v)

  // 64-bit systems

  #define VMPI_setjmp(loc,buf) \
    (loc = 0, ::setjmp(buf), loc)

  #define VMPI_longjmp(buf,v) \
    (loc = (v ? v : 1), ::longjmp(buf, 1))

The variable 'loc' is of type uintptr_t.  The user of setjmp and the user of longjmp must obviously agree on the loc.  In practice, it would be a field inside an AvmCore structure, ie, a typical use would be

  if (VMPI_setjmp(core->longjmpValue, jmp_buf)) ...

and

  VMPI_longjmp(core->longjmpValue, jmp_buf, v)

All that remains is for everyone to agree to go through these macros.

If everyone agrees that the loc is always inside an AvmCore structure then the first argument could just be the core object; depends on who might be using these functions outside the AVM (it's a general, and generally useful, abstraction).

Comment 7

[Edwin Smith]

I've heard no objections and this sounds like a good plan.  I was just scratching my head again today looking at the old hacky code.

Comment 8

[Lars T Hansen]

So my understanding is that an AvmCore instance may not always be available, as clients (notably the Player) sometimes refrains from creating it, but may still need the longjmp functionality.  So the loc should just be an uintptr_t name.

I'll implement the basic functionality but as the API is effectively changing there will be other work to be done in existing client code bases, I expect.

Comment 9

[Rick Reitmaier]

This seems safer as I found some early docs stating "If the return is from a call to longjmp(), setjmp() returns a non-zero value." So it's probably safer not to rely on the return value of setjmp.

Comment 10

[Lars T Hansen]

(In reply to comment #9)
> This seems safer as I found some early docs stating "If the return is from a
> call to longjmp(), setjmp() returns a non-zero value." So it's probably safer
> not to rely on the return value of setjmp.

What do you mean by "This" at the beginning above?  Do you mean that the setjmp replacement should be used on 32-bit systems as well?  I've never seen any problems there.

(Tamarin does not depend on the value returned by setjmp except for the zero/nonzero distinction, but I had the impression from Tinic that the Player does need to send back real values.)

Comment 11

[Rick Reitmaier]

>> What do you mean by "This" 

I was referring the proposal above.

WRT the usage in the player, that's why I was suggesting a non-reliance on the value.  It would be worthwhile checking this change against player semantics.

Comment 12

[Edwin Smith]

Attachment: removes gross LPTR hack, but still uses core->eceptionAddr

this patch fixes the bug directly, but doesn't improve portability of setjmp.

- updated mir to load Exception* from core->exceptionAddr. now all setjmp callers in tamarin are just checking setjmp != 0 and loading from exceptionAddr.  I left the longjmp64() case in place for windows-x64, pending testing.

- not tested on windows x64 yet.

this patch unblocks me for x64 nanojit but i'm up for integrating with the VMPI_setjmp patch if we want to get it all taken care of at once.

Comment 13

[Edwin Smith]

the jit's need to call VMPI_setjmp, which means we need to put the address of the function in a table, so the requirement of VMPI macros being like functions is extra strict in this case.  given that setjmp is the one function not easily wrapped, this seems doable.

Comment 14

[Lars T Hansen]

Comment on attachment 358705 [details] [diff] [review]
removes gross LPTR hack, but still uses core->eceptionAddr

Looks OK.

Wasn't there a problem with the Player using this API too and relying on the existing semantics, though?

Comment 15

[Edwin Smith]

Comment on attachment 358705 [details] [diff] [review]
removes gross LPTR hack, but still uses core->eceptionAddr

I ran it by Tinic (who wrote the lptr hack) a few weeks ago and he said he wasn't aware of the core->exceptionAddr workaround, and it could be deleted.  

doing r?stejohns just to be really sure.

Comment 16

[Steven Johnson]

Comment on attachment 358705 [details] [diff] [review]
removes gross LPTR hack, but still uses core->eceptionAddr

Both you and Tinic are more familiar with this than me, so my review is hardly necessary. Still, I see no obvious flaw, so r+ it is.

Comment 17

[Edwin Smith]

I was hoping you could validate that indeed the player isn't depending on the old lptr hack.  but the stars are aligning and i'm going to push this.

Comment 18

[Steven Johnson]

Ah. I get it, my bad. I'll check, but go ahead and push.

Comment 19

[Steven Johnson]

I can't find any evidence that Flash depends on this, so you're good to go.

Comment 20

[Edwin Smith]

Comment on attachment 358705 [details] [diff] [review]
removes gross LPTR hack, but still uses core->eceptionAddr

pushed to tamarin-redux
changeset:   1354:41fc2ca2041a

Comment 21

[Steven Johnson]

Still open? Looks like fix has been pushed.

Comment 22

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.