Skip to content
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

Fix .cfi_undefined metadata with LLVM 16 #2202

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

arichardson
Copy link
Member

LLVM 16 places .cfi_undefined injected via inline assembly at the "wrong" location in the call frame info, so libunwind continues unwinding beyond _start and crashes.

Fix this by using a naked function instead.

Without this, libunwind will attempt to unwind beyond the end of
.rtld_start and eventually attempt to parse whatever data happens to
be on the stack. This matches Morello/AArch64 where the annotation
already exists.
@arichardson
Copy link
Member Author

First commit fixes shared libunwind tests on RISC-V (Morello already had the annotation), second one fixes it for static linking and ensures we stop unwinding at _start (I guess we could make it conditional on static vs dynamic crt1, but I think always doing it makes more sense).

lib/csu/aarch64c/crt1_c.c Outdated Show resolved Hide resolved
lib/csu/riscv64c/crt1_c.c Outdated Show resolved Hide resolved
The existing approach of using inline assembly to inject .cfi_undefined
into _start was fragile: after upgrading to LLVM 16, the undefined
information was placed before the normal stack setup metadata which
meant it was being ignored by libunwind and it continues unwinding.
For dynamically linked libraries this should not cause any problems
since (as of the last commit) RTLD's entry point has the required
termination annotation, but for statically linked binaries we keep
unwinding and eventually trigger a bounds violation while parsing
nonsense data.

Make this current annotation less fragile by using a naked function
that has the .cfi_undefined annotation and calls the real C entry.
@@ -68,12 +68,11 @@ Elf_Auxinfo *__auxargs;
* This restriction only applies to statically linked binaries since the dynamic
* linker takes care of initialization otherwise.
*/
void
_start(void *auxv,
__dead2 static void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you don't need the explicit __dead2 as __libc_start1 is already tagged with that so the compiler will infer it automatically. That said, if you do keep it, I would spell this as eitherstatic void __dead2 or static __dead2 void. Those are the predominate styles in the tree (a few more of the former).

* function could be overridden by the default unwinding information.
*/
__attribute__((naked, used)) void
_start(void *auxv,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of doing this in C rather than just writing assembly? Though I continue to feel that, longer-term, we should push for some GNU attribute that allows you to eschew the assembly entirely, and can make things less stupid upstream.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means I don't need to add another file to the build system. Also means we don't need to be careful about adding all the required extra directives to the .S file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning to upstream this to FreeBSD as well

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

Successfully merging this pull request may close these issues.

3 participants