Page MenuHomeFreeBSD
Differential D40623

csu: Add a stop indicator to _start to satisfy unwinders on aarch64
ClosedPublic
Actions

Authored by dchagin on Jun 20 2023, 12:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 29, 4:54 PM
Unknown Object (File)
Sun, Nov 24, 2:18 AM
Unknown Object (File)
Sat, Nov 23, 8:53 PM
Unknown Object (File)
Fri, Nov 22, 2:11 PM
Unknown Object (File)
Thu, Nov 21, 9:00 AM
Unknown Object (File)
Mon, Nov 18, 4:22 PM
Unknown Object (File)
Mon, Nov 18, 3:26 AM
Unknown Object (File)
Fri, Nov 15, 4:11 AM
View All 82 Files
Subscribers
arichardson
emaste
imp

Details

Reviewers
andrew
Commits
rGbae6bb0698ee: csu: Implement _start using as to satisfy unwinders on aarch64
Summary

The right unwinding stop indicator should be CFI-undefined PC.
https://dwarfstd.org/doc/Dwarf3.pdf - page 118:
If a Return Address register is defined in the virtual unwind table,
and its rule is undefined (for example, by DW_CFA_undefined), then
there is no return address and no call address, and the virtual
unwind of stack activations is complete.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 52427
Build 49318: arc lint + arc unit

Event Timeline

dchagin created this revision.Jun 20 2023, 12:21 PM
Herald added a subscriber: imp. · View Herald TranscriptJun 20 2023, 12:21 PM
dchagin requested review of this revision.Jun 20 2023, 12:21 PM
Harbormaster completed remote builds in B52159: Diff 123489.Jun 20 2023, 12:21 PM
dchagin retitled this revision from csu: Add an annotated _start to satisfy unwinders to csu: Add an annotated _start to satisfy unwinders on x86_64.Jun 20 2023, 12:32 PM
dchagin edited the summary of this revision. (Show Details)Jun 20 2023, 12:51 PM
dchagin added a reviewer: kib.
kib added inline comments.Jun 22 2023, 9:34 AM
lib/csu/amd64/crt1_s.S
34 ↗(On Diff #123489)

%rbp id already cleared by kernel; callq breaks stack alignment.

dchagin updated this revision to Diff 123756.Jun 26 2023, 11:49 AM
dchagin edited the summary of this revision. (Show Details)

tack alignment is fixed, merged x86 and arm64 changes into one commit
due o -fno-asynchronous-unwind-tables removal requirments

Herald added a reviewer: andrew. · View Herald TranscriptJun 26 2023, 11:49 AM
Herald added a subscriber: emaste. · View Herald Transcript
Harbormaster completed remote builds in B52284: Diff 123756.Jun 26 2023, 11:49 AM
dchagin retitled this revision from csu: Add an annotated _start to satisfy unwinders on x86_64 to csu: Add a stop indicator to _start to satisfy unwinders on x86 and arm64.Jun 26 2023, 11:52 AM
dchagin edited the summary of this revision. (Show Details)
dchagin added a reviewer: dim.
dchagin added inline comments.
lib/csu/amd64/crt1_s.S
34 ↗(On Diff #123489)

after rtld it is not 0

dchagin mentioned this in D40624: csu: Implement _start using as to satisfy unwinders on i386.Jun 26 2023, 12:18 PM
dchagin mentioned this in D40277: arm64: Add unwind information to _start.
imp added inline comments.Jun 26 2023, 3:08 PM
lib/csu/aarch64/crt1_c.c
35

why have this. It's useless here since there's no _start here.

imp added inline comments.Jun 26 2023, 3:13 PM
lib/csu/aarch64/crt1_c.c
35

the email makes this sound confusing: Why have _start declared here at all. Seems like a nop since _start isn't referenced in this file.

dchagin added inline comments.Jun 26 2023, 3:20 PM
lib/csu/aarch64/crt1_c.c
35

just due to attributes, _start is not referenced anywhere since it is entry point

dchagin mentioned this in D40758: libexecinfo: Enable backtrace_test.backtrace_fmt_basic again on amd64.Jun 26 2023, 3:23 PM
dim added a comment.Jun 26 2023, 6:37 PM

Hmm this might also allow us to revert rG5866c369e4fd917c0d456f0f10b92ee354b82279, which was added to work around https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256864 . As far as I remember, this only affected aarch64 though, and that is tricky for me to test.

dchagin added a comment.Jun 26 2023, 6:49 PM
In D40623#927474, @dim wrote:

Hmm this might also allow us to revert rG5866c369e4fd917c0d456f0f10b92ee354b82279, which was added to work around https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256864 . As far as I remember, this only affected aarch64 though, and that is tricky for me to test.

for aarch64 unwinding through signal frame is not implemented, it would be nice to add something like https://reviews.llvm.org/rGc82deed6764cbc63966374baf9721331901ca958#change-9Mv5koDaVLbn

kib added inline comments.Jun 26 2023, 9:05 PM
lib/csu/amd64/crt1_s.S
32 ↗(On Diff #123756)

Perhaps add a comment that the only reason for _start existence is to issue .cfi_undefined.

That said, how hard is to rewrite _start1 in asm?

dchagin added inline comments.Jun 27 2023, 11:39 PM
lib/csu/amd64/crt1_s.S
32 ↗(On Diff #123756)

https://reviews.freebsd.org/D40780

however I don't know what is GCRT and how test under it

dchagin updated this revision to Diff 124026.Jun 30 2023, 8:23 AM

arm64 only

Harbormaster completed remote builds in B52356: Diff 124026.Jun 30 2023, 8:23 AM
dchagin retitled this revision from csu: Add a stop indicator to _start to satisfy unwinders on x86 and arm64 to csu: Add a stop indicator to _start to satisfy unwinders on aarch64.Jun 30 2023, 8:24 AM
dchagin edited the summary of this revision. (Show Details)
dchagin removed reviewers: kib, dim.
dchagin updated this revision to Diff 124106.Jul 2 2023, 7:07 PM
dchagin edited the summary of this revision. (Show Details)

Fix how address of a main is resolved, follow p 7.3.1 of sysv ABI for Aarch64

Harbormaster completed remote builds in B52387: Diff 124106.Jul 2 2023, 7:07 PM
dchagin updated this revision to Diff 124116.Jul 3 2023, 12:32 PM

ugh, fix GCRT case, gprof fine

Harbormaster completed remote builds in B52396: Diff 124116.Jul 3 2023, 12:32 PM
andrew added inline comments.Jul 4 2023, 8:35 AM
lib/csu/aarch64/crt1_s.S
50

If you're adding support for the large code model why didn't you use the following sequence? We know the code length will be under 1MiB so it is safe to use.

ldr x0, .L0
.L0
.xword var
dchagin updated this revision to Diff 124161.Jul 4 2023, 10:35 AM

Thank you, mostly due to I doubts as Im new to it, so I used this approach, fixed now.

Harbormaster completed remote builds in B52427: Diff 124161.Jul 4 2023, 10:35 AM
dchagin marked an inline comment as done.Jul 4 2023, 10:41 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 7 2023, 4:56 PM
Closed by commit rGbae6bb0698ee: csu: Implement _start using as to satisfy unwinders on aarch64 (authored by dchagin). · Explain Why
This revision was automatically updated to reflect the committed changes.
dchagin added a commit: rGbae6bb0698ee: csu: Implement _start using as to satisfy unwinders on aarch64.
arichardson added a subscriber: arichardson.Sep 18 2023, 6:07 PM

Is there a reason you didn't add __asm__ volatile(".cfi_undefined x30"); to the start of the C function? In my testing with CheriBSD this appears to work in the same way (libunwind tests fail before, succeed after).

arichardson added a comment.Sep 18 2023, 6:13 PM
In D40623#955061, @arichardson wrote:

Is there a reason you didn't add __asm__ volatile(".cfi_undefined x30"); to the start of the C function? In my testing with CheriBSD this appears to work in the same way (libunwind tests fail before, succeed after).

For context, here is the change I just submitted to CheriBSD: https://github.com/CTSRD-CHERI/cheribsd/pull/1859

dchagin added a comment.Sep 18 2023, 6:14 PM
In D40623#955061, @arichardson wrote:

Is there a reason you didn't add __asm__ volatile(".cfi_undefined x30"); to the start of the C function? In my testing with CheriBSD this appears to work in the same way (libunwind tests fail before, succeed after).

libunwind version? AFAIR, I fixed this in the libunwind but 8.1 libunwind version has not been released yet, seems

dchagin added a comment.Sep 18 2023, 6:25 PM
In D40623#955061, @arichardson wrote:

Is there a reason you didn't add __asm__ volatile(".cfi_undefined x30"); to the start of the C function? In my testing with CheriBSD this appears to work in the same way (libunwind tests fail before, succeed after).

stock aarch64 main, fresh libunwind:

make check-TESTS
PASS: test-proc-info
PASS: test-static-link
PASS: test-strerror
PASS: Gtest-bt
PASS: Ltest-bt
PASS: Gtest-init
PASS: Ltest-init
PASS: Gtest-concurrent
PASS: Ltest-concurrent
PASS: Gtest-trace
PASS: Ltest-trace
PASS: Ltest-mem-validate
PASS: test-async-sig
PASS: test-flush-cache
PASS: test-init-remote
PASS: test-mem
PASS: test-reg-state
PASS: Ltest-varargs
PASS: Ltest-nomalloc
PASS: Ltest-nocalloc
PASS: Lrs-race
PASS: Ltest-init-local-signal
PASS: Gtest-exc
PASS: Ltest-exc
PASS: Gtest-resume-sig
PASS: Ltest-resume-sig
PASS: Gtest-resume-sig-rt
PASS: Ltest-resume-sig-rt
PASS: test-ptrace
PASS: test-setjmp
Illegal instruction (core dumped)
XFAIL: Garm64-test-sve-signal
Illegal instruction (core dumped)
XFAIL: Larm64-test-sve-signal
PASS: aarch64-test-plt
FAIL: run-check-namespace
PASS: run-ptrace-mapper

PASS: run-ptrace-misc

Testsuite summary for libunwind 1.8-pre

  1. TOTAL: 36
  2. PASS: 33
  3. SKIP: 0
  4. XFAIL: 2
  5. FAIL: 1
  6. XPASS: 0
  7. ERROR: 0

Revision Contents
Changeset List

PathSize
lib/
csu/
aarch64/
3 lines
15 lines
24 lines

Diff 124161

View Options

lib/csu/aarch64/Makefile

Loading...
View Options

lib/csu/aarch64/crt1_c.c

Loading...
View Options

lib/csu/aarch64/crt1_s.S

Loading...