diff mbox series

RISC-V: gas: Change initial CFI operation from DW_CFA_def_cfa_register to DW_CFA_def_cfa

Message ID 20240513081149.32515-1-sfoon.kim@samsung.com
State New
Headers
Series RISC-V: gas: Change initial CFI operation from DW_CFA_def_cfa_register to DW_CFA_def_cfa |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed

Commit Message

Sung-hun Kim May 13, 2024, 8:11 a.m. UTC 
  The DWARF specification (especially, DWARF4 and 5 [1,2]) states that
DW_CFA_def_cfa_register cannot be used as the first CFI operation.
It said DW_CFA_def_cfa_register as follows:

  ... This operation is valid only if the current CFA rule is defined
  to use a register and offset.

So, DW_CFA_def_cfa_register can be used after that other definition
operation such as DW_CFA_def_cfa is called. However, the current gas
code emits DW_CFA_def_cfa_register as an initial CFI operation for RISCV.

In the libgcc, the unwinding function does not care about it, so it can
unwind the call stack. However, on the third party library such as
libunwindstack in Android, it causes a fatal error.

This patch changes the initial CFI operation to DW_CFA_def_cfa with
offset 0. It works as same as the previous one, but it does not have
any limitation so it satisfies the DWARF spec. This change resolves
the compatibility issue while preserving the original behaviour.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31733

[1] DWARF4 specification, https://dwarfstd.org/doc/DWARF4.pdf
[2] DWARF5 specification, https://dwarfstd.org/doc/DWARF5.pdf

Signed-off-by: Sung-hun Kim <sfoon.kim@samsung.com>
---
 gas/config/tc-riscv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nelson Chu May 14, 2024, 11:19 p.m. UTC | #1 
Also send this to gdb mailing list as there should be more DWARF experts
out there.

On Mon, May 13, 2024 at 4:13 PM Sung-hun Kim <sfoon.kim@samsung.com> wrote:

> The DWARF specification (especially, DWARF4 and 5 [1,2]) states that
> DW_CFA_def_cfa_register cannot be used as the first CFI operation.
> It said DW_CFA_def_cfa_register as follows:
>
>   ... This operation is valid only if the current CFA rule is defined
>   to use a register and offset.
>
> So, DW_CFA_def_cfa_register can be used after that other definition
> operation such as DW_CFA_def_cfa is called. However, the current gas
> code emits DW_CFA_def_cfa_register as an initial CFI operation for RISCV.
>
> In the libgcc, the unwinding function does not care about it, so it can
> unwind the call stack. However, on the third party library such as
> libunwindstack in Android, it causes a fatal error.
>
> This patch changes the initial CFI operation to DW_CFA_def_cfa with
> offset 0. It works as same as the previous one, but it does not have
> any limitation so it satisfies the DWARF spec. This change resolves
> the compatibility issue while preserving the original behaviour.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31733
>
> [1] DWARF4 specification, https://dwarfstd.org/doc/DWARF4.pdf
> [2] DWARF5 specification, https://dwarfstd.org/doc/DWARF5.pdf
>
> Signed-off-by: Sung-hun Kim <sfoon.kim@samsung.com>
> ---
>  gas/config/tc-riscv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 1757ff6e9e2..8d749581c1d 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -5209,7 +5209,7 @@ RISC-V options:\n\
>  void
>  riscv_cfi_frame_initial_instructions (void)
>  {
> -  cfi_add_CFA_def_cfa_register (X_SP);
> +  cfi_add_CFA_def_cfa (X_SP, 0);
>  }
>
>  int
> --
> 2.25.1
>
>
Andrew Burgess May 17, 2024, 1:57 p.m. UTC | #2 
Sung-hun Kim <sfoon.kim@samsung.com> writes:

> The DWARF specification (especially, DWARF4 and 5 [1,2]) states that
> DW_CFA_def_cfa_register cannot be used as the first CFI operation.
> It said DW_CFA_def_cfa_register as follows:
>
>   ... This operation is valid only if the current CFA rule is defined
>   to use a register and offset.
>
> So, DW_CFA_def_cfa_register can be used after that other definition
> operation such as DW_CFA_def_cfa is called. However, the current gas
> code emits DW_CFA_def_cfa_register as an initial CFI operation for RISCV.
>
> In the libgcc, the unwinding function does not care about it, so it can
> unwind the call stack. However, on the third party library such as
> libunwindstack in Android, it causes a fatal error.
>
> This patch changes the initial CFI operation to DW_CFA_def_cfa with
> offset 0. It works as same as the previous one, but it does not have
> any limitation so it satisfies the DWARF spec. This change resolves
> the compatibility issue while preserving the original behaviour.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31733

I'm not a binutils maintainer, so can't approve this patch.

But, for what it's worth, I think this is the right change to make.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>
> [1] DWARF4 specification, https://dwarfstd.org/doc/DWARF4.pdf
> [2] DWARF5 specification, https://dwarfstd.org/doc/DWARF5.pdf
>
> Signed-off-by: Sung-hun Kim <sfoon.kim@samsung.com>
> ---
>  gas/config/tc-riscv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 1757ff6e9e2..8d749581c1d 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -5209,7 +5209,7 @@ RISC-V options:\n\
>  void
>  riscv_cfi_frame_initial_instructions (void)
>  {
> -  cfi_add_CFA_def_cfa_register (X_SP);
> +  cfi_add_CFA_def_cfa (X_SP, 0);
>  }
>  
>  int
> -- 
> 2.25.1
Nelson Chu May 20, 2024, 2:54 a.m. UTC | #3 
Thanks for helping, Andrew :-)  Approved and committed.

Nelson

On Fri, May 17, 2024 at 9:59 PM Andrew Burgess <aburgess@redhat.com> wrote:

> Sung-hun Kim <sfoon.kim@samsung.com> writes:
>
> > The DWARF specification (especially, DWARF4 and 5 [1,2]) states that
> > DW_CFA_def_cfa_register cannot be used as the first CFI operation.
> > It said DW_CFA_def_cfa_register as follows:
> >
> >   ... This operation is valid only if the current CFA rule is defined
> >   to use a register and offset.
> >
> > So, DW_CFA_def_cfa_register can be used after that other definition
> > operation such as DW_CFA_def_cfa is called. However, the current gas
> > code emits DW_CFA_def_cfa_register as an initial CFI operation for RISCV.
> >
> > In the libgcc, the unwinding function does not care about it, so it can
> > unwind the call stack. However, on the third party library such as
> > libunwindstack in Android, it causes a fatal error.
> >
> > This patch changes the initial CFI operation to DW_CFA_def_cfa with
> > offset 0. It works as same as the previous one, but it does not have
> > any limitation so it satisfies the DWARF spec. This change resolves
> > the compatibility issue while preserving the original behaviour.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31733
>
> I'm not a binutils maintainer, so can't approve this patch.
>
> But, for what it's worth, I think this is the right change to make.
>
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
>
> Thanks,
> Andrew
>
> >
> > [1] DWARF4 specification, https://dwarfstd.org/doc/DWARF4.pdf
> > [2] DWARF5 specification, https://dwarfstd.org/doc/DWARF5.pdf
> >
> > Signed-off-by: Sung-hun Kim <sfoon.kim@samsung.com>
> > ---
> >  gas/config/tc-riscv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> > index 1757ff6e9e2..8d749581c1d 100644
> > --- a/gas/config/tc-riscv.c
> > +++ b/gas/config/tc-riscv.c
> > @@ -5209,7 +5209,7 @@ RISC-V options:\n\
> >  void
> >  riscv_cfi_frame_initial_instructions (void)
> >  {
> > -  cfi_add_CFA_def_cfa_register (X_SP);
> > +  cfi_add_CFA_def_cfa (X_SP, 0);
> >  }
> >
> >  int
> > --
> > 2.25.1
>
>
diff mbox series

Patch

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 1757ff6e9e2..8d749581c1d 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -5209,7 +5209,7 @@  RISC-V options:\n\
 void
 riscv_cfi_frame_initial_instructions (void)
 {
-  cfi_add_CFA_def_cfa_register (X_SP);
+  cfi_add_CFA_def_cfa (X_SP, 0);
 }
 
 int