Decoding instructions should handle ENTER_EXECUTOR

@markshannon

There's a variety of places where we walk over an array of instructions, using some variant of

for (int i = 0; i < num_instructions;) {
    int opcode = instructions[i].op.code;
    // <decode EXTENDED_ARG>
    opcode = _PyOpcode_Deopt[opcode];  // or _Py_GetBaseOpcode(code, i)
    // <handle opcode>
    i += 1 + _PyOpcode_Caches[opcode];  // or _PyInstruction_GetLength(code, i)
}

All these make the mistake that if opcode is ENTER_EXECUTOR, it may obscure an underlying JUMP_BACKWARD instruction, which has a cache entry.

I fixed this in gh-107256 for _PyInstruction_GetLength() and it was first reported in gh-107082, but there are a number of other occurrences. Basically every time we consult _PyOpcode_Caches we should ensure that the index is not ENTER_EXECUTOR.

CC: @markshannon

Places where I found this:

There are possibly others.

Linked PRs

@markshannon

Open question. Should ENTER_EXECUTOR be replaced by _Py_GetBaseOpcode() or is that its caller's responsibility? In deopt_code() the caller handles it, but in other cases the caller doesn't, but (probably) should. But you could also argue that the fix is to switch to using _PyInstruction_GetLength() instead of 1 + _PyOpcode_Caches[opcode].

@markshannon Do you have an opinion?

pythongh-107265: Fix code_richcompare to lookup actual opcode when EN…

Merge remote-tracking branch 'upstream/main' into pythongh-107265

I am working on code_hash too

Merge remote-tracking branch 'upstream/main' into pythongh-107265

gh-107265: Fix code_richcompare for ENTER_EXECUTOR case (gh-108165)

pythongh-107265: Fix code_hash for ENTER_EXECUTOR case

Ideally, everything would use the same set of functions to get instruction lengths, opcodes and opargs.

The interaction between instrumentation and ENTER_EXECUTOR is more than just the ENTER_EXECUTOR itself, as we need to invalidate any executor if any of the instructions that it covers is instrumented.

I think we can remove _PyFrame_OpAlreadyRan altogether. If not now, definitely when PEP 667 is implemented.

gh-107265: Fix code_hash for ENTER_EXECUTOR case (#108188)

I think we can remove _PyFrame_OpAlreadyRan altogether. If not now, definitely when PEP 667 is implemented.

But PEP 667 seems in limbo (and rather ambitious, so I wouldn't want to wait for it if we can drop _PyFrame_OpAlreadyRan).

I don't understand at all what _PyFrame_OpAlreadyRan does or why, or if there's a test for it. I added

assert(check_opcode != ENTER_EXECUTOR);

to it and am now running the test suite with -Xuops always on; we'll see in about an hour how that goes. (Hum, well, if at least one standard CI job runs runs with assertions enabled.)

So the entire test suite run with -Xuops hard-coded and a hard-coded check in _PyFrame_OpAlreadyRan that check_opcode != ENTER_EXECUTOR found nothing, so maybe there's some good reason why that function isn't a problem. In any case it's not a priority.

Guido, IIUC, we don't need to fix de_instrument.
Because de_instrument is only called from removetools and it requires that opcode should have event.
I will send a PR to add one line of assert statement for the context.

Please let me know if I miss something.

cpython/Python/instrumentation.c

Lines 705 to 727 in 154477b

    
           static void 
        
           remove_tools(PyCodeObject * code, int offset, int event, int tools) 
        
           { 
        
               assert(event != PY_MONITORING_EVENT_LINE); 
        
               assert(event != PY_MONITORING_EVENT_INSTRUCTION); 
        
               assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event)); 
        
               assert(opcode_has_event(_Py_GetBaseOpcode(code, offset))); 
        
               _PyCoMonitoringData *monitoring = code->_co_monitoring; 
        
               if (monitoring && monitoring->tools) { 
        
                   monitoring->tools[offset] &= ~tools; 
        
                   if (monitoring->tools[offset] == 0) { 
        
                       de_instrument(code, offset, event); 
        
                   } 
        
               } 
        
               else { 
        
                   /* Single tool */ 
        
                   uint8_t single_tool = code->_co_monitoring->active_monitors.tools[event]; 
        
                   assert(_Py_popcount32(single_tool) <= 1); 
        
                   if (((single_tool & tools) == single_tool)) { 
        
                       de_instrument(code, offset, event); 
        
                   } 
        
               } 
        
           }

cpython/Python/instrumentation.c

Lines 121 to 127 in 154477b

    
           opcode_has_event(int opcode) 
        
           { 
        
               return ( 
        
                   opcode != INSTRUMENTED_LINE && 
        
                   INSTRUMENTED_OPCODES[opcode] > 0 
        
               ); 
        
           }

gh-107265: Fix initialize/remove_tools for ENTER_EXECUTOR case (gh-10…

Revert "pythongh-107265: Fix initialize/remove_tools for ENTER_EXECUT…

pythongh-107265: Remove all ENTER_EXECUTOR when execute _Py_Instrument

Merge remote-tracking branch 'upstream/main' into pythongh-107265-mark

gh-107265: Remove all ENTER_EXECUTOR when execute _Py_Instrument (gh-…

About _PyFrame_OpAlreadyRan we don't have to do anything if it is removed: faster-cpython/ideas#623 (comment)

GH-107265: Add missing deoptimizations for ENTER_EXECUTOR's original …

pythonGH-107265: Add missing deoptimizations for ENTER_EXECUTOR's ori…

pythongh-107265: Fix _PyFrame_OpAlreadyRan for ENTER_EXECUTOR case

PEP 667 landed for 3.13, and a code search indicates _PyFrame_OpAlreadyRan is gone, so is there anything left to resolve for this issue?

(This issue came up while doing a search for "PEP 667" to check for issues that could be closed now that has been implemented)

pythonGH-107265: Add missing deoptimizations for ENTER_EXECUTOR's ori…

We now have _Py_GetBaseCodeUnit which returns the underlying code unit, handling instrumentation and ENTER_EXECUTOR, and is used in (almost?) all places it is needed.

Navigation Menu

Repository navigation

Decoding instructions should handle ENTER_EXECUTOR #107265

Linked PRs

14 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	static void
	remove_tools(PyCodeObject * code, int offset, int event, int tools)
	{
	assert(event != PY_MONITORING_EVENT_LINE);
	assert(event != PY_MONITORING_EVENT_INSTRUCTION);
	assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event));
	assert(opcode_has_event(_Py_GetBaseOpcode(code, offset)));
	_PyCoMonitoringData *monitoring = code->_co_monitoring;
	if (monitoring && monitoring->tools) {
	monitoring->tools[offset] &= ~tools;
	if (monitoring->tools[offset] == 0) {
	de_instrument(code, offset, event);
	}
	}
	else {
	/* Single tool */
	uint8_t single_tool = code->_co_monitoring->active_monitors.tools[event];
	assert(_Py_popcount32(single_tool) <= 1);
	if (((single_tool & tools) == single_tool)) {
	de_instrument(code, offset, event);
	}
	}
	}

	opcode_has_event(int opcode)
	{
	return (
	opcode != INSTRUMENTED_LINE &&
	INSTRUMENTED_OPCODES[opcode] > 0
	);
	}

Decoding instructions should handle ENTER_EXECUTOR #107265

Description

Linked PRs

Activity

gvanrossum commented on Jul 25, 2023

corona10 commented on Aug 21, 2023

markshannon commented on Aug 21, 2023

markshannon commented on Aug 21, 2023

gvanrossum commented on Aug 21, 2023

gvanrossum commented on Aug 22, 2023

corona10 commented on Aug 23, 2023

14 remaining items

corona10 commented on Sep 13, 2023

ncoghlan commented on Aug 1, 2024

markshannon commented on Jan 7, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions