Re: [PATCH] Implement no_stack_protect attribute.

2020-10-22 Thread Martin Liška

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.

2020-10-21 Thread Nick Desaulniers via Gcc-patches
+ 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.

2020-10-21 Thread Nick Desaulniers via Gcc-patches
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.

2020-10-21 Thread Jakub Jelinek via Gcc-patches
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.

2020-10-21 Thread Nick Desaulniers via Gcc-patches
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.

2020-10-20 Thread Richard Biener via Gcc-patches
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.

2020-10-20 Thread Martin Liška

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.

2020-08-26 Thread Martin Liška

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.

2020-08-25 Thread Nick Desaulniers via Gcc-patches
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.

2020-08-17 Thread Martin Liška

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.

2020-07-23 Thread Martin Liška

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.

2020-06-24 Thread Martin Liška

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.

2020-06-10 Thread Martin Sebor via Gcc-patches

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.

2020-06-10 Thread Martin Liška

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.

2020-05-25 Thread Martin Liška

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.

2020-05-25 Thread Richard Biener via Gcc-patches
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.

2020-05-25 Thread Martin Liška

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.

2020-05-22 Thread Richard Biener via Gcc-patches
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.

2020-05-21 Thread Martin Sebor via Gcc-patches

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.

2020-05-21 Thread Martin Liška

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.

2020-05-21 Thread Martin Liška

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.

2020-05-21 Thread Martin Liška

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.

2020-05-18 Thread Martin Sebor via Gcc-patches

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.

2020-05-18 Thread Andreas Schwab
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.

2020-05-18 Thread Michael Matz
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.

2020-05-18 Thread Florian Weimer via Gcc-patches
* 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.

2020-05-18 Thread Michael Matz
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.

2020-05-18 Thread Florian Weimer via Gcc-patches
* 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.

2020-05-18 Thread 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 
:-) ).

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.

2020-05-18 Thread Richard Biener via Gcc-patches
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.

2020-05-18 Thread Florian Weimer via Gcc-patches
* 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.

2020-05-18 Thread Jakub Jelinek via Gcc-patches
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.

2020-05-18 Thread Jakub Jelinek via Gcc-patches
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.

2020-05-18 Thread Martin Liška

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.

2020-05-18 Thread Jakub Jelinek via Gcc-patches
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