Ticket #470 (closed todo: fixed)

Opened 6 years ago

Last modified 5 years ago

[BUG] branch_cs opcode is buggy and unused

Reported by: cotto Owned by: whiteknight
Priority: normal Milestone:
Component: core Version:
Severity: medium Keywords: ops
Cc: Language:
Patch status: Platform:

Description

The branch_cs opcode causes a nice segfault when called with a random string. When I looked into it a little more deeply, it became apparent that this opcode isn't used at all during make test. If it's worth fixing and testing then we should do that. Otherwise it needs to be deprecated.

Attachments

kill_branch_cs.patch Download (3.0 KB) - added by whiteknight 5 years ago.
patch to remove branch_cs opcode

Change History

Changed 6 years ago by whiteknight

  • status changed from new to assigned
  • owner set to whiteknight
  • component changed from none to core
  • keywords ops added
  • type changed from bug to RFC

Can we get some kind of answer about this one before the release? Do we want to deprecate branch_cs, or do we want to fix/test it? If the former, I would like to get in the deprecation notice before tuesday.

Changed 6 years ago by coke

FYI, deprecation notices must be present in stable releases before something can be removed. Next stable release is 1.4; next monthly release is 1.2.

Changed 6 years ago by whiteknight

Okay, got the answer from Allison today in #ps. We're removing this opcode. I'll put in the deprecation notice ASAP.

Changed 5 years ago by coke

  • type changed from RFC to todo

from src/runcore/main.c:

 /* if we have fallen out with resume and we were running CGOTO, set
         * the stacktop again to a sane value, so that restarting the runloop
         * is ok. */
        if (interp->resume_flag & RESUME_RESTART) {
            if ((int)interp->resume_offset < 0)
                Parrot_ex_throw_from_c_args(interp, NULL, 1,
                    "branch_cs: illegal resume offset");
            stop_prederef(interp);
        }

branch_ch is just a string here, but does this mean this whole conditional can be removed? If not, can someone suggest a better error message?

Changed 5 years ago by whiteknight

patch to remove branch_cs opcode

Changed 5 years ago by whiteknight

I've attached a patch that should perform the removal of this opcode (except for the error message that Coke pointed out above).

DEPRECATED.pod says that this is eligible in 1.5, although I think I added that notice so I could have been wrong. Can I remove this now, or do we need to wait till after the August release?

Changed 5 years ago by coke

This can be removed now. Go for it.

Changed 5 years ago by cotto

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

branch_cs was removed in r40432.

Note: See TracTickets for help on using tickets.