On Fri, Jun 28, 2019 at 2:16 AM Jeff Law <l...@redhat.com> wrote:
>
> On 6/27/19 10:12 AM, Jakub Jelinek wrote:
> > On Thu, Jun 27, 2019 at 09:15:41AM -0600, Jeff Law wrote:
> >> Actually it was trivial to create with store merging.
> >>
> >> char x[20];
> >> foo()
> >> {
> >> x[0] = 0x41;
> >> x[1] = 0x42;
> >> }
> >>
> >> MEM <unsigned short> [(char *)&x] = 16961;
> >>
> >> So clearly we can get this in gimple. So we need a check of some kind,
> >> either on the LHS or the RHS to ensure that we actually have a single
> >> character store as opposed to something like the above.
> >
> > handle_char_store is only called if:
> > tree type = TREE_TYPE (lhs);
> > if (TREE_CODE (type) == ARRAY_TYPE)
> > type = TREE_TYPE (type);
> > if (TREE_CODE (type) == INTEGER_TYPE
> > && TYPE_MODE (type) == TYPE_MODE (char_type_node)
> > && TYPE_PRECISION (type) == TYPE_PRECISION (char_type_node))
> > {
> > if (! handle_char_store (gsi))
> > return false;
> > }
> > so the above case will not trigger, the only way to call it for multiple
> > character stores is if you have an array. And so far I've succeeded
> > creating that just with strcpy builtin.
> How I could have missed it so many times is a mystery to me. I
> literally was looking at this on the phone with Martin today for the nth
> time and my brain just wouldn't parse this code correctly.
>
> We can only get into handle_char_store for an LHS that is a character or
> array of characters. So the only case we have to worry about would be
> if we have a constant array on the RHS. Something like this from Jakub:
>
> MEM <char[2]> [(char *)&x] = MEM <char[2]> [(char *)"ab"];
>
> But in this case the RHS is going to be an ARRAY_TYPE. And thus
> Martin's new code would reject it because it would only do the
> optmiization if the RHS has INTEGER_TYPE.
>
> So I think my concerns are ultimately a non-issue.
>
> FWIW if you're trying to get something like Jakub's assignment, this
> will do it:
>
> char a[7];
> int f ()
> {
> __builtin_strcpy (a, "654321");
> }
>
> Which results in this being passed to handle_char_store:
>
> MEM <unsigned char[7]> [(char * {ref-all})&a] = MEM <unsigned char[7]>
> [(char * {ref-all})"654321"];
>
> We can make it more problematical by first doing a strcpy into a so that
> we create an SI structure. Something like this:
>
> char a[7];
> int f ()
> {
> strcpy (a, "123");
> __builtin_strcpy (a, "654321");
> }
>
> Compiled with -O2 -fno-tree-dse (the -fno-tree-dse ensures the first
> strcpy is not eliminated).
>
> That should get us into handle_char_store and into the new code from
> Martin's patch that wants to test:
>
>
>
> + if (cmp > 0
> + && storing_nonzero_p
> + && TREE_CODE (TREE_TYPE (rhs)) == INTEGER_TYPE)
>
> But the RHS in this case has an ARRAY_TYPE and Martin's test would
> reject it.
>
>
> So at this point my concerns are alleviated.
>
> Jakub, Richi, do either of you have concerns? I've kindof owned the
> review of this patch from Martin, but since y'all have chimed in, I'd
> like to give you another chance to chime in before I ACK.
>
> jeff
>
> ps. FWIW, the gimple FE seems to really dislike constant strings in MEM
> expressions like we've been working with. It also seems to go into an
> infinite loop if you've got an extra closing paren on the RHS.. It was
> still useful to poke at things a bit. One could argue that if we get
> the FE to a suitable state that it could define what is and what is not
> valid gimple. That would likely be incredibly help for this kind of stuff.
;) error handling is a bit weak indeed.
I'm testing/committing the following to allow string literals in __MEM
which needed string address literals. I guess for Fortran we need
to extend this to cover CONST_DECLs which should eventually
appear as _Literal (int *) &5 for example.
2019-07-01 Richard Biener <rguent...@suse.de>
c/
* gimple-parser.c (c_parser_gimple_postfix_expression): Handle
_Literal (char *) &"foo" for address literals pointing to
STRING_CSTs.
* gcc.dg/gimplefe-42.c: New testcase.
gimplefe-addr-taken-string-literal
Description: Binary data
