This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Handle G in personality string
ClosedPublic

Authored by fmayer on Jul 1 2022, 9:34 AM.

Details

Summary

Tested with the following program:

static volatile int* x = nullptr;

void throws()  __attribute__((noinline)) {
  if (getpid() == 0)
    return;
  throw "error";
}

void maybe_throws()  __attribute__((noinline)) {
  volatile int y = 1;
  x = &y;
  throws();
  y = 2;
}

int main(int argc, char** argv) {
  int y;
  try {
    maybe_throws();
  } catch (const char* e) {
    //printf("Caught\n");
  }
  y = *x;
  printf("%d\n", y); // should be MTE failure.
  return 0;
}

Built using clang++ -c -O2 -target aarch64-linux -fexceptions -march=armv8-a+memtag -fsanitize=memtag-heap,memtag-stack

Currently only Android implements runtime support for MTE stack tagging.

Without this change, we crash on __cxa_get_globals when trying to catch
the exception (because the stack frame __cxa_get_globals frame will fail due
to tags left behind on the stack). With this change, we crash on the y = *x;
as expected, because the stack frame has been untagged, but the pointer hasn't.

Diff Detail

Event Timeline

fmayer created this revision.Jul 1 2022, 9:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 1 2022, 9:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
fmayer updated this revision to Diff 441774.Jul 1 2022, 12:58 PM

fix unused argument error

fmayer updated this revision to Diff 441789.Jul 1 2022, 3:02 PM
fmayer added a comment.Jul 1 2022, 3:02 PM

fix mingw build

fmayer updated this revision to Diff 441811.Jul 1 2022, 4:21 PM

clang fmt

fmayer updated this revision to Diff 441825.Jul 1 2022, 5:30 PM

fix build

fmayer updated this revision to Diff 441826.Jul 1 2022, 5:32 PM

comment

fmayer updated this revision to Diff 449466.Aug 2 2022, 3:52 PM

update comment

fmayer published this revision for review.Aug 4 2022, 7:32 PM
fmayer added a reviewer: eugenis.

In addition to the test described in the message, I ran the libunwind test suites on FVP with SANITIZE_TARGET=memtag-stack and they still pass.

Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 7:32 PM
eugenis added inline comments.Aug 5 2022, 11:13 AM
libunwind/src/DwarfInstructions.hpp
221

Is this right? I'd expect to untag from current frame SP to previous frame SP, not to CFA (which could be at arbitrary offset in the frame AFAIK).

fmayer added inline comments.Aug 5 2022, 12:42 PM
libunwind/src/DwarfInstructions.hpp
221

ARM64 ABI says this: "The CFA is the value of the stack pointer (sp) at the call site in the previous frame."

(see https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#42canonical-frame-address)

fmayer updated this revision to Diff 450374.Aug 5 2022, 1:24 PM

add comment

This seems ok in general, but I'd like someone with better knowledge of the code to review.

libunwind/src/DwarfInstructions.hpp
221

SGTM but change the comment - this is not about SP in the previous frame at all, this is about CFA being at the bottom of the current stack frame.

libunwind/src/DwarfParser.hpp
54

whitespace here and below

libunwind/src/UnwindCursor.hpp
1019

Do we need MTE support here?

fmayer updated this revision to Diff 450452.Aug 5 2022, 5:14 PM
fmayer marked 4 inline comments as done.

address comments

libunwind/src/UnwindCursor.hpp
1019

I don't think so: This also doesn't handle the PAC BKey, and the plumbing through LLVM and the linker of that is the same as our 'G' flag.

MaskRay added a comment.EditedAug 13 2022, 7:02 PM

Without this change, we crash on cxa_get_globals when trying to catch the exception (because the stack frame hadn't been cleared up).

cxa_get_globals => __cxa_get_globals

Can you describe the clang command line? It's unclear how MTE is used.

The description should mention the introduction of __unw_clean_step. The clean part in the name and the clean parameter feels unclear to me.

(because the stack frame hadn't been cleared up)

Can you elaborate what this means?

MaskRay added inline comments.Aug 13 2022, 7:07 PM
libunwind/src/DwarfInstructions.hpp
220

-16ULL

About __unw_clean_step:

libunwind/src/libunwind_ext.h provides a bunch of API which are not implemented by https://github.com/libunwind/libunwind .
Before we settle on an API there, it's worth raising an issue on https://github.com/libunwind/libunwind and giving them some time for the API design.

About __unw_clean_step:

libunwind/src/libunwind_ext.h provides a bunch of API which are not implemented by https://github.com/libunwind/libunwind .
Before we settle on an API there, it's worth raising an issue on https://github.com/libunwind/libunwind and giving them some time for the API design.

AFAICT my change does not in fact expose a new API, because it does not use _LIBUNWIND_WEAK_ALIAS, so __unw_clean_step will remain hidden visiblity.

fmayer edited the summary of this revision. (Show Details)Aug 15 2022, 10:07 AM
fmayer edited the summary of this revision. (Show Details)
fmayer edited the summary of this revision. (Show Details)
fmayer added inline comments.Aug 15 2022, 10:17 AM
libunwind/src/DwarfInstructions.hpp
220

Can you explain why this is preferred? At least for me, using only bitwise operators is more intuitive.

About __unw_clean_step:

libunwind/src/libunwind_ext.h provides a bunch of API which are not implemented by https://github.com/libunwind/libunwind .
Before we settle on an API there, it's worth raising an issue on https://github.com/libunwind/libunwind and giving them some time for the API design.

AFAICT my change does not in fact expose a new API, because it does not use _LIBUNWIND_WEAK_ALIAS, so __unw_clean_step will remain hidden visiblity.

Ping. Should I move the extern int __unw_clean_step(unw_cursor_t *); declaration out of libunwind_ext.h if I don't want it to be an API?

Can you describe the clang command line? It's unclear how MTE is used.

Unaddressed.

Should I move the extern int __unw_clean_step(unw_cursor_t *); declaration out of libunwind_ext.h if I don't want it to be an API?

Good idea. __unw_clean_step does not seem suitable to be an API when the nongnu libunwind discussion has not happened.

The clean part in the name and the clean parameter feels unclear to me.

And this.

fmayer edited the summary of this revision. (Show Details)Aug 29 2022, 6:02 PM

The clean part in the name and the clean parameter feels unclear to me.

And this.

This is meant to express that this cleans up the stack frame, and it cannot be used afterwards (because the tags will not match anymore). Is cleanup better? destroy_frame?

fmayer edited the summary of this revision. (Show Details)Sep 14 2022, 5:17 PM
compnerd added inline comments.
libunwind/src/DwarfInstructions.hpp
224

Would be nice to split this over two lines.

libunwind/src/libunwind.cpp
186

I think that untag is better than clean - __unw_step_untag. We are untagging the memory and that makes it more clear.

fmayer updated this revision to Diff 460493.Sep 15 2022, 1:37 PM
fmayer marked 2 inline comments as done.

comments

libunwind/src/libunwind.cpp
186

OK. I wanted to keep this name generic in case we need to do other cleanup steps later, but we can also cross that bridge when/if we get there.

danielkiss added inline comments.
libunwind/src/libunwind.cpp
186

__unw_step is used in phase 1(search) and phase 2(apply).
maybe would be better to differentiate as unw_step_search and unw_step_apply (or similar) to make architecture agonistic.
For PAC we need to calculate the return value always but with MTE we could just untag memory when actually doing the unwinding.
same for step(bool untag = false); could be step(bool search_phase = false);

fmayer added inline comments.Sep 15 2022, 2:11 PM
libunwind/src/libunwind.cpp
186

I assume for step(bool search_phase = false) you meant step(bool apply_phase = false) (or step(bool search_phase = true)).

I'll hold off on further renames until we can get some consensus here.

fmayer added inline comments.Sep 15 2022, 3:02 PM
libunwind/src/libunwind.cpp
186

For PAC we need to calculate the return value always but with MTE we could just untag memory when actually doing the unwinding.

Also, we *must not* untag the frame in the search phase, because then we can cause tag mismatches in what you call the apply phase.

fmayer added inline comments.Sep 15 2022, 4:50 PM
libunwind/src/libunwind.cpp
186

Another naming option would be __unw_step_stage2

compnerd added inline comments.Sep 16 2022, 12:30 PM
libunwind/src/libunwind.cpp
186

I like __unw_step_stage2 better.

danielkiss added inline comments.Sep 16 2022, 1:25 PM
libunwind/src/libunwind.cpp
186

__unw_step_stage2 works for me too.
step(bool stage2 = false) would be it's pair IMHO.

fmayer added inline comments.Sep 16 2022, 1:51 PM
libunwind/src/libunwind.cpp
186

@MaskRay do you also agree with this name?

fmayer edited reviewers, added: compnerd; removed: MaskRay.Sep 19 2022, 2:33 PM
MaskRay added inline comments.
libunwind/src/libunwind.cpp
186

__unw_step_stage2 works for me, too.

stage2 is still a bit weird since we have phase1/phase2 distinction, but in the absence of a better name stage2 is fine to me.

fmayer marked 4 inline comments as done.Sep 19 2022, 5:16 PM

stage2 is fine with me, too
LGTM

MaskRay accepted this revision.Sep 20 2022, 2:46 PM
MaskRay added inline comments.
libunwind/src/libunwind.cpp
185

Mixed comment markers.

Using // for everything should be fine.

This revision is now accepted and ready to land.Sep 20 2022, 2:46 PM
fmayer updated this revision to Diff 461719.Sep 20 2022, 2:48 PM
fmayer marked an inline comment as done.

comment

compnerd accepted this revision.Sep 21 2022, 8:36 AM
This revision was landed with ongoing or failed builds.Sep 21 2022, 2:13 PM
This revision was automatically updated to reflect the committed changes.
mgorny added a subscriber: mgorny.Sep 30 2022, 4:57 AM

This change broke building with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON:

$ ninja -C build -j1  -k99
ninja: Entering directory `build'
[1/5] Building CXX object libunwind/src/CMakeFiles/unwind_shared_objects.dir/libunwind.cpp.o
FAILED: libunwind/src/CMakeFiles/unwind_shared_objects.dir/libunwind.cpp.o 
/usr/bin/c++ -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/mgorny/git/llvm-bisect/libunwind/include -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wimplicit-fallthrough -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -fdata-sections -Os -DNDEBUG -fPIC -Werror=return-type -W -Wall -Wchar-subscripts -Wconversion -Wmismatched-tags -Wmissing-braces -Wno-unused-function -Wshadow -Wsign-compare -Wsign-conversion -Wstrict-aliasing=2 -Wstrict-overflow=4 -Wunused-parameter -Wunused-variable -Wwrite-strings -Wundef -Wno-suggest-override -Wno-error -pedantic -funwind-tables -nostdinc++ -D_DEBUG -UNDEBUG -fno-rtti -std=c++11  -fstrict-aliasing -fno-exceptions -fno-rtti -MD -MT libunwind/src/CMakeFiles/unwind_shared_objects.dir/libunwind.cpp.o -MF libunwind/src/CMakeFiles/unwind_shared_objects.dir/libunwind.cpp.o.d -o libunwind/src/CMakeFiles/unwind_shared_objects.dir/libunwind.cpp.o -c /home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp
In file included from /home/mgorny/git/llvm-bisect/libunwind/src/DwarfParser.hpp:22,
                 from /home/mgorny/git/llvm-bisect/libunwind/src/EHHeaderParser.hpp:17,
                 from /home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:23,
                 from /home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp:30:
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp:2871:5: warning: "__mips_isa_rev" is not defined, evaluates to 0 [-Wundef]
 2871 | #if __mips_isa_rev != 6
      |     ^~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp:3195:5: warning: "__mips_isa_rev" is not defined, evaluates to 0 [-Wundef]
 3195 | #if __mips_isa_rev != 6
      |     ^~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp: In member function ‘bool libunwind::Registers_ppc64::validVectorRegister(int) const’:
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp:1549:54: warning: unused parameter ‘regNum’ [-Wunused-parameter]
 1549 | inline bool Registers_ppc64::validVectorRegister(int regNum) const {
      |                                                  ~~~~^~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp: In static member function ‘static int64_t libunwind::LocalAddressSpace::getSLEB128(pint_t&, pint_t)’:
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:256:42: warning: conversion to ‘long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Wsign-conversion]
  256 |     result |= (uint64_t)(byte & 0x7f) << bit;
      |                                          ^~~
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:261:26: warning: conversion to ‘long long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Wsign-conversion]
  261 |     result |= (-1ULL) << bit;
      |                          ^~~
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:261:12: warning: conversion to ‘int64_t’ {aka ‘long int’} from ‘long long unsigned int’ may change the sign of the result [-Wsign-conversion]
  261 |     result |= (-1ULL) << bit;
      |     ~~~~~~~^~~~~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp: At global scope:
/home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp:39:23: warning: type-punning to incomplete type might break strict-aliasing rules [-Wstrict-aliasing]
   39 |     (unw_addr_space_t)&LocalAddressSpace::sThisAddressSpace;
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/DwarfInstructions.hpp: Assembler messages:
/home/mgorny/git/llvm-bisect/libunwind/src/DwarfInstructions.hpp:224: Error: unknown pseudo-op: `.arch_extension'
/home/mgorny/git/llvm-bisect/libunwind/src/DwarfInstructions.hpp:225: Error: no such instruction: `stg %rax,[%rax]'
[2/5] Building CXX object libunwind/src/CMakeFiles/unwind_static_objects.dir/libunwind.cpp.o
FAILED: libunwind/src/CMakeFiles/unwind_static_objects.dir/libunwind.cpp.o 
/usr/bin/c++ -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/mgorny/git/llvm-bisect/libunwind/include -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wimplicit-fallthrough -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -fdata-sections -Os -DNDEBUG -Werror=return-type -W -Wall -Wchar-subscripts -Wconversion -Wmismatched-tags -Wmissing-braces -Wno-unused-function -Wshadow -Wsign-compare -Wsign-conversion -Wstrict-aliasing=2 -Wstrict-overflow=4 -Wunused-parameter -Wunused-variable -Wwrite-strings -Wundef -Wno-suggest-override -Wno-error -pedantic -funwind-tables -nostdinc++ -D_DEBUG -UNDEBUG -fno-rtti -std=c++11  -fstrict-aliasing -fno-exceptions -fno-rtti -MD -MT libunwind/src/CMakeFiles/unwind_static_objects.dir/libunwind.cpp.o -MF libunwind/src/CMakeFiles/unwind_static_objects.dir/libunwind.cpp.o.d -o libunwind/src/CMakeFiles/unwind_static_objects.dir/libunwind.cpp.o -c /home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp
In file included from /home/mgorny/git/llvm-bisect/libunwind/src/DwarfParser.hpp:22,
                 from /home/mgorny/git/llvm-bisect/libunwind/src/EHHeaderParser.hpp:17,
                 from /home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:23,
                 from /home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp:30:
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp:2871:5: warning: "__mips_isa_rev" is not defined, evaluates to 0 [-Wundef]
 2871 | #if __mips_isa_rev != 6
      |     ^~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp:3195:5: warning: "__mips_isa_rev" is not defined, evaluates to 0 [-Wundef]
 3195 | #if __mips_isa_rev != 6
      |     ^~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp: In member function ‘bool libunwind::Registers_ppc64::validVectorRegister(int) const’:
/home/mgorny/git/llvm-bisect/libunwind/src/Registers.hpp:1549:54: warning: unused parameter ‘regNum’ [-Wunused-parameter]
 1549 | inline bool Registers_ppc64::validVectorRegister(int regNum) const {
      |                                                  ~~~~^~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp: In static member function ‘static int64_t libunwind::LocalAddressSpace::getSLEB128(pint_t&, pint_t)’:
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:256:42: warning: conversion to ‘long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Wsign-conversion]
  256 |     result |= (uint64_t)(byte & 0x7f) << bit;
      |                                          ^~~
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:261:26: warning: conversion to ‘long long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Wsign-conversion]
  261 |     result |= (-1ULL) << bit;
      |                          ^~~
/home/mgorny/git/llvm-bisect/libunwind/src/AddressSpace.hpp:261:12: warning: conversion to ‘int64_t’ {aka ‘long int’} from ‘long long unsigned int’ may change the sign of the result [-Wsign-conversion]
  261 |     result |= (-1ULL) << bit;
      |     ~~~~~~~^~~~~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp: At global scope:
/home/mgorny/git/llvm-bisect/libunwind/src/libunwind.cpp:39:23: warning: type-punning to incomplete type might break strict-aliasing rules [-Wstrict-aliasing]
   39 |     (unw_addr_space_t)&LocalAddressSpace::sThisAddressSpace;
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/git/llvm-bisect/libunwind/src/DwarfInstructions.hpp: Assembler messages:
/home/mgorny/git/llvm-bisect/libunwind/src/DwarfInstructions.hpp:224: Error: unknown pseudo-op: `.arch_extension'
/home/mgorny/git/llvm-bisect/libunwind/src/DwarfInstructions.hpp:225: Error: no such instruction: `stg %rax,[%rax]'
ninja: build stopped: cannot make progress due to previous errors.

Please fix or revert.

This change broke building with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON:

Thanks for the report and sorry about that, sent https://reviews.llvm.org/D134969.

JOE1994 added inline comments.Feb 8 2023, 8:39 AM
libunwind/src/DwarfInstructions.hpp
224

This added inline assembly (stg instruction).
According to the ARM instruction reference guide, stg instruction is only supported in ARMv8.5 and later.

Since this change (D128998), I can no longer build libunwind on my ARMv8.1 machine.
The assembler gives the following error:

/tmp/ccim5Evd.s: Assembler messages:
/tmp/ccim5Evd.s:9031: Error: unknown architectural extension `memtag'
/tmp/ccim5Evd.s:9031: Error: unknown mnemonic `stg' -- `stg x2,[x2]'
/tmp/ccim5Evd.s:9040: Error: unknown architectural extension `memtag'
/tmp/ccim5Evd.s:9040: Error: unknown mnemonic `stg' -- `stg x2,[x2]'

Is there a way to add a guard to only insert the stg instruction in supported ISAs?

MaskRay added inline comments.Feb 8 2023, 9:10 AM
libunwind/src/DwarfInstructions.hpp
224

You'll need to use the latest Clang to build the file. Older Clang is not supported. See https://libcxx.llvm.org/BuildingLibcxx.html

JOE1994 added inline comments.Feb 8 2023, 5:26 PM
libunwind/src/DwarfInstructions.hpp
224

Thank you so much for the pointer. It helped me tremendously :)