This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [PATCH 08/22] Add Intel CET support for EH in libgcc.
- From: "Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>
- To: Jeff Law <law at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Cc: "ian at airs dot com" <ian at airs dot com>, "Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>
- Date: Wed, 8 Nov 2017 21:29:50 +0000
- Subject: RE: [PATCH 08/22] Add Intel CET support for EH in libgcc.
- Authentication-results: sourceware.org; auth=none
- Dlp-product: dlpe-windows
- Dlp-reaction: no-action
- Dlp-version: 11.0.0.116
- References: <D511F25789BA7F4EBA64C8A63891A00291F42252@IRSMSX102.ger.corp.intel.com> <fb89b327-a7d8-ac5c-824a-7851388c7836@redhat.com> <D511F25789BA7F4EBA64C8A63891A00291F521E3@IRSMSX102.ger.corp.intel.com> <d411234a-cdfc-2859-3d57-487c114d584d@redhat.com>
> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Wednesday, November 8, 2017 8:06 PM
> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc-
> patches@gcc.gnu.org
> Cc: ian@airs.com
> Subject: Re: [PATCH 08/22] Add Intel CET support for EH in libgcc.
>
> On 11/04/2017 06:43 AM, Tsimbalist, Igor V wrote:
> >> -----Original Message-----
> >> From: Jeff Law [mailto:law@redhat.com]
> >> Sent: Tuesday, October 31, 2017 5:49 AM
> >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc-
> >> patches@gcc.gnu.org
> >> Cc: ian@airs.com
> >> Subject: Re: [PATCH 08/22] Add Intel CET support for EH in libgcc.
> >>
> >> On 10/12/2017 01:56 PM, Tsimbalist, Igor V wrote:
> >>> Control-flow Enforcement Technology (CET), published by Intel,
> Introduces
> >>> the Shadow Stack feature, which ensures a return from a function is
> done
> >>> to exactly the same location from where the function was called. When
> EH
> >>> is present the control-flow transfer may skip some stack frames and the
> >>> shadow stack has to be adjusted not to signal a violation of a
> >>> control-flow transfer. It's done by counting a number of skipping frames
> >>> and adjusting shadow stack pointer by this number.
> >>>
> >>> gcc/
> >>> * config/i386/i386.c (ix86_expand_epilogue): Change simple
> >>> return to indirect jump for EH return. Change explicit 'false'
> >>> argument in pro_epilogue_adjust_stack with a value of
> >>> flag_cf_protection.
> >>> * config/i386/i386.md (simple_return_indirect_internal): Remove
> >>> SImode restriction to support 64-bit.
> >>>
> >>> libgcc/
> >>> * config/i386/linux-unwind.h: Include
> >>> config/i386/shadow-stack-unwind.h.
> >>> * config/i386/shadow-stack-unwind.h: New file.
> >>> * unwind-dw2.c: (uw_install_context): Add a FRAMES argument and
> >>> pass it to _Unwind_Frames_Extra.
> >>> * unwind-generic.h (FRAMES_P_DECL): New.
> >>> (FRAMES_VAR): Likewise.
> >>> (FRAMES_VAR_P): Likewise.
> >>> (FRAMES_VAR_DECL): Likewise.
> >>> (FRAMES_VAR_DECL_1): Likewise.
> >>> (FRAMES_VAR_INC): Likewise.
> >>> (FRAMES_P_UPDATE): Likewise.
> >>> (_Unwind_Frames_Extra): Likewise.
> >>> * unwind.inc (_Unwind_RaiseException_Phase2): Use
> >> FRAMES_P_DECL,
> >>> FRAMES_VAR_DECL_1, FRAMES_VAR_INC and FRAMES_P_UPDATE.
> >>> (_Unwind_RaiseException): Use FRAMES_VAR_DECL,
> >> FRAMES_VAR_P and
> >>> FRAMES_VAR.
> >>> (_Unwind_ForcedUnwind_Phase2): Use FRAMES_P_DECL,
> >>> FRAMES_VAR_DECL_1, FRAMES_VAR_INC, FRAMES_P_UPDATE.
> >>> (_Unwind_ForcedUnwind): Use FRAMES_VAR_DECL,
> >> FRAMES_VAR_P and
> >>> FRAMES_VAR.
> >>> (_Unwind_Resume): Use FRAMES_VAR_DECL, FRAMES_VAR_P and
> >>> FRAMES_VAR.
> >>> (_Unwind_Resume_or_Rethrow): Use FRAMES_VAR_DECL,
> >> FRAMES_VAR_P
> >>> and FRAMES_VAR.
> >>>
> >>> Igor
> >>>
> >>>
> >>>
> >>> 0008-Add-Intel-CET-support-for-EH-in-libgcc.patch
> >>>
> >>>
> >>> From 16eb1d0d9239e039fba28f1ae71762f19061b157 Mon Sep 17
> 00:00:00
> >> 2001
> >>> From: Igor Tsimbalist <igor.v.tsimbalist@intel.com>
> >>> Date: Wed, 19 Jul 2017 03:04:46 +0300
> >>> Subject: [PATCH 08/22] Add Intel CET support for EH in libgcc.
> >>>
> >>> Control-flow Enforcement Technology (CET), published by Intel,
> >>> introduces
> >>> the Shadow Stack feature, which ensures a return from a function is
> done
> >>> to exactly the same location from where the function was called. When
> EH
> >>> is present the control-flow transfer may skip some stack frames and the
> >>> shadow stack has to be adjusted not to signal a violation of a
> >>> control-flow transfer. It's done by counting a number of skiping frames
> >>> and adjasting shadow stack pointer by this number.
> >>>
> >>> Having new semantic of the 'ret' instruction if CET is supported in HW
> >>> the 'ret' instruction cannot be generated in ix86_expand_epilogue when
> >>> we are returning after EH is processed. Added a code in
> >>> ix86_expand_epilogue to adjust Shadow Stack pointer and to generate
> an
> >>> indirect jump instead of 'ret'. As sp register is used during this
> >>> adjustment thus the argument in pro_epilogue_adjust_stack is changed
> >>> to update cfa_reg based on whether control-flow instrumentation is set.
> >>> Without updating the cfa_reg field there is an assert later in dwarf2
> >>> pass related to mismatch the stack register and cfa_reg value.
> >>>
> >>> gcc/
> >>> * config/i386/i386.c (ix86_expand_epilogue): Change simple
> >>> return to indirect jump for EH return. Change explicit 'false'
> >>> argument in pro_epilogue_adjust_stack with a value of
> >>> flag_cf_protection.
> >>> * config/i386/i386.md (simple_return_indirect_internal): Remove
> >>> SImode restriction to support 64-bit.
> >>>
> >>> libgcc/
> >>> * config/i386/linux-unwind.h: Include
> >>> config/i386/shadow-stack-unwind.h.
> >>> * config/i386/shadow-stack-unwind.h: New file.
> >>> * unwind-dw2.c: (uw_install_context): Add a FRAMES argument and
> >>> pass it to _Unwind_Frames_Extra.
> >>> * unwind-generic.h (FRAMES_P_DECL): New.
> >>> (FRAMES_VAR): Likewise.
> >>> (FRAMES_VAR_P): Likewise.
> >>> (FRAMES_VAR_DECL): Likewise.
> >>> (FRAMES_VAR_DECL_1): Likewise.
> >>> (FRAMES_VAR_INC): Likewise.
> >>> (FRAMES_P_UPDATE): Likewise.
> >>> (_Unwind_Frames_Extra): Likewise.
> >>> * unwind.inc (_Unwind_RaiseException_Phase2): Use
> >> FRAMES_P_DECL,
> >>> FRAMES_VAR_DECL_1, FRAMES_VAR_INC and FRAMES_P_UPDATE.
> >>> (_Unwind_RaiseException): Use FRAMES_VAR_DECL,
> >> FRAMES_VAR_P and
> >>> FRAMES_VAR.
> >>> (_Unwind_ForcedUnwind_Phase2): Use FRAMES_P_DECL,
> >>> FRAMES_VAR_DECL_1, FRAMES_VAR_INC, FRAMES_P_UPDATE.
> >>> (_Unwind_ForcedUnwind): Use FRAMES_VAR_DECL,
> >> FRAMES_VAR_P and
> >>> FRAMES_VAR.
> >>> (_Unwind_Resume): Use FRAMES_VAR_DECL, FRAMES_VAR_P and
> >>> FRAMES_VAR.
> >>> (_Unwind_Resume_or_Rethrow): Use FRAMES_VAR_DECL,
> >> FRAMES_VAR_P
> >>> and FRAMES_VAR.
> >>>
> >>>
> >>
>
> >> I must say that I'm not happy with all the macro games we're playing in
> >> this patch. Is there no cleaner way to get the desired behavior?
> >>
> >> Are there any ABI/API implications of this patch? Ie, does the
> >> signature of any exported function change?
> > There is no ABI/API implications. It's done through macro to keep existing
> infrastructure
> > and functions. Otherwise a copy of unwind functions have to be introduced
> and modified
> > for CET support.
> It just seems to me there ought to be a way to handle this without
> making a copy and without the macro games.
>
> So for example, why can't we make the handling of FRAMES_VAR_*
> unconditional. Just go ahead and declare the local variable and compute
> it. Pass its address to Unwind_RaiseException_Phase2 unconditionally.
> The parameter might need to be marked as ATTRIBUTE_UNUSED.
>
> Then the only macro games we need is _Unwind_Frames_Extra.
>
> We end up with some potentially dead code at the source level (for cases
> where _Unwind_Frames_Extra does nothing). Ideally the IPA bits would
> realize the code is dead and specialize UnwindRaiseException_Phase2
> removing the unnecessary overhead. But even if IPA didn't do that I
> think the result is just so much cleaner that a bit of overhead on the
> exception/unwinding path is warranted.
Ok, I will implement your suggestion. I agree that is cleaner.
> >
> >> Has this been tested anywhere other than x86/x86_64 linux?
> > Yes, I tested it on arm64 system. I applied 2 patches, previous 07/22 and
> this one 08/22. Everything
> > was built successfully. Further to the building I did testing also. No new
> fails.
> So how does that reconcile with H-P's note about the calls to
> uw_install_context when FRAMES_VAR is defined as nothinng?
>
> + uw_install_context (&this_context, &cur_context, FRAMES_VAR);
>
> Doesn't that create a syntax error when FRAMES_VAR is defined, but with
> no content?
It doesn't give a syntax error. HJ has given an example in this thread.
Igor
> Jeff