| Message ID | c763d912-0806-7007-0381-807c94a9b746@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On 10/20/2016 09:28 AM, Jiong Wang wrote: > The current code suppose targetm.stack_protect_fail always generate > something. > But in case one target start to generate NULL_TREE, there will be ICE. > This > patch adds a simple sanity check to only call expand if it's not NULL_TREE. > > OK for trunk? > > gcc/ > 2016-10-20 Jiong Wang <jiong.wang@arm.com> > > * function.c (stack_protect_epilogue): Only expands > targetm.stack_protect_fail if it's not NULL_TREE. Is there some reason we don't want to issue an error here and stop compilation? I'm not at all comfortable silently ignoring failure to generate stack protector code. jeff
2016-10-20 19:50 GMT+01:00 Jeff Law <law@redhat.com>: > > On 10/20/2016 09:28 AM, Jiong Wang wrote: >> >> The current code suppose targetm.stack_protect_fail always generate >> something. >> But in case one target start to generate NULL_TREE, there will be ICE. >> This >> patch adds a simple sanity check to only call expand if it's not NULL_TREE. >> >> OK for trunk? >> >> gcc/ >> 2016-10-20 Jiong Wang <jiong.wang@arm.com> >> >> * function.c (stack_protect_epilogue): Only expands >> targetm.stack_protect_fail if it's not NULL_TREE. > > Is there some reason we don't want to issue an error here and stop compilation? I'm not at all comfortable silently ignoring failure to generate stack protector code. > > jeff Hi Jeff, That's because I am doing some work where I will borrow stack-protector's analysis infrastructure but for those stack-protector standard rtl insn, they just need to be expanded into empty, for example stack_protect_set/test just need to be expanded into NOTE_INSN_DELETED. The same for targetm.stack_protect_fail () which I want to simply return NULL_TREE. but it's not an error. This do seems affect other targets (x86, rs6000) if NULL_TREE should never be returned for them. Currently I can see all of them use the either default_external_stack_protect_fail or default_hidden_stack_protect_fail, both of which are "return build_call_expr (..", so I should also assert the the return value of build_call_expr? Thanks.
On 10/20/2016 01:46 PM, Jiong Wang wrote: > 2016-10-20 19:50 GMT+01:00 Jeff Law <law@redhat.com>: >> >> On 10/20/2016 09:28 AM, Jiong Wang wrote: >>> >>> The current code suppose targetm.stack_protect_fail always generate >>> something. >>> But in case one target start to generate NULL_TREE, there will be ICE. >>> This >>> patch adds a simple sanity check to only call expand if it's not NULL_TREE. >>> >>> OK for trunk? >>> >>> gcc/ >>> 2016-10-20 Jiong Wang <jiong.wang@arm.com> >>> >>> * function.c (stack_protect_epilogue): Only expands >>> targetm.stack_protect_fail if it's not NULL_TREE. >> >> Is there some reason we don't want to issue an error here and stop compilation? I'm not at all comfortable silently ignoring failure to generate stack protector code. >> >> jeff > > > Hi Jeff, > > That's because I am doing some work where I will borrow > stack-protector's analysis infrastructure but for those > stack-protector standard rtl insn, they just need to be expanded into > empty, for example stack_protect_set/test just need to be expanded > into NOTE_INSN_DELETED. The same for targetm.stack_protect_fail () > which I want to simply return NULL_TREE. but it's not an error. Right. But your change could mask backend problems. Specifically if their expander for stack_protect_fail did fail and returned NULL_TREE. That would cause it to silently ignore stack protector failures, which seems inadvisable. Is there another way you can re-use the analysis code without resorting to something like this? > > This do seems affect other targets (x86, rs6000) if NULL_TREE should > never be returned for them. Currently I can see all of them use the > either default_external_stack_protect_fail or > default_hidden_stack_protect_fail, both of which are "return > build_call_expr (..", so I should also assert the the return value of > build_call_expr? Asserting couldn't hurt. I'd much rather have the compiler issue an error, ICE or somesuch than silently not generate a call to the stack protector fail routine. jeff
On 24/10/16 16:22, Jeff Law wrote: > On 10/20/2016 01:46 PM, Jiong Wang wrote: >> 2016-10-20 19:50 GMT+01:00 Jeff Law <law@redhat.com>: >>> >>> On 10/20/2016 09:28 AM, Jiong Wang wrote: >>>> >>>> The current code suppose targetm.stack_protect_fail always generate >>>> something. >>>> But in case one target start to generate NULL_TREE, there will be ICE. >>>> This >>>> patch adds a simple sanity check to only call expand if it's not >>>> NULL_TREE. >>>> >>>> OK for trunk? >>>> >>>> gcc/ >>>> 2016-10-20 Jiong Wang <jiong.wang@arm.com> >>>> >>>> * function.c (stack_protect_epilogue): Only expands >>>> targetm.stack_protect_fail if it's not NULL_TREE. >>> >>> Is there some reason we don't want to issue an error here and stop >>> compilation? I'm not at all comfortable silently ignoring failure >>> to generate stack protector code. >>> >>> jeff >> >> >> Hi Jeff, >> >> That's because I am doing some work where I will borrow >> stack-protector's analysis infrastructure but for those >> stack-protector standard rtl insn, they just need to be expanded into >> empty, for example stack_protect_set/test just need to be expanded >> into NOTE_INSN_DELETED. The same for targetm.stack_protect_fail () >> which I want to simply return NULL_TREE. but it's not an error. > Right. But your change could mask backend problems. Specifically if > their expander for stack_protect_fail did fail and returned NULL_TREE. > > That would cause it to silently ignore stack protector failures, which > seems inadvisable. > > Is there another way you can re-use the analysis code without > resorting to something like this? In my case, I only want the canary variable which is "crtl->stack_protect_guard", then I don't want the current runtime support which GCC will always generate once crl->stack_protect_guard is initialized. I was thinking to let stack_protect_fail to generate a tree that expand_call will expand into NULL_RTX unconditionally under any optimization level, but it seems impossible. Really appreicate for any idea on this. > > >> >> This do seems affect other targets (x86, rs6000) if NULL_TREE should >> never be returned for them. Currently I can see all of them use the >> either default_external_stack_protect_fail or >> default_hidden_stack_protect_fail, both of which are "return >> build_call_expr (..", so I should also assert the the return value of >> build_call_expr? > Asserting couldn't hurt. I'd much rather have the compiler issue an > error, ICE or somesuch than silently not generate a call to the stack > protector fail routine.
On 10/24/2016 10:29 AM, Jiong Wang wrote: > Right. But your change could mask backend problems. Specifically if >> their expander for stack_protect_fail did fail and returned NULL_TREE. >> >> That would cause it to silently ignore stack protector failures, which >> seems inadvisable. >> >> Is there another way you can re-use the analysis code without >> resorting to something like this? > > In my case, I only want the canary variable which is > "crtl->stack_protect_guard", then I don't want the current runtime > support which GCC will always generate once crl->stack_protect_guard is > initialized. Presumably you're using this with the new AArch64 security features. ISTM we ought to be able to separate access to the guard from the rest of the runtime bits. Jeff
diff --git a/gcc/function.c b/gcc/function.c index cdd2721cdf904be6457d090fe20345d3dee0b4dd..304c32ed2b1ace06139786680f30502d8483a8ed 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -5077,7 +5077,9 @@ stack_protect_epilogue (void) if (JUMP_P (tmp)) predict_insn_def (tmp, PRED_NORETURN, TAKEN); - expand_call (targetm.stack_protect_fail (), NULL_RTX, /*ignore=*/true); + tree fail_check = targetm.stack_protect_fail (); + if (fail_check != NULL_TREE) + expand_call (fail_check, NULL_RTX, /*ignore=*/true); free_temp_slots (); emit_label (label); }
The current code suppose targetm.stack_protect_fail always generate something. But in case one target start to generate NULL_TREE, there will be ICE. This patch adds a simple sanity check to only call expand if it's not NULL_TREE. OK for trunk? gcc/ 2016-10-20 Jiong Wang <jiong.wang@arm.com> * function.c (stack_protect_epilogue): Only expands targetm.stack_protect_fail if it's not NULL_TREE.