Skip to content

[CIR][Lowering] Erase op through rewriter instead of directly #853

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

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

smeenai
Copy link
Collaborator

@smeenai smeenai commented Sep 18, 2024

Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes clang/test/CIR/CodeGen/pointer-arith-ext.c with ASAN.

Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
@smeenai
Copy link
Collaborator Author

smeenai commented Sep 18, 2024

Both this and #819 were caused by incorrectly erasing a redundant op during lowering. A potential alternative would be leaving those ops alone during lowering and running DCE afterwards to eliminate them. Would that be workable, and if so, what would be the pros and cons compared to the current way?

@Lancern
Copy link
Member

Lancern commented Sep 18, 2024

A potential alternative would be leaving those ops alone during lowering and running DCE afterwards to eliminate them.

Apparently if the operation you are lowering has side effects, you have to erase it or DCE won't help. On the other hand, it may (?) work to keep only those pure operations and rely on DCE to eliminate them after lowering, but we may hit verification errors since after lowering all value types are converted to LLVM types and CIR operations don't accept LLVM value types.

BTW I also met this problem before in #809, see #809 (comment). However I was working on the CIR simplify pass rather than the LLVM lowering pass. In the context of any kind of rewriting you have to erase operations through the rewriter or UAF could occur. I believe we don't have to introduce an additional round of DCE to just avoid such a programming error.

Copy link
Member

@Lancern Lancern left a comment

Choose a reason for hiding this comment

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

LGTM

@smeenai smeenai merged commit 759de62 into llvm:main Sep 18, 2024
6 of 7 checks passed
@smeenai smeenai deleted the fix-sub-erase branch September 18, 2024 19:50
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
smeenai added a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
smeenai added a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
smeenai added a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
lanza pushed a commit that referenced this pull request Nov 5, 2024
Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
lanza pushed a commit that referenced this pull request Mar 18, 2025
Directly erasing the op causes a use after free later on, presumably
because the lowering framework isn't aware of the op being deleted. This
fixes `clang/test/CIR/CodeGen/pointer-arith-ext.c` with ASAN.
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.

2 participants