This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PR middle-end/30017 (ICE in C++ size hook)


> Jan Hubicka wrote:
> 
> > the attached testcase hits sanity check in C++ frontend after converting:
> >             __builtin_memcpy(this,&f,sizeof(FIND_RESULT));
> > to an assignment.  The hook cp_expr_size is called when expanding this
> > assignment resulting in ICE.  I think the check is just too strict
> > and can safely be dropped (as it also mentions that assignment is OK).
> 
> That check has been valuable in finding back-end bugs.  The problem is
> that asking for the size of an expression is not well-defined in the
> presence of C++ virtual bases.  In other words, while "sizeof (C)" is
> well-defined in C++ for all "C", that does not mean that given a "C*" it
> is safe to copy "sizeof (C)" bytes to/from there!  And, the back end
> must never make copies of C++ objects with constructors/assignment
> operators since all such copies need to go through the user-defined
> functions for copying.

Yes, I think what we have problem with is mixing the two highlevel and
lowlevel point of views.  For C++ up to gimplification it is perfectly
valid to stick on the C++ conventions (such as requiring copy
constructor to be used at copies), after gimplification we however
should make IL independent on such a details so the structure remains
just structure.

Backend itself should know it can't copy structures around unless it can
prove it does not make difference and it does so.
> 
> > I wonder why we still need expr_size hook that late in gimple form?
> 
> That's my question too.  Why does the backend need the size of
> FIND_RESULT at all in this context?
> 
> I suspect that we're trying to change:
> 
>   __builtin_memcpy(a, b, sizeof (*a))
> 
> into:
> 
>   *a = *b
> 
> at least, if we can prove alignments are OK.  That's fine -- but we
> shouldn't be recomputing the number of bytes to copy, since we already
> know it!

Yes, this is what code checks:

      if (!srctype || !desttype
	  || !TYPE_SIZE_UNIT (srctype)
	  || !TYPE_SIZE_UNIT (desttype)
	  || TREE_CODE (TYPE_SIZE_UNIT (srctype)) != INTEGER_CST
	  || TREE_CODE (TYPE_SIZE_UNIT (desttype)) != INTEGER_CST
	  || !operand_equal_p (TYPE_SIZE_UNIT (srctype), len, 0)
	  || !operand_equal_p (TYPE_SIZE_UNIT (desttype), len, 0))
	return 0;

This is supposed to check that the size of block to be copied by
assignment is the same as size of memcpy operand.  However the later
expanding code use the expr_size hook to compute this value and it
crash.
Even moving the expr_size hook here won't save me (since it would just
crash earlier).

One possible solution would be to adjust expr_size to still execute the
check, just return NULL if it don't pass promptly crashing every user
not prepared to cases where sizes are undefined.  I would laso have to
construct the INDIRECT_REF expr a bit earlier producing little more
GGC overhead so I have the expr to pass.

But I would rather go way where this construct is allowed on low level
gimple in order to get symmetry across types and possibly remove this
late use of the langhook as it is consistent with LTO plans.  The
transformation we crash on would be quite useful alowing optimizers to
copy propagate and reducing expected function size in the inliner.

I guess you have a lot better understanding of those details, do you see
some way?

Honza


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]