[Bug rtl-optimization/23369] New: [4.0.x regression] Generates wrong code for funcptr comparison

[Bug rtl-optimization/23369] New: [4.0.x regression] Generates wrong code for funcptr comparison

[tausq at debian dot org]

Compiling the attached testcase with gcc -O2 -S signals.c, the generated asm
contains:

        .loc 1 51 0
        bl __canonicalize_funcptr_for_compare,%r2
        ldo -2(%r7),%r26
        ldi -4,%r26
        bl __canonicalize_funcptr_for_compare,%r2
        copy %r28,%r4
        .loc 1 52 0
        .loc 1 51 0
        comb,>>= %r28,%r4,.L15
        ldi 0,%r28

"old" is assigned to %r7. Why is it subtracting 2 from it before passing it to
__cffc?

gcc-3.x does not have this problem.

This testcase was extracted from signals.c from glibc/linuxthreads, based on our
investigation of Debian BTS bug #321785

-- 
           Summary: [4.0.x regression] Generates wrong code for funcptr
                    comparison
           Product: gcc
           Version: 4.0.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: rtl-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: tausq at debian dot org
                CC: gcc-bugs at gcc dot gnu dot org
 GCC build triplet: hppa-unknown-linux
  GCC host triplet: hppa-unknown-linux
GCC target triplet: hppa-unknown-linux


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug rtl-optimization/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[tausq at debian dot org]

------- Additional Comments From tausq at debian dot org  2005-08-13 02:51 -------
Created an attachment (id=9487)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=9487&action=view)
Testcase

Compile with gcc -O2 -S

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug rtl-optimization/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[tausq at debian dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
      Known to fail|                            |4.0.1
      Known to work|                            |3.3.5


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug rtl-optimization/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[tausq at debian dot org]

------- Additional Comments From tausq at debian dot org  2005-08-13 03:08 -------
The line of code in question is:

     51           if (old == SIG_IGN || old == SIG_DFL || old == SIG_ERR)

after preprocessing, this becomes

          if (old == ((__sighandler_t) 1) || old == ((__sighandler_t) 0) || old
== ((__sighandler_t) -1))

It appears gcc optimized out the the old == 0 case, and it does the right thing
for the old == 1 case. For the old == -1 case, it seems to subtract 2 from old,
and converts -1 to -4. 

Weird.

This also happens with -O1. Dave says it looks like the initial rtl generation
is wrong, so maybe this is not an optimization bug.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug rtl-optimization/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[danglin at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |danglin at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug rtl-optimization/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[danglin at gcc dot gnu dot org]

------- Additional Comments From danglin at gcc dot gnu dot org  2005-08-13 03:30 -------
Actually, I believe that this is a tree optimization bug.  The comparison
of old - 2 against -4 appears in the initial rtl generation.

Could you also provide the preprocessed source?

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug rtl-optimization/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[tausq at debian dot org]

------- Additional Comments From tausq at debian dot org  2005-08-13 03:46 -------
Created an attachment (id=9488)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=9488&action=view)
Preprocessed source


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2005-08-13 06:44 -------
This is a middle-end bug rather than a rtl optimization issue.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|rtl-optimization            |middle-end


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2005-08-13 06:47 -------
I am starting to think canonicalize_funcptr_for_compare is just wrong for any target and shoud be fixed 
the ABI level and not in GCC.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
  GCC build triplet|hppa-unknown-linux          |
   GCC host triplet|hppa-unknown-linux          |
 GCC target triplet|hppa-unknown-linux          |hppa-*-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[danglin at gcc dot gnu dot org]

------- Additional Comments From danglin at gcc dot gnu dot org  2005-08-13 21:42 -------
Bah, while this is probably going to happen for hppa-linux, it'snever going to happen for hppa-hpux.  GCC's treatment of functionpointers here is just WRONG.There are a couple of issues.  The most serious is the following:We have the following code from __pthread_sigaction in the .i file:   if (old == ((__sighandler_t) 1) || old == ((__sighandler_t) 0) || old == ((__sighandler_t) -1))     __sighandler[sig].old = (arch_sighandler_t) act->__sigaction_handler.sa_handler;This is what the above turns into in the tree dump filebug321785.c.t02.original:          if ((void (*<T2bd>) (int)) old - 2B > -4B)            {              __sighandler[sig].old = (void (*<T431>) (int, struct sigcontext *)) act->__sigaction_handler.sa_handler;            }I don't believe that the transformation is valid.  Also, it not validC to do relational comparisons where one of the operands is a functionpointer.  See 6.5.8 (2).do_compare_and_jump doesn't check code before canonicalizing functionpointers.  I think we need an assert in do_compare_and_jump to catchinvalid function pointer comparisons.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[pinskia at physics dot uc dot edu]

------- Additional Comments From pinskia at physics dot uc dot edu  2005-08-13 21:44 -------
Subject: Re:  [4.0.x regression] Generates wrong code for funcptr comparison


On Aug 13, 2005, at 5:42 PM, danglin at gcc dot gnu dot org wrote:

>
> ------- Additional Comments From danglin at gcc dot gnu dot org  
> 2005-08-13 21:42 -------
> Bah, while this is probably going to happen for hppa-linux, it'snever 
> going to happen for hppa-hpux.  GCC's treatment of functionpointers 
> here is just WRONG.There are a couple of issues.  The most serious is 
> the following:We have the following code from __pthread_sigaction in 
> the .i file:   if (old == ((__sighandler_t) 1) || old == 
> ((__sighandler_t) 0) || old == ((__sighandler_t) -1))     
> __sighandler[sig].old = (arch_sighandler_t) 
> act->__sigaction_handler.sa_handler;This is what the above turns into 
> in the tree dump filebug321785.c.t02.original:          if ((void 
> (*<T2bd>) (int)) old - 2B > -4B)            {              
> __sighandler[sig].old = (void (*<T431>) (int, struct sigcontext *)) 
> act->__sigaction_handler.sa_handler;            }I don't believe that 
> the transformation is valid.


hmm, looks like another build_range_test bug.

-- Pinski



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

Re: [Bug middle-end/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[Andrew Pinski]

On Aug 13, 2005, at 5:42 PM, danglin at gcc dot gnu dot org wrote:

>
> ------- Additional Comments From danglin at gcc dot gnu dot org  
> 2005-08-13 21:42 -------
> Bah, while this is probably going to happen for hppa-linux, it'snever 
> going to happen for hppa-hpux.  GCC's treatment of functionpointers 
> here is just WRONG.There are a couple of issues.  The most serious is 
> the following:We have the following code from __pthread_sigaction in 
> the .i file:   if (old == ((__sighandler_t) 1) || old == 
> ((__sighandler_t) 0) || old == ((__sighandler_t) -1))     
> __sighandler[sig].old = (arch_sighandler_t) 
> act->__sigaction_handler.sa_handler;This is what the above turns into 
> in the tree dump filebug321785.c.t02.original:          if ((void 
> (*<T2bd>) (int)) old - 2B > -4B)            {              
> __sighandler[sig].old = (void (*<T431>) (int, struct sigcontext *)) 
> act->__sigaction_handler.sa_handler;            }I don't believe that 
> the transformation is valid.


hmm, looks like another build_range_test bug.

-- Pinski

[Bug middle-end/23369] [4.0.x regression] Generates wrong code for funcptr comparison

[dave at hiauly1 dot hia dot nrc dot ca]

------- Additional Comments From dave at hiauly1 dot hia dot nrc dot ca  2005-08-13 21:46 -------
Subject: Re:  [4.0.x regression] Generates wrong code for funcptr comparison

> Bah, while this is probably going to happen for hppa-linux, it'snever going to happen for hppa-hpux.  GCC's treatment of functionpointers here is just WRONG.There are a couple of issues.  The most serious is the following:We have the following code from __pthread_sigaction in the .i file:   if (old == ((__sighandler_t) 1) || old == ((__sighandler_t) 0) || old == ((__sighandler_t) -1))     __sighandler[sig].old = (arch_sighandler_t) act->__sigaction_handler.sa_handler;This is what the above turns into in the tree dump filebug321785.c.t02.original:          if ((void (*<T2bd>) (int)) old - 2B > -4B)            {              __sighandler[sig].old = (void (*<T431>) (int, struct sigcontext *)) act->__sigaction_handler.sa_handler;            }I don't believe that the transformation is valid.  Also, it not validC to do relational comparisons where one of the operands is a functionpointer.  See 6.5.8 (2).do_compare_and_jump doesn't check code before canonicalizing functionpointer!
 s.!
>    I think we need an assert in do_compare_and_jump to catchinvalid function pointer comparisons.

Yuck.  I used Konqueror to add this comment and it totally botched up
the white space ;(

Dave


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_test generates wrong code for funcptr comparison

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2005-08-13 21:52 -------
Confirmed,
A small testcase:
typedef void(*func_type) (void);

void g(void);

void f(func_type a)
{
  if (a == (func_type) 1 || a == (func_type)0 || a == (func_type) -1)
   g();
}


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|                            |1
      Known to fail|4.0.1                       |4.0.1 4.1.0
   Last reconfirmed|0000-00-00 00:00:00         |2005-08-13 21:52:19
               date|                            |
            Summary|[4.0.x regression] Generates|[4.0/4.1 regression]
                   |wrong code for funcptr      |build_range_test generates
                   |comparison                  |wrong code for funcptr
                   |                            |comparison
   Target Milestone|---                         |4.0.2


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_test generates wrong code for funcptr comparison

[dave at hiauly1 dot hia dot nrc dot ca]

------- Additional Comments From dave at hiauly1 dot hia dot nrc dot ca  2005-08-13 22:12 -------
Subject: Re:  [4.0/4.1 regression] build_range_test generates wrong code for funcptr comparison

>                    |wrong code for funcptr      |build_range_test generates
>                    |comparison                  |wrong code for funcptr
>                    |                            |comparison

build_range_test?

Dave


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2005-08-13 22:14 -------
(In reply to comment #11)
> build_range_test?

woops, I mean build_range_check.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[4.0/4.1 regression]        |[4.0/4.1 regression]
                   |build_range_test generates  |build_range_check generates
                   |wrong code for funcptr      |wrong code for funcptr
                   |comparison                  |comparison


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[tausq at debian dot org]

------- Additional Comments From tausq at debian dot org  2005-08-15 05:31 -------
Do I understand correctly that there are two distinct problems:

1) gcc should not be canonicalizing constants casted as function pointers

2) gcc should not generate relational comparisons against function pointers

?

it seems from my quick tests that #1 is not affected by build_range_test (i.e.
something else is wrong).

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[dave at hiauly1 dot hia dot nrc dot ca]

------- Additional Comments From dave at hiauly1 dot hia dot nrc dot ca  2005-08-15 13:46 -------
Subject: Re:  [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

> Do I understand correctly that there are two distinct problems:
> 
> 1) gcc should not be canonicalizing constants casted as function pointers

I think it has to.  GCC has no way of knowing whether the constant is
a "real" function pointer or not.

> 2) gcc should not generate relational comparisons against function pointers

Relational comparisons against function pointers are not allowed in
C.  However, what GCC does internally is a different matter as it knows
whether function pointers need to be canonicalized or not.

> it seems from my quick tests that #1 is not affected by build_range_test (i.e.
> something else is wrong).

I have a patch to build_range_check that fixes the problem in the PR.
I'll submit it this evening.

I do have a concern that as GCC's optimizations improve we seem to be
encountering more and more issues with respect to the handling of
function pointers.  The problem in the PR is latent in 3.3 and 3.4.
I'm not sure why it doesn't happen there.

Dave


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[randolph at tausq dot org]

------- Additional Comments From randolph at tausq dot org  2005-08-15 15:18 -------
Subject: Re:  [4.0/4.1 regression] build_range_check
 generates wrong code for funcptr comparison

>>1) gcc should not be canonicalizing constants casted as function pointers
> 
> I think it has to.  GCC has no way of knowing whether the constant is
> a "real" function pointer or not.

Doesn't this break __cffc completely? What if I really wanted to do "if 
(fptr == (func_t)-2)" or "if (fptr == (func_t)5000)?

randolph


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[dave at hiauly1 dot hia dot nrc dot ca]

------- Additional Comments From dave at hiauly1 dot hia dot nrc dot ca  2005-08-15 17:24 -------
Subject: Re:  [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

> Subject: Re:  [4.0/4.1 regression] build_range_check
>  generates wrong code for funcptr comparison
> 
> >>1) gcc should not be canonicalizing constants casted as function pointers
> > 
> > I think it has to.  GCC has no way of knowing whether the constant is
> > a "real" function pointer or not.
> 
> Doesn't this break __cffc completely? What if I really wanted to do "if 
> (fptr == (func_t)-2)" or "if (fptr == (func_t)5000)?

Why would you want to?  My opinion is that __cffc only has to deal
with the values traditionally used by "signal".  GCC doesn't know
whether (func_t)5000 needs to be canonicalized or not.  It will
happen anyway if (func_t)5000 is passed around.

The behavior of __cffc was more or less copied from the corresponding
HP routine.  __cffc expects to be passed a valid function pointer.
Currently, __cffc doesn't canonicalize small positive integers and -1
because they are used by signal and friends, and they can't be a valid
function pointer address.  I guess this might be extended somewhat but 
the point.  Nobody has defined what's ok.

When the plabel bit is set in the function pointer, __cffc has to
load data from the function descriptor and this can cause a segmentation
fault if the address isn't valid.  I don't think trying to avoid these
faults is worth the added complexity.  This will eventually go away when
Carlos' opd patch is installed.

I also think that the definitions of SIG_IGN, etc., are suspect.
These problems wouldn't occur if there were actual functions in the
C library for them.

There are also ways to avoid canonicalization in compares.  I recognize
that this problem is in generic code.  However, it might be possible
to replace the generic routine with a hppa specific routine that avoids
canonicalization when comparing a function pointer with a special value.
This would make the code more efficient.

Dave


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2005-08-16 02:21 -------
Subject: Bug 23369

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	danglin@gcc.gnu.org	2005-08-16 02:20:45

Modified files:
	gcc            : ChangeLog fold-const.c 

Log message:
	PR middle-end/23369
	* fold-const.c (build_range_check): Disable optimization for function
	pointer expressions on targets that require function pointer
	canonicalization.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.9739&r2=2.9740
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&r1=1.616&r2=1.617



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2005-08-16 03:21 -------
Subject: Bug 23369

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	danglin@gcc.gnu.org	2005-08-16 03:21:40

Modified files:
	gcc            : ChangeLog fold-const.c 

Log message:
	PR middle-end/23369
	* fold-const.c (build_range_check): Disable optimization for function
	pointer expressions on targets that require function pointer
	canonicalization.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.369&r2=2.7592.2.370
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.517.2.13&r2=1.517.2.14



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369

[Bug middle-end/23369] [4.0/4.1 regression] build_range_check generates wrong code for funcptr comparison

[danglin at gcc dot gnu dot org]

------- Additional Comments From danglin at gcc dot gnu dot org  2005-08-20 16:15 -------
Fixed in 4.0.2 and 4.1.0.


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23369