Skip to content

Conversation

@WolframAlph
Copy link
Contributor

@WolframAlph WolframAlph commented Dec 27, 2024

@WolframAlph WolframAlph changed the title gh-128295: Add value and backoff to CACHE disassembly` gh-128295: Add value and backoff to CACHE disassembly Dec 27, 2024
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

This definitely needs a What's New entry as well as a .. versionchanged:: next for the various dis functions.

I also don't really know how this could help for debugging though, because we only fancy render the very first CACHE entry. In addition, are there situations where the value or the backoff are nonzero and are rendered? because otherwise it wouldn't really help IMO.

Comment on lines +463 to +466
data_int = int.from_bytes(data, sys.byteorder)
argrepr = f"{name}: {data_int}"
if name == "counter":
argrepr += f" (value: {data_int >> 4}, backoff: {data_int & 15})"
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be extracted into a helper function in case we want to add more information later.

@WolframAlph
Copy link
Contributor Author

@picnixz

are there situations where the value or the backoff are nonzero and are rendered?

What do you mean? Value and backoff are rendered no matter what they are, zero or non-zero. Idea is to extract and show those components in disassembly. Since counter is 16bit integer with low 4 bits as backoff and 12 high bits as value and it requires you to figure out manually what those numbers are when debugging

@picnixz
Copy link
Member

picnixz commented Dec 28, 2024

What do you mean? Value and backoff are rendered no matter what they are, zero or non-zero.

The problem is that we only fancy render the instruction when it's the first entry of a CACHE, as mentioned in this comment:

# Only show the fancy argrepr for a CACHE instruction when it's
# the first entry for a particular cache value:

So what I want to see is a test case which renders something else than a counter of 0 and a backoff of 0.

@WolframAlph
Copy link
Contributor Author

WolframAlph commented Dec 28, 2024

@picnixz

So what I want to see is a test case which renders something else than a counter of 0 and a backoff of 0.

I see.

I also don't really know how this could help for debugging though, because we only fancy render the very first CACHE entry.

If it is counter - it is first & single cache entry for this particular value, if this is what you are asking

@picnixz
Copy link
Member

picnixz commented Dec 29, 2024

If it is counter - it is first & single cache entry for this particular value, if this is what you are asking

Arf, sorry I misunderstood the comment. NVM on this one. Now for the test:

I want a test where what is being rendered is not (value: 0, backoff: 0) but (value: XXX, backof: YYY). AFAIR, this requires specialized bytecode (otherwise, it will always be 0).
The test you modified is actually a test for the CLI. Instead, we should validate in another test function the following (or any other kind of code, doesn't really matter):

>>> dis.dis('print()', show_caches=True)
0           RESUME                   0

1           LOAD_NAME                0 (print)
            PUSH_NULL
            CALL                     0
            CACHE                    0 (counter: 0)
            CACHE                    0 (func_version: 0)
            CACHE                    0
            RETURN_VALUE
>>> dis.dis('print()', show_caches=True, adaptive=True)
0           RESUME                   0

1           LOAD_NAME                0 (print)
            PUSH_NULL
            CALL                     0
            CACHE                    0 (counter: 17 (value: 1, backoff: 0))
            CACHE                    0 (func_version: 0)
            CACHE                    0
            RETURN_VALUE

You might also want to update test_show_caches() to check for the counter pattern as well.

@WolframAlph
Copy link
Contributor Author

@picnixz

AFAIR, this requires specialized bytecode (otherwise, it will always be 0).

Inline CACHE exists only for adaptive bytecodes so counter CACHE is present for every one of them

I want a test where what is being rendered is not (value: 0, backoff: 0) but (value: XXX, backof: YYY)

Makes sense, but I would wait if this feature gets accepted, otherwise no point in adding tests now

@picnixz
Copy link
Member

picnixz commented Dec 29, 2024

Inline CACHE exists only for adaptive bytecodes so counter CACHE is present for every one of them

Yes, but what I meant is that the output of --show-caches in this case will not help you (I mean, if your counter is 0, you don't need to know that it's value and backoff is 0 IMO, you can do the maths). This was more an observation for the tests (the test you modified didn't show specialized bytecode).

I would wait if this feature gets accepted, otherwise no point in adding tests now

I think it always makes sense to add test (and if one wants to wait for the feature to be accepted, I wouldn't have written a PR, but that's my own opinion). Btw, I'm neutral on this feature (for those who can make >> 4 and & 0xff on the fly, it wouldn't be useful but I don't know how many users manage to directly convert a number into its hexadecimal and binary representation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants