Skip to content

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jan 2, 2020

@pablogsal
Copy link
Member Author

pablogsal commented Jan 2, 2020

@markshannon Could you take a look at this when you have time? I am not happy about this solution as is just adding more shrinkwrap but solving the general case is not immediate. Do you have a better idea for now?

@nedbat Could you check if this breaks anything else in your test suite?

@nedbat
Copy link
Member

nedbat commented Jan 2, 2020

All my tests pass with this change, thanks!

@pablogsal
Copy link
Member Author

pablogsal commented Jan 3, 2020

s/incorrent/incorrect/

Thanks for the catch!

Will correct the other one in a separate PR!

@markshannon
Copy link
Member

Unless I am misunderstanding the code, this seems rather fragile as it relies on the compiler emitting the async for exception handler non-aligned with a line and all other exception handlers aligned with a new line.
That seems an unreasonable restraint on the compiler and is not documented anywhere.

Again, it seems like we need a way to signal that instructions are artificial and should be ignored by tracing.

@pablogsal
Copy link
Member Author

pablogsal commented Jan 6, 2020

all other exception handlers aligned with a new line.

Not exactly, because other exception handlers will have needs_line_update = True.

That seems an unreasonable restraint on the compiler and is not documented anywhere.

Yup, that is why I said:

I am not happy about this solution as is just adding more shrinkwrap


Again, it seems like we need a way to signal that instructions are artificial and should be ignored by tracing.

Yup, I fully agree, but is not going to be that immediate, because we need to manage also pushing and poping blocks in the frame in the eval loop from artificial instructions.

How do you want to proceed? Do you want to close this and wait until we have a more resilient solution or you want to merge this (and the tests) meanwhile?

@markshannon
Copy link
Member

I'm going to merge this.
It isn't perfect , but it is an improvement and it doesn't prevent further improvements.
@nedbat, @pablogsal Any final comments before I merge?

@nedbat
Copy link
Member

nedbat commented Jan 9, 2020

None from my side. I'm still wondering if there's a lowish-cost way to transfer some of the coverage.py tests to the CPython test suite, to shrink the feedback loop.

@pablogsal
Copy link
Member Author

I'm still wondering if there's a lowish-cost way to transfer some of the coverage.py tests to the CPython test suite, to shrink the feedback loop.

I have plans to work on this soonish, I will let you know once I have some free cycles. :)

@pablogsal
Copy link
Member Author

Any final comments before I merge?

Nop, thanks for the review! Let's sync in the future to try to work on a more resilient solution :)

@markshannon markshannon merged commit 4c53e63 into python:master Jan 10, 2020
@bedevere-bot
Copy link

@markshannon: Please replace # with GH- in the commit message next time. Thanks!

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@pablogsal pablogsal deleted the bpo-39166 branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants