public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Kyrill Tkachov <kyrylo.tkachov@arm.com>
To: Jeff Law <law@redhat.com>, GCC Patches <gcc-patches@gcc.gnu.org>
Cc: Honggyu Kim <hong.gyu.kim@lge.com>
Subject: Re: [PATCH][expr.c] PR 65358 Avoid clobbering partial argument during sibcall
Date: Tue, 21 Apr 2015 17:33:00 -0000	[thread overview]
Message-ID: <553689F0.5090606@arm.com> (raw)
In-Reply-To: <55365A06.7010408@redhat.com>


On 21/04/15 15:09, Jeff Law wrote:
> On 04/21/2015 02:30 AM, Kyrill Tkachov wrote:
>>   From reading config/stormy16/stormy-abi it seems to me that we don't
>> pass arguments partially in stormy16, so this code would never be called
>> there. That leaves pa as the potential problematic target.
>> I don't suppose there's an easy way to test on pa? My checkout of binutils
>> doesn't seem to include a sim target for it.
> No simulator, no machines in the testfarm, the box I had access to via
> parisc-linux.org seems dead and my ancient PA overheats well before a
> bootstrap could complete.  I often regret knowing about the backwards
> way many things were done on the PA because it makes me think about
> cases that only matter on dead architectures.

So what should be the action plan here? I can't add an assert on
positive result as a negative result is valid.

We want to catch the case where this would cause trouble on
pa, or change the patch until we're confident that it's fine
for pa.

That being said, reading the documentation of STACK_GROWS_UPWARD
and ARGS_GROW_DOWNWARD I'm having a hard time visualising a case
where this would cause trouble on pa.

Is the problem that in the function:

+/* Add SIZE to X and check whether it's greater than Y.
+   If it is, return the constant amount by which it's greater or smaller.
+   If the two are not statically comparable (for example, X and Y contain
+   different registers) return -1.  This is used in expand_push_insn to
+   figure out if reading SIZE bytes from location X will end up reading from
+   location Y.  */
+static int
+memory_load_overlap (rtx x, rtx y, HOST_WIDE_INT size)
+{
+  rtx tmp = plus_constant (Pmode, x, size);
+  rtx sub = simplify_gen_binary (MINUS, Pmode, tmp, y);
+
+  if (!CONST_INT_P (sub))
+    return -1;
+
+  return INTVAL (sub);
+}

for ARGS_GROW_DOWNWARD we would be reading 'backwards' from x,
so the function should something like the following?

static int
memory_load_overlap (rtx x, rtx y, HOST_WIDE_INT size)
{
#ifdef ARGS_GROW_DOWNWARD
   rtx tmp = plus_constant (Pmode, x, -size);
#else
   rtx tmp = plus_constant (Pmode, x, size);
#endif
   rtx sub = simplify_gen_binary (MINUS, Pmode, tmp, y);

   if (!CONST_INT_P (sub))
     return -1;

#ifdef ARGS_GROW_DOWNWARD
   return INTVAL (-sub);
#else
   return INTVAL (sub);
#endif
}

now, say for x == sp + 4,  y == sp + 8, size == 16:
This would be a problematic case for arm, so this code on arm
(where ARGS_GROW_DOWNWARD is *not* defined) would return
12, which is the number of bytes that overlap.

On a target where ARGS_GROW_DOWNWARD is defined this would return
-20, meaning that no overlap occurs (because we read in the descending
direction from x, IIUC).


Thanks,
Kyrill

>
>
> Jeff
>

  reply	other threads:[~2015-04-21 17:33 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-19 14:39 Kyrill Tkachov
2015-03-27 10:06 ` Kyrill Tkachov
2015-03-29 11:29 ` Honggyu Kim
2015-04-13 14:01 ` Kyrill Tkachov
2015-04-13 16:33   ` Jeff Law
2015-04-17 17:26 ` Jeff Law
2015-04-20  8:25   ` Kyrill Tkachov
2015-04-20 18:02     ` Jeff Law
2015-04-21  8:30       ` Kyrill Tkachov
2015-04-21 14:09         ` Jeff Law
2015-04-21 17:33           ` Kyrill Tkachov [this message]
2015-04-22 11:51             ` Kyrill Tkachov
2015-04-27 10:12               ` Kyrill Tkachov
2015-04-27 13:16                 ` John David Anglin
2015-05-06 18:57                   ` John David Anglin
2015-04-27 20:13             ` Jeff Law
2015-04-28 10:19               ` Kyrill Tkachov
2015-04-30 12:09                 ` Kyrill Tkachov
2015-05-01 18:51                 ` Jeff Law
2015-05-11  9:28                   ` Kyrill Tkachov
2015-05-12 22:12                     ` Jeff Law
2015-05-27 14:00                       ` Kyrill Tkachov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=553689F0.5090606@arm.com \
    --to=kyrylo.tkachov@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hong.gyu.kim@lge.com \
    --cc=law@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).