Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing

[Alexandre Oliva <oliva@adacore.com>]

On Nov 29, 2023, Richard Biener <richard.guenther@gmail.com> wrote:

>> Because &arg_#(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 &MEM[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 hacker            https://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