-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-105481: Generate the opcode lists in dis from data extracted from bytecodes.c #106758
Conversation
… opcode.py with the functions in _opcode.py
Maybe in this case actually the new list is incorrect - all the opcodes with DEREF seem to belong to hasfree rather than haslocal:
|
Lib/opcode.py
Outdated
|
||
opname = ['<%r>' % (op,) for op in range(MAX_PSEUDO_OPCODE + 1)] | ||
for op, i in opmap.items(): | ||
opname[i] = op | ||
|
||
# _opcode may not be ready during early stages of the build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that separate from Larry's limerick at the top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same thing. I'm planning to try and get rid of this issue - figure out what is needed in the build and separate it out somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bot we've gotta keep a limerick in. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it in the stable ABI?
Ugh. Worse, now LOAD_CLOSURE is gone, it’s ambiguous. Who uses this? How? (Only dis.py?) |
LOAD_CLOSURE is just an alias (pseudo op) for LOAD_FAST. It's in the haslocal list in the old and new version, no change there. |
Now it's like this: Old:
New:
The "<148>" is because in opcode.py in main we have In the haslocal list, we see that the old list is missing the super instructions. |
For the other lists: Old:
New:
old is missing the Old:
New:
Old is missing Old:
New:
New has For hasarg, the old list contained a few that the new list does not:
|
It mostly looks right, except for |
My point is that LOAD_CLOSURE used to be separate because it was in the hasfree list. Now some LOAD_FAST instances refer to a local and others to a free variable. Example:
The LOAD_FAST (a) at offset 26 used to be a LOAD_CLOSURE in 3.12 and before (and in 3.13 until LOAD_CLOSURE was made a pseudo-op). It's harmless, because we have I don't think |
Looks like a relic. In 3.11, 148 was LOAD_CLASSDEREF. That opcode no longer exists in 3.12 (it was removed in gh-103764 for PEP 695). So let's get rid of it. |
So we merge hasfree into haslocal and remove hasfree? Or leave it empty and soft deprecate it? |
Hm, I was just writing up a rationale for this when I realized that we should just keep |
Ok, then this PR is probably fine w.r.t. hasfree and haslocal. The only list in oplists now is hascompare, which contains only COMPARE_OP:
It is used by dis to identify that its op (>=, ==, etc) needs to be displayed. |
Since there's only one opcode like this, maybe dis can just check whether it's that opcode? Or do you anticipate that in the future there will be multiple ones? (Were there ever?) |
It's just this one all the way back to 3.8. dis can stop using the list, but it's documented in the dis module so can we remove it? |
I guess deprecate it like hasjabs/hasjrel. |
|
||
opname = ['<%r>' % (op,) for op in range(MAX_PSEUDO_OPCODE + 1)] | ||
for op, i in opmap.items(): | ||
opname[i] = op | ||
|
||
# The build uses older versions of Python which do not have _opcode.has_* functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite mysterious. IIRC Brandt (?) wrote some hack that reads opcode.py and then execs it? But I can't find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in Tools/build/generate_opcode_h.py. But when it execs opcode.py that tries to import _opcode and call its has_arg etc to construct the oplists (which are not used during the build).
Ideally we should just get rid of generate_opcode_h.py if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to import _opcode into opcode at all. We can just import it directly into dis (which is not used by any build scripts). I'll try that.
Lib/test/test__opcode.py
Outdated
self.assertIsInstance(func(op), bool) | ||
self.assertEqual(func(op), expected) | ||
|
||
EXPECTED_OPLISTS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that each time we add or remove an opcode we need to update this test? That feels tedious. (Even though it looks like it was always done like this and you're just refactoring that code a bit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past the list was built in opcode.py so we would need to make sure we added i there. Now the lists are derived automatically from bytecodes.c via heuristics, and you will need to update the test (which you are less likely to be able to do incorrectly). I'm not sure it's safe to not have these full tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. So the point of the heuristics is that we don't have to manually maintain it, but the tests are manually maintained to check that the heuristics are still working. If they are, and if an instruction is added (or deleted) that changes one of these lists, we'll get a bogus test failure, and hopefully the contributor who gets to debug the test failure understands they have to fix the test. I think this warrants a comment right there where we expect the test to fail (I think on line 102, self.assertEqual(res, op in expected)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manually maintained lists were not tested and had some errors. We could remove the tests and trust people to examine the diff in the metadata file when they make changes. If we do that we should remove it from the autogenerated list so that it's not collapsed in a PR diff.
…ode_metadata rather than via opcode
…ad of opcode for _specialized_instructions
I think I need to add in whatsnew that the dis module's opcode lists are no longer also defined in the opcode module. The opcode module is not documented, but if you look on GitHub you can see some places where people import it. |
Or maybe this is a breaking change that I can't make without deprecation? I'm not sure what the status of the opcode module is. I can put the lists back in opcode, and instead work to remove the dependency of the build on opcode. |
I have a feeling that opcode is actually a better place than dis for the official public API. And that's probably why people have been importing it despite its undocumented status. |
OK, I reverted the last few commits so we leave everything in opcode. We will rid the build of opcode in a future PR once the opcode.h file is generated from bytecodes.c. |
The generated lists are more correct than the (old) hard-coded ones, which were not maintained properly.
For example:
Old:
New: