This is an archive of the discontinued LLVM Phabricator instance.

Emit CFI directives in epilogue and enable CFIFixup pass for RISC-V.
Needs ReviewPublic

Authored by varunkumare99 on Feb 3 2023, 1:11 AM.

Details

Summary

This patch tries to add cfi directives in the epilogue which were disabled in D69723
Also enables CFIFixup pass which was added in D114545, to emit cfi_remember_state and cfi_restore_state when reordering is done.
And emits cfi_same_value directives when the unwind table needs to be reset to the initial one.
By default emission unwind tables is disabled for RISC-V.
Need to pass -fasynchronous-unwind-tables to emit unwind tables.

Diff Detail

Event Timeline

varunkumare99 created this revision.Feb 3 2023, 1:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 1:11 AM
varunkumare99 requested review of this revision.Feb 3 2023, 1:11 AM
lenary resigned from this revision.Feb 10 2023, 3:51 AM
asb added a comment.Feb 13 2023, 7:06 AM

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

varunkumare99 edited reviewers, added: probinson, MaskRay, chill; removed: lenary.Feb 15 2023, 4:02 AM

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

If that's the case it sounds like we have a lot of test cases that are emitting CFI directives that really should have nounwind.

Hi Varun,

It might be good to have a simple case to illustrate that cfi_remember_state and cfi_restore_state can be generated properly with epilogue CFI.
For examle, could the following case generated with cfi_remember_state and cfi_restore_state as GCC?

extern void foo(void);
void f(int *i) {
  if (*i == 0) {
    *i = 1;
    foo();
    return;
  } else {
    __builtin_printf("Hi");
    *i=0;
  }
}

This patch emits cfi_same_value directives when the unwind table needs to be reset to the initial one.
By default emission unwind tables is disabled for RISC-V.
Need to pass -fasynchronous-unwind-tables to emit unwind tables.

Hi Varun,

It might be good to have a simple case to illustrate that cfi_remember_state and cfi_restore_state can be generated properly with epilogue CFI.
For examle, could the following case generated with cfi_remember_state and cfi_restore_state as GCC?

extern void foo(void);
void f(int *i) {
  if (*i == 0) {
    *i = 1;
    foo();
    return;
  } else {
    __builtin_printf("Hi");
    *i=0;
  }
}

Hi Shiva, the following is the assembly emitted.

f:
.Lfunc_begin0:
	.loc	0 2 0
	.cfi_startproc
	addi	sp, sp, -16
	.cfi_def_cfa_offset 16
	sd	ra, 8(sp)
	sd	s0, 0(sp)
	.cfi_offset ra, -8
	.cfi_offset s0, -16
	.cfi_remember_state
	mv	s0, a0
.Ltmp0:
	.loc	0 3 6 prologue_end
	lw	a0, 0(a0)
.Ltmp1:
	.loc	0 3 6 is_stmt 0
	beqz	a0, .LBB0_2
.Ltmp2:
	.loc	0 8 3 is_stmt 1
	lui	a0, %hi(.L.str)
	addi	a0, a0, %lo(.L.str)
	call	printf
.Ltmp3:
	.loc	0 9 5
	sw	zero, 0(s0)
.Ltmp4:
	.loc	0 11 1
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp5:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 11 1 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
	ret
.LBB0_2:
	.cfi_restore_state
.Ltmp6:
	.loc	0 0 1
	li	a0, 1
.Ltmp7:
	.loc	0 4 6 is_stmt 1
	sw	a0, 0(s0)
	.loc	0 5 3
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp8:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 5 3 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
.Ltmp9:
	tail	foo
.Ltmp10:
.Lfunc_end0:
	.size	f, .Lfunc_end0-f
	.cfi_endproc
asb added a comment.Mar 16 2023, 9:55 AM

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

Sorry for not getting back on this. I was thinking more about real-world testing rather than just test case changes.

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

Sorry for not getting back on this. I was thinking more about real-world testing rather than just test case changes.

Hi Alex,
I have tested it with Eigen and RangeV3 test-suite. They have passed with no failures.

asb added a comment.EditedMar 30 2023, 4:49 AM

@kito-cheng, @MaskRay is this something you might be able to help review?

varunkumare99 edited the summary of this revision. (Show Details)Mar 30 2023, 5:08 AM

@asb ack, I think I can help review this from the functionality layer and compare to GCC, queue to my TODO :P

@kito-cheng, @MaskRay is this something you might be able to help review?

Yes, this is in my queue. I am mostly on trips between March 10 and April 10, so I haven't had enough time to read the issue in depth.

Hi Varun,

It might be good to have a simple case to illustrate that cfi_remember_state and cfi_restore_state can be generated properly with epilogue CFI.
For examle, could the following case generated with cfi_remember_state and cfi_restore_state as GCC?

extern void foo(void);
void f(int *i) {
  if (*i == 0) {
    *i = 1;
    foo();
    return;
  } else {
    __builtin_printf("Hi");
    *i=0;
  }
}

Hi Shiva, the following is the assembly emitted.

f:
.Lfunc_begin0:
	.loc	0 2 0
	.cfi_startproc
	addi	sp, sp, -16
	.cfi_def_cfa_offset 16
	sd	ra, 8(sp)
	sd	s0, 0(sp)
	.cfi_offset ra, -8
	.cfi_offset s0, -16
	.cfi_remember_state
	mv	s0, a0
.Ltmp0:
	.loc	0 3 6 prologue_end
	lw	a0, 0(a0)
.Ltmp1:
	.loc	0 3 6 is_stmt 0
	beqz	a0, .LBB0_2
.Ltmp2:
	.loc	0 8 3 is_stmt 1
	lui	a0, %hi(.L.str)
	addi	a0, a0, %lo(.L.str)
	call	printf
.Ltmp3:
	.loc	0 9 5
	sw	zero, 0(s0)
.Ltmp4:
	.loc	0 11 1
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp5:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 11 1 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
	ret
.LBB0_2:
	.cfi_restore_state
.Ltmp6:
	.loc	0 0 1
	li	a0, 1
.Ltmp7:
	.loc	0 4 6 is_stmt 1
	sw	a0, 0(s0)
	.loc	0 5 3
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp8:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 5 3 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
.Ltmp9:
	tail	foo
.Ltmp10:
.Lfunc_end0:
	.size	f, .Lfunc_end0-f
	.cfi_endproc

Hi Varun,

Could you add the case as a unit test?
Shouldn't all the epilogue CFI guard by EmitCFI?

The content of epilogue looks good to me.
Please wait for Maskray and Kito's comments.

Added testcase to verify emission of cfi_remember_state and cfi_restore_state.

added EmitCFI guard for emission of .cfi_def_cfa_offset in epilogue

Hi Varun,

It might be good to have a simple case to illustrate that cfi_remember_state and cfi_restore_state can be generated properly with epilogue CFI.
For examle, could the following case generated with cfi_remember_state and cfi_restore_state as GCC?

extern void foo(void);
void f(int *i) {
  if (*i == 0) {
    *i = 1;
    foo();
    return;
  } else {
    __builtin_printf("Hi");
    *i=0;
  }
}

Hi Shiva, the following is the assembly emitted.

f:
.Lfunc_begin0:
	.loc	0 2 0
	.cfi_startproc
	addi	sp, sp, -16
	.cfi_def_cfa_offset 16
	sd	ra, 8(sp)
	sd	s0, 0(sp)
	.cfi_offset ra, -8
	.cfi_offset s0, -16
	.cfi_remember_state
	mv	s0, a0
.Ltmp0:
	.loc	0 3 6 prologue_end
	lw	a0, 0(a0)
.Ltmp1:
	.loc	0 3 6 is_stmt 0
	beqz	a0, .LBB0_2
.Ltmp2:
	.loc	0 8 3 is_stmt 1
	lui	a0, %hi(.L.str)
	addi	a0, a0, %lo(.L.str)
	call	printf
.Ltmp3:
	.loc	0 9 5
	sw	zero, 0(s0)
.Ltmp4:
	.loc	0 11 1
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp5:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 11 1 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
	ret
.LBB0_2:
	.cfi_restore_state
.Ltmp6:
	.loc	0 0 1
	li	a0, 1
.Ltmp7:
	.loc	0 4 6 is_stmt 1
	sw	a0, 0(s0)
	.loc	0 5 3
	ld	ra, 8(sp)
	ld	s0, 0(sp)
.Ltmp8:
	.cfi_restore ra
	.cfi_restore s0
	.loc	0 5 3 epilogue_begin is_stmt 0
	addi	sp, sp, 16
	.cfi_def_cfa_offset 0
.Ltmp9:
	tail	foo
.Ltmp10:
.Lfunc_end0:
	.size	f, .Lfunc_end0-f
	.cfi_endproc

Hi Varun,

Could you add the case as a unit test?
Shouldn't all the epilogue CFI guard by EmitCFI?

The content of epilogue looks good to me.
Please wait for Maskray and Kito's comments.

Hi Shiva,
I have updated that changes.

@kito-cheng, @MaskRay is this something you might be able to help review?

Yes, this is in my queue. I am mostly on trips between March 10 and April 10, so I haven't had enough time to read the issue in depth.

Could you please have a look at this.
Thanks

asb added a comment.May 22 2023, 1:29 AM

@kito-cheng, @MaskRay is this something you might be able to help review?

Yes, this is in my queue. I am mostly on trips between March 10 and April 10, so I haven't had enough time to read the issue in depth.

Hi @MaskRay - is this a patch you might still be able to help review? Thanks.

@kito-cheng, @MaskRay is this something you might be able to help review?

Yes, this is in my queue. I am mostly on trips between March 10 and April 10, so I haven't had enough time to read the issue in depth.

Hi @MaskRay - is this a patch you might still be able to help review? Thanks.

is there anyone else maybe I can add as a reviewer?
thanks

asb added a comment.Jun 6 2023, 1:08 AM

I dropped MaskRay a quick email in case they're not seeing pings on this thread, but otherwise additional reviewer help very greatly appreciated.