Ticket #102 (closed patch: fixed)

Opened 6 years ago

Last modified 5 years ago

cpu_ret segfaults

Reported by: coke Owned by: whiteknight
Priority: major Milestone:
Component: imcc Version:
Severity: high Keywords: imcc
Cc: Language:
Patch status: new Platform: all

Description

While the opcode says "don't use", if you do, you get a segfault:

.sub main :main cpu_ret .end

On feather, this generates a segfault. Run with -G, it generates "Illegal Instruction".

Attachments

imcc_cpu_ret_patch.patch Download (0.5 KB) - added by whiteknight 6 years ago.
quick'n'dirty patch for this

Change History

Changed 6 years ago by coke

  • priority changed from normal to major
  • component changed from none to core
  • severity changed from medium to high

Changed 6 years ago by whiteknight

This is the desired behavior, or at least the behavior that I would expect from it. The cpu_ret opcode is a thin wrapper over an actual asm "ret" instruction. In the slow core, all opcodes are defined as functions with this prototype:

opcode_t * opcode_name(opcode_t *cur_opcode, PARROT_INTERP);

They should be returning a pointer to the next opcode to execute. The "ret" command is going to return from the function with whatever is in EAX (which is probably garbage) as the function's return value. The next iteration of the slow core will attempt to deference that garbage pointer and will probably run into a segfault.

This opcode is only useful when using JIT with CGP, and nowhere else (to my knowledge) attempting to use it in other places or directly in PIR with different cores will almost always cause a segfault or other terrible behavior. I would venture that IMCC/PIRC should be modified to disallow directly calling this opcode.

Changed 6 years ago by coke

parrot segfaulting is never the desired behavior.

Changed 6 years ago by whiteknight

  • owner set to whiteknight
  • status changed from new to assigned

The question isn't whether we should change or remove the cpu_ret opcode, because we shouldn't. The question is how we should make Parrot fail more gracefully if somebody does try to call this opcode when it's not supposed to be used (which is almost always). I'll try to throw together an IMCC patch for this later today to have Parrot reject these opcodes when they are found.

Changed 6 years ago by whiteknight

quick'n'dirty patch for this

Changed 6 years ago by whiteknight

  • keywords imcc added
  • platform set to all
  • component changed from core to imcc
  • type changed from bug to patch
  • patch set to new

I've attached a quick patch for this issue. It modifies imcc.l to catch any instances of "cpu_ret" and throw an exception instead of passing them on to the parser.

I don't have flex/bison on this machine, so I can't test this until I get home tonight.

Changed 6 years ago by coke

I am not allison, but that shouldn't the segfault prevention here go into cpu_ret (or whatever it calls), rather than preventing a single (granted, our most common) way of generating bytecode from using this op (and the others that say "don't use?")

Changed 6 years ago by NotFound

I think that preventing to generate it from pir has the only effect of making more difficult to test when we have a more general solution. A warning will be better, IMO.

Changed 5 years ago by whiteknight

  • status changed from assigned to closed
  • resolution set to fixed

committed a fix in r36411. Instead of segfaulting, it throws an exception to the effect of "neener neener neener, I told you not to use this". I also added a long comment to the file, which basically says "seriously, don't use this opcode. I'm warning you!"

This change doesn't appear to affect any tests using any of the runcores.

Note: See TracTickets for help on using tickets.