Parse CFI instructions to create SFrame FREs #155496
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Looks pretty good overall. I have a bunch of comments, but I don't feel particularly strongly about most of them, so feel free to push back if you disagree (in particular, fields like Invalid are a pet peeve of mine).
The hardcoding of register constants is somewhat unfortunate, not so much because it can change (it's an ABI constant), but because it looks bad. It should be possible to get the RA register number generically (MCRegisterInfo::getRARegister), but it looks like there's no way to get the FP value without depending on a particular backend, so I guess we can live with that.
llvm/test/MC/ELF/cfi-sframe-errors.s
Outdated
| // asm parser doesn't give a location with .cfi_def_cfa_offset | ||
| // CHECK: :0:{{.*}} Adjusting CFA offset without a base register.{{.*}} | ||
| // .cfi_adjust_cfa_offset | ||
| // CHECK: :13:{{.*}} Adjusting CFA offset without a base register. {{.*}} |
There was a problem hiding this comment.
This will make it hard to update the file. Please move the checks next to the code which generates those diagnostics. That makes it easier to see what's being checked, and you can use [[@LINE-1]] to match the line numbers.
| .cfi_def_cfa 0, 4 | ||
| nop | ||
|
|
||
| .cfi_endproc |
There was a problem hiding this comment.
something wrong with the indentation here. Tabs?
There was a problem hiding this comment.
There's still something wrong with the indentation on this line (.cfi_endproc), at least when I view it in github UI. (I would expect it to start at the same column as the other .cfi directives.
llvm/lib/MC/MCSFrame.cpp
Outdated
| // Instructions from InitialFrameState may not have a label, but if | ||
| // these instructions don't, then they are in dead code or otherwise | ||
| // unused. | ||
| auto *L = CFI.getLabel(); | ||
| if (L && !L->isDefined()) | ||
| continue; |
There was a problem hiding this comment.
When does this happen? Can we have a test case for these undead CFI insns?
There was a problem hiding this comment.
This is copied from the corresponding eh_frame code, which was added in 2010 (commit 85d9198), and does not include a test for that case.
Nothing in the testsuite triggers it today, and I haven't been able to reproduce it myself in either location. So it's possible that it isn't necessary today.
On the other hand, there are only seven (non-error checking) tests for .cfi_startproc simple in the codebase. So we are in solidly under-tested waters. The alternative here would be to warn and skip with some sort of "missing label" failure. But I'm wary of erroring on invalid input where eh_frame doesn't.
There was a problem hiding this comment.
I'd be tempted to add an assert and see what/if anything breaks there, but I guess following eh_frame makes sense as well. It would be nice to make a note of why you're doing that, to help someone looking at this in the future.
There was a problem hiding this comment.
I'll add a TODO here and when this is in a state to run against some much bigger and a wider variety test cases, will see if it can be removed.
llvm/lib/MC/MCSFrame.cpp
Outdated
| setCFAOffset(FRE, CFI.getLoc(), CFI.getOffset()); | ||
| return true; |
There was a problem hiding this comment.
Should this be:
| setCFAOffset(FRE, CFI.getLoc(), CFI.getOffset()); | |
| return true; | |
| return setCFAOffset(FRE, CFI.getLoc(), CFI.getOffset()); |
I guess the difference is that if this was the only error, the code would still go on to produce a (broken) FDE entry.
| .cfi_def_cfa 0, 4 | ||
| nop | ||
|
|
||
| .cfi_endproc |
There was a problem hiding this comment.
There's still something wrong with the indentation on this line (.cfi_endproc), at least when I view it in github UI. (I would expect it to start at the same column as the other .cfi directives.
llvm/lib/MC/MCSFrame.cpp
Outdated
| // Instructions from InitialFrameState may not have a label, but if | ||
| // these instructions don't, then they are in dead code or otherwise | ||
| // unused. | ||
| auto *L = CFI.getLabel(); | ||
| if (L && !L->isDefined()) | ||
| continue; |
There was a problem hiding this comment.
I'd be tempted to add an assert and see what/if anything breaks there, but I guess following eh_frame makes sense as well. It would be nice to make a note of why you're doing that, to help someone looking at this in the future.
| // hand-written assembly. | ||
| if (DF.RAReg != RAReg) { | ||
| Streamer.getContext().reportWarning( | ||
| SMLoc(), "Non-default RA register in .cfi_return_column " + |
There was a problem hiding this comment.
Nit: non-default to conform with LLVM warning style guide.
This PR parses CFI instructions to generate FREs.
Unfortunately, actually emitting the FREs into the object file is somewhat involved and would make this PR quite a bit harder to review. And the dumper itself properly errors if the FRE count is included in the header or FDEs, but they are not actually emitted. So actually testing that the proper FREs are generated will have to wait for a subsequent PR.
For now, just check for common issues with CFI that sframe doesn't support, and that proper error handling is done.