This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] AARCH64 use inline assembly for pointer authentication
AcceptedPublic

Authored by kitaisreal on May 21 2021, 3:48 AM.

Details

Reviewers
compnerd
danielkiss
Group Reviewers
Restricted Project
Summary

Clang does not support local register variables.

Diff Detail

Event Timeline

kitaisreal created this revision.May 21 2021, 3:48 AM
kitaisreal requested review of this revision.May 21 2021, 3:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2021, 3:48 AM

Formatted code using clang-format

Clang does not support local register variables.

LGTM just please run once more git clang-format HEAD^1 on the patch.

libunwind/src/DwarfInstructions.hpp
225–246

the code style is that { in the same line with the if statement.

compnerd added inline comments.
libunwind/src/DwarfInstructions.hpp
228

Could you use \n\t instead of the ; for the inline assembly please?

230

I wonder if this would be easier to spot with a constant being passed in as:

static const uint16_t autia1716 = 0xc;
static const uint16_t autib1716 = 0xe;
__asm__ __volatile__ ("mov x17, %x0\n\t"
                      "mov x16, %x1\n\t"
                      "hint %[key]\n\t"
                      "mov %0, x17\n\t"
                      : "+r" (ra)
                      : "r" (cfa), [key] "i" (autib1716)
                      : "x16", "x17");

Fixed code review issues.

kitaisreal marked 3 inline comments as done.May 21 2021, 2:31 PM

Clang does not support local register variables.

https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html is supported but I am sure there can be weak points (especially for some older releases). So avoiding the feature sounds good for portability.

libunwind/src/DwarfInstructions.hpp
242

Just use hint 0xe?

pcc added a subscriber: pcc.May 21 2021, 4:44 PM

Which version of Clang are you using? I'm reasonably sure that we're building libunwind on Android with Clang, and the pointer authentication code works there.

Which version of Clang are you using? I'm reasonably sure that we're building libunwind on Android with Clang, and the pointer authentication code works there.

I tried to make native AARCH64 build with clang-11.
I am using clang-11, native build on AARCH64, there was a crash during O0 build. Let me show example:

#include <stdint.h>

uintptr_t __attribute__ ((noinline)) authenticatePtrKeyB(uintptr_t pointer, uintptr_t modifier)
{
    register unsigned long long x17 __asm("x17") = pointer;
    register unsigned long long x16 __asm("x16") = modifier;

    asm volatile("hint 0xe" : "+r"(x17) : "r"(x16)); // autib1716
    pointer = x17;

    return pointer;
}

int main(int argc, char **argv)
{
    uintptr_t value_to_take = 5;
    uintptr_t ptr_to_value = reinterpret_cast<uintptr_t>(&value_to_take);
    authenticatePtrKeyB(ptr_to_value, 3);

    return 0;
}
/usr/bin/clang-11 -O0 register_example.cpp -o register_example

Stack dump:
0.	Program arguments: /usr/lib/llvm-11/bin/clang -cc1 -triple aarch64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -disable-llvm-verifier -discard-value-names -main-file-name register_example.cpp -mrelocation-model static -mframe-pointer=non-leaf -fmath-errno -fno-rounding-math -mconstructor-aliases -target-cpu generic -target-feature +neon -target-abi aapcs -fallow-half-arguments-and-returns -fno-split-dwarf-inlining -debugger-tuning=gdb -resource-dir /usr/lib/llvm-11/lib/clang/11.0.0 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/10/../../../../include/c++/10 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/10/../../../../include/aarch64-linux-gnu/c++/10 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/10/../../../../include/aarch64-linux-gnu/c++/10 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/10/../../../../include/c++/10/backward -internal-isystem /usr/local/include -internal-isystem /usr/lib/llvm-11/lib/clang/11.0.0/include -internal-externc-isystem /usr/include/aarch64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -O0 -fdeprecated-macro -fdebug-compilation-dir /home/ubuntu/test -ferror-limit 19 -fno-signed-char -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -fcolor-diagnostics -faddrsig -o /tmp/register_example-0ba515.o -x c++ register_example.cpp
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module 'register_example.cpp'.
4.	Running pass 'RegBankSelect' on function '@_Z19authenticatePtrKeyBmm'
#0 0x0000ffff8f9809fc llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/lib/aarch64-linux-gnu/libLLVM-11.so.1+0xa5c9fc)

If I compile this file using O2 it will work and in objdump code for function is correctly generated.

0000000000400544 <_Z19authenticatePtrKeyBmm>:
  400544:       aa0003f1        mov     x17, x0
  400548:       aa0103f0        mov     x16, x1
  40054c:       d50321df        autib1716
  400550:       aa1103e0        mov     x0, x17
  400554:       d65f03c0        ret
/usr/bin/clang-11 --version
Ubuntu clang version 11.0.0-2~ubuntu20.04.1
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

I checked following links:

  1. https://clang.llvm.org/docs/UsersManual.html#gcc-extensions-not-implemented-yet.
  2. It is for other libunwind implementation https://github.com/libunwind/libunwind/issues/215

Seems like it is better to not use partially supported gcc extensions.

libunwind/src/DwarfInstructions.hpp
242

Discussed in previous comments that it make sense to be more specific and use constant.

kitaisreal marked an inline comment as done.May 24 2021, 2:36 AM
compnerd accepted this revision.May 26 2021, 8:38 AM
This revision is now accepted and ready to land.May 26 2021, 8:38 AM
pcc added inline comments.May 26 2021, 10:36 AM
libunwind/src/DwarfInstructions.hpp
242

(I would vote for the way you had this originally, FWIW.)

MaskRay added inline comments.May 26 2021, 11:03 AM
libunwind/src/DwarfInstructions.hpp
242

"hint 0xe;" // autib1716

Updated assembly hints to previous revision.

kitaisreal marked 2 inline comments as done.May 26 2021, 1:53 PM
kitaisreal added inline comments.
libunwind/src/DwarfInstructions.hpp
242

Updated to previous revision.

242

Updated to previous revision.

/usr/bin/clang-11 --version
Ubuntu clang version 11.0.0-2~ubuntu20.04.1

Seems it is fixed in 11.0.1 and trunk or at least I was not able to reproduce the crash, codegen looks fine.

kitaisreal marked an inline comment as done.May 28 2021, 12:54 PM

/usr/bin/clang-11 --version
Ubuntu clang version 11.0.0-2~ubuntu20.04.1

Seems it is fixed in 11.0.1 and trunk or at least I was not able to reproduce the crash, codegen looks fine.

Yes also could not reproduce it.

danielkiss requested changes to this revision.Nov 16 2021, 12:17 AM

I think we can drop this change.

This revision now requires changes to proceed.Nov 16 2021, 12:17 AM

@danielkiss it seems that this patch fixes libunwind build for arm for old llvm versions, and also removes dependency on local register variables feature. Should not we apply it ?
Reproducible crash scenario for clang-11.

cat /etc/os-release
NAME="Ubuntu"
VERSION="20.04.4 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.4 LTS"
VERSION_ID="20.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=focal
UBUNTU_CODENAME=focal
uname -a
Linux ip-172-31-37-142 5.13.0-1022-aws #24~20.04.1-Ubuntu SMP Thu Apr 7 22:14:11 UTC 2022 aarch64 aarch64 aarch64 GNU/Linux
arch
aarch64

Download clang-11 using llvm.sh

wget https://apt.llvm.org/llvm.sh
chmod +x llvm.sh
sudo ./llvm.sh 11

clang version

clang++-11 --version
Ubuntu clang version 11.0.0-2~ubuntu20.04.1
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

/usr/bin/clang-11 --version
Ubuntu clang version 11.0.0-2~ubuntu20.04.1
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Now lets reproduce bug

#include <stdint.h>

uintptr_t __attribute__ ((noinline)) authenticatePtrKeyB(uintptr_t pointer, uintptr_t modifier)
{
    register unsigned long long x17 __asm("x17") = pointer;
    register unsigned long long x16 __asm("x16") = modifier;

    asm volatile("hint 0xe" : "+r"(x17) : "r"(x16)); // autib1716
    pointer = x17;

    return pointer;
}

int main(int argc, char **argv)
{
    uintptr_t value_to_take = 5;
    uintptr_t ptr_to_value = reinterpret_cast<uintptr_t>(&value_to_take);
    authenticatePtrKeyB(ptr_to_value, 3);

    return 0;
}

Compile with clang++-11

/usr/bin/clang++-11 -O0 register_example.cpp -o register_example
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /usr/lib/llvm-11/bin/clang -cc1 -triple aarch64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -disable-llvm-verifier -discard-value-names -main-file-name register_example.cpp -mrelocation-model static -mframe-pointer=non-leaf -fmath-errno -fno-rounding-math -mconstructor-aliases -target-cpu generic -target-feature +neon -target-abi aapcs -fallow-half-arguments-and-returns -fno-split-dwarf-inlining -debugger-tuning=gdb -resource-dir /usr/lib/llvm-11/lib/clang/11.0.0 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/c++/9 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/aarch64-linux-gnu/c++/9 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/aarch64-linux-gnu/c++/9 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/c++/9/backward -internal-isystem /usr/local/include -internal-isystem /usr/lib/llvm-11/lib/clang/11.0.0/include -internal-externc-isystem /usr/include/aarch64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -O0 -fdeprecated-macro -fdebug-compilation-dir /home/ubuntu -ferror-limit 19 -fno-signed-char -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -fcolor-diagnostics -faddrsig -o /tmp/register_example-8aae67.o -x c++ register_example.cpp 
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module 'register_example.cpp'.
4.	Running pass 'RegBankSelect' on function '@_Z19authenticatePtrKeyBmm'
#0 0x0000ffff9775a9fc llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/lib/aarch64-linux-gnu/libLLVM-11.so.1+0xa5c9fc)
clang: error: unable to execute command: Segmentation fault (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see invocation)
Ubuntu clang version 11.0.0-2~ubuntu20.04.1
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
clang: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /tmp/register_example-814e11.cpp
clang: note: diagnostic msg: /tmp/register_example-814e11.sh
clang: note: diagnostic msg: 

********************

Compile with clang-11

/usr/bin/clang-11 -O0 register_example.cpp -o register_example
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /usr/lib/llvm-11/bin/clang -cc1 -triple aarch64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -disable-llvm-verifier -discard-value-names -main-file-name register_example.cpp -mrelocation-model static -mframe-pointer=non-leaf -fmath-errno -fno-rounding-math -mconstructor-aliases -target-cpu generic -target-feature +neon -target-abi aapcs -fallow-half-arguments-and-returns -fno-split-dwarf-inlining -debugger-tuning=gdb -resource-dir /usr/lib/llvm-11/lib/clang/11.0.0 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/c++/9 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/aarch64-linux-gnu/c++/9 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/aarch64-linux-gnu/c++/9 -internal-isystem /usr/bin/../lib/gcc/aarch64-linux-gnu/9/../../../../include/c++/9/backward -internal-isystem /usr/local/include -internal-isystem /usr/lib/llvm-11/lib/clang/11.0.0/include -internal-externc-isystem /usr/include/aarch64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -O0 -fdeprecated-macro -fdebug-compilation-dir /home/ubuntu -ferror-limit 19 -fno-signed-char -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -fcolor-diagnostics -faddrsig -o /tmp/register_example-035369.o -x c++ register_example.cpp 
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module 'register_example.cpp'.
4.	Running pass 'RegBankSelect' on function '@_Z19authenticatePtrKeyBmm'
#0 0x0000ffffb54469fc llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/lib/aarch64-linux-gnu/libLLVM-11.so.1+0xa5c9fc)
clang: error: unable to execute command: Segmentation fault (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see invocation)
Ubuntu clang version 11.0.0-2~ubuntu20.04.1
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
clang: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /tmp/register_example-afe67d.cpp
clang: note: diagnostic msg: /tmp/register_example-afe67d.sh
clang: note: diagnostic msg: 

********************
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 2:29 AM

Address code review comments.

kitaisreal updated this revision to Diff 435540.Jun 9 2022, 7:24 AM

Patch is only needed due to a clang bug which is fixed already in 11.x releases and TOT. For me the original code looks better and I don't see the need for the patch but I let other from the @libunwind group to disagree with me.

Could CMake catch this problem? The minimal supported Clang is 12.0?

ldionne added a subscriber: ldionne.Sep 7 2022, 2:21 PM

Indeed, if this only catches issues on a now-unsupported version of Clang, I think this should be abandoned.

danielkiss resigned from this revision.Jan 17 2023, 3:59 AM
This revision is now accepted and ready to land.Jan 17 2023, 3:59 AM