Skip to content
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

[MachineBasicBlock] Fix use after free in SplitCriticalEdge #68786

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

perlfu
Copy link
Contributor

@perlfu perlfu commented Oct 11, 2023

Remove use after free when attempting to update SlotIndexes in MachineBasicBlock::SplitCriticalEdge.

Add delegate mechanism for MachineBasicBlock to observe instructions added or removed when calling target specific functions for updating branches and update SlotIndexes appropriately.

Remove use after free when attempting to update SlotIndexes in
MachineBasicBlock::SplitCriticalEdge.

Add delegate mechanism for MachineBasicBlock to observe instructions
added or removed when calling target specific functions for updating
branches and update SlotIndexes appropriately.
- Use MachineFunction delegate instead
@@ -14,6 +14,7 @@
#define LLVM_CODEGEN_MACHINEBASICBLOCK_H

#include "llvm/ADT/GraphTraits.h"
#include "llvm/ADT/SmallPtrSet.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

LGTM. I like how simple this is. Just some minor nits inline.

Copy link
Contributor

@MatzeB MatzeB left a comment

Choose a reason for hiding this comment

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

LGTM

llvm/lib/CodeGen/MachineBasicBlock.cpp Outdated Show resolved Hide resolved
llvm/lib/CodeGen/MachineBasicBlock.cpp Outdated Show resolved Hide resolved
- Remove unnecessary include
- Change MBB pointer to MF reference
- Rename Delegate
- Tidy constructor parameter names
@qcolombet
Copy link
Collaborator

I was wary that MachineFunction only admits a single delegate, so we might overlap with another user.
However looking at it again, the only user I can see is GlobalISel, and this shouldn't overlap.
I guess we could always extend MachineFunction in the future to allow multiple delegates.

Exactly!

Thanks for the fix!

@perlfu perlfu merged commit e1bb059 into llvm:main Oct 15, 2023
2 of 3 checks passed
perlfu added a commit to perlfu/llvm-project that referenced this pull request Oct 18, 2023
Follow up fix for llvm#68786 to address that MachineFunction
handleInsertion is actually called before a new instruction has
been inserted into the block. Hence new instructions must be
recorded and SlotIndex updates performed after the delegate call.
perlfu added a commit that referenced this pull request Oct 21, 2023
Follow up fix for #68786 to address that MachineFunction handleInsertion
is actually called before a new instruction has been inserted into the
block. Hence new instructions must be recorded and SlotIndex updates
performed after the delegate call.
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.

None yet

4 participants