Movatterモバイル変換

brandtbucher requested a review fromericsnowcurrently

May 3, 2023 21:39

brandtbucher self-assigned this

bedevere-bot mentioned this pull request

_Py_RefcntAdd doesn't respect immortality#104142

Closed

bedevere-bot added the awaiting core review label

brandtbucher added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Copy link

bedevere-bot commentedMay 3, 2023

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commitbe3b103 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Copy link

Member

ericsnowcurrently commentedMay 4, 2023

CC@eduardo-elizondo

Copy link

MemberAuthor

brandtbucher commentedMay 4, 2023

(Not sure why buildbots aren't running...)

corona10 previously approved these changes

bedevere-bot added awaiting merge and removed awaiting core review labels

cpython/Modules/_testcapi/immortal.c

Copy link

Member

corona10 commentedMay 4, 2023•
edited
Loading

@brandtbucher~~Ah would you like to add the C API test in here too?~~

Lines 34 to 38 in9885677

	staticPyMethodDeftest_methods[]= {
	{"test_immortal_builtins",test_immortal_builtins,METH_NOARGS},
	{"test_immortal_small_ints",test_immortal_small_ints,METH_NOARGS},
	{NULL},
	};

~~Adding a code line somewhere of here would be enough.~~

cpython/Modules/_testcapi/immortal.c

Lines 5 to 10 in9885677

	assert(_Py_IsImmortal(object));
	Py_ssize_told_count=Py_REFCNT(object);
	for (intj=0;j<10000;j++) {
	Py_DECREF(object);
	}
	Py_ssize_tcurrent_count=Py_REFCNT(object);

corona10 dismissed theirstale review

May 4, 2023 02:11

#104143 (comment)

bedevere-bot added awaiting review and removed awaiting merge labels

Copy link

Member

corona10 commentedMay 4, 2023•
edited
Loading

Ah sorry please ignore#104143 (comment)
it's not public API...

corona10 approved these changes

bedevere-bot added awaiting merge and removed awaiting review labels

eduardo-elizondo reviewed

Include/internal/pycore_object.h

Comment on lines +61 to +63

		if (_Py_IsImmortal(op)) {
		return;
		}

Copy link

Contributor

eduardo-elizondoMay 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

One quick comment here is that if this were to be in a hot path, I would recommend using the same technique that we use forPy_INCREF to minimize the number of generated instructions to do the check and add:https://github.com/python/cpython/blob/main/Include/object.h#L620-L634

Now, given that it's only in use forsq_repeat in tuple and list, I don't think this is perf sensitive so keeping the code as you have it here should be good.

eduardo-elizondo reviewed

Lib/test/test_builtin.py

		self.assertEqual(sys.getrefcount(immortal), self.IMMORTAL_REFCOUNT)

		def test_immortals(self):
		for immortal in self.IMMORTALS:

Copy link

Contributor

eduardo-elizondoMay 4, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Small nit: I generally prefer rolling out assertions in unit tests to make it easier to isolate by line-number in the place where the test failed. But I don't think there's a standard in the code base so I'm fine either way

Copy link

MemberAuthor

brandtbucherMay 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah, I'd like comprehensive testing of these immortals though, and I don't want to roll out ~250 different assertions for each test.

I figured making each one asubTest inassert_immortal is a pretty good compromise.

Copy link

Contributor

eduardo-elizondo commentedMay 4, 2023•
edited
Loading

Nice catch, I could have sworn that I had added the check to_Py_RefcntAdd as well but I somehow missed it. I added two comments but they are minor so this LGTM.

Thanks for the fix!

eduardo-elizondo approved these changes

Copy link

Member

sunmy2019 commentedMay 4, 2023

LGTM, all increments toob->ob_refcnt should be guarded now.

Copy link

Member

JelleZijlstra commentedMay 4, 2023

There is a USan failure:

Include/object.h:227:12: runtime error: member access within address 0x7fff7ae78b80 with insufficient space for an object of type 'PyObject' (aka 'struct _object')0x7fff7ae78b80: note: pointer points here b8 7f 00 00  00 47 a8 c2 97 55 00 00  00 b5 59 7e 78 cd 65 f5  00 8a ea c2 b8 7f 00 00  39 7a 90 c0              ^ SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Include/object.h:227:12 in make: *** [Makefile:1106: checksharedmods] Error 1

Not sure what that means, but the line is in _Py_IsImmortal (https://github.com/brandtbucher/cpython/blob/py-refcnt-add-immortal/Include/object.h#L227), so it may be real.

Merge branch 'main' into py-refcnt-add-immortal

1471b39

JelleZijlstra added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Copy link

bedevere-bot commentedMay 4, 2023

🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commit1471b39 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label