Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Thu, Nov 30, 2023 at 5:13 AM Alexandre Oliva wrote:
>
> On Nov 29, 2023, Richard Biener wrote:
>
> >> Because _#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.
>
> > 'arg_#(D)' looks like a SSA name, and no, taking the address doesn't work,
> > so I assume it was [arg_(D)][n_#] which is indeed OK.
>
> Yeah.
>
> > But you shouldn't need to change a pointer argument to be passed by
> > reference, do you?
>
> True, my attempt to simplify the example moved it past the breaking point.
>
> IIRC the actual situations I hit involved computing address of members
> of compound objects, such as struct members, even array elements
> thereof. They became problematic after replacing the object with a
> dereference in gimple stmts. The (effectively) offsetting operation is
> well-formed gimple, but IIRC adding dereferencing to it made it
> malformed gimple. I don't immediately see why this should be the case,
> since it's still offsetting, so perhaps I misremember.
>
> > As said, you want to restrict by-reference passing to arguments
> > that are !is_gimple_reg_type ()
>
> *nod*, it was already there:
>
> if (!(0 /* DECL_BY_REFERENCE (narg) */
> || is_gimple_reg_type (TREE_TYPE (nparm))
> ...
> {
> indirect_nparms.add (nparm);
>
> >> Here are changes.html entries for this and for the other newly-added
> >> features:
>
> > LGTM.
>
> Was that an ok to install, once the relevant pieces are in?
See below.
> > Can you check on the pass-by-reference thing again please?
>
> Sure. I'll get back to you shortly.
>
> If argument indirection becomes the only blocking issue, I'd be happy to
> disable it, or even split out the patch that introduces it, so that the
> bulk of the feature can go in while we sort out these details.
> Disabling it is as simple as:
>
> diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc
> index 293bec132b885..90770202fb851 100644
> --- a/gcc/ipa-strub.cc
> +++ b/gcc/ipa-strub.cc
> @@ -2831,6 +2831,7 @@ pass_ipa_strub::execute (function *)
>parm = DECL_CHAIN (parm),
>nparm = DECL_CHAIN (nparm),
>nparmt = nparmt ? TREE_CHAIN (nparmt) : NULL_TREE)
> + if (true) ; else // ??? Disable parm indirection for now.
>if (!(0 /* DECL_BY_REFERENCE (narg) */
> || is_gimple_reg_type (TREE_TYPE (nparm))
> || VECTOR_TYPE_P (TREE_TYPE (nparm))
>
>
> > Let's see if Honza or Martin have any comments on the IPA bits, I just
> > mentioned what I think should be doable ...
>
> I'm curious as to what you're hoping for. I mean, I am using
> create_version_clone_with_body, adding the new params and copying the
> preexisting ones, and modifying some argument types for indirection
> after cloning. The problems I faced were as I tried to replace params
> with their indirected versions. According to my notes and my
> recollection, that's where I hit most of the trouble. But what would
> this really buy us? Do you envision a possibility of actually splitting
> out the original function body, so that IPA takes care of the whole
> wrapping? AFAICT that would require a lot more infrastructure to deal
> with new and modified parameters, though the details of what I learned
> about this API back then, and that made it clear I wouldn't be able to
> use it, seem to have faded away from my memory.
In the end I was hoping for general comments on the cgraph usage
and for the specifics indeed being able to use IPA mechanisms
to perform the stmt re-writing (and re-gimplification) for the value to
by-reference replacement. The core copy_body mechanism
might support it by simply registering *new_param as replacement
for 'old_param' in the copy_body_data decl_map.
Iff the IPA folks (Honza or Martin) don't have any further comments
the patch is OK to install by next Monday.
Thanks,
Richard.
>
> --
> Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
>Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Thu, Nov 30, 2023 at 6:04 AM Alexandre Oliva wrote:
>
> On Nov 29, 2023, Richard Biener wrote:
>
> > On Wed, Nov 29, 2023 at 9:53 AM Alexandre Oliva wrote:
>
> >> Because _#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.
>
> > 'arg_#(D)' looks like a SSA name, and no, taking the address doesn't work,
> > so I assume it was [arg_(D)][n_#] which is indeed OK. But you
> > shouldn't need to change a pointer argument to be passed by reference,
> > do you? As said, you want to restrict by-reference passing to arguments
> > that are !is_gimple_reg_type (). Everywhere where a plain PARM_DECL
> > was valid a *ptr indirection is as well.
>
> > Can you check on the pass-by-reference thing again please?
>
> Applying the following patchlet on top of refs/users/aoliva/heads/strub
> (that has a -fstrub=all patchlet in it) I get a build error in libgo
> building golang.org/x/mod/sumdb.o:
>
> In function ‘golang_0org_1x_1mod_1sumdb.Client.checkTrees.strub.0’:
> go1: error: invalid argument to gimple call
> _195(D)->Hash
> # VUSE <.MEM_55>
> _16 = __builtin_memcmp (, _195(D)->Hash, 32);
> during IPA pass: strub
>
> golang.org/x/mod/sumdb.go.057i.remove_symbols: _5 = __builtin_memcmp (,
> , 32);
Ah, yeah - is considered a constant while _195(D)->Hash isn't.
So indeed you are correct - you'll need adjustments, sorry for
misleading you ...
Richard.
> within golang_0org_1x_1mod_1sumdb.Client.checkTrees becomes, in wrapped
> version thereof:
>
> golang.org/x/mod/sumdb.go.058i.strub: _16 = __builtin_memcmp (,
> _195(D)->Hash, 32);
>
> It's not even the case that Hash is at offset 0 into older's type, but I
> suspect the reason why the former is well-formed while the latter is not
> the offset, but the SSA_NAME.
>
>
> diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc
> index 293bec132b8..515ab9a2ee5 100644
> --- a/gcc/ipa-strub.cc
> +++ b/gcc/ipa-strub.cc
> @@ -1950,7 +1950,7 @@ walk_regimplify_addr_expr (tree *op, int *rec, void
> *arg)
>if (!*op || TREE_CODE (*op) != ADDR_EXPR)
> return NULL_TREE;
>
> - if (!is_gimple_val (*op))
> + if (0 && !is_gimple_val (*op))
> {
>tree ret = force_gimple_operand_gsi (, *op, true,
>NULL_TREE, true, GSI_SAME_STMT);
>
> --
> Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
>Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Nov 29, 2023, Richard Biener wrote:
> On Wed, Nov 29, 2023 at 9:53 AM Alexandre Oliva wrote:
>> Because _#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.
> 'arg_#(D)' looks like a SSA name, and no, taking the address doesn't work,
> so I assume it was [arg_(D)][n_#] which is indeed OK. But you
> shouldn't need to change a pointer argument to be passed by reference,
> do you? As said, you want to restrict by-reference passing to arguments
> that are !is_gimple_reg_type (). Everywhere where a plain PARM_DECL
> was valid a *ptr indirection is as well.
> Can you check on the pass-by-reference thing again please?
Applying the following patchlet on top of refs/users/aoliva/heads/strub
(that has a -fstrub=all patchlet in it) I get a build error in libgo
building golang.org/x/mod/sumdb.o:
In function ‘golang_0org_1x_1mod_1sumdb.Client.checkTrees.strub.0’:
go1: error: invalid argument to gimple call
_195(D)->Hash
# VUSE <.MEM_55>
_16 = __builtin_memcmp (, _195(D)->Hash, 32);
during IPA pass: strub
golang.org/x/mod/sumdb.go.057i.remove_symbols: _5 = __builtin_memcmp (,
, 32);
within golang_0org_1x_1mod_1sumdb.Client.checkTrees becomes, in wrapped
version thereof:
golang.org/x/mod/sumdb.go.058i.strub: _16 = __builtin_memcmp (,
_195(D)->Hash, 32);
It's not even the case that Hash is at offset 0 into older's type, but I
suspect the reason why the former is well-formed while the latter is not
the offset, but the SSA_NAME.
diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc
index 293bec132b8..515ab9a2ee5 100644
--- a/gcc/ipa-strub.cc
+++ b/gcc/ipa-strub.cc
@@ -1950,7 +1950,7 @@ walk_regimplify_addr_expr (tree *op, int *rec, void *arg)
if (!*op || TREE_CODE (*op) != ADDR_EXPR)
return NULL_TREE;
- if (!is_gimple_val (*op))
+ if (0 && !is_gimple_val (*op))
{
tree ret = force_gimple_operand_gsi (, *op, true,
NULL_TREE, true, GSI_SAME_STMT);
--
Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
Free Software Activist GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Nov 29, 2023, Richard Biener wrote:
>> Because _#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.
> 'arg_#(D)' looks like a SSA name, and no, taking the address doesn't work,
> so I assume it was [arg_(D)][n_#] which is indeed OK.
Yeah.
> But you shouldn't need to change a pointer argument to be passed by
> reference, do you?
True, my attempt to simplify the example moved it past the breaking point.
IIRC the actual situations I hit involved computing address of members
of compound objects, such as struct members, even array elements
thereof. They became problematic after replacing the object with a
dereference in gimple stmts. The (effectively) offsetting operation is
well-formed gimple, but IIRC adding dereferencing to it made it
malformed gimple. I don't immediately see why this should be the case,
since it's still offsetting, so perhaps I misremember.
> As said, you want to restrict by-reference passing to arguments
> that are !is_gimple_reg_type ()
*nod*, it was already there:
if (!(0 /* DECL_BY_REFERENCE (narg) */
|| is_gimple_reg_type (TREE_TYPE (nparm))
...
{
indirect_nparms.add (nparm);
>> Here are changes.html entries for this and for the other newly-added
>> features:
> LGTM.
Was that an ok to install, once the relevant pieces are in?
> Can you check on the pass-by-reference thing again please?
Sure. I'll get back to you shortly.
If argument indirection becomes the only blocking issue, I'd be happy to
disable it, or even split out the patch that introduces it, so that the
bulk of the feature can go in while we sort out these details.
Disabling it is as simple as:
diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc
index 293bec132b885..90770202fb851 100644
--- a/gcc/ipa-strub.cc
+++ b/gcc/ipa-strub.cc
@@ -2831,6 +2831,7 @@ pass_ipa_strub::execute (function *)
parm = DECL_CHAIN (parm),
nparm = DECL_CHAIN (nparm),
nparmt = nparmt ? TREE_CHAIN (nparmt) : NULL_TREE)
+ if (true) ; else // ??? Disable parm indirection for now.
if (!(0 /* DECL_BY_REFERENCE (narg) */
|| is_gimple_reg_type (TREE_TYPE (nparm))
|| VECTOR_TYPE_P (TREE_TYPE (nparm))
> Let's see if Honza or Martin have any comments on the IPA bits, I just
> mentioned what I think should be doable ...
I'm curious as to what you're hoping for. I mean, I am using
create_version_clone_with_body, adding the new params and copying the
preexisting ones, and modifying some argument types for indirection
after cloning. The problems I faced were as I tried to replace params
with their indirected versions. According to my notes and my
recollection, that's where I hit most of the trouble. But what would
this really buy us? Do you envision a possibility of actually splitting
out the original function body, so that IPA takes care of the whole
wrapping? AFAICT that would require a lot more infrastructure to deal
with new and modified parameters, though the details of what I learned
about this API back then, and that made it clear I wouldn't be able to
use it, seem to have faded away from my memory.
--
Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
Free Software Activist GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Wed, Nov 29, 2023 at 9:53 AM Alexandre Oliva wrote:
>
> On Nov 23, 2023, Richard Biener wrote:
>
> > Conceptually it shouldn't be much different from what IPA-SRA does
> > which is cloning a function but with different arguments, the function
> > signature transform described in terms of ipa-param-manipulation bits.
> > I've talked with Martin and at least there's currently no
> > by-value-to-by-reference
> > "transform", but IPA-SRA can pass two registers instead of one aggregate
> > for example. There's IPA_PARAM_OP_NEW already to add a new param.
> > In principle the whole function rewriting (apart of recovering
> > from inlining) should be doable within this framework.
>
> I agree, but my attempts to do so have been unfruitful, and hit various
> very obscure issues that made it unworkable. Maybe someone smarter than
> I am, or with more time than I had, can make the conversion. The uses
> of IPA_PARAM_OP_NEW for the cloning are there, and the more conservative
> choices I made got it to work reliably, unlike the more adventurous
> alternatives I tried. One of the obscure issues I recall hitting was
> this one. I can probably still locate the early strub patch that hit
> the described issue back then, if there's interest.
> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575044.html
>
> >> The
> >> wrapper already needs a custom ABI, because of the added watermark
> >> pointer, and va_list and whatnot when needed, so a little further
> >> customization to avoid a quite significant overhead seemed desirable.
>
> > I understood the purpose of this, but I also saw it's only ever needed for
> > non-strict operation
>
> Erhm... I suppose you're not talking about -fstrub=strict, because I
> can't see how this relates with the by-reference argument passing from
> wrapper to wrapped in internal strubbing.
>
> > and I think that if you care about security you'd never
> > want to operate in a way that you don't absolutely know the function
> > you call isn't going to scrub your secrets ...
>
> The ABI between wrapper and wrapped function splitting in internal strub
> doesn't change this.
>
> > But yes, I also wondered why you even run into re-gimplification issues.
>
> Because _#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.
'arg_#(D)' looks like a SSA name, and no, taking the address doesn't work,
so I assume it was [arg_(D)][n_#] which is indeed OK. But you
shouldn't need to change a pointer argument to be passed by reference,
do you? As said, you want to restrict by-reference passing to arguments
that are !is_gimple_reg_type (). Everywhere where a plain PARM_DECL
was valid a *ptr indirection is as well.
> Maybe instead of going through regimplify, we could explicitly create an
> SSA name for the indirection load, so that the purpose is made more
> explicit.
>
> > replacing the PARM_DECL there with a MEM_REF (new-PARM_DECL) should
> > work without re-gimplification.
>
> FWIW, I thought so as well ;-)
>
> > I didn't remember seeing that you do sth like wrapping
> > each "strubbed call" inside a try { call(); } finally { do-strub } to
> > ensure this.
> > Guess it was well hidden in the large patch ;)
>
> Indeed, gsi_insert_finally_seq_after_call is where this is taken care of.
>
> >> > As it's only for optimization, how much of the code would go away when
> >> > you avoid changing the parameters of the wrapped function?
> >>
> >> We can't avoid changing them entirely, and IIRC some of the
> >> infrastructure for regimplification was also used for va_list and
> >> apply_args handling, so it wouldn't save all that much.
>
> > Ah, var-args ... indeed for simple forwarding it shouldn't be too bad.
> > I wonder if this were a way to remove the restriction on function
> > splitting of var-args functions - there's a recent bugreport about that
> > (before any va_arg () has been called, of course).
>
> If running va_start unconditionally in varargs functions is generally
> acceptable, then the method I've used for wrapping varargs functions
> should work generally, yeah. The trick was to turn:
>
> foo (...)
> {
> va_list ap;
>
> ...
> va_start (, );
> ...
> }
>
> into
>
> wrapped_foo (va_list )
> {
> va_list ap;
>
> ...
> va_copy (ap, wrap);
> ...
> }
>
> foo (...)
> {
> va_list wrap;
>
> va_start (wrap, );
> wrapped_foo (wrap);
> va_end (wrap);
> }
Ah, nice trick indeed. I think that should work, it might not
be optimal (I don't think we will optimize the va_copy).
>
> Here's the opening comment I added to ipa-strub.cc:
>
> /* This file introduces two passes that, together, implement
>machine-independent stack scrubbing, strub for short. It arranges
>for stack frames that have strub enabled to be zeroed-out after
>relinquishing control to a caller, whether by returning or by
>propagating an exception. This admittedly unusual design decision
>was driven by exception support (one needs a stack frame to be
>active to propagate
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Nov 23, 2023, Richard Biener wrote:
> Conceptually it shouldn't be much different from what IPA-SRA does
> which is cloning a function but with different arguments, the function
> signature transform described in terms of ipa-param-manipulation bits.
> I've talked with Martin and at least there's currently no
> by-value-to-by-reference
> "transform", but IPA-SRA can pass two registers instead of one aggregate
> for example. There's IPA_PARAM_OP_NEW already to add a new param.
> In principle the whole function rewriting (apart of recovering
> from inlining) should be doable within this framework.
I agree, but my attempts to do so have been unfruitful, and hit various
very obscure issues that made it unworkable. Maybe someone smarter than
I am, or with more time than I had, can make the conversion. The uses
of IPA_PARAM_OP_NEW for the cloning are there, and the more conservative
choices I made got it to work reliably, unlike the more adventurous
alternatives I tried. One of the obscure issues I recall hitting was
this one. I can probably still locate the early strub patch that hit
the described issue back then, if there's interest.
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575044.html
>> The
>> wrapper already needs a custom ABI, because of the added watermark
>> pointer, and va_list and whatnot when needed, so a little further
>> customization to avoid a quite significant overhead seemed desirable.
> I understood the purpose of this, but I also saw it's only ever needed for
> non-strict operation
Erhm... I suppose you're not talking about -fstrub=strict, because I
can't see how this relates with the by-reference argument passing from
wrapper to wrapped in internal strubbing.
> and I think that if you care about security you'd never
> want to operate in a way that you don't absolutely know the function
> you call isn't going to scrub your secrets ...
The ABI between wrapper and wrapped function splitting in internal strub
doesn't change this.
> But yes, I also wondered why you even run into re-gimplification issues.
Because _#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.
Maybe instead of going through regimplify, we could explicitly create an
SSA name for the indirection load, so that the purpose is made more
explicit.
> replacing the PARM_DECL there with a MEM_REF (new-PARM_DECL) should
> work without re-gimplification.
FWIW, I thought so as well ;-)
> I didn't remember seeing that you do sth like wrapping
> each "strubbed call" inside a try { call(); } finally { do-strub } to
> ensure this.
> Guess it was well hidden in the large patch ;)
Indeed, gsi_insert_finally_seq_after_call is where this is taken care of.
>> > As it's only for optimization, how much of the code would go away when
>> > you avoid changing the parameters of the wrapped function?
>>
>> We can't avoid changing them entirely, and IIRC some of the
>> infrastructure for regimplification was also used for va_list and
>> apply_args handling, so it wouldn't save all that much.
> Ah, var-args ... indeed for simple forwarding it shouldn't be too bad.
> I wonder if this were a way to remove the restriction on function
> splitting of var-args functions - there's a recent bugreport about that
> (before any va_arg () has been called, of course).
If running va_start unconditionally in varargs functions is generally
acceptable, then the method I've used for wrapping varargs functions
should work generally, yeah. The trick was to turn:
foo (...)
{
va_list ap;
...
va_start (, );
...
}
into
wrapped_foo (va_list )
{
va_list ap;
...
va_copy (ap, wrap);
...
}
foo (...)
{
va_list wrap;
va_start (wrap, );
wrapped_foo (wrap);
va_end (wrap);
}
Here's the opening comment I added to ipa-strub.cc:
/* This file introduces two passes that, together, implement
machine-independent stack scrubbing, strub for short. It arranges
for stack frames that have strub enabled to be zeroed-out after
relinquishing control to a caller, whether by returning or by
propagating an exception. This admittedly unusual design decision
was driven by exception support (one needs a stack frame to be
active to propagate exceptions out of it), and it enabled an
implementation that is entirely machine-independent (no custom
epilogue code is required).
Strub modes can be selected for stack frames by attaching attribute
strub to functions or to variables (to their types, actually).
Different strub modes, with different implementation details, are
available, and they can be selected by an argument to the strub
attribute. When enabled by strub-enabled variables, whether by
accessing (as in reading from) statically-allocated ones, or by
introducing (as in declaring) automatically-allocated ones, a
suitable mode is selected automatically.
At-calls mode modifies the interface of a function, adding a stack
watermark argument, that callers use to clean up the
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Thu, Nov 23, 2023 at 11:56 AM Alexandre Oliva wrote:
>
> Hello, Richi,
>
> Thanks for the extensive review!
>
> On Nov 22, 2023, Richard Biener wrote:
>
> > On Mon, Nov 20, 2023 at 1:40 PM Alexandre Oliva wrote:
> >>
> >> On Oct 26, 2023, Alexandre Oliva wrote:
> >>
> >> >> This is a refreshed and improved version of the version posted back in
> >> >> June. https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621936.html
> >>
> >> > Ping? https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633675.html
> >> > I'm combining the gcc/ipa-strub.cc bits from
> >> > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633526.html
> >>
> >> Ping?
> >> Retested on x86_64-linux-gnu, with and without -fstrub=all.
>
> > @@ -898,7 +899,24 @@ decl_attributes (tree *node, tree attributes, int
> > flags,
> >TYPE_NAME (tt) = *node;
> > }
>
> > - *anode = cur_and_last_decl[0];
> > + if (*anode != cur_and_last_decl[0])
> > +{
> > + /* Even if !spec->function_type_required, allow the attribute
> > + handler to request the attribute to be applied to the function
> > + type, rather than to the function pointer type, by setting
> > + cur_and_last_decl[0] to the function type. */
> > + if (!fn_ptr_tmp
> > + && POINTER_TYPE_P (*anode)
> > + && TREE_TYPE (*anode) == cur_and_last_decl[0]
> > + && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (*anode)))
> > + {
> > + fn_ptr_tmp = TREE_TYPE (*anode);
> > + fn_ptr_quals = TYPE_QUALS (*anode);
> > + anode = _ptr_tmp;
> > + }
> > + *anode = cur_and_last_decl[0];
> > +}
> > +
>
> > what is this a workaround for?
>
> For the fact that the strub attribute attaches to types, whether data or
> function types, so we can't have fn_type_req, but when it's a function
> or pointer-to-function type, we want to affect the function type, rather
> than the pointer type, when the attribute has an argument. The argument
> names the strub mode for a function; that only applies to function
> types, never to data types.
>
> The hunk above introduces the means for the attribute handler to choose
> what to attach the attribute t.
>
> > Isn't there a suitable parsing position for placing the attribute?
>
> It's been a while, but IIRC the need for this first came up in Ada,
> where attributes can't just go anywhere, and it was further complicated
> by the fact that Ada doesn't have first-class function or procedure
> types, only access-to-them, but we needed some means for the attributes
> to apply to the function type.
>
> > +#ifndef STACK_GROWS_DOWNWARD
> > +# define STACK_TOPS GT
> > +#else
> > +# define STACK_TOPS LT
> > +#endif
>
> > according to docs this is defined to 0 or 1 so the above looks wrong
> > (it's always defined).
>
> Ugh. Thanks, will fix. (I'm pretty sure I had notes somewhere stating
> that stack-grows-upwards hadn't been tested, and that was for the sheer
> lack of platforms making that choice, but I hoped it wasn't that broken
> :-(
>
> > + if (optimize < 2 || optimize_size || flag_no_inline)
> > +return NULL_RTX;
>
> > I'm wondering about these checks in the expansions of the builtins,
> > I think this is about inline expanding or emitting a libcall, right?
>
> Yeah.
>
> > I wonder if you should use optimize_function_for_speed (cfun) instead?
> > Usually -fno-inline shouldn't affect such calls, but -fno-builtin-FOO would.
> > I have no strong opinion here though.
>
> I've occasionally wondered whether builtins were the best framework for
> these semi-internal calls.
>
> > The new builtins seem undocumented - usually those are documented
> > within extend.texi
>
> Erhm... Weird. I had documentation for them.
>
> (checks)
>
> No, it's there, in extend.texi, right after __builtin_stack_address.
> It's admittedly a big patch :-/
>
> > I guess placing __builtin___strub_enter calls in the code manually
> > will break in interesting ways - if that's not supposed to happen the
> > trick is to embed a space in the name of the built-in.
>
> Yeah, I was a little torn between the choices here. On the one hand, I
> needed visible symbols for the out-of-line implementations, so I figured
> that trying to hide the builtins wouldn't bring any advantage.
>
> However, I've also designed the builtins with interfaces that would
> avoid disruption even with explicit calls. __strub_enter and
> __strub_update only initialize or adjust a pointer handed to them.
> __strub_leave will erase things from the top of the stack to the
> pointer, so if the watermark is "active stack", nothing happens, and
> things only get cleared if it points to "unused stack space". There's
> potential for disruption if one passes a statically-allocated pointer to
> it, but nothing much different from memsetting that memory range, core
> wars-style.
>
> > -symtab_node::reset (void)
> > +symtab_node::reset (bool preserve_comdat_group)
>
> > not sure what for, I'll leave Honza to comment.
>
> This restores the possibility of getting the pre-PR107897 behavior, that
> the strub
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
Hello, Richi,
Thanks for the extensive review!
On Nov 22, 2023, Richard Biener wrote:
> On Mon, Nov 20, 2023 at 1:40 PM Alexandre Oliva wrote:
>>
>> On Oct 26, 2023, Alexandre Oliva wrote:
>>
>> >> This is a refreshed and improved version of the version posted back in
>> >> June. https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621936.html
>>
>> > Ping? https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633675.html
>> > I'm combining the gcc/ipa-strub.cc bits from
>> > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633526.html
>>
>> Ping?
>> Retested on x86_64-linux-gnu, with and without -fstrub=all.
> @@ -898,7 +899,24 @@ decl_attributes (tree *node, tree attributes, int flags,
>TYPE_NAME (tt) = *node;
> }
> - *anode = cur_and_last_decl[0];
> + if (*anode != cur_and_last_decl[0])
> +{
> + /* Even if !spec->function_type_required, allow the attribute
> + handler to request the attribute to be applied to the function
> + type, rather than to the function pointer type, by setting
> + cur_and_last_decl[0] to the function type. */
> + if (!fn_ptr_tmp
> + && POINTER_TYPE_P (*anode)
> + && TREE_TYPE (*anode) == cur_and_last_decl[0]
> + && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (*anode)))
> + {
> + fn_ptr_tmp = TREE_TYPE (*anode);
> + fn_ptr_quals = TYPE_QUALS (*anode);
> + anode = _ptr_tmp;
> + }
> + *anode = cur_and_last_decl[0];
> +}
> +
> what is this a workaround for?
For the fact that the strub attribute attaches to types, whether data or
function types, so we can't have fn_type_req, but when it's a function
or pointer-to-function type, we want to affect the function type, rather
than the pointer type, when the attribute has an argument. The argument
names the strub mode for a function; that only applies to function
types, never to data types.
The hunk above introduces the means for the attribute handler to choose
what to attach the attribute t.
> Isn't there a suitable parsing position for placing the attribute?
It's been a while, but IIRC the need for this first came up in Ada,
where attributes can't just go anywhere, and it was further complicated
by the fact that Ada doesn't have first-class function or procedure
types, only access-to-them, but we needed some means for the attributes
to apply to the function type.
> +#ifndef STACK_GROWS_DOWNWARD
> +# define STACK_TOPS GT
> +#else
> +# define STACK_TOPS LT
> +#endif
> according to docs this is defined to 0 or 1 so the above looks wrong
> (it's always defined).
Ugh. Thanks, will fix. (I'm pretty sure I had notes somewhere stating
that stack-grows-upwards hadn't been tested, and that was for the sheer
lack of platforms making that choice, but I hoped it wasn't that broken
:-(
> + if (optimize < 2 || optimize_size || flag_no_inline)
> +return NULL_RTX;
> I'm wondering about these checks in the expansions of the builtins,
> I think this is about inline expanding or emitting a libcall, right?
Yeah.
> I wonder if you should use optimize_function_for_speed (cfun) instead?
> Usually -fno-inline shouldn't affect such calls, but -fno-builtin-FOO would.
> I have no strong opinion here though.
I've occasionally wondered whether builtins were the best framework for
these semi-internal calls.
> The new builtins seem undocumented - usually those are documented
> within extend.texi
Erhm... Weird. I had documentation for them.
(checks)
No, it's there, in extend.texi, right after __builtin_stack_address.
It's admittedly a big patch :-/
> I guess placing __builtin___strub_enter calls in the code manually
> will break in interesting ways - if that's not supposed to happen the
> trick is to embed a space in the name of the built-in.
Yeah, I was a little torn between the choices here. On the one hand, I
needed visible symbols for the out-of-line implementations, so I figured
that trying to hide the builtins wouldn't bring any advantage.
However, I've also designed the builtins with interfaces that would
avoid disruption even with explicit calls. __strub_enter and
__strub_update only initialize or adjust a pointer handed to them.
__strub_leave will erase things from the top of the stack to the
pointer, so if the watermark is "active stack", nothing happens, and
things only get cleared if it points to "unused stack space". There's
potential for disruption if one passes a statically-allocated pointer to
it, but nothing much different from memsetting that memory range, core
wars-style.
> -symtab_node::reset (void)
> +symtab_node::reset (bool preserve_comdat_group)
> not sure what for, I'll leave Honza to comment.
This restores the possibility of getting the pre-PR107897 behavior, that
the strub wrapper/wrapped splitting relied on. Conceptually, the
original function becomes the wrapped one, and the wrapper that calls it
is kind of an implementation detail to preserve the exposed API/ABI
while introducing strubbing around the body, so preserving the comdat
group makes sense.
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Mon, Nov 20, 2023 at 1:40 PM Alexandre Oliva wrote:
>
> On Oct 26, 2023, Alexandre Oliva wrote:
>
> >> This is a refreshed and improved version of the version posted back in
> >> June. https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621936.html
>
> > Ping? https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633675.html
> > I'm combining the gcc/ipa-strub.cc bits from
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633526.html
>
> Ping?
> Retested on x86_64-linux-gnu, with and without -fstrub=all.
@@ -898,7 +899,24 @@ decl_attributes (tree *node, tree attributes, int flags,
TYPE_NAME (tt) = *node;
}
- *anode = cur_and_last_decl[0];
+ if (*anode != cur_and_last_decl[0])
+{
+ /* Even if !spec->function_type_required, allow the attribute
+ handler to request the attribute to be applied to the function
+ type, rather than to the function pointer type, by setting
+ cur_and_last_decl[0] to the function type. */
+ if (!fn_ptr_tmp
+ && POINTER_TYPE_P (*anode)
+ && TREE_TYPE (*anode) == cur_and_last_decl[0]
+ && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (*anode)))
+ {
+ fn_ptr_tmp = TREE_TYPE (*anode);
+ fn_ptr_quals = TYPE_QUALS (*anode);
+ anode = _ptr_tmp;
+ }
+ *anode = cur_and_last_decl[0];
+}
+
what is this a workaround for? Isn't there a suitable parsing position
for placing the attribute?
+#ifndef STACK_GROWS_DOWNWARD
+# define STACK_TOPS GT
+#else
+# define STACK_TOPS LT
+#endif
according to docs this is defined to 0 or 1 so the above looks wrong
(it's always defined).
+ if (optimize < 2 || optimize_size || flag_no_inline)
+return NULL_RTX;
I'm wondering about these checks in the expansions of the builtins,
I think this is about inline expanding or emitting a libcall, right?
I wonder if you should use optimize_function_for_speed (cfun) instead?
Usually -fno-inline shouldn't affect such calls, but -fno-builtin-FOO would.
I have no strong opinion here though.
The new builtins seem undocumented - usually those are documented
within extend.texi - I guess placing __builtin___strub_enter calls in
the code manually will break in interesting ways - if that's not supposed
to happen the trick is to embed a space in the name of the built-in.
__builtin_stack_address looks like something users will pick up though
(and thus should be documented)?
-symtab_node::reset (void)
+symtab_node::reset (bool preserve_comdat_group)
not sure what for, I'll leave Honza to comment.
+/* Create a distinct copy of the type of NODE's function, and change
+ the fntype of all calls to it with the same main type to the new
+ type. */
+
+static void
+distinctify_node_type (cgraph_node *node)
+{
+ tree old_type = TREE_TYPE (node->decl);
+ tree new_type = build_distinct_type_copy (old_type);
+ tree new_ptr_type = NULL_TREE;
+
+ /* Remap any calls to node->decl that use old_type, or a variant
+ thereof, to new_type as well. We don't look for aliases, their
+ declarations will have their types changed independently, and
+ we'll adjust their fntypes then. */
+ for (cgraph_edge *e = node->callers; e; e = e->next_caller)
+{
+ if (!e->call_stmt)
+ continue;
+ tree fnaddr = gimple_call_fn (e->call_stmt);
+ gcc_checking_assert (TREE_CODE (fnaddr) == ADDR_EXPR
+ && TREE_OPERAND (fnaddr, 0) == node->decl);
+ if (strub_call_fntype_override_p (e->call_stmt))
+ continue;
+ if (!new_ptr_type)
+ new_ptr_type = build_pointer_type (new_type);
+ TREE_TYPE (fnaddr) = new_ptr_type;
+ gimple_call_set_fntype (e->call_stmt, new_type);
+}
+
+ TREE_TYPE (node->decl) = new_type;
it does feel like there's IPA mechanisms to deal with what you are trying to do
here (or in the caller(s)).
+unsigned int
+pass_ipa_strub_mode::execute (function *)
+{
+ last_cgraph_order = 0;
+ ipa_strub_set_mode_for_new_functions ();
+
+ /* Verify before any inlining or other transformations. */
+ verify_strub ();
if (flag_checking) verify_strub ();
please. I guess we talked about this last year - what's the reason to have both
an IPA pass and a simple IPA pass? IIRC the simple IPA pass is a simple
one because it wants to see inlined bodies and "fixes" those up? Some toplevel
comments explaining both passes in the ipa-strub.cc pass would be nice to
have. I guess I also asked before - did you try it with -flto?
+/* Decide which of the wrapped function's parms we want to turn into
+ references to the argument passed to the wrapper. In general,
we want to
+ copy small arguments, and avoid copying large ones.
Variable-sized array
+ lengths given by other arguments, as in 20020210-1.c, would lead to
+ problems if passed by value, after resetting the original function and
+ dropping the length computation; passing them by reference works.
+ DECL_BY_REFERENCE is *not* a substitute for this: it involves copying
+ anyway, but performed at the caller. */
+indirect_parms_t indirect_nparms (3,
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
On Oct 26, 2023, Alexandre Oliva wrote: >> This is a refreshed and improved version of the version posted back in >> June. https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621936.html > Ping? https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633675.html > I'm combining the gcc/ipa-strub.cc bits from > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633526.html Ping? Retested on x86_64-linux-gnu, with and without -fstrub=all. >> This patch adds the strub attribute for function and variable types, >> command-line options, passes and adjustments to implement it, >> documentation, and tests. >> Stack scrubbing is implemented in a machine-independent way: functions ... -- Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer More tolerance and less prejudice are key for inclusion and diversity Excluding neuro-others for not behaving ""normal"" is *not* inclusive
Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing
Ping? https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633675.html I'm combining the gcc/ipa-strub.cc bits from https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633526.html -- Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer More tolerance and less prejudice are key for inclusion and diversity Excluding neuro-others for not behaving ""normal"" is *not* inclusive
