Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor: deduplicate "can this unwind?" logic #65303

Closed
RalfJung opened this issue Oct 11, 2019 · 8 comments
Closed

Refactor: deduplicate "can this unwind?" logic #65303

RalfJung opened this issue Oct 11, 2019 · 8 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Currently, we have two separate codepaths to decide if a function needs an abort-on-panic shim and if it gets the nounwind attribute. This risks them getting out of sync -- and indeed they are out-of-sync right now, but there's lots of opposition to fix that critical bug, so I closed #63884. But I still think that refactor should happen eventually, hence this issue. That PR, as well as #63909, also have some testcases that could be added once a proper solution has been implemented.

Cc @BatmanAoD (not sure who else is in the team for this)

@jonas-schievink jonas-schievink added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 11, 2019
@BatmanAoD
Copy link
Member

I'm not sure this sort of implementation work is within the purview of the unwind-FFI project charter, but I agree with you that the refactor and fix should happen.

@RalfJung
Copy link
Member Author

With #76570 having landed, these two functions are in sync again, so there might again be a potential for code deduplication. I did not yet take a closer look though.

@cratelyn
Copy link

Hi! I would be happy to investigate this and fix it if possible. 🙂

@RalfJung
Copy link
Member Author

@cratelyn great, you're now much more familiar with this code than I am anyway. :)

I am talking about the duplication between this code and this code.

@RalfJung
Copy link
Member Author

@alexcrichton does your PR #86155 also solve this? It sounds a bit like it does, since I think fn_can_unwind is used for more things; it is not clear to me if should_abort_on_panic inside MIR building is also still a thing -- but I guess that got replaced by the new pass, and that pass uses fn_can_unwind?

@alexcrichton
Copy link
Member

Ah yes that PR would indeed close this. I removed the should_abort_on_panic flag during MIR building and the logic that computes that flag (which was the duplication of fn_can_unwind)

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2021

@alexcrichton your PR changed since you first posted it; does it still close this issue in the form that now landed?

@alexcrichton
Copy link
Member

I believe so, yes, there's only one deduction location in the compiler now for whether a function can unwind or not.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants