Page MenuHomePhabricator

[AArch64] Add v8.5 Branch Target Identification support.
ClosedPublic

Authored by danielkiss on Apr 9 2020, 2:34 AM.

Details

Summary

The .note.gnu.property must be in the assembly file to indicate the
support for BTI otherwise BTI will be disabled for the whole library.
__unw_getcontext and libunwind::Registers_arm64::jumpto() may be called
indirectly therefore they should start with a landing pad.

Diff Detail

Event Timeline

danielkiss created this revision.Apr 9 2020, 2:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 9 2020, 2:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Please help me how to add a test for it, in the current state this patch won't do anything because the standard build for libunwind does not add -mbranch-protection=standard (or bti).

chill added inline comments.Apr 9 2020, 2:53 AM
libunwind/src/assembly.h
47

This will be emitted for all functions, but we need only one per object file, isn't it ?

Also, could you, please, rename PPC64OPD{1,2} to TARGET_OPD{1,2}, or {ARCH ...}, something like that.

miyuki added a comment.Apr 9 2020, 4:23 AM

Please help me how to add a test for it, in the current state this patch won't do anything because the standard build for libunwind does not add -mbranch-protection=standard (or bti).

I suggest adding a CMake variable to configure the branch protection option used when building the C++ libraries

scw added a subscriber: scw.Apr 9 2020, 4:11 PM
MaskRay added a subscriber: MaskRay.Apr 9 2020, 5:27 PM
MaskRay added inline comments.
libunwind/src/assembly.h
46

Just use defined(__aarch64__). I deleted __arm64__ in D77829

48

I am worried that we may want to change the section type for .note.gnu.property

https://sourceware.org/pipermail/binutils/2020-April/000372.html

danielkiss marked 4 inline comments as done.Apr 14 2020, 3:10 PM

Please help me how to add a test for it, in the current state this patch won't do anything because the standard build for libunwind does not add -mbranch-protection=standard (or bti).

I suggest adding a CMake variable to configure the branch protection option used when building the C++ libraries

Thanks, it will be a new patch.

libunwind/src/assembly.h
47

Yes, only one function per file today, but let's add the section per include.

Rename is done.

48

Thanks for the heads-up, I'm not on that mailing list...

Mostly good apart from the renaming issue and a nit about comments. I have some patches in the BTI/PAC area, so I am confident.

libunwind/src/assembly.h
47

@chill
Please don't rename OPD{1,2}. They are really specific to PowerPC ELFv1 function descriptors. No other architecture needs them.

53

The comments can be added right after .long

MaskRay added inline comments.Apr 14 2020, 3:34 PM
libunwind/src/assembly.h
62

This really needs to use a new macro.

I expect a __ARM_FEATURE_BTI_DEFAULT toolchain capable of assembling bti c

chill added inline comments.Apr 15 2020, 1:10 AM
libunwind/src/assembly.h
163

All right, of these are to stay named PPC64_OPD, then just add a new line with, say, AARCH64_BTI, e..g

#define DEFINE_LIBUNWIND_FUNCTION(name)                                        \
.globl SYMBOL_NAME(name) SEPARATOR                                           \
HIDDEN_SYMBOL(SYMBOL_NAME(name)) SEPARATOR                                   \
SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR                                  \
PPC64_OPD1                                                                    \ 
SYMBOL_NAME(name):                                                           \
PPC64_OPD2                                                                   \
AARCH64_BTI
danielkiss marked an inline comment as done.
danielkiss marked 3 inline comments as done.Apr 15 2020, 8:58 AM
danielkiss added inline comments.
libunwind/src/assembly.h
62

Expectation is right, D71658 broke it, I will take a look.

danielkiss marked an inline comment as done.
danielkiss marked 2 inline comments as done.Apr 15 2020, 9:03 AM
danielkiss added inline comments.
libunwind/src/assembly.h
53

Missed this comment in the previous version.

danielkiss marked an inline comment as done.

D81257 is merged, so now BTI C mnemonic can be used instead of the hint.

compnerd accepted this revision.Jul 28 2020, 8:59 AM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
libunwind/src/assembly.h
51

I'm not sure if the defined(__aarch64__) check is absolutely required, but it doesn't give the wrong result.

This revision is now accepted and ready to land.Jul 28 2020, 8:59 AM
danielkiss updated this revision to Diff 282596.Aug 3 2020, 5:23 AM

fix review comment.

danielkiss marked an inline comment as done.Aug 3 2020, 5:24 AM
danielkiss added inline comments.
libunwind/src/assembly.h
51

Thanks, fixed it.

danielkiss marked 3 inline comments as done.Sep 4 2020, 4:39 AM
danielkiss added inline comments.Sep 17 2020, 1:59 AM
libunwind/src/assembly.h
51–63

I would drop this because it could be now replaced with a compiler flag -mmark-bti-property (D81930)

danielkiss added inline comments.Mon, Sep 21, 2:20 PM
libunwind/src/assembly.h
51–63

adding the flag to only the assembly files would be huge change in the cmake files, so I revoke my idea to drop the note from here.