DWARF CFI Checker prototype #148296

amsen20 · 2025-07-11T21:01:09Z

In this RFC, the design behind this prototype is explained.

This prototype implements the full algorithm explained in the RFC, as opposed to the current DWARFCFIChecker in LLVM, which is only capable of implementing the invalidation part of the algorithm.

The main difference is that this implementation has access to semantic information through BOLT MCPlusBuilder, which is an extension of MCInstrAnalysis providing the following information:

matches, which can be used to see if an instruction is an add/sub/multiple/... and what the arguments are, are they const or registers.
Helper functions for extracting load and storing memory address targets.
And a lot more information about control flow.

Using this information, this implementation is capable of applying the abstract execution mentioned in the RFC properly.
Consider the following example:

pushq   %rdi # spill rdi
.cfi_adjust_cfa_offset 8
.cfi_rel_offset %rdi, 0
pushq   %rsi # spill rsi
.cfi_adjust_cfa_offset 8
.cfi_rel_offset %rsi, 0

callq some_func

popq    %rsi # load rsi
.cfi_adjust_cfa_offset -8
.cfi_same_value %rdi # prototype: error: loading %rdi from wrong location
popq    %rdi # spill rdi
# (landed and prototype) error: changed register RDI, that register RDI's unwinding rule uses, but there is no CFI directives about it
.cfi_adjust_cfa_offset -8
.cfi_same_value %rsi # prototype: error: loading %rsi from wrong location

In this example, two registers (rdi and rsi) are spilled but are loaded in reverse order. The prototype can identify when rdi and rsi are loaded from incorrect memory locations, a capability not shared by the currently landed checker, which only detects modifications to rdi, with its unwinding rule set to same_value.

To fix the issue, we need to add more semantic information to MCInst. There are a couple of options proposed in the RFC. I recommend the first option as my personal choice, which is:
Option 1: Extend MCInstrAnalysis with functionalities currently in BOLT’s MCPlus.
The reason to choose this option is that it’s the only truly incremental option that has already been implemented, tested, and explored by BOLT.

…mple CFI analysis

…/Write conditions

- Changing the register and not informing it using cfi directives - Changing the CFA and report it wrong - Load a register from another register stored location

paschalis-mpeis

Hey @amsen20,

I've only read the introduction to your proposal and it looks interesting!
Thanks for your effort. I'll try to have a better look in the coming days.

I know this is only a draft, but regarding the couple of 'nit' bolt changes, those could be added in a separate patch?

paschalis-mpeis · 2025-07-14T05:44:38Z

bolt/lib/Utils/CommandLineOpts.cpp

@@ -146,7 +146,7 @@ cl::opt<bool> Lite("lite", cl::desc("skip processing of cold functions"),
                   cl::cat(BoltCategory));

 cl::opt<std::string>
-OutputFilename("o",
+OutputFilename("ooo",


Maybe this is renamed by mistake?

It was a temporary fix for linking the llvm-mc tool with BOLT, when the linker tried to link them, having the same CL option with the same value caused errors.

amsen20 · 2025-07-14T16:01:33Z

Hey @amsen20,

I've only read the introduction to your proposal and it looks interesting! Thanks for your effort. I'll try to have a better look in the coming days.

I know this is only a draft, but regarding the couple of 'nit' bolt changes, those could be added in a separate patch?

This draft is not meant to be merged; the reviewers are added automatically because I changed parts of BOLT's code.
The minimal version of the checker has landed in this PR.
However, I was wondering if any of the BOLT owners are interested in moving MCPlusBuilder to the main LLVM MC layer. In the RFC, I have discussed that the main bottleneck of the analysis is the semantic information, which only MCPlusBuilder in BOLT currently possesses.

Thank you for your time. Let me know if you have any suggestions/ideas on how we should add more semantic information to the MC layer.

amsen20 · 2025-07-16T21:14:27Z

Hey @amsen20,
I've only read the introduction to your proposal and it looks interesting! Thanks for your effort. I'll try to have a better look in the coming days.
I know this is only a draft, but regarding the couple of 'nit' bolt changes, those could be added in a separate patch?

This draft is not meant to be merged; the reviewers are added automatically because I changed parts of BOLT's code. The minimal version of the checker has landed in this PR. However, I was wondering if any of the BOLT owners are interested in moving MCPlusBuilder to the main LLVM MC layer. In the RFC, I have discussed that the main bottleneck of the analysis is the semantic information, which only MCPlusBuilder in BOLT currently possesses.

Thank you for your time. Let me know if you have any suggestions/ideas on how we should add more semantic information to the MC layer.

@paschalis-mpeis
Just wanted to remind you in case you missed this.

Thanks

Amirhossein Pashaeehir added 22 commits May 6, 2025 17:45

Forward CFI directives alongside their associated instruction to a si…

211ec57

…mple CFI analysis

Create a new CFI analysis for each dwarf separately

29325f7

Separate the logic and display

946b5c6

Compile llvm mc depending on bolt

2376ee8

Extract semantic info on MCInst from bolt

2cfa1be

Typo in evaluateStackOffsetExpr doc

f01a9fd

Add a DS to keep track of each register CFI state

610e393

Check if the CFA change is correct or not

ccf43ad

Add the CFI validation as a option to llvm-mc tool

aa95e82

Provide the directives as an arrayref instead of index range

fc88bfa

Remove TODOs

832d5a6

Wrap all the MCPB info needed into a ExtendedMCInstrAnalysis class

5271303

Use extended MCAnalysis instead of MCplus (directly)

84f8d18

Make the CFA checker to first generate, if not matched check the Read…

37186b8

…/Write conditions

Check register states as well

203a238

Process prologue as well

5389a61

Ignore flag registers as well

fe03122

Enable dump

9ded4f3

Enable getting flag register through extended MCInstrAnalysis

6b7dd6a

Move the isolated MC semantic methods to ExtendedMCAnalysis

9724907

Remove unused includes

6a58a99

Test cfi validator on following cases:

63b8cfb

- Changing the register and not informing it using cfi directives - Changing the CFA and report it wrong - Load a register from another register stored location

amsen20 requested review from aaupov, maksfb, rafaelauler, ayermolo, yota9 and paschalis-mpeis as code owners July 11, 2025 21:01

amsen20 marked this pull request as draft July 11, 2025 21:08

paschalis-mpeis reviewed Jul 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DWARF CFI Checker prototype #148296

DWARF CFI Checker prototype #148296

Uh oh!

amsen20 commented Jul 11, 2025

Uh oh!

paschalis-mpeis left a comment

Uh oh!

paschalis-mpeis Jul 14, 2025

Uh oh!

amsen20 Jul 14, 2025

Uh oh!

amsen20 commented Jul 14, 2025

Uh oh!

amsen20 commented Jul 16, 2025

Uh oh!

Uh oh!

DWARF CFI Checker prototype #148296

Are you sure you want to change the base?

DWARF CFI Checker prototype #148296

Uh oh!

Conversation

amsen20 commented Jul 11, 2025

Uh oh!

paschalis-mpeis left a comment

Choose a reason for hiding this comment

Uh oh!

paschalis-mpeis Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

amsen20 Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

amsen20 commented Jul 14, 2025

Uh oh!

amsen20 commented Jul 16, 2025

Uh oh!

Uh oh!