Closed Bug 464643 Opened 12 years ago Closed 11 years ago

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

Categories

(Tamarin Graveyard :: Virtual Machine, defect)

x86_64
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: lhansen, Assigned: lhansen)

Details

Attachments

(1 obsolete file)

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.
Apparently there is also a problem with the MOPS with -Dinterp on Windows.  Could be a different problem there, haven't investigated yet.
Blocks: 465737
The Windows problem is very likely something else, see https://bugzilla.mozilla.org/show_bug.cgi?id=466529.
No longer blocks: 465737
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.)
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))
This is also in Watson Express (high priority) as #1926333.
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).
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
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.
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.
Hardware: x86 → x86_64
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.
(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.)
>> 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.
Blocks: 464476
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.
Attachment #358705 - Flags: review?
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.
Attachment #358705 - Flags: review? → review?(lhansen)
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?
Attachment #358705 - Flags: review?(lhansen) → review+
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.
Attachment #358705 - Flags: review+ → review?(stejohns)
Attachment #358705 - Flags: review?(stejohns) → review+
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.
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.
Ah. I get it, my bad. I'll check, but go ahead and push.
I can't find any evidence that Flash depends on this, so you're good to go.
Comment on attachment 358705 [details] [diff] [review]
removes gross LPTR hack, but still uses core->eceptionAddr

pushed to tamarin-redux
changeset:   1354:41fc2ca2041a
Attachment #358705 - Attachment is obsolete: true
No longer blocks: 464476
Still open? Looks like fix has been pushed.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.