We saw a failure caused by unwinding with incomplete CFIs, so we
can't outline CFI instructions when they are needed in EH.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
We this change we now don't have any tests that show a difference compared with just if (MI.isPosition()) return outliner::InstrType::Illegal;
Please add a test that showcases that new, more fined grained, outlining criteria for position instructions.
Thanks for your advice, but what I changed in this patch is just the way to handle CFIs when unwinding is needed.
The original code is:
if (MI.isPosition()) { // We can manually strip out CFI instructions later. if (MI.isCFIInstruction()) return outliner::InstrType::Invisible; return outliner::InstrType::Illegal; }
These conditions existed since the first version of outlining implementation (D66210), so we won't see any differences of position instructions even if I added some tests in this patch.
It's been a few days since my original comment so I may be misremembering the details but here's my perspective:
- Before your patch we had test coverage that showed the difference between a plain if (MI.isPosition()) return outliner::InstrType::Illegal; and the then-current implementation.
- With this patch we no longer have.
- With this patch you are asserting that we want something more fine-grained than just "don't outline positions". Then let's have test coverage for that more fine-grained condition checking.
Also, in general, I'm in favor of the position that when you touch things you should take the opportunity to address closely related issues, within reason. That's always a tricky balance, and I don't know what the LLVM project policy is for that fine line, but it seems like something that we should strive for :)
Thank you for your explanation, I misunderstood what you meant :)
So I added some more fine-grained tests in D123364:
- machine-outliner-cfi.mir shows how we treat CFIs if there is no EH.
- machine-outliner-throw.ll shows how we treat CFIs if unwinding is needed.
- machine-outliner-position.mir shows that position instructions(which are labels) are illegal to outline.
Are there some other things that I should pay attention to?