Bug in expand_builtin_sprintf

Bug in expand_builtin_sprintf

[Heikki Linnakangas]

I think there's a small bug in expand_builtin_sprintf in trunk,
introduced by commit 122018. Fix below:

Index: builtins.c
===================================================================
--- builtins.c  (revision 129124)
+++ builtins.c  (working copy)
@@ -5449,7 +5449,7 @@
   dest = CALL_EXPR_ARG (exp, 0);
   if (! POINTER_TYPE_P (TREE_TYPE (dest)))
     return NULL_RTX;
-  fmt = CALL_EXPR_ARG (exp, 0);
+  fmt = CALL_EXPR_ARG (exp, 1);
   if (! POINTER_TYPE_P (TREE_TYPE (fmt)))
     return NULL_RTX;

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Re: Bug in expand_builtin_sprintf

[Kaveh R. GHAZI]

On Mon, 8 Oct 2007, Heikki Linnakangas wrote:

> I think there's a small bug in expand_builtin_sprintf in trunk,
> introduced by commit 122018. Fix below:
>
> Index: builtins.c
> ===================================================================
> --- builtins.c  (revision 129124)
> +++ builtins.c  (working copy)
> @@ -5449,7 +5449,7 @@
>    dest = CALL_EXPR_ARG (exp, 0);
>    if (! POINTER_TYPE_P (TREE_TYPE (dest)))
>      return NULL_RTX;
> -  fmt = CALL_EXPR_ARG (exp, 0);
> +  fmt = CALL_EXPR_ARG (exp, 1);
>    if (! POINTER_TYPE_P (TREE_TYPE (fmt)))
>      return NULL_RTX;
>

Testcase please?

--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Bug in expand_builtin_sprintf

[Heikki Linnakangas]

Kaveh R. GHAZI wrote:
> On Mon, 8 Oct 2007, Heikki Linnakangas wrote:
> 
>> I think there's a small bug in expand_builtin_sprintf in trunk,
>> introduced by commit 122018. Fix below:
>>
>> Index: builtins.c
>> ===================================================================
>> --- builtins.c  (revision 129124)
>> +++ builtins.c  (working copy)
>> @@ -5449,7 +5449,7 @@
>>    dest = CALL_EXPR_ARG (exp, 0);
>>    if (! POINTER_TYPE_P (TREE_TYPE (dest)))
>>      return NULL_RTX;
>> -  fmt = CALL_EXPR_ARG (exp, 0);
>> +  fmt = CALL_EXPR_ARG (exp, 1);
>>    if (! POINTER_TYPE_P (TREE_TYPE (fmt)))
>>      return NULL_RTX;
>>
> 
> Testcase please?

I don't have one, unfortunately. I spotted this by looking at the code.
I believe the only consequence is that the simplifications are never
done in expand_builtin_sprintf.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Re: Bug in expand_builtin_sprintf

[Jakub Jelinek]

On Wed, Oct 10, 2007 at 08:46:44AM +0100, Heikki Linnakangas wrote:
> Kaveh R. GHAZI wrote:
> > On Mon, 8 Oct 2007, Heikki Linnakangas wrote:
> > 
> >> I think there's a small bug in expand_builtin_sprintf in trunk,
> >> introduced by commit 122018. Fix below:
> >>
> >> Index: builtins.c
> >> ===================================================================
> >> --- builtins.c  (revision 129124)
> >> +++ builtins.c  (working copy)
> >> @@ -5449,7 +5449,7 @@
> >>    dest = CALL_EXPR_ARG (exp, 0);
> >>    if (! POINTER_TYPE_P (TREE_TYPE (dest)))
> >>      return NULL_RTX;
> >> -  fmt = CALL_EXPR_ARG (exp, 0);
> >> +  fmt = CALL_EXPR_ARG (exp, 1);
> >>    if (! POINTER_TYPE_P (TREE_TYPE (fmt)))
> >>      return NULL_RTX;
> >>
> > 
> > Testcase please?
> 
> I don't have one, unfortunately. I spotted this by looking at the code.
> I believe the only consequence is that the simplifications are never
> done in expand_builtin_sprintf.

I guess the reproducer would need to make the fmt string visible to
fold_builtin only during loop optimizations or later (certainly after fab
pass) and then expand_builtin_sprintf could do this optimization.

	Jakub

Re: Bug in expand_builtin_sprintf

[Kaveh R. GHAZI]

On Wed, 10 Oct 2007, Jakub Jelinek wrote:

> > > Testcase please?
> >
> > I don't have one, unfortunately. I spotted this by looking at the code.
> > I believe the only consequence is that the simplifications are never
> > done in expand_builtin_sprintf.
>
> I guess the reproducer would need to make the fmt string visible to
> fold_builtin only during loop optimizations or later (certainly after fab
> pass) and then expand_builtin_sprintf could do this optimization.
> 	Jakub

While you're at it, IMHO there's some unnecessary code duplication here.
We should have expand_builtin_sprintf call fold_builtin_sprintf instead of
having it's own copy of the transformation code.  (This wouldn't have
prevented the bug you found, because you still have to pull out the args
in the expand_ version before you call the fold_ one.  But still... it'll
get even worse as you add more opts.)

You have to be careful to make sure the code is actually the same.  If
either version does something different, figure out why and try to
reconcile them.  In the past I believe we've found cases where one did
more and/or different opts than the other and they needed to be merged.

See most of the the expand_builtin_str* functions for examples of what I
mean.  If you would please do this and queue it up for stage1, I would
much appreciate it.

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu