This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][ASAN] unwind fixup
ClosedPublic

Authored by EccoTheDolphin on Sep 12 2020, 6:15 PM.

Details

Summary

[8/11] patch series to port ASAN for riscv64

Depends On D87577

Diff Detail

Event Timeline

EccoTheDolphin created this revision.Sep 12 2020, 6:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2020, 6:15 PM
Herald added subscribers: Restricted Project, evandro, luismarques and 11 others. · View Herald Transcript
luismarques added inline comments.Sep 14 2020, 6:43 AM
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp
23–48

This block is incorrectly marked in the patch as already existing code, instead of new code.
For performance, it should probably do the checks in order of most to least likely case. That's probably 4, 2, 6, 8.

120

Where does this -1 value come from?

eugenis added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp
120

I'm not familiar with the ISA at all, but if I understand this correctly, return address is at frame[-1] and previous frame pointer is at frame[-2], so perhaps GetCanonicFrame must be updated as well?

2:   1141                    addi    sp,sp,-16
4:   e406                    sd      ra,8(sp)
6:   e022                    sd      s0,0(sp)
8:   0800                    addi    s0,sp,16
134–140

What I meant earlier is that is needs to be frame[-2] maybe?

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp
120

Yes, this the case. The proposed adjustment seems to be valid. We've checked it on a manually written test and the produced backtrace looks much better. I have strong suspicion that the adjustment in question should fix most of the remaining tests (the ones marked as haveing "backtrace" issues).

I've prepared relevant patches but I'd like to re-run all tests once again before pushing the fix (re-building and re-running tests takes time )

more adjustments to UnwindFast + added more comments

EccoTheDolphin edited the summary of this revision. (Show Details)Sep 20 2020, 11:36 PM
EccoTheDolphin marked 2 inline comments as done.
EccoTheDolphin edited the summary of this revision. (Show Details)
EccoTheDolphin marked 2 inline comments as done.Sep 20 2020, 11:49 PM
EccoTheDolphin edited the summary of this revision. (Show Details)Sep 22 2020, 11:40 AM
eugenis accepted this revision.Sep 22 2020, 1:12 PM

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

This revision is now accepted and ready to land.Sep 22 2020, 1:12 PM

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

I'll double-check.

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

I'll double-check.

Looks like it does.

clang -O0
	int foo(int):
        addi    sp, sp, -32
        sd      ra, 24(sp)
        sd      s0, 16(sp)
        addi    s0, sp, 32
gcc -O0 -march=rv64imafdc -mabi=lp64d
	int foo(int):
        addi    sp,sp,-32
        sd      ra,24(sp)
        sd      s0,16(sp)
        addi    s0,sp,32

Example with gcc/clang (-O2 -fno-omit-frame-pointer')

https://godbolt.org/z/3GeET1

They match too.

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

I'll double-check.

Looks like it does.

Is this something it would be useful for the psABI to specify? I can look at an addition along the lines of "If you save ra, it must go in a certain place in the frame. If you save fp, it also has to go in a certain place." I think other ABIs require this too, but I could be wrong.

Documenting this would be a good idea.
You can say something along the lines of:
if a function has a frame pointer, it must point one byte past the end of a record that contains the previous frame pointer in the lower word, and the previous frame return address in the high word.

@eugenis, @luismarques - I believe all concerns for this one were addressed. Do you mind if I commit this as is?

I have no objections.

vitalybuka accepted this revision.Oct 1 2020, 3:39 PM

@eugenis, @luismarques - I believe all concerns for this one were addressed. Do you mind if I commit this as is?

If some revision of the patch is approved, and all comments are addressed no need to wait for re-approval.
If you still want another look, it's better to somehow attract reviewer's attention, as phabricator hides patches with previously approved revisions from "Ready to Review" list.

@eugenis, @luismarques - I believe all concerns for this one were addressed. Do you mind if I commit this as is?

If some revision of the patch is approved, and all comments are addressed no need to wait for re-approval.
If you still want another look, it's better to somehow attract reviewer's attention, as phabricator hides patches with previously approved revisions from "Ready to Review" list.

Got it. Thank you very much for clarifying!

This revision was automatically updated to reflect the committed changes.