Skip to content

[BOLT][RISCV] Handle CIE's produced by GNU as #69578

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

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

mtvec
Copy link
Contributor

@mtvec mtvec commented Oct 19, 2023

On RISC-V, GNU as produces the following initial instruction in CIE's:

DW_CFA_def_cfa_register: r2

While I believe it is technically illegal to use this instruction without first using a DW_CFA_def_cfa (since the offset is undefined), both readelf and llvm-dwarfdump accept this and implicitly set the offset to 0.

In BOLT, however, this triggers an assert (in CFISnapshot::advanceTo) as it (correctly) believes the offset is not set. This patch fixes this by setting the offset to 0 whenever executing DW_CFA_def_cfa_register while the offset is undefined.

Note that this is probably the simplest workaround but it has a downside: while emitting CFI start, we check if the initial instructions are contained within MCAsmInfo::getInitialFrameState and omit them if they are. This will not be true for GNU CIE's (since they differ from LLVM's) which causes an unnecessary DW_CFA_def_cfa_register to be emitted.

While technically correct, it would probably be better to replace the GNU CIE with the one used by LLVM to avoid this situation. This would solve the problem this patch solves while also preventing unnecessary CFI instructions. However, this is a bit trickier to implement correctly so I propose to keep this for a later time.

Note on testing: the test creates a simple function with three basic blocks and forces the CFI state of the last one to be different from the others using an arbitrary CFI instruction. Then,
--reorder-blocks=reverse is used to force CFISnapshot::advanceTo to be called. This causes an assert on the current main branch.

On RISC-V, GNU as produces the following initial instruction in CIE's:

```
DW_CFA_def_cfa_register: r2
```

While I believe it is technically illegal to use this instruction
without first using a `DW_CFA_def_cfa` (since the offset is undefined),
both `readelf` and `llvm-dwarfdump` accept this and implicitly set the
offset to 0.

In BOLT, however, this triggers an assert (in `CFISnapshot::advanceTo`)
as it (correctly) believes the offset is not set. This patch fixes this
by setting the offset to 0 whenever executing `DW_CFA_def_cfa_register`
while the offset is undefined.

Note that this is probably the simplest workaround but it has a
downside: while emitting CFI start, we check if the initial instructions
are contained within `MCAsmInfo::getInitialFrameState` and omit them if
they are. This will not be true for GNU CIE's (since they differ from
LLVM's) which causes an unnecessary `DW_CFA_def_cfa_register` to be
emitted.

While technically correct, it would probably be better to replace the
GNU CIE with the one used by LLVM to avoid this situation. This would
solve the problem this patch solves while also preventing unnecessary
CFI instructions. However, this is a bit trickier to implement correctly
so I propose to keep this for a later time.

Note on testing: the test creates a simple function with three basic
blocks and forces the CFI state of the last one to be different from the
others using an arbitrary CFI instruction. Then,
`--reorder-block=reverse` is used to force `CFISnapshot::advanceTo` to
be called. This causes an assert on the current main branch.
@mtvec mtvec added the BOLT label Oct 19, 2023
@mtvec mtvec merged commit 6795bfc into llvm:main Oct 20, 2023
@mtvec mtvec deleted the bolt-riscv-gnu-cie branch October 20, 2023 15:49
@rafaelauler
Copy link
Contributor

While technically correct, it would probably be better to replace the GNU CIE with the one used by LLVM to avoid this situation.

I think the extra CFI being inserted is a small price to pay for compatibility with GNU binaries. I wouldn't worry about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants