This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][CET] Support exception handling stack unwind in CET environment
ClosedPublic

Authored by xiongji90 on Jul 14 2021, 1:47 AM.

Details

Summary

Control-flow Enforcement Technology (CET), published by Intel, introduces shadow stack feature aiming to ensure a return from a function is directed to where the function was called.
In a CET enabled system, each function call will push return address into normal stack and shadow stack, when the function returns, the address stored in shadow stack will be popped and compared with the return address, program will fail if the 2 addresses don't match.
In exception handling, the control flow may skip some stack frames and we must adjust shadow stack to avoid violating CET restriction.
In order to achieve this, we count the number of stack frames skipped and adjust shadow stack by this number before jumping to landing pad.
We have run libcxx, libcxxabi, libunwind tests with this patch in a CET enabled Fedora34 system, no failed tests found.
To build an application with CET enabled. we need to ensure:

  1. build the source code with "-fcf-protection=full"
  2. all the libraries linked with .o files must be CET enabled too

To run libcxx and libcxxabi tests with libunwind in a CET enabled system, we built libc++ and libc++abi.so with "-fcf-protection". Another patch has been uploaded for review to enable building llvm toolchian with CET enabled: https://reviews.llvm.org/D105603 This patch is not enough to enable all components but it does build libcxx, libcxxabi with CET enabled.

Diff Detail

Event Timeline

xiongji90 created this revision.Jul 14 2021, 1:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2021, 1:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
xiongji90 requested review of this revision.Jul 14 2021, 1:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2021, 1:47 AM
hjl.tools added inline comments.Jul 14 2021, 5:23 AM
libunwind/src/UnwindRegistersRestore.S
51

I don't think it is safe to access memory below stack pointer in 32-bit mode since there is no red zone. Please check what
the GCC unwinder does.

126

It is OK to access memory below stack pointer in 64-bit mode since there is a red zone.

libunwind/src/cet_unwind.h
29

Why are / and % used here? You pop 255 frames a time if numbers of frame > 255.

xiongji90 added inline comments.Jul 14 2021, 7:14 AM
libunwind/src/UnwindRegistersRestore.S
51

Hi, @hjl.tools
Before jumping to -4(%esp), we have added 4 to %esp value, so we should access a valid stack frame here.
Before adding 4 to esp, it should point to memory location where jump target address resides.
Thanks very much.

hjl.tools added inline comments.Jul 14 2021, 7:19 AM
libunwind/src/UnwindRegistersRestore.S
51

When an interrupt comes between "add $4, %esp" and "jmp -4(%esp)", the content at -4(%esp) can be changed by
the interrupt.

This update fixes some format issue and updates the way to jump to landing pad in 32bit node.
Previously, we used following code to jump to landing pad:
"
add $4, %esp
jmp *-4(%esp)
"
If interrupt comes in between these 2 instructions, interrupt handler may change the contents of "-4(%esp)" since there is no red zone in 32bit mode. To fix this issue, we align with gcc unwind to use %ecx to store landing pad address and jmp to it:
"
pop %ecx
jmp *%ecx
"

According to i386 ABI:
"Prior to executing code in the landing pad, the unwind library restores registers not altered by the personality routine, using the context record, to their state in that frame before the call that threw the exception, as follows. All registers specified as callee-saved by the base ABI are restored, as well as scratch registers %eax and %edx (see below). Except for those exceptions, scratch (or caller-saved) registers are not preserved, and their contents are undefined on transfer."
It is OK to use %ecx for jumping to landing pad.

xiongji90 added inline comments.Jul 15 2021, 1:01 AM
libunwind/src/UnwindRegistersRestore.S
51

Hi, @hjl.tools
For 32bit mode, I have aligned with gcc unwind to use %ecx to store landing pad address then jump to it.
Thanks very much.

libunwind/src/cet_unwind.h
29

Removed unnecessary % and /.

hjl.tools added inline comments.Jul 15 2021, 5:56 AM
libunwind/src/cet_unwind.h
29

Can you do

while (tmp > 255)
{
...
}

and remove if/else?

hjl.tools added inline comments.Jul 15 2021, 6:02 AM
libunwind/src/UnwindLevel1.c
42

Why is this needed? Can you unwind the shadow stack and jump to the landing pad as usual?

Use scratch register %rcx for indirect jumping to landing pad, there are 2 reasons for this:

  1. it is safer since red zone may be unavailable
  2. align with 32bit mode

Hi, @manojgupta
This patch aims to fix https://bugs.llvm.org/show_bug.cgi?id=45946
Could you help me review it?
Thanks very much!

@pcc @cferris Would you be able to review this libunwind patch?

xiongji90 added reviewers: pcc, cferris.

Add a build option "LIBUNWIND_BUILD_CET_ENABLE" to enable CET for libunwind.

This isn't code I'm really familiar with. You probably want @rprichard since he's worked on this recently.

xiongji90 updated this revision to Diff 361940.Jul 27 2021, 1:38 AM
xiongji90 added inline comments.
libunwind/src/UnwindLevel1.c
42

Hi, @hjl.tools
I have updated the patch to remove some unnecessary code, could you help review again?
Thanks very much.

xiongji90 updated this revision to Diff 361944.Jul 27 2021, 1:53 AM
compnerd added inline comments.
libunwind/CMakeLists.txt
55

Please name this LIBUNWIND_ENABLE_CET

146

I don't think that this is needed. You should be able to directly use LIBUNWIND_BUILD_CET_ENABLE at the sites using this local variable.

182

I think that we want to test and set:

  • /Gy
  • /guard:ehcont
  • /guard:cf
  • /cetcompat

Or explicitly error with MSVC.

libunwind/src/UnwindLevel1.c
47

Why does this have to be a macro? Can we just define the function instead?

48

Can we avoid the value here and use #if defined(__x86_64__) || defined(_M_ARM64) instead to select between the inline assembly?

libunwind/src/assembly.h
18

Do you need to consider MinGW or cywin?

libunwind/src/cet_unwind.h
13

Please add a newline above this line.

libunwind/src/libunwind_ext.h
37

Why is this new interface needed? Who is the expected user?

hjl.tools added inline comments.Jul 27 2021, 6:30 PM
libunwind/src/UnwindLevel1.c
42

__unw_phase2_resume looks quite fragile. Can you call the function pointer, cetJumpAddress, in C directly?

xiongji90 added inline comments.Jul 27 2021, 8:56 PM
libunwind/CMakeLists.txt
182

Hi, @compnerd
I suggest to report error with MSVC currently since CET enabling for Windows is still ongoing, once MS claims the enabling work is done, we can support MSVC.
Thanks very much.

libunwind/src/UnwindLevel1.c
42

Hi, @hjl.tools
I am afraid calling a function pointer in C is risky. The cetJumpAddress is address of libunwind_Registers_x86/x86_64_jumpto and these functions will never return, if we call this function or call the function pointer, compiler codegen may generate following code:
"
mov
libunwind_Registers_x86_64_jumpto, %rax
call *%rax
"
Under such circumstance, we must adjust CET SSP by 1 in __libunwind_Registers_x86_64_jumpto since the call instruction will push CET shadow stack.

However, if compiler codegen use "jmp" instead of "call", for example:
"
jmp xxxxx (the addr of __libunwind_Registers_x86_64_jumpto)
"
we shouldn't adjust CET shadow stack. It is difficult for us to decide whether or not should we adjust CET shadow stack since we can't know what code compiler will generate for the function/function pointer call in C source level.

Thanks very much.

47

Hi, @compnerd
In previous version, I defined a function the code looks like following:
void unw_phase2_resume(cursor, fn) {
LIBUNWIND_POP_CET_SSP(...);
....
....
asm volatile(....)
}
The stack layout looks like following if compiler doesn't do any optimization to __unw_phase2_resume:


.....user functions


__cxx_throw


_Unwind_RaiseException


unwind_phase2


__unw_phase2_resume


Under such circumstances, the stack frame for unw_phase2_resume will be skipped too and we must adjust CET shadow stack for it.
However, as we can see, the
unw_phase2_resume will never return. I am afraid compiler may directly jump to this function instead of "call" it. If so, there will be no stack frame for unw_phase2_resume and if compiler uses "jmp" to jump to it instead of calling it, we mustn't adjust CET shadow stack for it.
In our source code, we can't know whether compiler will do such optimization and we can't decide whether we should adjust CET shadow stack for
unw_phase2_resume. So I used macro here to avoid this problem.
I used inline asm to jump to __libunwind_Registers_x86_64_jumpto for the same purpose.
Thanks very much.

libunwind/src/assembly.h
18

Hi, @compnerd
I suggest to ignore MinGW and cywin currently.
Hi, @hjl.tools
Do you think we can ignore MinGW and cywin?

Thanks very much.

hjl.tools added inline comments.Jul 28 2021, 6:29 AM
libunwind/src/UnwindLevel1.c
42

GCC uses an undocumented intrinsic, __builtin_eh_return, to avoid such issue.

libunwind/src/assembly.h
18

If MinGW and cywin support CET, libunwind should support MinGW and cywin.

  1. Rename LIBUNWIND_BUILD_CET_ENABLE to LIBUNWIND_ENABLE_CET in libunwind CMakeFile and remove unnecessary LIBUNWIND_CET_ON local variable
  2. Get rid of using value of _LIBUNWIND_CET_ENABLED and using "_LIBUNWIND_TARGET_I386" and "_LIBUNWIND_TARGET_X86_64" to select inline asm.
xiongji90 added inline comments.Jul 29 2021, 12:01 AM
libunwind/CMakeLists.txt
55

Done.

Thanks very much!

146

Removed the unnecessary local variable and use LIBUNWIND_ENABLE_CET directly.

Thanks very much!

libunwind/src/UnwindLevel1.c
48

Hi, @compnerd
I have abandoned usage of _LIBUNWIND_CET_ENABLED value and used "_LIBUNWIND_TARGET_I386" and "_LIBUNWIND_TARGET_X86_64" to select inline assembly. Currently CET is implemented in GNU Linux + Intel platform, so we don't need to check for ARM64.

Thanks very much.

xiongji90 added inline comments.Jul 29 2021, 12:47 AM
libunwind/src/UnwindLevel1.c
42

Hi, @hjl.tools
The undocumented GCC intrinsic builtin_eh_return are used by GCC unwinder and its semantic are strongly related to GCC unwind's implementation.
For example, GCC's unwinder will install target context and then get target landing pad address, after that GCC unwinder will use
builtin_eh_return to adjust the SP value and jump to landing pad, builtin_eh_return assumes that target context has been prepared.
In our implementation, we need to adjust CET SSP and then jump to
libunwind_Registers_X86/_64_jumpto(asm code), this asm function will prepare target context (restore registers), adjust SP and jump to final landing pad. Our requirement here is just directly jump to libunwind_Registers_X86/_64_jumpto function to avoid pushing CET shadow stack again. So, I think builtin_eh_return can't help much in libunwind.

Thanks very much.

libunwind/src/assembly.h
18

Hi, @hjl.tools and @compnerd
I didn't find any info of CET support for MinGW and cywin after some google search, so I suggest we can ignore MinGW and cywin currently.

Thanks very much.

libunwind/src/libunwind_ext.h
37

Hi, @compnerd
Currently, Registers are encapsulated in UnwindCursor class and libunwind_Registers_x86_jumpto/libunwind_Registers_x86_64_jumpto need Registers as input parameter. We need to prepare the parameter and then jump to libunwind_Registers_x86/_64_jumpto function, so we need to get internal Registers from a cursor. This function is added for this purpose and we don't expect other users to use it.
Do you think we should define it as an internal function instead of a function with "
unw" prefix?
Thanks very much.

hjl.tools added inline comments.Jul 29 2021, 7:56 AM
libunwind/src/UnwindLevel1.c
42

One feature of __builtin_eh_return is to perform the special tail call. Can clang provide a way to do that?

compnerd added inline comments.Jul 29 2021, 1:30 PM
libunwind/CMakeLists.txt
182

I'm not sure about the statement (I believe that the CET support is available today), but I would be okay with the error stating that libunwind doesn't support CET on Windows yet. I'm more concerned that someone who builds the library may be under the false impression that CET is enabled when it is not.

libunwind/src/Registers.hpp
46

A newline before this line like in the x86_64 case would be nice.

libunwind/src/UnwindLevel1.c
47

I wonder if we can get away with some combination of noinline, and pragmas for no-optimize-sibling-calls.

libunwind/src/libunwind_ext.h
37

Yes please move this to an internal function. This header is exported to users, and this is why I was asking specifically about this function.

xiongji90 added inline comments.Jul 29 2021, 8:04 PM
libunwind/CMakeLists.txt
182

Hi, @compnerd
I will update the patch to report error for MSVC + CET. All components including OS, toolchain, system libraries need to enable CET so that CET feature can be fully enabled in a platform. I know MS is working on CET but I am not sure whether they have finished the work.
Hi, @hjl.tools
Could you please correct me if anything is wrong about CET enabling status for Windows and MSVC.
Thanks very much.

libunwind/src/UnwindLevel1.c
42

Hi, @hjl.tools
I think clang is compatible with GCC for __builtin_eh_return and could you share some sample code or info about this special feature?
If it can work, we can get rid of the inline asm.
Thanks very much.

xiongji90 updated this revision to Diff 362984.Jul 30 2021, 1:05 AM
  1. Report error for LIBUNWIND_ENABLE_CET + MSVC
  2. Turn unw_cet_get_registers to an internal function libunwind_cet_get_registers
  3. Some tiny code format change.
xiongji90 added inline comments.Jul 30 2021, 1:07 AM
libunwind/src/Registers.hpp
46

Done.
Thanks very much.

xiongji90 added inline comments.Aug 1 2021, 8:42 PM
libunwind/src/UnwindLevel1.c
42

Hi, @hjl.tools
I tried to use the undocumented intrinsic "builtin_eh_return" to replace inline asm. The code for x86_64 is like following:
builtin_eh_return(0, libunwind_Registers_x86_64_jumpto);
With "-fcf-protection=full", both gcc and clang will use a "jmpq *%rcx" to jump to
libunwind_Registers_x86_64 which meets our requirement.
However, this intrinsic is used to jump to landing pad and expects to pass parameters via rax and rdx while libunwind_Registers_x86_64_jumpto is not a landing pad but a "normal" function which doesn't return. libunwind_Registers_x86_64_jumpto follows x86_64 abi to receive input parameters which means it expects input parameters from rdi, rsi.... And libunwind_Registers_x86_jumpto follows x86 abi to receive input parameters which means it expects input parameters stored in stack by caller. So I am afraid builtin_eh_return can't fully meet our requirement.
Thanks very much.

xiongji90 updated this revision to Diff 363381.EditedAug 1 2021, 11:16 PM

Chang default value of LIBUNWIND_ENABLE_CET to OFF in libunwind CMakeFile.
Since the LLVM_BUILD_CET_ENABLE has not been added to LLVM building system, we can set LIBUNWIND_ENABLE_CET default value to OFF currently.
When LLVM_BUILD_CET_ENABLE is supported in LLVM building, we can set the default value to LLVM_BUILD_CET_ENABLE.

xiongji90 added inline comments.Aug 2 2021, 1:39 AM
libunwind/src/UnwindLevel1.c
47

Hi, @compnerd
I think using combination of following:

  1. using attribute((noinline)) to decorate target functions
  2. building source file containing target functions with -fno-optimize-sibling-calls

can help us to get away with the issue mentioned above. I only found gcc/clang compiling options for "-fno-optimize-sibling-calls" and didn't find corresponding pragma or attribute, so if we choose to use "-fno-optimize-sibling-calls", tail call optimization for other functions will be disabled too which seems to be overshooting.
And if choosing the combination of "noinline" and "-fno-optimize-sibling-calls", we are relying on gcc/clang's implementation which is a implicit dependency, using macro here should be safe to all compilers.

Thanks very much.

xiongji90 added inline comments.
libunwind/src/UnwindLevel1.c
47

Hi, @compnerd
If you have concern about the macro "unw_phase_resume", I can try to update the patch to use combination of "noinline" and "-fno-optimize-sibling-calls". And do we have any way to use "no-optimize-sibling-calls" to decorate a function? I only found compiling option "-fno-optimize-sibling-calls".
Thanks very much.

Hi, @compnerd
I also uploaded a patch to enable building libc++ and libc++abi with CET enabled https://reviews.llvm.org/D107560
libc++ and libc++abi source code doesn't include any assembly, we just need to build the source with "-fcf-protection=full". Could you help review the libc++,libc++abi patch too?
BTW, if using combination of "noinline" and "-fno-optimize-sibling-calls" is the right approach, I will update the patch.
Thanks very much.

Have you checked check-cxxabi test/forced_unwind[12].pass.cpp? Do they pass with CET?

libunwind/src/UnwindLevel1.c
47

I think using a macro is a right call here, more robust than dancing with -noinline, no-optimize-sibling-calls and using functions and computing how many additional frames are skipped.

51

Question: why is this 1 + (fn)?

55

Is the calling convention here wrong?

173
223
287
libunwind/src/cet_unwind.h
21

Since CET is x86 only, (defined(_LIBUNWIND_TARGET_I386) || defined(_LIBUNWIND_TARGET_X86_64)) can be omitted.

Have you checked check-cxxabi test/forced_unwind[12].pass.cpp? Do they pass with CET?

Hi, @MaskRay
Yes, all libcxx and libcxxabi case including forced_unwind[12].pass.cpp have been tested using libunwind. Those "forced_unwind" test cases depend on _Unwind_ForcedUnwind and this patch also supports _Unwind_ForcedUnwind in CET system.
Thanks very much.

xiongji90 added inline comments.Aug 9 2021, 10:27 PM
libunwind/src/UnwindLevel1.c
51

Hi, @MaskRay
This is related to libunwind implementation for _Unwind_RaiseException/Resume and _Unwind_ForcedUnwind.
For example, following is _Unwind_RaiseException code:

/// Called by __cxa_throw. Only returns if there is a fatal error.
_LIBUNWIND_EXPORT _Unwind_Reason_Code
_Unwind_RaiseException(_Unwind_Exception *exception_object) {

_LIBUNWIND_TRACE_API("_Unwind_RaiseException(ex_obj=%p)",
                     (void *)exception_object);
unw_context_t uc;
unw_cursor_t cursor;
__unw_getcontext(&uc);

// Mark that this is a non-forced unwind, so _Unwind_Resume()
// can do the right thing.
exception_object->private_1 = 0;
exception_object->private_2 = 0;

// phase 1: the search phase
_Unwind_Reason_Code phase1 = unwind_phase1(&uc, &cursor, exception_object);
if (phase1 != _URC_NO_REASON)
  return phase1;

// phase 2: the clean up phase
return unwind_phase2(&uc, &cursor, exception_object);

}

In phase2, _Unwind_RaiseException will call unwind_phase2 and will never return if landing pad is found. So, we need to adjust CET shadow stack for "unwind_phase2" too.
In unwind_phase2, we will record how many stack frames we need to skip(starting from _Unwind_RaiseException and unwind_phase2 is not included), store the value into framesWalked. At last, we use _LIBUNWIND_POP_CET_SSP to adjust CET shadow stack by 1 + framesWalked frames.

It seems we can have a clear way to achieve this, initializing framesWalked to 1 instead 0 and add some comments to demonstrate the initialization is for unwind_phase2.
Thanks very much.

55

Hi, @MaskRay
The code here is to emulate x86 calling convention. As we know, x86 uses stack to pass input parameter and store return address.
We will jump to "libunwind_Registers_x86_jumpto" instead of calling it to avoid any change to CET shadow stack.
In libunwind, "
libunwind_Registers_x86_jumpto" is a pure x86 assembly function, the implementation expects the stack frame to be a standard x86 function call stack frame like following:

+______________+
+ input param +
+______________+
+ return addr +
+___________ +
.....
libunwind_Registers_x86_jumpto will never return, so the return address stored in stack will not be used. However, the implementation does rely on the stack layout, following code is from __libunwind_Registers_x86_jumpto:

DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_x86_jumpto)

  1. extern "C" void __libunwind_Registers_x86_jumpto(Registers_x86 *); #
  2. On entry:
  3. + +
  4. +-----------------------+
  5. + thread_state pointer +
  6. +-----------------------+
  7. + return address +
  8. +-----------------------+ <-- SP
  9. + + _LIBUNWIND_CET_ENDBR movl 4(%esp), %eax
    1. set up eax and ret on new stack location movl 28(%eax), %edx # edx holds new stack pointer subl $8,%edx movl %edx, 28(%eax) movl 0(%eax), %ebx

......
As we can see, we must ensure the input parameter is placed in 4(%esp), 0(%esp) stores dummy return address which will never be used.
So, after pushing input parameter for libunwind_Registers_x86_jumpto into stack, we have to "sub $4, %%esp" in order to emulate pushing return address into stack, the pushing is aimed to make stack layout to meet libunwind_Registers_x86_jumpto's requirement.
Thanks very much.

xiongji90 updated this revision to Diff 365384.Aug 10 2021, 1:52 AM
xiongji90 added inline comments.
libunwind/src/UnwindLevel1.c
173

Fixed.
Thanks very much.

223

Fixed.
Thanks very much.

287

Fixed.
Thanks very much.

Mostly looks good.

libunwind/CMakeLists.txt
181

add_compile_flags.

When LIBUNWIND_ENABLE_CET=on, it is an error if the compiler does not support the options.

libunwind/src/UnwindLevel1.c
55

sub $4, %%esp is for the dummay return address. Haven't used x86-32 assembly for a while and forgot this. Sorry!

You may just write asm instead of __asm__. Other places use asm directly as well.

173

git diff -U0 --no-color 'HEAD^' | clang/tools/clang-format/clang-format-diff.py -i -p1 reformat as needed.

libunwind/src/cet_unwind.h
18

on Linux x86 platforms.

libunwind/test/lit.site.cfg.in
30

perhaps config.x86_cet

  1. Check if compiler used to build libunwind supports CET compiling options.
  2. rename "cet_on" to "x86_cet"
xiongji90 added inline comments.Aug 10 2021, 11:25 PM
libunwind/CMakeLists.txt
181

Hi, @MaskRay
I update the patch to check "LIBUNWIND_SUPPORTS_FCF_PROTECTION_EQ_FULL_FLAG" and "LIBUNWIND_SUPPORTS_MSHSTK_FLAG", if either flag is not set, error will be reported.
Thanks very much.

libunwind/src/UnwindLevel1.c
55

Hi, @MaskRay
I also think using C++ keyword "asm" is better but libunwind source has already used "asm": https://github.com/llvm/llvm-project/blob/main/libunwind/src/config.h#L79 So I aligned with this style.
Thanks very much.

173

Fixed.
Thanks very much.

libunwind/src/cet_unwind.h
18

Fixed.
Thanks very much.

libunwind/test/lit.site.cfg.in
30

Fixed.
Thanks very much.

MaskRay added inline comments.Aug 11 2021, 12:32 AM
libunwind/src/cet_unwind.h
2

This file probably should just be moved into config.h, then other files don't need to add a new #include.

20

Perhaps rename to _LIBUNWIND_USE_CET

MaskRay accepted this revision.Aug 11 2021, 12:33 AM

Hi, @compnerd
Do you have any other concerns or comments?
Thanks very much.

MaskRay accepted this revision.Aug 12 2021, 12:56 PM

It'd be good to get compnerd's opinion as well.

This revision is now accepted and ready to land.Aug 12 2021, 12:56 PM
compnerd accepted this revision.Aug 20 2021, 1:44 PM

Minor comments about the implementation, but I think that this is reasonable.

libunwind/CMakeLists.txt
184

I would recommend SEND_ERROR here instead of FATAL_ERROR. This will allow getting subsequent errors as well.

187

I would recommend SEND_ERROR here instead of FATAL_ERROR.

libunwind/src/UnwindCursor.hpp
456

Most of the warnings indicate the name. I would suggest we follow the same convention to help quickly identify the function which is unimplemented.

libunwind/src/UnwindLevel1.c
56
57–58
66
67–68
libunwind/src/cet_unwind.h
11–12
18

Hi, @compnerd and @MaskRay
I just updated the patch to fix some code format/comments issue, do I need your approval again to commit this patch?
Thanks very much.

libunwind/CMakeLists.txt
184

Done.

Thanks very much.

187

Done.

Thanks very much.

libunwind/src/cet_unwind.h
11–12

Done.

Thanks very much.

18

Done.

Thanks very much.

MaskRay accepted this revision.Aug 23 2021, 5:58 PM

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

xiongji90 updated this revision to Diff 368265.Aug 23 2021, 9:07 PM
xiongji90 added inline comments.Aug 23 2021, 9:10 PM
libunwind/src/UnwindCursor.hpp
456

Done.

Thanks very much.

libunwind/src/UnwindLevel1.c
57–58

Done.

Thanks very much.

67–68

Done.

Thanks very much.

xiongji90 updated this revision to Diff 368299.Aug 24 2021, 1:35 AM

Rebase the patch to latest llvm and address some tiny issues.

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

Hi, @MaskRay
Left unresolved comments are resolved and I also rebased the patch to latest llvm libunwind code.

Thanks very much.

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

Hi, @MaskRay
Left unresolved comments are resolved and I also rebased the patch to latest llvm libunwind code.

Thanks very much.

You marked your own comments as "Done" but not reviewers' comments as "Done".

So I currently see 35 among 79 comments are done, while the rest are not.

Please mark resolved ones as done.

If you need a reviewer to commit this patch on your behalf, please provide git commit --amend --author="name <email>".

xiongji90 marked 41 inline comments as done.Aug 24 2021, 7:28 PM
xiongji90 updated this revision to Diff 368540.Aug 24 2021, 7:56 PM
xiongji90 marked an inline comment as done.
xiongji90 marked an inline comment as done.Aug 24 2021, 7:59 PM

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

Hi, @MaskRay
Left unresolved comments are resolved and I also rebased the patch to latest llvm libunwind code.

Thanks very much.

You marked your own comments as "Done" but not reviewers' comments as "Done".

So I currently see 35 among 79 comments are done, while the rest are not.

Please mark resolved ones as done.

If you need a reviewer to commit this patch on your behalf, please provide git commit --amend --author="name <email>".

Hi, @MaskRay

Sorry for the mistake, I just marked all reviewers' comments as "Done". I just got the commit access and will commit the code soon.

Thanks very much for your kind help.

libunwind/src/cet_unwind.h
2

Hi, @MaskRay
I suggest to keep cet_unwind.h currently since CET is still a narrow used hardware feature right now and all macro definitions in config.h seems to have general usage. In the long term, if CET is enabled by default in x86/x86_64 platforms, we can move the contents in cet_unwind.h into config.h.

Thanks very much.

20

Done.
Thanks very much.

xiongji90 marked an inline comment as done.Aug 24 2021, 8:17 PM
This revision was landed with ongoing or failed builds.Aug 26 2021, 1:14 AM
This revision was automatically updated to reflect the committed changes.