Minimal unwinding information (DWARF CFI) checker #145633

amsen20 · 2025-06-25T04:47:29Z

This PR adds a minimal version of UnwindInfoChecker described in here.

This implementation looks into the modified registers by each instruction and checks:

If one of them is the CFA register, and it informs the user if the CFA data is not modified as well.
If one of them is used in another register's unwinding rule, it informs the user if the unwind info is not modified after this instruction.

This implementation does not support DWARF expressions and treats them as unknown unwinding rules.

…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

…mer.h

statham-arm · 2025-06-30T08:34:19Z

llvm/include/llvm/DWARFCFIChecker/DWARFCFIState.h

+/// way to modify it is to append a new row by updating it with a CFI directive,
+/// and the only way to read from it is to read the last row (i.e., current CFI
+/// state) from the table. The fetched row is constant and should not be
+/// modified or deleted.


Thanks for adding all the comments!

This comment still leaves me confused about why it's necessary to store a table of rows at all. Why doesn't this class just store a single dwarf::UnwindRow (or perhaps an optional one), update it in place in update(), and return it by value from getCurrentUnwindRow()? What is the purpose of storing the whole history of previous rows, if there's no API to get them out again, and nothing inside the class implementation needs to refer to them to compute the new rows?

Perhaps you have an intention for future development which will make use of the stored history? If so, that seems like a thing to put in the comment.

Yes, the main reason for the previous iterator design was that when reporting the error to the user, we want to be able to refer to the exact directive that caused this error. To do that, the only way is to iterate the table from the last row to the current row, one by one, find the row that applies the change, and report the error to the corresponding directive.

But for now, in this version, errors are reported to the instruction. I can:

Try to figure out a way to report the error to the directive: The logic is not very simple. To do so, I need to add a row <-> directive mapping. Additionally, between the last row and the current row, a value may change multiple times, which means we must choose a policy to determine which changes correspond to the reporting changes.

Store a single shared_pointer<UnwindRow> as the current row, the analysis can read it, and it will be freed when no one has access to it. Aside from using smart pointers, I don't see a way to properly implement this.

Let me know which one I should pursue.

I see; so you don't need to keep all the rows right back to the start of the function, only the ones between the PrevRow and NextRow rows in DWARFCFIAnalysis::update?

So it would be enough to give DWARFCFIState a method to nearly empty its vector (reducing it to just one entry which was previously the final entry). Then DWARFCFIAnalysis::update could start by calling that. And now the size of DWARFCFIState doesn't grow in proportion to the size of the whole function, only the number of table rows generated by a single directive at a time.

But whichever approach you take, if you're storing information in a vector which is not currently used for anything, please write down in a comment why you're storing it at all, and what it's intended to be used for in future.

Fixed.
The state now only stores the last row, and the other rows are copied instead of being shared with a smart pointer.
Sharing through a pointer might save one copy (which may be optimized away by the compiler), and that one extra copy is OK to have. Each row size is linear to the number of register bytes.

statham-arm · 2025-06-30T09:23:20Z

llvm/include/llvm/DWARFCFIChecker/DWARFCFIFunctionFrameStreamer.h

-        Analyzer(std::move(Analyzer)), LastDirectiveIndex(0) {
-    assert(this->Analyzer && "Analyzer should not be null");
+        Receiver(std::move(Receiver)), LastDirectiveIndex(0) {
+    assert(this->Receiver && "Analyzer should not be null");


I like the rename of Analyzer to the more purpose-neutral Receiver, but you left one unchanged in the assert failure message here 🙂

statham-arm · 2025-07-01T08:28:31Z

llvm/lib/DWARFCFIChecker/DWARFCFIAnalysis.cpp

-    }
-
+  // Each case is annotated with its corresponding number as described in the
+  // `DWARFCFIAnalysis`.


Could you add a reference to the specific file, please? "As described in llvm/include/llvm/DWARFCFIChecker/DWARFCFIAnalysis.h." If you're going to carefully map the code here to the comment in another file, it should be as easy as possible for someone to find that comment to compare against.

Fixed, I guess this was the only place I referenced another file documentation. Let me know if there is another place that I can fix.

statham-arm · 2025-07-01T08:38:16Z

llvm/lib/DWARFCFIChecker/DWARFCFIAnalysis.cpp

+  // Case 2
+  if (PrevLoc.getLocation() != NextLoc.getLocation()) { // Case 2.a
+    Context->reportWarning(Inst.getLoc(),
+                           formatv("uncheckable change happened to register "


A small wording point: could you make sure this checker takes the blame for not being able to check this?

The current wording is ambiguous enough that a user might think it's their own fault. "Is my change uncheckable? Should I have written a different one to make it easier to check?" But in fact many of these changes are checkable in principle, it's just that this particular checker doesn't have the ability to check them yet.

I think wording along the lines of "checking correctness of changes to register unwinding rules is not yet implemented" would communicate more clearly what the user needs to know:

it's not my fault

there's nothing I can change in my code to make this warning go away

the checker isn't telling me my rule is wrong, just that it doesn't know how to make sure it's right

maybe in a future version this will be better.

statham-arm · 2025-07-01T08:43:53Z

llvm/include/llvm/DWARFCFIChecker/DWARFCFIState.h

+/// way to modify it is to append a new row by updating it with a CFI directive,
+/// and the only way to read from it is to read the last row (i.e., current CFI
+/// state) from the table. The fetched row is constant and should not be
+/// modified or deleted.


But whichever approach you take, if you're storing information in a vector which is not currently used for anything, please write down in a comment why you're storing it at all, and what it's intended to be used for in future.

…m#145804) All these changes are being used in [PR#145633](llvm#145633) `CFIProgram`: - `addInstruction` methods already exists, but more convenient ones are private, this PR makes them public `UnwindLocation`: - Added a field accessor method for `Dereference` like other field access methods.

dwblaikie · 2025-07-01T17:41:02Z

llvm/tools/llvm-mc/llvm-mc.cpp

-  if (FileType == OFT_AssemblyFile) {
+  if (ValidateCFI) {
+    assert(FileType == OFT_Null);
+    auto *FFS = new CFIFunctionFrameStreamer(Ctx, std::move(FFA));


prefer make_unique here

… class definition

…ammer's fault and the validation is not implemented yet. Change the tests accordingly

statham-arm

Thanks for all the fixes!

I've run out of things to complain about now 🙂 but I see that Petr has just nominated a couple more reviewers, so please give those people time to have thoughts as well.

Sterling-Augustine · 2025-07-02T00:38:12Z

llvm/include/llvm/DWARFCFIChecker/DWARFCFIFunctionFrameAnalyzer.h

+/// instantiates a `DWARFCFIAnalysis` for each frame. The errors/warnings are
+/// emitted through the `MCContext` instance to the constructor. If a frame
+/// finishes without being started or if all the frames are not finished before
+/// this classes is destructured, the program fails through an assertion.


"destructed".

Sterling-Augustine · 2025-07-02T17:22:33Z

llvm/lib/DWARFCFIChecker/DWARFCFIFunctionFrameStreamer.cpp

+
+void CFIFunctionFrameStreamer::emitInstruction(const MCInst &Inst,
+                                               const MCSubtargetInfo &STI) {
+  updateReceiver();


This feels weird to me. updateReceiver works on LastInstruction, but not the instruction that came via emitInstruction. So updateReceiver defers analyzing Inst until the next call. It seems likely that there is a good reason for this, but I can't tell what it is.

At the very least this needs some kind of comment explaining what is going on. I might even have updateReciever take the instruction to analyze as a parameter, which makes things even more clear that current instruction is being deferred.

By my count, updateReciever has three call sites, and I can't tell off the top of my head which ones I would expect LastInstruction to be not present. I think two? Given that it is only used at the end of the function, perhaps it makes sense to split that portion into a new function that is called more selectively? (But perhaps that isn't possible.

In any event. Needs comments. Especially on why the very last LastInstruction will get analyzed even though it is deferred here.

Thanks to your comment, I was able to find a bug/design choice in MCStreamer that the frames are always forming a stack, which is not always true. But this analysis, like MCStreamer assumes it too.

The main reason for the delay is that the directives associated with an instruction come after the instruction, so when the streamer encounters an instruction, it means that the directives appended from the last instruction to now are the directives associated with the last instruction, not the new instruction.

I moved the updateDirectivesRange method to the updateReceiver and added documentation explaining its purpose and potential side effects. Also, I added documentation for each internal state stored in CFIFunctionFrameStreamer and how updateReceiver maintains them.

Sterling-Augustine · 2025-07-02T17:37:58Z

llvm/include/llvm/DWARFCFIChecker/DWARFCFIFunctionFrameReceiver.h

+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file declares CFIFunctionFrameReceiver class.


What motivates the abstract base class here? it feels a little yagni, and adds complexity that may not be realistically needed. But perhaps there are plans for multiple implementations later?

dwblaikie · 2025-07-02T17:38:23Z

llvm/tools/llvm-mc/llvm-mc.cpp

@@ -519,8 +526,15 @@ int main(int argc, char **argv) {
  std::unique_ptr<MCInstrInfo> MCII(TheTarget->createMCInstrInfo());
  assert(MCII && "Unable to create instruction info!");

+  auto FFA = std::make_unique<CFIFunctionFrameAnalyzer>(Ctx, *MCII);


Why is this constructed here, rather than created down at line 534 - the only place it's used?

dwblaikie · 2025-07-02T17:50:35Z

llvm/lib/DWARFCFIChecker/Registers.h

+inline MCPhysReg getSuperReg(const MCRegisterInfo *MCRI, MCPhysReg Reg) {
+  if (isSuperReg(MCRI, Reg))
+    return Reg;
+  for (auto SuperReg : MCRI->superregs(Reg)) {


Some inconsistency with bracing in this patch - like this loop has braces, but a very similar loop at line 49 doesn't... I think technically according to the style guide, both should omit braces, but I could go either way.

(there's also some other rather long-bodied for/if{} without the braces on the for which get a bit of a stretch - but, yeah, the rules are pretty vague)

Fixed.
Tried to change them to match the styling guide.

…the previous state

Amirhossein Pashaeehir added 30 commits June 25, 2025 03:55

Forward CFI directives alongside their associated instruction to a si…

5b78ea0

…mple CFI analysis

Create a new CFI analysis for each dwarf separately

b781e83

Separate the logic and display

8b43f8c

Compile llvm mc depending on bolt

6790f90

Extract semantic info on MCInst from bolt

75f3b2d

Typo in evaluateStackOffsetExpr doc

a3d08d8

Add a DS to keep track of each register CFI state

7a10923

Check if the CFA change is correct or not

5841b8f

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

69a8e9d

Provide the directives as an arrayref instead of index range

43b54fd

Remove TODOs

474ff1c

Wrap all the MCPB info needed into a ExtendedMCInstrAnalysis class

8ea5d9c

Use extended MCAnalysis instead of MCplus (directly)

e92005c

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

7527ba2

…/Write conditions

Check register states as well

3d65eeb

Process prologue as well

79c1664

Ignore flag registers as well

02410ac

Enable dump

dfdd22f

Enable getting flag register through extended MCInstrAnalysis

4fd37bf

Move the isolated MC semantic methods to ExtendedMCAnalysis

07d38b2

Remove unused includes

eb06aa4

Test cfi validator on following cases:

e7c3140

- 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

Prune the CFA diff checker to only check read/write

6347ef4

Move the CFI analysis headers to MC

453917b

Move all the CFI analysis implementations to lib

9f8bca8

Remove stack pointer method

2c233f9

Comment out Ignoring flags at the beginning CFI analysis

12faad8

Move the necessary MCStreamer interface methods to CFIAnalysisMCStrea…

282cb77

…mer.h

Make the update method const

903e3fb

Move CFI analysis out of MC layer

7bb20f8

statham-arm reviewed Jun 30, 2025

View reviewed changes

Amirhossein Pashaeehir added 5 commits June 30, 2025 17:50

Sync an assertion message with the refactors happened around it

68fc758

Merge branch 'main' into cfi-validation-generation

62c41f7

Merge branch 'main' into cfi-validation-generation

f1f4796

Sync implementation and DWARFCFIAnalysis explaination

77168bb

Change error messages for CFA

627e8b3

amsen20 marked this pull request as ready for review July 1, 2025 00:55

Test CFA corner cases

9abf615

statham-arm reviewed Jul 1, 2025

View reviewed changes

petrhosek requested review from igorkudrin and dwblaikie July 1, 2025 17:06

Warn the user about ignoring .cfi_escape directive

3c632ba

dwblaikie reviewed Jul 1, 2025

View reviewed changes

Amirhossein Pashaeehir added 8 commits July 1, 2025 17:42

Move warning to the point happening

86cdd3d

Support .cfi_label

ccfeaa5

Store only the last row in the state

8a87530

Reference another file properly

f080e0d

Sort fields by size for memory layout and move them to the end of the…

10a84ff

… class definition

Change the error messages to show that the warnings are not the progr…

d6025c6

…ammer's fault and the validation is not implemented yet. Change the tests accordingly

Delete unwanted commited test

c8c6582

Use make unique instead of newing a raw pointer

f2efe74

statham-arm approved these changes Jul 2, 2025

View reviewed changes

Sterling-Augustine reviewed Jul 2, 2025

View reviewed changes

dwblaikie reviewed Jul 2, 2025

View reviewed changes

Amirhossein Pashaeehir added 5 commits July 2, 2025 20:55

Fix a typo

4d934c0

[TEMP] Store FrameStreamer state in stack instead of trying to guess …

1e9b4e6

…the previous state

Fix the nested frame case and document the updateReceiver behavior

1981374

Move the definition to usage

d231713

Sync the braces styles

aa1fbf3

Minimal unwinding information (DWARF CFI) checker #145633

Are you sure you want to change the base?

Minimal unwinding information (DWARF CFI) checker #145633

Conversation

amsen20 commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

statham-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amsen20 Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amsen20 Jul 3, 2025 •

edited

Loading