Skip to content

Commit 6693f73

Browse files
bpo-38187: Fix a refleak in Tools/c-analyzer. (gh-16304)
The "Slot" helper (descriptor) is leaking references due to its caching mechanism. The change includes a partial fix to Slot, but also adds Variable.storage to replace the problematic use of Slot. https://bugs.python.org/issue38187
1 parent 9055815 commit 6693f73

File tree

13 files changed

+208
-89
lines changed

13 files changed

+208
-89
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import os.path
2+
from test.support import load_package_tests
3+
4+
5+
def load_tests(*args):
6+
return load_package_tests(os.path.dirname(__file__), *args)

Lib/test/test_tools/test_c_analyzer/test_c_analyzer_common/test_known.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,6 @@ class FromFileTests(unittest.TestCase):
1515

1616
_return_read_tsv = ()
1717

18-
def tearDown(self):
19-
Variable._isglobal.instances.clear()
20-
2118
@property
2219
def calls(self):
2320
try:
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import os.path
2+
from test.support import load_package_tests
3+
4+
5+
def load_tests(*args):
6+
return load_package_tests(os.path.dirname(__file__), *args)

Lib/test/test_tools/test_c_analyzer/test_c_globals/test_find.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,9 @@ def test_typical(self):
6464
**self.kwargs))
6565

6666
self.assertEqual(found, [
67+
info.Variable.from_parts('dir1/spam.c', None, 'var1', 'int'),
6768
info.Variable.from_parts('dir1/spam.c', None, 'var2', 'static int'),
69+
info.Variable.from_parts('dir1/spam.c', None, 'var3', 'char *'),
6870
info.Variable.from_parts('dir1/eggs.c', None, 'var1', 'static int'),
6971
info.Variable.from_parts('dir1/eggs.c', 'func1', 'var2', 'static char *'),
7072
])
@@ -299,7 +301,7 @@ def test_typical(self):
299301
info.Variable.from_parts('src1/spam.c', None, 'var1', 'static const char *'),
300302
info.Variable.from_parts('src1/spam.c', None, 'var1b', 'const char *'),
301303
info.Variable.from_parts('src1/spam.c', 'ham', 'initialized', 'static int'),
302-
info.Variable.from_parts('src1/spam.c', 'ham', 'result', 'int'),
304+
info.Variable.from_parts('src1/spam.c', 'ham', 'result', 'int'), # skipped
303305
info.Variable.from_parts('src1/spam.c', None, 'var2', 'static PyObject *'),
304306
info.Variable.from_parts('src1/eggs.c', 'tofu', 'ready', 'static int'),
305307
info.Variable.from_parts('src1/spam.c', None, 'freelist', 'static (PyTupleObject *)[10]'),
@@ -318,6 +320,7 @@ def test_typical(self):
318320

319321
self.assertEqual(found, [
320322
info.Variable.from_parts('src1/spam.c', None, 'var1', 'static const char *'),
323+
info.Variable.from_parts('src1/spam.c', None, 'var1b', 'const char *'),
321324
info.Variable.from_parts('src1/spam.c', 'ham', 'initialized', 'static int'),
322325
info.Variable.from_parts('src1/spam.c', None, 'var2', 'static PyObject *'),
323326
info.Variable.from_parts('src1/eggs.c', 'tofu', 'ready', 'static int'),
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import os.path
2+
from test.support import load_package_tests
3+
4+
5+
def load_tests(*args):
6+
return load_package_tests(os.path.dirname(__file__), *args)

Lib/test/test_tools/test_c_analyzer/test_c_parser/test_info.py

Lines changed: 72 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
from ..util import PseudoStr, StrProxy, Object
55
from .. import tool_imports_for_tests
66
with tool_imports_for_tests():
7-
from c_analyzer_common.info import ID
7+
from c_analyzer_common.info import ID, UNKNOWN
88
from c_parser.info import (
99
normalize_vartype, Variable,
1010
)
@@ -31,38 +31,47 @@ class VariableTests(unittest.TestCase):
3131

3232
VALID_ARGS = (
3333
('x/y/z/spam.c', 'func', 'eggs'),
34+
'static',
3435
'int',
3536
)
3637
VALID_KWARGS = dict(zip(Variable._fields, VALID_ARGS))
3738
VALID_EXPECTED = VALID_ARGS
3839

3940
def test_init_typical_global(self):
40-
static = Variable(
41-
id=ID(
42-
filename='x/y/z/spam.c',
43-
funcname=None,
44-
name='eggs',
45-
),
46-
vartype='int',
47-
)
41+
for storage in ('static', 'extern', 'implicit'):
42+
with self.subTest(storage):
43+
static = Variable(
44+
id=ID(
45+
filename='x/y/z/spam.c',
46+
funcname=None,
47+
name='eggs',
48+
),
49+
storage=storage,
50+
vartype='int',
51+
)
4852

49-
self.assertEqual(static, (
50-
('x/y/z/spam.c', None, 'eggs'),
51-
'int',
52-
))
53+
self.assertEqual(static, (
54+
('x/y/z/spam.c', None, 'eggs'),
55+
storage,
56+
'int',
57+
))
5358

5459
def test_init_typical_local(self):
55-
static = Variable(
56-
id=ID(
57-
filename='x/y/z/spam.c',
58-
funcname='func',
59-
name='eggs',
60-
),
61-
vartype='int',
62-
)
60+
for storage in ('static', 'local'):
61+
with self.subTest(storage):
62+
static = Variable(
63+
id=ID(
64+
filename='x/y/z/spam.c',
65+
funcname='func',
66+
name='eggs',
67+
),
68+
storage=storage,
69+
vartype='int',
70+
)
6371

6472
self.assertEqual(static, (
6573
('x/y/z/spam.c', 'func', 'eggs'),
74+
storage,
6675
'int',
6776
))
6877

@@ -71,10 +80,12 @@ def test_init_all_missing(self):
7180
with self.subTest(repr(value)):
7281
static = Variable(
7382
id=value,
83+
storage=value,
7484
vartype=value,
7585
)
7686

7787
self.assertEqual(static, (
88+
None,
7889
None,
7990
None,
8091
))
@@ -89,34 +100,42 @@ def test_init_all_coerced(self):
89100
PseudoStr('func'),
90101
PseudoStr('spam'),
91102
),
103+
storage=PseudoStr('static'),
92104
vartype=PseudoStr('int'),
93105
),
94106
(id,
107+
'static',
95108
'int',
96109
)),
97110
('non-str 1',
98111
dict(
99112
id=id,
113+
storage=Object(),
100114
vartype=Object(),
101115
),
102116
(id,
117+
'<object>',
103118
'<object>',
104119
)),
105120
('non-str 2',
106121
dict(
107122
id=id,
123+
storage=StrProxy('static'),
108124
vartype=StrProxy('variable'),
109125
),
110126
(id,
127+
'static',
111128
'variable',
112129
)),
113130
('non-str',
114131
dict(
115132
id=id,
116-
vartype=('a', 'b', 'c'),
133+
storage=('a', 'b', 'c'),
134+
vartype=('x', 'y', 'z'),
117135
),
118136
(id,
119137
"('a', 'b', 'c')",
138+
"('x', 'y', 'z')",
120139
)),
121140
]
122141
for summary, kwargs, expected in tests:
@@ -134,43 +153,57 @@ def test_init_all_coerced(self):
134153
def test_iterable(self):
135154
static = Variable(**self.VALID_KWARGS)
136155

137-
id, vartype = static
156+
id, storage, vartype = static
138157

139-
values = (id, vartype)
158+
values = (id, storage, vartype)
140159
for value, expected in zip(values, self.VALID_EXPECTED):
141160
self.assertEqual(value, expected)
142161

143162
def test_fields(self):
144-
static = Variable(('a', 'b', 'z'), 'x')
163+
static = Variable(('a', 'b', 'z'), 'x', 'y')
145164

146165
self.assertEqual(static.id, ('a', 'b', 'z'))
147-
self.assertEqual(static.vartype, 'x')
166+
self.assertEqual(static.storage, 'x')
167+
self.assertEqual(static.vartype, 'y')
148168

149169
def test___getattr__(self):
150-
static = Variable(('a', 'b', 'z'), 'x')
170+
static = Variable(('a', 'b', 'z'), 'x', 'y')
151171

152172
self.assertEqual(static.filename, 'a')
153173
self.assertEqual(static.funcname, 'b')
154174
self.assertEqual(static.name, 'z')
155175

156176
def test_validate_typical(self):
157-
static = Variable(
158-
id=ID(
159-
filename='x/y/z/spam.c',
160-
funcname='func',
161-
name='eggs',
162-
),
163-
vartype='int',
164-
)
177+
validstorage = ('static', 'extern', 'implicit', 'local')
178+
self.assertEqual(set(validstorage), set(Variable.STORAGE))
179+
180+
for storage in validstorage:
181+
with self.subTest(storage):
182+
static = Variable(
183+
id=ID(
184+
filename='x/y/z/spam.c',
185+
funcname='func',
186+
name='eggs',
187+
),
188+
storage=storage,
189+
vartype='int',
190+
)
165191

166-
static.validate() # This does not fail.
192+
static.validate() # This does not fail.
167193

168194
def test_validate_missing_field(self):
169195
for field in Variable._fields:
170196
with self.subTest(field):
171197
static = Variable(**self.VALID_KWARGS)
172198
static = static._replace(**{field: None})
173199

200+
with self.assertRaises(TypeError):
201+
static.validate()
202+
for field in ('storage', 'vartype'):
203+
with self.subTest(field):
204+
static = Variable(**self.VALID_KWARGS)
205+
static = static._replace(**{field: UNKNOWN})
206+
174207
with self.assertRaises(TypeError):
175208
static.validate()
176209

@@ -185,6 +218,7 @@ def test_validate_bad_field(self):
185218
) + badch
186219
tests = [
187220
('id', ()), # Any non-empty str is okay.
221+
('storage', ('external', 'global') + notnames),
188222
('vartype', ()), # Any non-empty str is okay.
189223
]
190224
seen = set()
@@ -199,6 +233,8 @@ def test_validate_bad_field(self):
199233
static.validate()
200234

201235
for field, invalid in tests:
236+
if field == 'id':
237+
continue
202238
valid = seen - set(invalid)
203239
for value in valid:
204240
with self.subTest(f'{field}={value!r}'):
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import os.path
2+
from test.support import load_package_tests
3+
4+
5+
def load_tests(*args):
6+
return load_package_tests(os.path.dirname(__file__), *args)

Tools/c-analyzer/c_analyzer_common/_generate.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ def _known(symbol):
262262
raise
263263
if symbol.name not in decl:
264264
decl = decl + symbol.name
265-
return Variable(varid, decl)
265+
return Variable(varid, 'static', decl)
266266

267267

268268
def known_row(varid, decl):
@@ -291,7 +291,7 @@ def known_rows(symbols, *,
291291
except KeyError:
292292
found = _find_match(symbol, cache, filenames)
293293
if found is None:
294-
found = Variable(symbol.id, UNKNOWN)
294+
found = Variable(symbol.id, UNKNOWN, UNKNOWN)
295295
yield _as_known(found.id, found.vartype)
296296
else:
297297
raise NotImplementedError # XXX incorporate KNOWN

Tools/c-analyzer/c_analyzer_common/known.py

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -34,34 +34,41 @@ def from_file(infile, *,
3434
id = ID(filename, funcname, name)
3535
if kind == 'variable':
3636
values = known['variables']
37-
value = Variable(id, declaration)
38-
value._isglobal = _is_global(declaration) or id.funcname is None
37+
if funcname:
38+
storage = _get_storage(declaration) or 'local'
39+
else:
40+
storage = _get_storage(declaration) or 'implicit'
41+
value = Variable(id, storage, declaration)
3942
else:
4043
raise ValueError(f'unsupported kind in row {row}')
41-
if value.name == 'id' and declaration == UNKNOWN:
42-
# None of these are variables.
43-
declaration = 'int id';
44-
else:
45-
value.validate()
44+
value.validate()
45+
# if value.name == 'id' and declaration == UNKNOWN:
46+
# # None of these are variables.
47+
# declaration = 'int id';
48+
# else:
49+
# value.validate()
4650
values[id] = value
4751
return known
4852

4953

50-
def _is_global(vartype):
54+
def _get_storage(decl):
5155
# statics
52-
if vartype.startswith('static '):
53-
return True
54-
if vartype.startswith(('Py_LOCAL(', 'Py_LOCAL_INLINE(')):
55-
return True
56-
if vartype.startswith(('_Py_IDENTIFIER(', '_Py_static_string(')):
57-
return True
58-
if vartype.startswith('PyDoc_VAR('):
59-
return True
60-
if vartype.startswith(('SLOT1BINFULL(', 'SLOT1BIN(')):
61-
return True
62-
if vartype.startswith('WRAP_METHOD('):
63-
return True
56+
if decl.startswith('static '):
57+
return 'static'
58+
if decl.startswith(('Py_LOCAL(', 'Py_LOCAL_INLINE(')):
59+
return 'static'
60+
if decl.startswith(('_Py_IDENTIFIER(', '_Py_static_string(')):
61+
return 'static'
62+
if decl.startswith('PyDoc_VAR('):
63+
return 'static'
64+
if decl.startswith(('SLOT1BINFULL(', 'SLOT1BIN(')):
65+
return 'static'
66+
if decl.startswith('WRAP_METHOD('):
67+
return 'static'
6468
# public extern
65-
if vartype.startswith('PyAPI_DATA('):
66-
return True
67-
return False
69+
if decl.startswith('extern '):
70+
return 'extern'
71+
if decl.startswith('PyAPI_DATA('):
72+
return 'extern'
73+
# implicit or local
74+
return None

0 commit comments

Comments
 (0)