Re: [PATCH] Implement no_stack_protect attribute.
On 10/20/20 2:19 PM, Richard Biener wrote: So can we use the same identifier as clang here as Nick requests? Thus, OK with re-naming everything alongside no_stack_protector. It isn't really the opposite of the stack_protect attribute since that only protects when -fstack-protector-explicit is enabled. Done that and installed the patch. Thanks for review, Martin
Re: [PATCH] Implement no_stack_protect attribute.
+ correct kernel mailing list this time. On Wed, Oct 21, 2020 at 2:33 PM Nick Desaulniers wrote: > > Thanks for the quick feedback! > > On Wed, Oct 21, 2020 at 2:13 PM Jakub Jelinek wrote: > > > > On Wed, Oct 21, 2020 at 02:04:15PM -0700, Nick Desaulniers via Gcc-patches > > wrote: > > > Tangentially related question: > > > We're running into a bug related to LTO for the kernel when code > > > compiled with -fno-stack-protector is called from and inlined into > > > code that is compiled with -fstack-protector. Specifically, stack > > > canaries get checked before they're restored post suspend/resume which > > > leads to spooky bugs. > > > > > > Once we have more fine grain function level attribute to explicitly > > > disable stack protectors on a per function basis, I'm considering > > > making this function attribute a barrier to inlining in LLVM so that > > > callers with stack protectors don't inline callees that explicitly > > > should not have a stack protector and vice versa (more concretely, > > > when they don't match). I think this would maximize which functions > > > are still covered by stack protectors, and be the most straightforward > > > to implement. > > > > That doesn't make sense to me. > > Stack protector doesn't affect in any way inlined code, the stack protection > > is always solely in the prologue and epilogue of out of line functions. > > So, if the (non-inlined) caller is -fstack-protector and inlined callee > > is -fno-stack-protector, there should be ssp store in the prologue of the > > resulting function and test in the epilogue. > > That is the case today, and I'm arguing that leads to bugs in the > Linux kernel when built with LTO. > > > The effect will be exactly > > like that if the function wouldn't be inlined. > > I don't follow. If the -fno-stack-protector callee was not inlined, > the caller would have a stack protector, while the callee would not. > I think today there's not a strong enough distinction between the > level of stack protection being specified vs explicit > annotations/flags that stack protectors MUST NOT be inserted. > > > Similarly, if the non-inlined caller is -fno-stack-protector and inlined > > callee is -fstack-protector, there will be no stack protection. This isn't > > And I'd argue that now we may have stripped off stack protection in > the pursuit of inlining. Consider for example the case where that > stack protected callee contained a large alloca; post inlining into a > -fno-stack-protected caller suddenly now it doesn't. Oops? > > Wouldn't it be safer to just prevent inlining, then the callee retains > the stack protector, regardless of caller stack protection? > > > exactly the effect one would get without the inlining (as in that case > > the callee would check it), but matches the general behavior that we allow > > inlining functions with -fstack-protector* at all (and only check it in the > > prologue/epilogue, not somewhere in the middle). > > > > Jakub > > > > > -- > Thanks, > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers
Re: [PATCH] Implement no_stack_protect attribute.
Thanks for the quick feedback! On Wed, Oct 21, 2020 at 2:13 PM Jakub Jelinek wrote: > > On Wed, Oct 21, 2020 at 02:04:15PM -0700, Nick Desaulniers via Gcc-patches > wrote: > > Tangentially related question: > > We're running into a bug related to LTO for the kernel when code > > compiled with -fno-stack-protector is called from and inlined into > > code that is compiled with -fstack-protector. Specifically, stack > > canaries get checked before they're restored post suspend/resume which > > leads to spooky bugs. > > > > Once we have more fine grain function level attribute to explicitly > > disable stack protectors on a per function basis, I'm considering > > making this function attribute a barrier to inlining in LLVM so that > > callers with stack protectors don't inline callees that explicitly > > should not have a stack protector and vice versa (more concretely, > > when they don't match). I think this would maximize which functions > > are still covered by stack protectors, and be the most straightforward > > to implement. > > That doesn't make sense to me. > Stack protector doesn't affect in any way inlined code, the stack protection > is always solely in the prologue and epilogue of out of line functions. > So, if the (non-inlined) caller is -fstack-protector and inlined callee > is -fno-stack-protector, there should be ssp store in the prologue of the > resulting function and test in the epilogue. That is the case today, and I'm arguing that leads to bugs in the Linux kernel when built with LTO. > The effect will be exactly > like that if the function wouldn't be inlined. I don't follow. If the -fno-stack-protector callee was not inlined, the caller would have a stack protector, while the callee would not. I think today there's not a strong enough distinction between the level of stack protection being specified vs explicit annotations/flags that stack protectors MUST NOT be inserted. > Similarly, if the non-inlined caller is -fno-stack-protector and inlined > callee is -fstack-protector, there will be no stack protection. This isn't And I'd argue that now we may have stripped off stack protection in the pursuit of inlining. Consider for example the case where that stack protected callee contained a large alloca; post inlining into a -fno-stack-protected caller suddenly now it doesn't. Oops? Wouldn't it be safer to just prevent inlining, then the callee retains the stack protector, regardless of caller stack protection? > exactly the effect one would get without the inlining (as in that case > the callee would check it), but matches the general behavior that we allow > inlining functions with -fstack-protector* at all (and only check it in the > prologue/epilogue, not somewhere in the middle). > > Jakub > -- Thanks, ~Nick Desaulniers
Re: [PATCH] Implement no_stack_protect attribute.
On Wed, Oct 21, 2020 at 02:04:15PM -0700, Nick Desaulniers via Gcc-patches wrote: > Tangentially related question: > We're running into a bug related to LTO for the kernel when code > compiled with -fno-stack-protector is called from and inlined into > code that is compiled with -fstack-protector. Specifically, stack > canaries get checked before they're restored post suspend/resume which > leads to spooky bugs. > > Once we have more fine grain function level attribute to explicitly > disable stack protectors on a per function basis, I'm considering > making this function attribute a barrier to inlining in LLVM so that > callers with stack protectors don't inline callees that explicitly > should not have a stack protector and vice versa (more concretely, > when they don't match). I think this would maximize which functions > are still covered by stack protectors, and be the most straightforward > to implement. That doesn't make sense to me. Stack protector doesn't affect in any way inlined code, the stack protection is always solely in the prologue and epilogue of out of line functions. So, if the (non-inlined) caller is -fstack-protector and inlined callee is -fno-stack-protector, there should be ssp store in the prologue of the resulting function and test in the epilogue. The effect will be exactly like that if the function wouldn't be inlined. Similarly, if the non-inlined caller is -fno-stack-protector and inlined callee is -fstack-protector, there will be no stack protection. This isn't exactly the effect one would get without the inlining (as in that case the callee would check it), but matches the general behavior that we allow inlining functions with -fstack-protector* at all (and only check it in the prologue/epilogue, not somewhere in the middle). Jakub
Re: [PATCH] Implement no_stack_protect attribute.
On Tue, Oct 20, 2020 at 5:19 AM Richard Biener wrote: > > On Tue, Oct 20, 2020 at 1:24 PM Martin Liška wrote: > > > > PING^5 > > So can we use the same identifier as clang here as Nick > requests? Thus, OK with re-naming everything alongside > no_stack_protector. It isn't really the opposite of the > stack_protect attribute since that only protects when > -fstack-protector-explicit is enabled. I'll be happy to help test and review with an updated/rebased patch. Tangentially related question: We're running into a bug related to LTO for the kernel when code compiled with -fno-stack-protector is called from and inlined into code that is compiled with -fstack-protector. Specifically, stack canaries get checked before they're restored post suspend/resume which leads to spooky bugs. Once we have more fine grain function level attribute to explicitly disable stack protectors on a per function basis, I'm considering making this function attribute a barrier to inlining in LLVM so that callers with stack protectors don't inline callees that explicitly should not have a stack protector and vice versa (more concretely, when they don't match). I think this would maximize which functions are still covered by stack protectors, and be the most straightforward to implement. The next question then is what happens when the callee is marked __attribute__((always_inline))? My answer for LLVM currently is "still disallow inline substitution" which is more surprising than I'd like, but we already have precedent for "always inline" not meaning "always." Warning from the frontend when mixing no_stack_protector and always_inline is possible if the callers are visible and don't match, but I don't think that works for cross translation unit calls. I guess I was curious if others have ideas for solutions to this particular problem? Otherwise I plan to implement the above logic in LLVM. We'd eventually need matching logic in GCC to support LTO kernels not having the same bug. https://reviews.llvm.org/D87956 -- Thanks, ~Nick Desaulniers
Re: [PATCH] Implement no_stack_protect attribute.
On Tue, Oct 20, 2020 at 1:24 PM Martin Liška wrote: > > PING^5 So can we use the same identifier as clang here as Nick requests? Thus, OK with re-naming everything alongside no_stack_protector. It isn't really the opposite of the stack_protect attribute since that only protects when -fstack-protector-explicit is enabled. Thanks, Richard. > On 8/17/20 2:35 PM, Martin Liška wrote: > > PING^4 > > > > On 7/23/20 1:10 PM, Martin Liška wrote: > >> PING^3 > >> > >> On 6/24/20 11:09 AM, Martin Liška wrote: > >>> PING^2 > >>> > >>> On 6/10/20 10:12 AM, Martin Liška wrote: > PING^1 > > On 5/25/20 3:10 PM, Martin Liška wrote: > > On 5/21/20 4:53 PM, Martin Sebor wrote: > >> On 5/21/20 5:28 AM, Martin Liška wrote: > >>> On 5/18/20 10:37 PM, Martin Sebor wrote: > I know there are some somewhat complex cases the attribute exclusion > mechanism isn't general enough to handle but this seems simple enough > that it should work. Unless I'm missing something that makes it not > feasible I would suggest to use it. > >>> > >>> Hi Martin. > >>> > >>> Do we have a better place where we check for attribute collision? > >> > >> If by collision you mean the same thing as the mutual exclusion I was > >> talking about then that's done by creating an > >> attribute_spec::exclusions > >> array like for instance attr_cold_hot_exclusions in c-attribs.c and > >> pointing to it from the attribute_spec entries for each of > >> the mutually exclusive attributes in the attribute table. Everything > >> else is handled automatically by decl_attributes. > >> > >> Martin > > > > Thanks, I'm sending updated version of the patch that utilizes the > > conflict > > detection. > > > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > > > Ready to be installed? > > Thanks, > > Martin > > >>> > >> > > >
Re: [PATCH] Implement no_stack_protect attribute.
PING^5 On 8/17/20 2:35 PM, Martin Liška wrote: PING^4 On 7/23/20 1:10 PM, Martin Liška wrote: PING^3 On 6/24/20 11:09 AM, Martin Liška wrote: PING^2 On 6/10/20 10:12 AM, Martin Liška wrote: PING^1 On 5/25/20 3:10 PM, Martin Liška wrote: On 5/21/20 4:53 PM, Martin Sebor wrote: On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin Thanks, I'm sending updated version of the patch that utilizes the conflict detection. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
Re: [PATCH] Implement no_stack_protect attribute.
On 8/25/20 4:46 PM, Nick Desaulniers wrote: This would solve a common pattern in the kernel where folks are using `extern inline` with `gnu_inline` semantics or worse (empty `asm("");` statements) in certain places where it would be much more preferable to have this attribute. Thank you very much Martin for writing it. is direct equivalent of Clang's no_stack_protector. Unlike Clang, I chose to name it no_stack_protect because we already have stack_protect attribute (used with -fstack-protector-explicit). That's pretty easy for us to work around the differences in the kernel, but one final plea for the users; it would simplify users' codebases not to have to shim this for differences between compilers. If I had a dollar for every time I had to implement something in LLVM where a different identifier or flag would be more consistent with another part of the codebase...I'm sure there's many examples of this on LLVM's side too, but I would prefer to stop the proliferation of subtle differences like this that harm toolchain portability when possible and when we can proactively address. All right, I'm open to unify the flag name to what LLVM has ;) Martin
Re: [PATCH] Implement no_stack_protect attribute.
This would solve a common pattern in the kernel where folks are using `extern inline` with `gnu_inline` semantics or worse (empty `asm("");` statements) in certain places where it would be much more preferable to have this attribute. Thank you very much Martin for writing it. > is direct equivalent of Clang's no_stack_protector. > Unlike Clang, I chose to name it no_stack_protect because we already > have stack_protect attribute (used with -fstack-protector-explicit). That's pretty easy for us to work around the differences in the kernel, but one final plea for the users; it would simplify users' codebases not to have to shim this for differences between compilers. If I had a dollar for every time I had to implement something in LLVM where a different identifier or flag would be more consistent with another part of the codebase...I'm sure there's many examples of this on LLVM's side too, but I would prefer to stop the proliferation of subtle differences like this that harm toolchain portability when possible and when we can proactively address. -- Thanks, ~Nick Desaulniers
Re: [PATCH] Implement no_stack_protect attribute.
PING^4 On 7/23/20 1:10 PM, Martin Liška wrote: PING^3 On 6/24/20 11:09 AM, Martin Liška wrote: PING^2 On 6/10/20 10:12 AM, Martin Liška wrote: PING^1 On 5/25/20 3:10 PM, Martin Liška wrote: On 5/21/20 4:53 PM, Martin Sebor wrote: On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin Thanks, I'm sending updated version of the patch that utilizes the conflict detection. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
Re: [PATCH] Implement no_stack_protect attribute.
PING^3 On 6/24/20 11:09 AM, Martin Liška wrote: PING^2 On 6/10/20 10:12 AM, Martin Liška wrote: PING^1 On 5/25/20 3:10 PM, Martin Liška wrote: On 5/21/20 4:53 PM, Martin Sebor wrote: On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin Thanks, I'm sending updated version of the patch that utilizes the conflict detection. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
Re: [PATCH] Implement no_stack_protect attribute.
PING^2 On 6/10/20 10:12 AM, Martin Liška wrote: PING^1 On 5/25/20 3:10 PM, Martin Liška wrote: On 5/21/20 4:53 PM, Martin Sebor wrote: On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin Thanks, I'm sending updated version of the patch that utilizes the conflict detection. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
Re: [PATCH] Implement no_stack_protect attribute.
On 6/10/20 2:12 AM, Martin Liška wrote: PING^1 The exclusion changes are just what I was suggesting, thanks. Martin On 5/25/20 3:10 PM, Martin Liška wrote: On 5/21/20 4:53 PM, Martin Sebor wrote: On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin Thanks, I'm sending updated version of the patch that utilizes the conflict detection. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
Re: [PATCH] Implement no_stack_protect attribute.
PING^1 On 5/25/20 3:10 PM, Martin Liška wrote: On 5/21/20 4:53 PM, Martin Sebor wrote: On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin Thanks, I'm sending updated version of the patch that utilizes the conflict detection. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
Re: [PATCH] Implement no_stack_protect attribute.
On 5/21/20 4:53 PM, Martin Sebor wrote: On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin Thanks, I'm sending updated version of the patch that utilizes the conflict detection. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin >From b026a8684a0a82394ce2faf010257fa3f0978ca0 Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Fri, 15 May 2020 14:42:12 +0200 Subject: [PATCH] Implement no_stack_protect attribute. gcc/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * cfgexpand.c (stack_protect_decl_phase): Guard with lookup_attribute("no_stack_protect") at various places. (expand_used_vars): Likewise here. * doc/extend.texi: Document no_stack_protect attribute. gcc/ada/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * gcc-interface/utils.c (handle_no_stack_protect_attribute): New. (handle_stack_protect_attribute): Add error message for a no_stack_protect function. gcc/c-family/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * c-attribs.c (handle_no_stack_protect_function_attribute): New. (handle_stack_protect_attribute): Add error message for a no_stack_protect function. gcc/testsuite/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * g++.dg/no-stack-protect-attr-2.C: New test. * g++.dg/no-stack-protect-attr-3.C: New test. * g++.dg/no-stack-protect-attr.C: New test. --- gcc/ada/gcc-interface/utils.c | 31 ++- gcc/c-family/c-attribs.c | 32 +++- gcc/cfgexpand.c | 82 ++- gcc/doc/extend.texi | 4 + .../g++.dg/no-stack-protect-attr-2.C | 7 ++ .../g++.dg/no-stack-protect-attr-3.C | 23 ++ gcc/testsuite/g++.dg/no-stack-protect-attr.C | 16 7 files changed, 154 insertions(+), 41 deletions(-) create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-2.C create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-3.C create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr.C diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c index fb08b6c90ed..b6a60eb1b11 100644 --- a/gcc/ada/gcc-interface/utils.c +++ b/gcc/ada/gcc-interface/utils.c @@ -91,6 +91,7 @@ static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *); static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *); static tree handle_noreturn_attribute (tree *, tree, tree, int, bool *); static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *); +static tree handle_no_stack_protect_attribute (tree *, tree, tree, int, bool *); static tree handle_noinline_attribute (tree *, tree, tree, int, bool *); static tree handle_noclone_attribute (tree *, tree, tree, int, bool *); static tree handle_noicf_attribute (tree *, tree, tree, int, bool *); @@ -115,6 +116,13 @@ static const struct attribute_spec::exclusions attr_cold_hot_exclusions[] = { NULL , false, false, false } }; +static const struct attribute_spec::exclusions attr_stack_protect_exclusions[] = +{ + { "stack_protect", true, false, false }, + { "no_stack_protect", true, false, false }, + { NULL, false, false, false }, +}; + /* Fake handler for attributes we don't properly support, typically because they'd require dragging a lot of the common-c front-end circuitry. */ static tree fake_attribute_handler (tree *, tree, tree, int, bool *); @@ -140,7 +148,11 @@ const struct attribute_spec gnat_internal_attribute_table[] = { "noreturn", 0, 0, true, false, false, false, handle_noreturn_attribute, NULL }, { "stack_protect",0, 0, true, false, false, false, -handle_stack_protect_attribute, NULL }, +handle_stack_protect_attribute, +attr_stack_protect_exclusions }, + { "no_stack_protect",0, 0, true, false, false, false, +handle_no_stack_protect_attribute, +attr_stack_protect_exclusions }, { "noinline", 0, 0, true, false, false, false, handle_noinline_attribute, NULL }, { "noclone", 0, 0, true, false, false, false, @@ -6524,6 +6536,23 @@ handle_stack_protect_attribute (tree *node, tree name, tree, int, return NULL_TREE; } +/* Handle a "no_stack_protect" attribute;
Re: [PATCH] Implement no_stack_protect attribute.
On Mon, May 25, 2020 at 11:27 AM Martin Liška wrote: > > On 5/22/20 12:51 PM, Richard Biener wrote: > > On Thu, May 21, 2020 at 12:09 PM Martin Liška wrote: > >> > >> On 5/18/20 1:49 PM, Richard Biener wrote: > >>> On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches > >>> wrote: > > On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote: > >> The optimize attribute is used to specify that a function is to be > >> compiled > >> with different optimization options than specified on the command line. > >> ``` > >> > >> It's pretty clear about the command line arguments (that are ignored). > > > > That is not clear at all. The difference is primarily in what the > > option > > string says in there. > > And if we decide we want to keep existing behavior (haven't checked if we > actually behave that way yet), we should add some syntax that will allow > ammending command line options rather than replacing it. > >> > >> Hello. > >> > >> Back to this, I must say that option handling is a complex thing in the > >> GCC. > >> > >>> > >>> We do behave this way - while we're running against the current > >>> gcc_options we call decode_options which first does > >>> default_options_optimization. So it behaves inconsistently with > >>> respect to options not explicitly listed in default_options_table. > >> > >> Yes, we basically build on top of the currently selection flags. > >> We use default_options_table because any optimization level selection > >> in an optimization attribute should efficiently enforce call of > >> default_options_table. > >> > >> What about using them only in case one changes optimization level (patch)? > > > > I'm not sure if that works, no -On means -O0, so one might interpret > > optimize("omit-frame-pointer") as -O0 -fomit-frame-pointer. Also -On > > are not treated in position dependent way, thus -fno-tree-pre -O2 is > > the same as -O2 -fno-tree-pre rather than re-enabling PRE over > > -fno-tree-pre. > > So your patch would turn a -O2 -fno-tree-pre invocation, when > > optimize("O3") is encountered, to one with PRE enabled, not matching > > behavior of -O2 -fno-tree-pre -O3. > > > > I think the only sensible way is to save the original decoded options > > from the gcc invocation (and have #pragma optimize push/pop append > > accordingly) and append the current attribute/pragmas optimization > > string to them and run that whole thing on a default option state. > > That should work. Anyway, that's a task with unclear finish. > Would it be possible to add the no_stack_protect attribute for the time being? I think it's a reasonable request, yes. Richard. > The limitation is there for ages and we have another "optimize" attributes > that can be theoretically replaced with proper optimize handling. > > > > > That makes semantics equivalent to appending more options to the > > command-line. Well, hopefully. Interaction with the target attribute > > might be interesting (that likely also needs to append to that > > "current decoded options" set). > > I fear from the interaction of optimization and target attribute. > > > > > As you say, option handling is difficult. > > Anyway, I'm planning to work on the options in stage1. It's quite close > relative to PR92860. > > Martin > > > > > Richard. > > > >> Martin > >> > >>> > >>> But I also think doing the whole processing as in decode_options > >>> is the only thing that's really tested. And a fix would be to not > >>> start from the current set of options but from a clean slate... > >>> > >>> The code clearly thinks we're doing incremental changes: > >>> > >>> /* Save current options. */ > >>> cl_optimization_save (_opts, _options); > >>> > >>> /* If we previously had some optimization options, use them as the > >>>default. */ > >>> if (old_opts) > >>> cl_optimization_restore (_options, > >>>TREE_OPTIMIZATION (old_opts)); > >>> > >>> /* Parse options, and update the vector. */ > >>> parse_optimize_options (args, true); > >>> DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) > >>> = build_optimization_node (_options); > >>> > >>> /* Restore current options. */ > >>> cl_optimization_restore (_options, _opts); > >>> > >>> so for example you should see -fipa-pta preserved from the > >>> command-line while -fno-tree-pta would be overridden even > >>> if not mentioned explicitely in the optimize attribute. > >>> > >>> IMHO the current situation is far from useful... > >>> > >>> Richard. > >>> > Could be say start the optimize attribute string with + or something > similar. > Does target attribute behave that way too? > > Jakub > > >> >
Re: [PATCH] Implement no_stack_protect attribute.
On 5/22/20 12:51 PM, Richard Biener wrote: On Thu, May 21, 2020 at 12:09 PM Martin Liška wrote: On 5/18/20 1:49 PM, Richard Biener wrote: On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches wrote: On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote: The optimize attribute is used to specify that a function is to be compiled with different optimization options than specified on the command line. ``` It's pretty clear about the command line arguments (that are ignored). That is not clear at all. The difference is primarily in what the option string says in there. And if we decide we want to keep existing behavior (haven't checked if we actually behave that way yet), we should add some syntax that will allow ammending command line options rather than replacing it. Hello. Back to this, I must say that option handling is a complex thing in the GCC. We do behave this way - while we're running against the current gcc_options we call decode_options which first does default_options_optimization. So it behaves inconsistently with respect to options not explicitly listed in default_options_table. Yes, we basically build on top of the currently selection flags. We use default_options_table because any optimization level selection in an optimization attribute should efficiently enforce call of default_options_table. What about using them only in case one changes optimization level (patch)? I'm not sure if that works, no -On means -O0, so one might interpret optimize("omit-frame-pointer") as -O0 -fomit-frame-pointer. Also -On are not treated in position dependent way, thus -fno-tree-pre -O2 is the same as -O2 -fno-tree-pre rather than re-enabling PRE over -fno-tree-pre. So your patch would turn a -O2 -fno-tree-pre invocation, when optimize("O3") is encountered, to one with PRE enabled, not matching behavior of -O2 -fno-tree-pre -O3. I think the only sensible way is to save the original decoded options from the gcc invocation (and have #pragma optimize push/pop append accordingly) and append the current attribute/pragmas optimization string to them and run that whole thing on a default option state. That should work. Anyway, that's a task with unclear finish. Would it be possible to add the no_stack_protect attribute for the time being? The limitation is there for ages and we have another "optimize" attributes that can be theoretically replaced with proper optimize handling. That makes semantics equivalent to appending more options to the command-line. Well, hopefully. Interaction with the target attribute might be interesting (that likely also needs to append to that "current decoded options" set). I fear from the interaction of optimization and target attribute. As you say, option handling is difficult. Anyway, I'm planning to work on the options in stage1. It's quite close relative to PR92860. Martin Richard. Martin But I also think doing the whole processing as in decode_options is the only thing that's really tested. And a fix would be to not start from the current set of options but from a clean slate... The code clearly thinks we're doing incremental changes: /* Save current options. */ cl_optimization_save (_opts, _options); /* If we previously had some optimization options, use them as the default. */ if (old_opts) cl_optimization_restore (_options, TREE_OPTIMIZATION (old_opts)); /* Parse options, and update the vector. */ parse_optimize_options (args, true); DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (_options); /* Restore current options. */ cl_optimization_restore (_options, _opts); so for example you should see -fipa-pta preserved from the command-line while -fno-tree-pta would be overridden even if not mentioned explicitely in the optimize attribute. IMHO the current situation is far from useful... Richard. Could be say start the optimize attribute string with + or something similar. Does target attribute behave that way too? Jakub
Re: [PATCH] Implement no_stack_protect attribute.
On Thu, May 21, 2020 at 12:09 PM Martin Liška wrote: > > On 5/18/20 1:49 PM, Richard Biener wrote: > > On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches > > wrote: > >> > >> On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote: > The optimize attribute is used to specify that a function is to be > compiled > with different optimization options than specified on the command line. > ``` > > It's pretty clear about the command line arguments (that are ignored). > >>> > >>> That is not clear at all. The difference is primarily in what the option > >>> string says in there. > >> > >> And if we decide we want to keep existing behavior (haven't checked if we > >> actually behave that way yet), we should add some syntax that will allow > >> ammending command line options rather than replacing it. > > Hello. > > Back to this, I must say that option handling is a complex thing in the GCC. > > > > > We do behave this way - while we're running against the current > > gcc_options we call decode_options which first does > > default_options_optimization. So it behaves inconsistently with > > respect to options not explicitly listed in default_options_table. > > Yes, we basically build on top of the currently selection flags. > We use default_options_table because any optimization level selection > in an optimization attribute should efficiently enforce call of > default_options_table. > > What about using them only in case one changes optimization level (patch)? I'm not sure if that works, no -On means -O0, so one might interpret optimize("omit-frame-pointer") as -O0 -fomit-frame-pointer. Also -On are not treated in position dependent way, thus -fno-tree-pre -O2 is the same as -O2 -fno-tree-pre rather than re-enabling PRE over -fno-tree-pre. So your patch would turn a -O2 -fno-tree-pre invocation, when optimize("O3") is encountered, to one with PRE enabled, not matching behavior of -O2 -fno-tree-pre -O3. I think the only sensible way is to save the original decoded options from the gcc invocation (and have #pragma optimize push/pop append accordingly) and append the current attribute/pragmas optimization string to them and run that whole thing on a default option state. That makes semantics equivalent to appending more options to the command-line. Well, hopefully. Interaction with the target attribute might be interesting (that likely also needs to append to that "current decoded options" set). As you say, option handling is difficult. Richard. > Martin > > > > > But I also think doing the whole processing as in decode_options > > is the only thing that's really tested. And a fix would be to not > > start from the current set of options but from a clean slate... > > > > The code clearly thinks we're doing incremental changes: > > > >/* Save current options. */ > >cl_optimization_save (_opts, _options); > > > >/* If we previously had some optimization options, use them as the > > default. */ > >if (old_opts) > > cl_optimization_restore (_options, > > TREE_OPTIMIZATION (old_opts)); > > > >/* Parse options, and update the vector. */ > >parse_optimize_options (args, true); > >DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) > > = build_optimization_node (_options); > > > >/* Restore current options. */ > >cl_optimization_restore (_options, _opts); > > > > so for example you should see -fipa-pta preserved from the > > command-line while -fno-tree-pta would be overridden even > > if not mentioned explicitely in the optimize attribute. > > > > IMHO the current situation is far from useful... > > > > Richard. > > > >> Could be say start the optimize attribute string with + or something > >> similar. > >> Does target attribute behave that way too? > >> > >> Jakub > >> >
Re: [PATCH] Implement no_stack_protect attribute.
On 5/21/20 5:28 AM, Martin Liška wrote: On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? If by collision you mean the same thing as the mutual exclusion I was talking about then that's done by creating an attribute_spec::exclusions array like for instance attr_cold_hot_exclusions in c-attribs.c and pointing to it from the attribute_spec entries for each of the mutually exclusive attributes in the attribute table. Everything else is handled automatically by decl_attributes. Martin
Re: [PATCH] Implement no_stack_protect attribute.
On 5/18/20 10:37 PM, Martin Sebor wrote: I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. Hi Martin. Do we have a better place where we check for attribute collision? Martin
Re: [PATCH] Implement no_stack_protect attribute.
On 5/18/20 5:56 PM, Florian Weimer via Gcc-patches wrote: * Michael Matz: So does -fcf-protection. -fno-omit-frame-pointer does not work for me at all for some reason, the frame pointer is always missing? Not for me: I see. I didn't know that -fno-omit-frame-pointer only applies to non-leaf functions. For them, the behavior I see matches yours. Thanks, Florian I bet it's related to this issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92860#c4. The issue is about update of target options from optimization option and there's proper restore to the original state. Martin
Re: [PATCH] Implement no_stack_protect attribute.
On 5/18/20 1:49 PM, Richard Biener wrote: On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches wrote: On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote: The optimize attribute is used to specify that a function is to be compiled with different optimization options than specified on the command line. ``` It's pretty clear about the command line arguments (that are ignored). That is not clear at all. The difference is primarily in what the option string says in there. And if we decide we want to keep existing behavior (haven't checked if we actually behave that way yet), we should add some syntax that will allow ammending command line options rather than replacing it. Hello. Back to this, I must say that option handling is a complex thing in the GCC. We do behave this way - while we're running against the current gcc_options we call decode_options which first does default_options_optimization. So it behaves inconsistently with respect to options not explicitly listed in default_options_table. Yes, we basically build on top of the currently selection flags. We use default_options_table because any optimization level selection in an optimization attribute should efficiently enforce call of default_options_table. What about using them only in case one changes optimization level (patch)? Martin But I also think doing the whole processing as in decode_options is the only thing that's really tested. And a fix would be to not start from the current set of options but from a clean slate... The code clearly thinks we're doing incremental changes: /* Save current options. */ cl_optimization_save (_opts, _options); /* If we previously had some optimization options, use them as the default. */ if (old_opts) cl_optimization_restore (_options, TREE_OPTIMIZATION (old_opts)); /* Parse options, and update the vector. */ parse_optimize_options (args, true); DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (_options); /* Restore current options. */ cl_optimization_restore (_options, _opts); so for example you should see -fipa-pta preserved from the command-line while -fno-tree-pta would be overridden even if not mentioned explicitely in the optimize attribute. IMHO the current situation is far from useful... Richard. Could be say start the optimize attribute string with + or something similar. Does target attribute behave that way too? Jakub diff --git a/gcc/opts.c b/gcc/opts.c index ec3ca0720f9..8c39562c697 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -571,6 +571,7 @@ default_options_optimization (struct gcc_options *opts, unsigned int i; int opt2; bool openacc_mode = false; + bool opt_level_changed = false; /* Scan to see what optimization level has been specified. That will determine the default value of many flags. */ @@ -603,6 +604,7 @@ default_options_optimization (struct gcc_options *opts, opts->x_optimize_debug = 0; } } + opt_level_changed = true; break; case OPT_Os: @@ -612,6 +614,7 @@ default_options_optimization (struct gcc_options *opts, opts->x_optimize = 2; opts->x_optimize_fast = 0; opts->x_optimize_debug = 0; + opt_level_changed = true; break; case OPT_Ofast: @@ -620,6 +623,7 @@ default_options_optimization (struct gcc_options *opts, opts->x_optimize = 3; opts->x_optimize_fast = 1; opts->x_optimize_debug = 0; + opt_level_changed = true; break; case OPT_Og: @@ -628,6 +632,7 @@ default_options_optimization (struct gcc_options *opts, opts->x_optimize = 1; opts->x_optimize_fast = 0; opts->x_optimize_debug = 1; + opt_level_changed = true; break; case OPT_fopenacc: @@ -641,10 +646,11 @@ default_options_optimization (struct gcc_options *opts, } } - maybe_default_options (opts, opts_set, default_options_table, - opts->x_optimize, opts->x_optimize_size, - opts->x_optimize_fast, opts->x_optimize_debug, - lang_mask, handlers, loc, dc); + if (opt_level_changed) +maybe_default_options (opts, opts_set, default_options_table, + opts->x_optimize, opts->x_optimize_size, + opts->x_optimize_fast, opts->x_optimize_debug, + lang_mask, handlers, loc, dc); /* -O2 param settings. */ opt2 = (opts->x_optimize >= 2);
Re: [PATCH] Implement no_stack_protect attribute.
On 5/18/20 4:37 AM, Martin Liška wrote: Hi. The patch adds new no_stack_protect attribute. The change is requested from kernel folks and is direct equivalent of Clang's no_stack_protector. Unlike Clang, I chose to name it no_stack_protect because we already have stack_protect attribute (used with -fstack-protector-explicit). First part of the patch contains a small refactoring of an enum, second implements the functionality. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin 0002-Implement-no_stack_protect-attribute.patch From 6b3e9d32150fe17acb271b7bedee7dc95cad7dc8 Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Fri, 15 May 2020 14:42:12 +0200 Subject: [PATCH 2/2] Implement no_stack_protect attribute. gcc/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * cfgexpand.c (stack_protect_decl_phase): Guard with lookup_attribute("no_stack_protect") at various places. (expand_used_vars): Likewise here. * doc/extend.texi: Document no_stack_protect attribute. gcc/ada/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * gcc-interface/utils.c (handle_no_stack_protect_attribute): New. (handle_stack_protect_attribute): Add error message for a no_stack_protect function. gcc/c-family/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * c-attribs.c (handle_no_stack_protect_function_attribute): New. (handle_stack_protect_attribute): Add error message for a no_stack_protect function. gcc/testsuite/ChangeLog: 2020-05-18 Martin Liska PR c/94722 * g++.dg/no-stack-protect-attr-2.C: New test. * g++.dg/no-stack-protect-attr-3.C: New test. * g++.dg/no-stack-protect-attr.C: New test. --- gcc/ada/gcc-interface/utils.c | 32 gcc/c-family/c-attribs.c | 33 gcc/cfgexpand.c | 82 ++- gcc/doc/extend.texi | 4 + .../g++.dg/no-stack-protect-attr-2.C | 7 ++ .../g++.dg/no-stack-protect-attr-3.C | 23 ++ gcc/testsuite/g++.dg/no-stack-protect-attr.C | 16 7 files changed, 158 insertions(+), 39 deletions(-) create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-2.C create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr-3.C create mode 100644 gcc/testsuite/g++.dg/no-stack-protect-attr.C diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c index 1527be4f6d1..144afe37d78 100644 --- a/gcc/ada/gcc-interface/utils.c +++ b/gcc/ada/gcc-interface/utils.c @@ -91,6 +91,7 @@ static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *); static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *); static tree handle_noreturn_attribute (tree *, tree, tree, int, bool *); static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *); +static tree handle_no_stack_protect_attribute (tree *, tree, tree, int, bool *); static tree handle_noinline_attribute (tree *, tree, tree, int, bool *); static tree handle_noclone_attribute (tree *, tree, tree, int, bool *); static tree handle_noicf_attribute (tree *, tree, tree, int, bool *); @@ -141,6 +142,8 @@ const struct attribute_spec gnat_internal_attribute_table[] = handle_noreturn_attribute, NULL }, { "stack_protect",0, 0, true, false, false, false, handle_stack_protect_attribute, NULL }, + { "no_stack_protect",0, 0, true, false, false, false, +handle_no_stack_protect_attribute, NULL }, { "noinline", 0, 0, true, false, false, false, handle_noinline_attribute, NULL }, { "noclone", 0, 0, true, false, false, false, @@ -6523,10 +6526,39 @@ handle_stack_protect_attribute (tree *node, tree name, tree, int, warning (OPT_Wattributes, "%qE attribute ignored", name); *no_add_attrs = true; } + else if (lookup_attribute ("no_stack_protect", DECL_ATTRIBUTES (*node))) +{ + warning (OPT_Wattributes, "%qE attribute ignored due to conflict " + "with %qs attribute", name, "no_stack_protect"); + *no_add_attrs = true; I know there are some somewhat complex cases the attribute exclusion mechanism isn't general enough to handle but this seems simple enough that it should work. Unless I'm missing something that makes it not feasible I would suggest to use it. diff --git a/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C new file mode 100644 index 000..dcac6f9d389 --- /dev/null +++ b/gcc/testsuite/g++.dg/no-stack-protect-attr-2.C @@ -0,0 +1,7 @@ +/* PR c/94722 */ +/* { dg-do compile } */ + +int __attribute__((no_stack_protect, stack_protect)) c() /* { dg-warning "'stack_protect' attribute ignored due to conflict with 'no_stack_protect' attribute" } */ The more likely misuse than applying incompatible
Re: [PATCH] Implement no_stack_protect attribute.
On Mai 18 2020, Florian Weimer via Gcc-patches wrote: > * Michael Matz: > >>> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at >>> all for some reason, the frame pointer is always missing? >> >> Not for me: > > I see. I didn't know that -fno-omit-frame-pointer only applies to > non-leaf functions. For them, the behavior I see matches yours. There is also -momit-leaf-frame-pointer. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [PATCH] Implement no_stack_protect attribute.
Hello, On Mon, 18 May 2020, Florian Weimer wrote: > * Michael Matz: > > >> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at > >> all for some reason, the frame pointer is always missing? > > > > Not for me: > > I see. I didn't know that -fno-omit-frame-pointer only applies to > non-leaf functions. Yeah, I actually consider this a bug as well (unrelated though). The user of that flag quite surely has good reasons to want to have a frame pointer, and those good reasons usually extend to all functions, not just to leaf ones. (E.g. ensuring that backtraces from async signal handlers are meaningful, for instance for poor mans profiling). Ciao, Michael.
Re: [PATCH] Implement no_stack_protect attribute.
* Michael Matz: >> So does -fcf-protection. -fno-omit-frame-pointer does not work for me at >> all for some reason, the frame pointer is always missing? > > Not for me: I see. I didn't know that -fno-omit-frame-pointer only applies to non-leaf functions. For them, the behavior I see matches yours. Thanks, Florian
Re: [PATCH] Implement no_stack_protect attribute.
Hello, On Mon, 18 May 2020, Florian Weimer wrote: > >> In glibc, we already have this: > >> > >> /* Used to disable stack protection in sensitive places, like ifunc > >>resolvers and early static TLS init. */ > >> #ifdef HAVE_CC_NO_STACK_PROTECTOR > >> # define inhibit_stack_protector \ > >> __attribute__ ((__optimize__ ("-fno-stack-protector"))) > >> #else > >> # define inhibit_stack_protector > >> #endif > >> > >> Is it broken? > > > > Depends on what your expectations are. It completely overrides all > > options given on the command line (including things like > > fno-omit-frame-pointer and -O2!). At least I was very surprised by that > > even though the current docu can be read that way; if you're similarly > > surprised, then yes, the above is broken, it does not only disable stack > > protection (but also e.g. all optimizations, despite the attributes name > > :-) ). > > Yes, that would qualify as broken. > > This is not what I observe with gcc-9.3.1-2.fc31.x86_64 from Fedora. > -O2 still has an effect. Indeed, I definitely remember an interaction with the attribute and -O{,2} (or something that I interpreted as such) but it obviously isn't as simple as plain disabling it, and right now I can't recreate the situation :-/ (It might be disabling of some further cmdline flags that I conflated in my brain with "effect of -O2") > So does -fcf-protection. -fno-omit-frame-pointer does not work for me at > all for some reason, the frame pointer is always missing? Not for me: % cat simple.c extern int bla(int *); int #ifdef ATTR __attribute__((__optimize__ ("-fno-stack-protector"))) #endif foo(int a, int b) { int c = b; return a * 42 + bla(); } % gcc-9 -fno-omit-frame-pointer -O -S -o - tryme.c | grep bp pushq %rbp movq%rsp, %rbp movl%esi, -20(%rbp) leaq-20(%rbp), %rdi popq%rbp % gcc-9 -fstack-protector-all -fno-omit-frame-pointer -O -S -o - tryme.c | grep bp pushq %rbp movq%rsp, %rbp movq%rax, -24(%rbp) movl%esi, -28(%rbp) leaq-28(%rbp), %rdi movq-24(%rbp), %rdx popq%rbp But using the attr: % gcc-9 -DATTR -fstack-protector-all -fno-omit-frame-pointer -O -S -o - tryme.c | grep bp % (gcc9 is gcc9-9.2.1+r275327-1.1.x86_64 on opensuse) Ciao, Michael.
Re: [PATCH] Implement no_stack_protect attribute.
* Michael Matz: > Hello, > > On Mon, 18 May 2020, Florian Weimer via Gcc-patches wrote: > >> > The patch adds new no_stack_protect attribute. The change is requested >> > from kernel folks and is direct equivalent of Clang's no_stack_protector. >> > Unlike Clang, I chose to name it no_stack_protect because we already >> > have stack_protect attribute (used with -fstack-protector-explicit). >> > >> > First part of the patch contains a small refactoring of an enum, second >> > implements the functionality. >> >> In glibc, we already have this: >> >> /* Used to disable stack protection in sensitive places, like ifunc >>resolvers and early static TLS init. */ >> #ifdef HAVE_CC_NO_STACK_PROTECTOR >> # define inhibit_stack_protector \ >> __attribute__ ((__optimize__ ("-fno-stack-protector"))) >> #else >> # define inhibit_stack_protector >> #endif >> >> Is it broken? > > Depends on what your expectations are. It completely overrides all > options given on the command line (including things like > fno-omit-frame-pointer and -O2!). At least I was very surprised by that > even though the current docu can be read that way; if you're similarly > surprised, then yes, the above is broken, it does not only disable stack > protection (but also e.g. all optimizations, despite the attributes name > :-) ). Yes, that would qualify as broken. This is not what I observe with gcc-9.3.1-2.fc31.x86_64 from Fedora. -O2 still has an effect. So does -fcf-protection. -fno-omit-frame-pointer does not work for me at all for some reason, the frame pointer is always missing? Not sure what is going on here … Thanks, Florian
Re: [PATCH] Implement no_stack_protect attribute.
Hello, On Mon, 18 May 2020, Florian Weimer via Gcc-patches wrote: > > The patch adds new no_stack_protect attribute. The change is requested > > from kernel folks and is direct equivalent of Clang's no_stack_protector. > > Unlike Clang, I chose to name it no_stack_protect because we already > > have stack_protect attribute (used with -fstack-protector-explicit). > > > > First part of the patch contains a small refactoring of an enum, second > > implements the functionality. > > In glibc, we already have this: > > /* Used to disable stack protection in sensitive places, like ifunc >resolvers and early static TLS init. */ > #ifdef HAVE_CC_NO_STACK_PROTECTOR > # define inhibit_stack_protector \ > __attribute__ ((__optimize__ ("-fno-stack-protector"))) > #else > # define inhibit_stack_protector > #endif > > Is it broken? Depends on what your expectations are. It completely overrides all options given on the command line (including things like fno-omit-frame-pointer and -O2!). At least I was very surprised by that even though the current docu can be read that way; if you're similarly surprised, then yes, the above is broken, it does not only disable stack protection (but also e.g. all optimizations, despite the attributes name :-) ). And given that there's no possibility to express "and please only _add_ to the cmdline options" (which implies also being able to override values from the cmdline with -fno-xxx or other -fyyy options) I consider our current GCC behaviour for the optimize attribute basically unusable. (One work-around would be to define a macro containing all cmdline options in string form in the build system. Obviously that's a silly solution.) So, yeah, IMHO the semantics of that attribute should be revised to be more useful by default (with the possibility to express the current behaviour, for whomever would like to have that (but who?)). Ciao, Michael.
Re: [PATCH] Implement no_stack_protect attribute.
On Mon, May 18, 2020 at 1:10 PM Jakub Jelinek via Gcc-patches wrote: > > On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote: > > > The optimize attribute is used to specify that a function is to be > > > compiled > > > with different optimization options than specified on the command line. > > > ``` > > > > > > It's pretty clear about the command line arguments (that are ignored). > > > > That is not clear at all. The difference is primarily in what the option > > string says in there. > > And if we decide we want to keep existing behavior (haven't checked if we > actually behave that way yet), we should add some syntax that will allow > ammending command line options rather than replacing it. We do behave this way - while we're running against the current gcc_options we call decode_options which first does default_options_optimization. So it behaves inconsistently with respect to options not explicitly listed in default_options_table. But I also think doing the whole processing as in decode_options is the only thing that's really tested. And a fix would be to not start from the current set of options but from a clean slate... The code clearly thinks we're doing incremental changes: /* Save current options. */ cl_optimization_save (_opts, _options); /* If we previously had some optimization options, use them as the default. */ if (old_opts) cl_optimization_restore (_options, TREE_OPTIMIZATION (old_opts)); /* Parse options, and update the vector. */ parse_optimize_options (args, true); DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (_options); /* Restore current options. */ cl_optimization_restore (_options, _opts); so for example you should see -fipa-pta preserved from the command-line while -fno-tree-pta would be overridden even if not mentioned explicitely in the optimize attribute. IMHO the current situation is far from useful... Richard. > Could be say start the optimize attribute string with + or something > similar. > Does target attribute behave that way too? > > Jakub >
Re: [PATCH] Implement no_stack_protect attribute.
* Martin Liška: > The patch adds new no_stack_protect attribute. The change is requested > from kernel folks and is direct equivalent of Clang's no_stack_protector. > Unlike Clang, I chose to name it no_stack_protect because we already > have stack_protect attribute (used with -fstack-protector-explicit). > > First part of the patch contains a small refactoring of an enum, second > implements the functionality. In glibc, we already have this: /* Used to disable stack protection in sensitive places, like ifunc resolvers and early static TLS init. */ #ifdef HAVE_CC_NO_STACK_PROTECTOR # define inhibit_stack_protector \ __attribute__ ((__optimize__ ("-fno-stack-protector"))) #else # define inhibit_stack_protector #endif Is it broken? Thanks, Florian
Re: [PATCH] Implement no_stack_protect attribute.
On Mon, May 18, 2020 at 01:03:40PM +0200, Jakub Jelinek wrote: > > The optimize attribute is used to specify that a function is to be compiled > > with different optimization options than specified on the command line. > > ``` > > > > It's pretty clear about the command line arguments (that are ignored). > > That is not clear at all. The difference is primarily in what the option > string says in there. And if we decide we want to keep existing behavior (haven't checked if we actually behave that way yet), we should add some syntax that will allow ammending command line options rather than replacing it. Could be say start the optimize attribute string with + or something similar. Does target attribute behave that way too? Jakub
Re: [PATCH] Implement no_stack_protect attribute.
On Mon, May 18, 2020 at 01:00:01PM +0200, Martin Liška wrote: > On 5/18/20 12:41 PM, Jakub Jelinek wrote: > > On Mon, May 18, 2020 at 12:37:58PM +0200, Martin Liška wrote: > > > The patch adds new no_stack_protect attribute. The change is requested > > > from kernel folks and is direct equivalent of Clang's no_stack_protector. > > > Unlike Clang, I chose to name it no_stack_protect because we already > > > have stack_protect attribute (used with -fstack-protector-explicit). > > > > Wouldn't it be better to look at the optimize attribute to see if it really > > does what has been said in the thread and if we don't want to change it, > > such that a function with optimize attribute xyz stands for the option > > defaults + command line options + xyz, rather than option defaults + xyz > > only? > > It's documented behavior what we do: > > ``` > The optimize attribute is used to specify that a function is to be compiled > with different optimization options than specified on the command line. > ``` > > It's pretty clear about the command line arguments (that are ignored). That is not clear at all. The difference is primarily in what the option string says in there. Jakub
Re: [PATCH] Implement no_stack_protect attribute.
On 5/18/20 12:41 PM, Jakub Jelinek wrote: On Mon, May 18, 2020 at 12:37:58PM +0200, Martin Liška wrote: The patch adds new no_stack_protect attribute. The change is requested from kernel folks and is direct equivalent of Clang's no_stack_protector. Unlike Clang, I chose to name it no_stack_protect because we already have stack_protect attribute (used with -fstack-protector-explicit). Wouldn't it be better to look at the optimize attribute to see if it really does what has been said in the thread and if we don't want to change it, such that a function with optimize attribute xyz stands for the option defaults + command line options + xyz, rather than option defaults + xyz only? It's documented behavior what we do: ``` The optimize attribute is used to specify that a function is to be compiled with different optimization options than specified on the command line. ``` It's pretty clear about the command line arguments (that are ignored). Martin Jakub
Re: [PATCH] Implement no_stack_protect attribute.
On Mon, May 18, 2020 at 12:37:58PM +0200, Martin Liška wrote: > The patch adds new no_stack_protect attribute. The change is requested > from kernel folks and is direct equivalent of Clang's no_stack_protector. > Unlike Clang, I chose to name it no_stack_protect because we already > have stack_protect attribute (used with -fstack-protector-explicit). Wouldn't it be better to look at the optimize attribute to see if it really does what has been said in the thread and if we don't want to change it, such that a function with optimize attribute xyz stands for the option defaults + command line options + xyz, rather than option defaults + xyz only? Jakub