[Bug c++/55742] New: __attribute__ in class function declaration cause "prototype does not match" errors.

[Bug c++/55742] New: __attribute__ in class function declaration cause "prototype does not match" errors.

[dnovillo at gcc dot gnu.org]

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

             Bug #: 55742
           Summary: __attribute__ in class function declaration cause
                    "prototype does not match" errors.
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: dnovillo@gcc.gnu.org
                CC: tmsriram@google.com
              Host: x86_64-unknown-linux-gnu
            Target: x86_64-unknown-linux-gnu
             Build: x86_64-unknown-linux-gnu


The following code used to compile with GCC 4.7.  It fails on trunk with:

$ bld/xgcc -Bbld/ -c a.cc
a.cc:10:6: error: prototype for 'void A::E(uint64*, uint64*, const void*,
int64) const' does not match any in class 'A'
 void A::E(uint64 *l, uint64 *h, const void *b, int64 g) const
      ^
a.cc:6:18: error: candidate is: virtual void A::E(uint64*, uint64*, const
void*, int64) const
     virtual void E(uint64 *l, uint64 *h, const void *b, int64 g) const


typedef unsigned long long uint64;
typedef long long int64;

class A {
  public:
    virtual void E(uint64 *l, uint64 *h, const void *b, int64 g) const
        __attribute__ ((__target__ ("sse4")));
};

void A::E(uint64 *l, uint64 *h, const void *b, int64 g) const
{
    *l = *h + g;
    if (b) return;
}

This seems to be a bug in the multiversioning logic.  We fail to match the
function to its declaration in decl2.c:check_classfn because
ix86_function_versions returns true.

676
677               /* While finding a match, same types and params are not
enough
678                  if the function is versioned.  Also check version
("target")
679                  attributes.  */
680               if (same_type_p (TREE_TYPE (TREE_TYPE (function)),
681                                TREE_TYPE (TREE_TYPE (fndecl)))
682                   && compparms (p1, p2)
683                   && !targetm.target_option.function_versions (function,
fndecl)
684                   && (!is_template
685                       || comp_template_parms (template_parms,
686                                               DECL_TEMPLATE_PARMS
(fndecl)))
687                   && (DECL_TEMPLATE_SPECIALIZATION (function)
688                       == DECL_TEMPLATE_SPECIALIZATION (fndecl))
689                   && (!DECL_TEMPLATE_SPECIALIZATION (function)
690                       || (DECL_TI_TEMPLATE (function)
691                           == DECL_TI_TEMPLATE (fndecl))))
692                 break;

While this agrees with the logic of the multiversion test, it is not the
appropriate context to be checking for multiversions, I think.

Here we are comparing a function *declaration* with a function *definition*. 
The function definition can never have attributes (only declarations do).  So I
*think* the right fix here is to not call the multiversion hook.

Sri, could you take a look?  This bug is causing build failures with our
internal code base.


Thanks.

[Bug c++/55742] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

Sriraman Tallam <tmsriram at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |davidxl at google dot com

--- Comment #1 from Sriraman Tallam <tmsriram at google dot com> 2012-12-19 23:29:51 UTC ---
(In reply to comment #0)
> The following code used to compile with GCC 4.7.  It fails on trunk with:
> 
> $ bld/xgcc -Bbld/ -c a.cc
> a.cc:10:6: error: prototype for 'void A::E(uint64*, uint64*, const void*,
> int64) const' does not match any in class 'A'
>  void A::E(uint64 *l, uint64 *h, const void *b, int64 g) const
>       ^
> a.cc:6:18: error: candidate is: virtual void A::E(uint64*, uint64*, const
> void*, int64) const
>      virtual void E(uint64 *l, uint64 *h, const void *b, int64 g) const
> 
> 
> typedef unsigned long long uint64;
> typedef long long int64;
> 
> class A {
>   public:
>     virtual void E(uint64 *l, uint64 *h, const void *b, int64 g) const
>         __attribute__ ((__target__ ("sse4")));
> };
> 
> void A::E(uint64 *l, uint64 *h, const void *b, int64 g) const
> {
>     *l = *h + g;
>     if (b) return;
> }
> 
> This seems to be a bug in the multiversioning logic.  We fail to match the
> function to its declaration in decl2.c:check_classfn because
> ix86_function_versions returns true.
> 
> 676
> 677               /* While finding a match, same types and params are not
> enough
> 678                  if the function is versioned.  Also check version
> ("target")
> 679                  attributes.  */
> 680               if (same_type_p (TREE_TYPE (TREE_TYPE (function)),
> 681                                TREE_TYPE (TREE_TYPE (fndecl)))
> 682                   && compparms (p1, p2)
> 683                   && !targetm.target_option.function_versions (function,
> fndecl)
> 684                   && (!is_template
> 685                       || comp_template_parms (template_parms,
> 686                                               DECL_TEMPLATE_PARMS
> (fndecl)))
> 687                   && (DECL_TEMPLATE_SPECIALIZATION (function)
> 688                       == DECL_TEMPLATE_SPECIALIZATION (fndecl))
> 689                   && (!DECL_TEMPLATE_SPECIALIZATION (function)
> 690                       || (DECL_TI_TEMPLATE (function)
> 691                           == DECL_TI_TEMPLATE (fndecl))))
> 692                 break;
> 
> While this agrees with the logic of the multiversion test, it is not the
> appropriate context to be checking for multiversions, I think.
> 
> Here we are comparing a function *declaration* with a function *definition*. 
> The function definition can never have attributes (only declarations do).

AFAIU, this is not true. Definitions can have target attributes too.

So, your example can be fixed by adding __attribute__ ((__target__ ("sse4")))
to the front of the definition:

__attribute__ ((__target__ ("sse4")))
void A::E(uint64 *l, uint64 *h, const void *b, int64 g) const
{
    *l = *h + g;
    if (b) return;
}

will now make that code compile.


Infact, I think this is necessary. It is *not correct* to add the target
attribute only to the declaration.

Example:


test.cc
=======

char a[17];
char c[16];

int bar () __attribute__ ((target ("sse4.2")));

int bar ()
{
  char *b = a+1;
  fprintf (stderr, "Foo\n");

  for (int i = 0; i< 16; i++)
    c[i] = b[i];

  return 255;
}

Here, only the declaration has sse4.2, not the definition.

$ g++ test.cc -ftree-vectorize  -c -O2 -mno-sse

Now, test.o does not contain any sse4.2 instructions.

Add "__attribute__ ((target ("sse4.2")))" to the definition, and recompile with
same command. test.o will have movdqa and movdqu instructions. So, the target
attributes should also be on the definition for functionality to be correct.

With Multiversioning, this problem is exposed. I think the source change is the
correct way to go here.








  So I
> *think* the right fix here is to not call the multiversion hook.
> 
> Sri, could you take a look?  This bug is causing build failures with our
> internal code base.
> 
> 
> Thanks.

[Bug c++/55742] __attribute__ in class function declaration cause "prototype does not match" errors.

[dnovillo at gcc dot gnu.org]

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

Diego Novillo <dnovillo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org

--- Comment #2 from Diego Novillo <dnovillo at gcc dot gnu.org> 2012-12-20 17:59:47 UTC ---
After thinking about this more, I think the problem here is that the attributes
specified in the declaration of the function are not being used in the function
definition.

Jason, shouldn't the attributes specified in the function declaration be
sufficient?  Or does the user really need to apply attributes to both the
declaration and the definition?


Thanks.

[Bug c++/55742] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #3 from Sriraman Tallam <tmsriram at google dot com> 2012-12-20 18:21:26 UTC ---
(In reply to comment #2)
> After thinking about this more, I think the problem here is that the attributes
> specified in the declaration of the function are not being used in the function
> definition.
> 
> Jason, shouldn't the attributes specified in the function declaration be
> sufficient?  Or does the user really need to apply attributes to both the
> declaration and the definition?

This can be done during decl merging, by adding the DECL_TARGET_SPECIFIC tree
of the declaration decl to the definition decl.

However, with function multiversioning, this will become a problem as
multiversioning does not treat two decls with different target attributes as
identical. Since we are enabling multiversioning by default, atleast in C++
front-end for now, IMO, it is better to insist that the definition and
declaration contain identical target attributes.


> 
> 
> Thanks.

[Bug c++/55742] __attribute__ in class function declaration cause "prototype does not match" errors.

[dnovillo at google dot com]

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

--- Comment #4 from dnovillo at google dot com <dnovillo at google dot com> 2012-12-20 18:23:55 UTC ---
On Thu, Dec 20, 2012 at 1:21 PM, tmsriram at google dot com
<gcc-bugzilla@gcc.gnu.org> wrote:

> However, with function multiversioning, this will become a problem as
> multiversioning does not treat two decls with different target attributes
> as
> identical. Since we are enabling multiversioning by default, atleast in
> C++
> front-end for now, IMO, it is better to insist that the definition and
> declaration contain identical target attributes.

Unfortunately, we cannot do that.  A lot of existing code relies on
this attribute merging.  The cleanest approach here is probably to add
an additional 'mv' attribute to explicitly enable multiversioning.
Breaking the existing semantics is going to break a lot of code.


Diego.

[Bug c++/55742] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #5 from Sriraman Tallam <tmsriram at google dot com> 2012-12-20 19:36:53 UTC ---
(In reply to comment #4)
> On Thu, Dec 20, 2012 at 1:21 PM, tmsriram at google dot com
> <gcc-bugzilla@gcc.gnu.org> wrote:
> 
> > However, with function multiversioning, this will become a problem as
> > multiversioning does not treat two decls with different target attributes
> > as
> > identical. Since we are enabling multiversioning by default, atleast in
> > C++
> > front-end for now, IMO, it is better to insist that the definition and
> > declaration contain identical target attributes.
> 
> Unfortunately, we cannot do that.  A lot of existing code relies on
> this attribute merging.  The cleanest approach here is probably to add
> an additional 'mv' attribute to explicitly enable multiversioning.
> Breaking the existing semantics is going to break a lot of code.

Ok, just to be clear, there are two problems here:

1) Target attribute merging. If the assumption that the target attributes of
the decls must be merged is valid, there is a bug. Also, this means that using
target attributes to do multiversioning is wrong.

2) Function multiversioning is exposing the bug, via build failures, the
problem of declarations and definitions not having identical target attributes.


First, we need to decide if target attribute merging is a valid assumption. If
so, we fix the bug and make function multiversioning use a new attribute.

If the assumption is not valid, changing the source is the only solution. The
source is invalid now since the intended behaviour is not happening.


> 
> 
> Diego.

[Bug c++/55742] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

davidxl <xinliangli at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xinliangli at gmail dot com

--- Comment #6 from davidxl <xinliangli at gmail dot com> 2012-12-20 19:52:32 UTC ---
Since target attribute merging/conflicts handling is not well specified nor
implemented, with the introduction of MV, it is a good opportunity to
straighten it out and document it.

MV helps providing diagnostics to detect the broken code (where the target
attribute is ignored) :)

David


(In reply to comment #5)
> (In reply to comment #4)
> > On Thu, Dec 20, 2012 at 1:21 PM, tmsriram at google dot com
> > <gcc-bugzilla@gcc.gnu.org> wrote:
> > 
> > > However, with function multiversioning, this will become a problem as
> > > multiversioning does not treat two decls with different target attributes
> > > as
> > > identical. Since we are enabling multiversioning by default, atleast in
> > > C++
> > > front-end for now, IMO, it is better to insist that the definition and
> > > declaration contain identical target attributes.
> > 
> > Unfortunately, we cannot do that.  A lot of existing code relies on
> > this attribute merging.  The cleanest approach here is probably to add
> > an additional 'mv' attribute to explicitly enable multiversioning.
> > Breaking the existing semantics is going to break a lot of code.
> 
> Ok, just to be clear, there are two problems here:
> 
> 1) Target attribute merging. If the assumption that the target attributes of
> the decls must be merged is valid, there is a bug. Also, this means that using
> target attributes to do multiversioning is wrong.
> 
> 2) Function multiversioning is exposing the bug, via build failures, the
> problem of declarations and definitions not having identical target attributes.
> 
> 
> First, we need to decide if target attribute merging is a valid assumption. If
> so, we fix the bug and make function multiversioning use a new attribute.
> 
> If the assumption is not valid, changing the source is the only solution. The
> source is invalid now since the intended behaviour is not happening.
> 
> 
> > 
> > 
> > Diego.

[Bug c++/55742] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

Sriraman Tallam <tmsriram at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |richard.guenther at gmail
                   |                            |dot com

--- Comment #7 from Sriraman Tallam <tmsriram at google dot com> 2013-01-10 22:10:14 UTC ---
Jason/Richard, Would like to hear your thoughts on this.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-01-14
            Summary|__attribute__ in class      |[4.8 regression]
                   |function declaration cause  |__attribute__ in class
                   |"prototype does not match"  |function declaration cause
                   |errors.                     |"prototype does not match"
                   |                            |errors.
     Ever Confirmed|0                           |1

--- Comment #8 from Jason Merrill <jason at gcc dot gnu.org> 2013-01-14 14:33:21 UTC ---
As I was saying on IRC, this is a change of behavior of the target attribute
relative to 4.7, and therefore needs to be fixed for 4.8.  The 4.7 behavior for
this testcase must be restored, and MV needs to come up with a way of
distinguishing between a redeclaration that omits optional attributes (as in
this testcase) and an overload between two different versions.

Perhaps use something like __attribute ((target ("any")) for the default
version so that it's clear that a declaration with no target attribute is a
redeclaration (which is ambiguous if there are already multiple versions).

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
   Target Milestone|---                         |4.8.0

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-14 14:38:04 UTC ---
I'd say once a target attribute appears at a declaration (non-definition) MV
needs to be disabled.  Or, the declarations target attribute and those at
the MV definitions need to be merged.

Thus, a declarations target attribute applies to all definitions (thus all
MV variants).

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-14 15:13:47 UTC ---
Either use a different name of the attribute (replace target with mv_target or
whatever), or require a new attribute (mv?) to be present for multi-versioning
(mv attribute on any of the decls would enable it, if mv attribute isn't
present on either of the two decls being merged, then the target attribute is
merged as before 4.8).

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #11 from davidxl <xinliangli at gmail dot com> 2013-01-14 17:17:21 UTC ---
(In reply to comment #9)
> I'd say once a target attribute appears at a declaration (non-definition) MV
> needs to be disabled.  Or, the declarations target attribute and those at
> the MV definitions need to be merged.
> 
> Thus, a declarations target attribute applies to all definitions (thus all
> MV variants).

This might reduce the usefulness of MV -- using declarations with attributes to
indicate MV candidates is the intended use model.

David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #12 from davidxl <xinliangli at gmail dot com> 2013-01-14 17:20:56 UTC ---
(In reply to comment #10)
> Either use a different name of the attribute (replace target with mv_target or
> whatever), or require a new attribute (mv?) to be present for multi-versioning
> (mv attribute on any of the decls would enable it, if mv attribute isn't
> present on either of the two decls being merged, then the target attribute is
> merged as before 4.8).


I like this proposal:

>require a new attribute (mv?) to be present for multi-versioning
> (mv attribute on any of the decls would enable it, if mv attribute isn't
> present on either of the two decls being merged, then the target attribute is
> merged as before 4.8)


David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #13 from Sriraman Tallam <tmsriram at google dot com> 2013-01-14 17:45:42 UTC ---
(In reply to comment #12)
> (In reply to comment #10)
> > Either use a different name of the attribute (replace target with mv_target or
> > whatever), or require a new attribute (mv?) to be present for multi-versioning
> > (mv attribute on any of the decls would enable it, if mv attribute isn't
> > present on either of the two decls being merged, then the target attribute is
> > merged as before 4.8).
> 
> 
> I like this proposal:

I too like just using a different attribute name. I will prepare a patch asap
for this.

Thanks
Sri.

> 
> >require a new attribute (mv?) to be present for multi-versioning
> > (mv attribute on any of the decls would enable it, if mv attribute isn't
> > present on either of the two decls being merged, then the target attribute is
> > merged as before 4.8)
> 
> 
> David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #14 from davidxl <xinliangli at gmail dot com> 2013-01-14 17:49:59 UTC ---
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > Either use a different name of the attribute (replace target with mv_target or
> > > whatever), or require a new attribute (mv?) to be present for multi-versioning
> > > (mv attribute on any of the decls would enable it, if mv attribute isn't
> > > present on either of the two decls being merged, then the target attribute is
> > > merged as before 4.8).
> > 
> > 
> > I like this proposal:
> 
> I too like just using a different attribute name. I will prepare a patch asap
> for this.
> 
> Thanks
> Sri.
> 
> > 
> > >require a new attribute (mv?) to be present for multi-versioning
> > > (mv attribute on any of the decls would enable it, if mv attribute isn't
> > > present on either of the two decls being merged, then the target attribute is
> > > merged as before 4.8)
> > 
> > 
> > David


I mean Jacub's second alternative -- adding additional attribute that alters
the meaning of 'target' attribute -- when it is present, no merging will be
done.


David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #15 from Sriraman Tallam <tmsriram at google dot com> 2013-01-14 18:07:28 UTC ---
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #10)
> > > > Either use a different name of the attribute (replace target with mv_target or
> > > > whatever), or require a new attribute (mv?) to be present for multi-versioning
> > > > (mv attribute on any of the decls would enable it, if mv attribute isn't
> > > > present on either of the two decls being merged, then the target attribute is
> > > > merged as before 4.8).
> > > 
> > > 
> > > I like this proposal:
> > 
> > I too like just using a different attribute name. I will prepare a patch asap
> > for this.
> > 
> > Thanks
> > Sri.
> > 
> > > 
> > > >require a new attribute (mv?) to be present for multi-versioning
> > > > (mv attribute on any of the decls would enable it, if mv attribute isn't
> > > > present on either of the two decls being merged, then the target attribute is
> > > > merged as before 4.8)
> > > 
> > > 
> > > David
> 
> 
> I mean Jacub's second alternative -- adding additional attribute that alters
> the meaning of 'target' attribute -- when it is present, no merging will be
> done.


Ok, so the two options are :

1) int foo __attribute__ ((mv_target ("sse4.2")));
2) int foo __attribute__ ((target("sse4.2")), mv);

I dont have  a strong preference. 

> 
> 
> David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-14 18:25:41 UTC ---
I guess if for multiversioning you want the two decls to be independent, like
overloaded functions with different argument types are, then IMHO the mv
attribute alternative and not merging anything at all in that case (neither the
decls, nor any of the attributes (not just target attribute) is better.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-14 18:32:44 UTC ---
Actually, what you'd merge is everything as usually if mv attribute isn't on
either of the decls, or if mv attribute is present on either one, and both
decls have either the same target attribute, or no target attribute at all.  If
mv attribute is on newdecl or olddecl, and target attribute is either present
on just one of the decls, or on both, but with different argument, it would
treat them as decls that mangle differently and won't be merged.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #18 from davidxl <xinliangli at gmail dot com> 2013-01-14 20:17:45 UTC ---
All target attributes on decls not tagged with 'mv' attribute should be merged
into the default definition. Any decl tagged with 'mv' should be treated as a
new decl. 

David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-14 20:23:54 UTC ---
That wouldn't work, because you would then have the default (non-mv) version,
possibly mv version with no target attribute, and then some other mv versions
with target attributes.  The problem with that is that the first two mangle the
same.  This means that a non-mv and mv with no target attribute needs to be
treated as the same decl (i.e. merged together).

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #20 from davidxl <xinliangli at gmail dot com> 2013-01-14 20:29:54 UTC ---
(In reply to comment #19)
> That wouldn't work, because you would then have the default (non-mv) version,
> possibly mv version with no target attribute, and then some other mv versions
> with target attributes.  The problem with that is that the first two mangle the
> same.  This means that a non-mv and mv with no target attribute needs to be
> treated as the same decl (i.e. merged together).

that makes sense -- mv annotated decl without target attribute gets merged with
default version -- basically it has no actual effect.

David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jason at gcc dot gnu.org]

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

--- Comment #21 from Jason Merrill <jason at gcc dot gnu.org> 2013-01-15 18:45:18 UTC ---
What does it mean to merge two declarations with different target attributes? 
Are the strings just combined?

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #22 from davidxl <xinliangli at gmail dot com> 2013-01-15 19:03:03 UTC ---
(In reply to comment #21)
> What does it mean to merge two declarations with different target attributes? 
> Are the strings just combined?

This is a good question.  'merge' needs to be clearly defined and can be
implementation defined. There might be conflicts between explicit target
strings) and the compiler can choose to pick one and discard others (with a
proper warning).

David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #23 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-16 08:11:42 UTC ---
Merging of target attribute is what gcc/g++ did though, the function would get
then both target attributes (seems later decl's target wins), and the first
target attribute in DECL_ATTRIBUTES would be the one to be used.  Perhaps we
can add a -Wattributes warning for that case if mv attribute isn't present and
are merging target attributes with different strings.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jason at gcc dot gnu.org]

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

--- Comment #24 from Jason Merrill <jason at gcc dot gnu.org> 2013-01-16 15:53:28 UTC ---
(In reply to comment #23)
> Merging of target attribute is what gcc/g++ did though, the function would get
> then both target attributes (seems later decl's target wins), and the first
> target attribute in DECL_ATTRIBUTES would be the one to be used.

This seems like broken behavior that we don't need to preserve, in which case
my suggestion in comment #8 could work.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #25 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-16 16:02:35 UTC ---
The actual merging of target attribute isn't that important, what would be more
important is that other attributes are merged together in that case and the
decls treated as the same thing.

Anyway, with target("any") attribute, what would happen for
void foo () __attribute__((target ("avx")));
void foo () __attribute__((target ("any")));
void foo () {}
Is the definition "any", something else?

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[richard.guenther at gmail dot com]

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

--- Comment #26 from richard.guenther at gmail dot com <richard.guenther at gmail dot com> 2013-01-16 16:05:01 UTC ---
On Wed, Jan 16, 2013 at 5:02 PM, jakub at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55742
>
> --- Comment #25 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-16 16:02:35 UTC ---
> The actual merging of target attribute isn't that important, what would be more
> important is that other attributes are merged together in that case and the
> decls treated as the same thing.
>
> Anyway, with target("any") attribute, what would happen for
> void foo () __attribute__((target ("avx")));
> void foo () __attribute__((target ("any")));

IMHO the re-declaration with a different target attribute should be an error.
A proper "forward" declaration for a function with MV applied shouldn't have
any target attribute.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #27 from Sriraman Tallam <tmsriram at google dot com> 2013-01-16 17:20:28 UTC ---
(In reply to comment #26)
> On Wed, Jan 16, 2013 at 5:02 PM, jakub at gcc dot gnu.org
> <gcc-bugzilla@gcc.gnu.org> wrote:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55742
> >
> > --- Comment #25 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-16 16:02:35 UTC ---
> > The actual merging of target attribute isn't that important, what would be more
> > important is that other attributes are merged together in that case and the
> > decls treated as the same thing.
> >
> > Anyway, with target("any") attribute, what would happen for
> > void foo () __attribute__((target ("avx")));
> > void foo () __attribute__((target ("any")));
> 
> IMHO the re-declaration with a different target attribute should be an error.
> A proper "forward" declaration for a function with MV applied shouldn't have
> any target attribute.

Richard, I am not sure I fully understand what you mean. In this example, with
your approach:

test.cc
------

int foo (); // forward declaration

int 
main ()
{
  foo ();
}

int foo ()
{
  ...
}
int foo ()  __attribute__ ("sse2")
{
 ....
}

How can you tell if the call to foo is multi-versioned or not?

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #28 from Sriraman Tallam <tmsriram at google dot com> 2013-01-16 17:25:21 UTC ---
(In reply to comment #25)
> The actual merging of target attribute isn't that important, what would be more
> important is that other attributes are merged together in that case and the
> decls treated as the same thing.
> 
> Anyway, with target("any") attribute, what would happen for
> void foo () __attribute__((target ("avx")));
> void foo () __attribute__((target ("any")));
> void foo () {}
> Is the definition "any", something else?

Further, if we have these three declarations in this order:

void foo () __attribute__((target ("avx")));
void foo () __attribute__((target ("sse4.2")));
void foo () __attribute__((target ("any")));

This seems to mean that we want foo to be multi-versioned. However, when the
front-end is processing the second declaration, how would it decide between
merging or not without seeing the third? IMHO, I think each declaration should
be self-contained like Jakub proposed,  and by just looking at the declaration
we should be able to tell if the target attribute affects the signature or not.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jason at gcc dot gnu.org]

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

--- Comment #29 from Jason Merrill <jason at gcc dot gnu.org> 2013-01-16 20:03:42 UTC ---
(In reply to comment #25)
> Anyway, with target("any") attribute, what would happen for
> void foo () __attribute__((target ("avx")));
> void foo () __attribute__((target ("any")));
> void foo () {}
> Is the definition "any", something else?

The definition is an error, because it's ambiguous which version it's defining.

(In reply to comment #28)
> Further, if we have these three declarations in this order:
> 
> void foo () __attribute__((target ("avx")));
> void foo () __attribute__((target ("sse4.2")));
> void foo () __attribute__((target ("any")));
> 
> This seems to mean that we want foo to be multi-versioned. However, when the
> front-end is processing the second declaration, how would it decide between
> merging or not without seeing the third?

It would always declare a separate version.  4.7 doesn't actually merge target
attributes, a later declaration just replaces the earlier target attribute;
this seems like useless behavior to me that could be replaced with
multiversioning semantics.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #30 from davidxl <xinliangli at gmail dot com> 2013-01-17 22:45:22 UTC ---
(In reply to comment #26)
> On Wed, Jan 16, 2013 at 5:02 PM, jakub at gcc dot gnu.org
> <gcc-bugzilla@gcc.gnu.org> wrote:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55742
> >
> > --- Comment #25 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-16 16:02:35 UTC ---
> > The actual merging of target attribute isn't that important, what would be more
> > important is that other attributes are merged together in that case and the
> > decls treated as the same thing.
> >
> > Anyway, with target("any") attribute, what would happen for
> > void foo () __attribute__((target ("avx")));
> > void foo () __attribute__((target ("any")));
> 
> IMHO the re-declaration with a different target attribute should be an error.

This can be a clean way to handle declarations. The definition should have
either 'default' attribute or a matching target attribute.

> A proper "forward" declaration for a function with MV applied shouldn't have
> any target attribute.

What does this sentence mean?


David

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[richard.guenther at gmail dot com]

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

--- Comment #31 from richard.guenther at gmail dot com <richard.guenther at gmail dot com> 2013-01-18 09:49:11 UTC ---
On Thu, Jan 17, 2013 at 11:45 PM, xinliangli at gmail dot com
<gcc-bugzilla@gcc.gnu.org> wrote:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55742
>
> --- Comment #30 from davidxl <xinliangli at gmail dot com> 2013-01-17 22:45:22 UTC ---
> (In reply to comment #26)
>> On Wed, Jan 16, 2013 at 5:02 PM, jakub at gcc dot gnu.org
>> <gcc-bugzilla@gcc.gnu.org> wrote:
>> >
>> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55742
>> >
>> > --- Comment #25 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-16 16:02:35 UTC ---
>> > The actual merging of target attribute isn't that important, what would be more
>> > important is that other attributes are merged together in that case and the
>> > decls treated as the same thing.
>> >
>> > Anyway, with target("any") attribute, what would happen for
>> > void foo () __attribute__((target ("avx")));
>> > void foo () __attribute__((target ("any")));
>>
>> IMHO the re-declaration with a different target attribute should be an error.
>
> This can be a clean way to handle declarations. The definition should have
> either 'default' attribute or a matching target attribute.
>
>> A proper "forward" declaration for a function with MV applied shouldn't have
>> any target attribute.
>
> What does this sentence mean?

I think it would require to list all MV variants in the single declaration

void foo () __attribute__ ((target ("avx"), target ("any")));

but I suppose this goes against the idea of treating MV variants as regular
overloads which you can all declare.

Of course the issue whether / how to "merge" target attributes then returns.
I didn't pay too close attention, but if you settled on using a different
attribute name and then never perform merging of that attribute that would
indeed work.

Richard.

>
> David
>
> --
> Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #32 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-18 14:18:58 UTC ---
Created attachment 29207
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29207
gcc48-pr55742.patch

This bug is open for way too long given its severity, so let's start talking
over patches.

This patch attempts to implement what I understand from Jason's comments, just
with "default" instead of "any", because it is indeed the default target
attribute (whatever you specify on the command line).

Say on:
void foo ();
void foo () __attribute__((target ("avx")));
void foo () __attribute__((target ("default")));
__attribute__((target ("default"))) void foo ()
{
}
__attribute__((target ("avx"))) void foo ()
{
}
void (*fn) () = foo;

first we merge the first two decls, because only if target attribute is present
on both, we consider it for multi-versioning, for compatibility with 4.7 and
older.  On e.g.
void foo ();
void foo () __attribute__((target ("sse4")));
void foo () __attribute__((target ("default")));
void foo ()
{
}
we reject the last fn definition, because at that point foo is already known to
be multi-versioned, thus it is required that target attribute is specified for
foo (either "default", or some other).  Unfortunately, for this case the error
is reported twice for some reason.

The #c0 testcase now compiles.

Now, the issues I discovered with multiversioning, still unfixed by the patch:
1) the mv*.C testcases should be moved, probably to g++.dg/ext/mv*.C
2) can you please explain the mess in handle_target_attribute?
  /* Do not strip invalid target attributes for targets which support function
     multiversioning as the target string is used to determine versioned
     functions.  */
  else if (! targetm.target_option.valid_attribute_p (*node, name, args,
                                                      flags)
           && ! targetm.target_option.supports_function_versions ())
    *no_add_attrs = true;
Why do you need that?  Consider complete garbage in target attribute arguments,
which is errored about, but the above for i386/x86_64 keeps the target
attribute around anyway, leading to lots of ICEs everywhere:
Consider e.g.:
__attribute__((target ("default"))) void foo (void)
{
}
__attribute__((target (128))) void foo (void)
{
}
3) the multiversioning code assumes that target has a single argument, but it
can have more than one.  Say for:
__attribute__((target ("avx,popcnt"))) void foo (void)
{
}
__attribute__((target ("popcnt","avx"))) void bar (void)
{
}
the compiler handles those two as equivalent, but with -Dbar=foo
multi-versioning only considers the first string out of that.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jason at gcc dot gnu.org]

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

--- Comment #33 from Jason Merrill <jason at gcc dot gnu.org> 2013-01-18 16:59:20 UTC ---
(In reply to comment #32)

That sounds good, thanks.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[xinliangli at gmail dot com]

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

--- Comment #34 from davidxl <xinliangli at gmail dot com> 2013-01-18 17:27:43 UTC ---

The patch is missing changes in documentation on the new attribute.

David


(In reply to comment #32)
> Created attachment 29207 [details]
> gcc48-pr55742.patch
> 
> This bug is open for way too long given its severity, so let's start talking
> over patches.
> 
> This patch attempts to implement what I understand from Jason's comments, just
> with "default" instead of "any", because it is indeed the default target
> attribute (whatever you specify on the command line).
> 
> Say on:
> void foo ();
> void foo () __attribute__((target ("avx")));
> void foo () __attribute__((target ("default")));
> __attribute__((target ("default"))) void foo ()
> {
> }
> __attribute__((target ("avx"))) void foo ()
> {
> }
> void (*fn) () = foo;
> 
> first we merge the first two decls, because only if target attribute is present
> on both, we consider it for multi-versioning, for compatibility with 4.7 and
> older.  On e.g.
> void foo ();
> void foo () __attribute__((target ("sse4")));
> void foo () __attribute__((target ("default")));
> void foo ()
> {
> }
> we reject the last fn definition, because at that point foo is already known to
> be multi-versioned, thus it is required that target attribute is specified for
> foo (either "default", or some other).  Unfortunately, for this case the error
> is reported twice for some reason.
> 
> The #c0 testcase now compiles.
> 
> Now, the issues I discovered with multiversioning, still unfixed by the patch:
> 1) the mv*.C testcases should be moved, probably to g++.dg/ext/mv*.C
> 2) can you please explain the mess in handle_target_attribute?
>   /* Do not strip invalid target attributes for targets which support function
>      multiversioning as the target string is used to determine versioned
>      functions.  */
>   else if (! targetm.target_option.valid_attribute_p (*node, name, args,
>                                                       flags)
>            && ! targetm.target_option.supports_function_versions ())
>     *no_add_attrs = true;
> Why do you need that?  Consider complete garbage in target attribute arguments,
> which is errored about, but the above for i386/x86_64 keeps the target
> attribute around anyway, leading to lots of ICEs everywhere:
> Consider e.g.:
> __attribute__((target ("default"))) void foo (void)
> {
> }
> __attribute__((target (128))) void foo (void)
> {
> }
> 3) the multiversioning code assumes that target has a single argument, but it
> can have more than one.  Say for:
> __attribute__((target ("avx,popcnt"))) void foo (void)
> {
> }
> __attribute__((target ("popcnt","avx"))) void bar (void)
> {
> }
> the compiler handles those two as equivalent, but with -Dbar=foo
> multi-versioning only considers the first string out of that.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #35 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-18 17:51:42 UTC ---
Created attachment 29211
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29211
gcc48-pr55742.patch

Updated patch with ChangeLog entry and code to prevent issuing errors for the
same bug multiple times.

As for documentation, the multiversioning was checked in without proper
documentation, so there is nothing to adjust in documentation, the feature
simply needs documentation written.

1), 2) and 3) are unsolved by the patch, similarly extensive test coverage (the
current one is insufficient).  Perhaps that can be solved incrementally?
I'm going to bootstrap/regtest this version now.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #36 from Sriraman Tallam <tmsriram at google dot com> 2013-01-18 18:03:21 UTC ---
(In reply to comment #32)
> Created attachment 29207 [details]
> gcc48-pr55742.patch
> 
> This bug is open for way too long given its severity, so let's start talking
> over patches.
> 
> This patch attempts to implement what I understand from Jason's comments, just
> with "default" instead of "any", because it is indeed the default target
> attribute (whatever you specify on the command line).
> 
> Say on:
> void foo ();
> void foo () __attribute__((target ("avx")));
> void foo () __attribute__((target ("default")));
> __attribute__((target ("default"))) void foo ()
> {
> }
> __attribute__((target ("avx"))) void foo ()
> {
> }
> void (*fn) () = foo;
> 
> first we merge the first two decls, because only if target attribute is present
> on both, we consider it for multi-versioning, for compatibility with 4.7 and
> older.  On e.g.
> void foo ();
> void foo () __attribute__((target ("sse4")));
> void foo () __attribute__((target ("default")));
> void foo ()
> {
> }
> we reject the last fn definition, because at that point foo is already known to
> be multi-versioned, thus it is required that target attribute is specified for
> foo (either "default", or some other).  Unfortunately, for this case the error
> is reported twice for some reason.
> 
> The #c0 testcase now compiles.
> 
> Now, the issues I discovered with multiversioning, still unfixed by the patch:
> 1) the mv*.C testcases should be moved, probably to g++.dg/ext/mv*.C
> 2) can you please explain the mess in handle_target_attribute?
>   /* Do not strip invalid target attributes for targets which support function
>      multiversioning as the target string is used to determine versioned
>      functions.  */
>   else if (! targetm.target_option.valid_attribute_p (*node, name, args,
>                                                       flags)
>            && ! targetm.target_option.supports_function_versions ())
>     *no_add_attrs = true;
> Why do you need that?  

This was added because previously if I had two declarations of foo like this:

void foo ();
void foo __target__(("sse4.2")));

int main ()
{
  foo ();
}

void foo ()
{
}

__target__(("sse4.2")));
void foo ()
{
}


The call to foo in main will be treated like 2 different versions of foo exist.

However with -msse4.2 on the command-line, the target attribute will be
stripped off the second declaration which makes foo no longer multi-versioned
when the call to foo is processed. The call to foo without -msse4.2 is
multi-versioned and with -msse4.2 is not. I wanted to avoid this behaviour.


Consider complete garbage in target attribute arguments,
> which is errored about, but the above for i386/x86_64 keeps the target
> attribute around anyway, leading to lots of ICEs everywhere:
> Consider e.g.:
> __attribute__((target ("default"))) void foo (void)
> {
> }
> __attribute__((target (128))) void foo (void)
> {
> }
> 3) the multiversioning code assumes that target has a single argument, but it
> can have more than one.  Say for:
> __attribute__((target ("avx,popcnt"))) void foo (void)
> {
> }
> __attribute__((target ("popcnt","avx"))) void bar (void)
> {
> }
> the compiler handles those two as equivalent, but with -Dbar=foo
> multi-versioning only considers the first string out of that.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #37 from Sriraman Tallam <tmsriram at google dot com> 2013-01-18 18:07:08 UTC ---
(In reply to comment #35)
> Created attachment 29211 [details]
> gcc48-pr55742.patch
> 
> Updated patch with ChangeLog entry and code to prevent issuing errors for the
> same bug multiple times.
> 
> As for documentation, the multiversioning was checked in without proper
> documentation, so there is nothing to adjust in documentation, the feature
> simply needs documentation written.
> 
> 1), 2) and 3) are unsolved by the patch, similarly extensive test coverage (the
> current one is insufficient).  Perhaps that can be solved incrementally?
> I'm going to bootstrap/regtest this version now.


Thanks for the patch. Sorry I could not produce a patch earlier. I am working
on addressing 1,2 and 3 and will provide more test coverage over the next
couple of days.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #38 from Sriraman Tallam <tmsriram at google dot com> 2013-01-18 19:53:16 UTC ---
(In reply to comment #32)
> Created attachment 29207 [details]
> gcc48-pr55742.patch
> 
> This bug is open for way too long given its severity, so let's start talking
> over patches.
> 
> This patch attempts to implement what I understand from Jason's comments, just
> with "default" instead of "any", because it is indeed the default target
> attribute (whatever you specify on the command line).
> 
> Say on:
> void foo ();
> void foo () __attribute__((target ("avx")));
> void foo () __attribute__((target ("default")));
> __attribute__((target ("default"))) void foo ()
> {
> }
> __attribute__((target ("avx"))) void foo ()
> {
> }
> void (*fn) () = foo;
> 
> first we merge the first two decls, because only if target attribute is present
> on both, we consider it for multi-versioning, for compatibility with 4.7 and
> older.  On e.g.
> void foo ();
> void foo () __attribute__((target ("sse4")));
> void foo () __attribute__((target ("default")));
> void foo ()
> {
> }
> we reject the last fn definition, because at that point foo is already known to
> be multi-versioned, thus it is required that target attribute is specified for
> foo (either "default", or some other).  Unfortunately, for this case the error
> is reported twice for some reason.
> 
> The #c0 testcase now compiles.
> 
> Now, the issues I discovered with multiversioning, still unfixed by the patch:
> 1) the mv*.C testcases should be moved, probably to g++.dg/ext/mv*.C
> 2) can you please explain the mess in handle_target_attribute?
>   /* Do not strip invalid target attributes for targets which support function
>      multiversioning as the target string is used to determine versioned
>      functions.  */
>   else if (! targetm.target_option.valid_attribute_p (*node, name, args,
>                                                       flags)
>            && ! targetm.target_option.supports_function_versions ())
>     *no_add_attrs = true;
> Why do you need that?  Consider complete garbage in target attribute arguments,
> which is errored about, but the above for i386/x86_64 keeps the target
> attribute around anyway, leading to lots of ICEs everywhere:

Without bringing in your patch, I removed this line with patch:


--- gcc/c-family/c-common.c    (revision 195302)
+++ gcc/c-family/c-common.c    (working copy)
@@ -8763,8 +8763,7 @@
      multiversioning as the target string is used to determine versioned
      functions.  */
   else if (! targetm.target_option.valid_attribute_p (*node, name, args,
-                              flags)
-       && ! targetm.target_option.supports_function_versions ())
+                              flags))
     *no_add_attrs = true;

   return NULL_TREE;

and then tried the new compiler on the following example:

int foo ();
int foo () __attribute__ ((target("mmx")));

int main ()
{
  return foo ();
}

int
foo ()
{
  return 0;
}

int __attribute__ ((target("mmx")))
foo ()
{
  return 0;
}

and with -mno-mmx added to the compile options, everything is fine. However,
with -mmmx in the compile options, I get:

fe_example.cc: In function ‘int foo()’:
fe_example.cc:16:1: error: redefinition of ‘int foo()’
 foo ()
 ^
fe_example.cc:10:1: error: ‘int foo()’ previously defined here
 foo ()

Reason is the stripping of target attributes that do not make sense. But, for
MV that creates duplicate functions. 

I can change this to only keep the attribute tagged it is recognized by the
target. That way I will strip out erroneous values for target attribute.



> Consider e.g.:
> __attribute__((target ("default"))) void foo (void)
> {
> }
> __attribute__((target (128))) void foo (void)
> {
> }
> 3) the multiversioning code assumes that target has a single argument, but it
> can have more than one.  Say for:
> __attribute__((target ("avx,popcnt"))) void foo (void)
> {
> }
> __attribute__((target ("popcnt","avx"))) void bar (void)
> {
> }
> the compiler handles those two as equivalent, but with -Dbar=foo
> multi-versioning only considers the first string out of that.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #39 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-19 10:14:04 UTC ---
Then to fix that perhaps we want to change ix86_valid_target_attribute_tree and
its caller.
Currently ix86_valid_target_attribute_tree returns NULL_TREE both when the
target string(s) are invalid (i.e. when ix86_valid_target_attribute_inner_p
returned false) and when the target flags match the default ones.
So, let's change ix86_valid_target_attribute_tree to e.g. return
error_mark_node
instead of NULL_TREE if the attribute is invalid, and NULL_TREE only if it
matches the default.  Then, ix86_valid_target_attribute_p could treat
new_target
== error_mark_node similarly to new_target == NULL_TREE with the exception of
ret.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29207|0                           |1
        is obsolete|                            |

--- Comment #40 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-19 10:28:02 UTC ---
Created attachment 29217
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29217
gcc48-pr55742-2.patch

The following I mean (incremental patch).  No test coverage for that, of course
that needs to be added.

BTW, I've noticed wrong ChangeLog entries, your gcc/ChangeLog-2012 CL entries
refer to c-family/ (and cp/) files, while those should be moved (without
prefix) to corresponding c-family/ChangeLog resp. cp/ChangeLog-2012.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[tmsriram at google dot com]

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

--- Comment #41 from Sriraman Tallam <tmsriram at google dot com> 2013-01-19 17:18:02 UTC ---
(In reply to comment #40)
> Created attachment 29217 [details]
> gcc48-pr55742-2.patch
> 
> The following I mean (incremental patch).  No test coverage for that, of course
> that needs to be added.

The incremental patch is fine, thanks. However, the supports_function_versions
target hook is no longer necessary and can be removed.

I will add the tests for it.

> 
> BTW, I've noticed wrong ChangeLog entries, your gcc/ChangeLog-2012 CL entries
> refer to c-family/ (and cp/) files, while those should be moved (without
> prefix) to corresponding c-family/ChangeLog resp. cp/ChangeLog-2012.

Sorry about this

Thanks
Sri

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29211|0                           |1
        is obsolete|                            |
  Attachment #29217|0                           |1
        is obsolete|                            |
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #42 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-21 11:41:11 UTC ---
Created attachment 29234
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29234
gcc48-pr55742.patch

Updated patch that subsumes both the original patch and the incremental one,
and hopefully cures also (1), (2) and (3) above, except that the testsuite
coverage still should be improved (I've added just 5 new tests from the
snippets I had around) and documentation needs to be written.

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |crrodriguez at opensuse dot
                   |                            |org

--- Comment #43 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-23 16:38:31 UTC ---
*** Bug 56056 has been marked as a duplicate of this bug. ***

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

--- Comment #44 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-30 18:04:49 UTC ---
Author: jakub
Date: Wed Jan 30 18:04:34 2013
New Revision: 195584

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195584
Log:
    PR c++/55742
    * config/i386/i386.c (ix86_valid_target_attribute_inner_p): Diagnose
    invalid args instead of ICEing on it.
    (ix86_valid_target_attribute_tree): Return error_mark_node if
    ix86_valid_target_attribute_inner_p failed.
    (ix86_valid_target_attribute_p): Return false only if
    ix86_valid_target_attribute_tree returned error_mark_node.  Allow
    target("default") attribute.
    (sorted_attr_string): Change argument from const char * to tree,
    merge in all target attribute arguments rather than just one.
    Formatting fix.  Use XNEWVEC instead of xmalloc and XDELETEVEC
    instead of free.  Avoid using strcat.
    (ix86_mangle_function_version_assembler_name): Mangle
    target("default") as if no target attribute is present.  Adjust
    sorted_attr_string caller.  Avoid leaking memory.  Use XNEWVEC
    instead of xmalloc and XDELETEVEC instead of free.
    (ix86_function_versions): Don't return true if one of the decls
    doesn't have target attribute.  If they don't and one of the decls
    is DECL_FUNCTION_VERSIONED, report an error.  Adjust
    sorted_attr_string caller.  Use XDELETEVEC instead of free.
    (ix86_supports_function_versions): Remove.
    (make_name): Fix up formatting.
    (make_dispatcher_decl): Remove resolver_name and its initialization.
    Avoid leaking memory.
    (is_function_default_version): Return true if there is
    target("default") attribute rather than no target attribute at all.
    (make_resolver_func): Avoid leaking memory.
    (ix86_generate_version_dispatcher_body): Likewise.
    (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS): Remove.
    * target.def (supports_function_versions): Remove.
    * doc/tm.texi.in (SUPPORTS_FUNCTION_VERSIONS): Remove.
    * doc/tm.texi: Regenerated.

    * c-common.c (handle_target_attribute): Revert 2012-12-26 change.

    * g++.dg/mv1.C: Moved to...
    * g++.dg/ext/mv1.C: ... here.  Adjust test.
    * g++.dg/mv2.C: Moved to...
    * g++.dg/ext/mv2.C: ... here.  Adjust test.
    * g++.dg/mv3.C: Moved to...
    * g++.dg/ext/mv3.C: ... here.
    * g++.dg/mv4.C: Moved to...
    * g++.dg/ext/mv4.C: ... here.
    * g++.dg/mv5.C: Moved to...
    * g++.dg/ext/mv5.C: ... here.  Adjust test.
    * g++.dg/mv6.C: Moved to...
    * g++.dg/ext/mv6.C: ... here.  Adjust test.
    * g++.dg/ext/mv7.C: New test.
    * g++.dg/ext/mv8.C: New test.
    * g++.dg/ext/mv9.C: New test.
    * g++.dg/ext/mv10.C: New test.
    * g++.dg/ext/mv11.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/ext/mv1.C
      - copied, changed from r195583, trunk/gcc/testsuite/g++.dg/mv1.C
    trunk/gcc/testsuite/g++.dg/ext/mv10.C
    trunk/gcc/testsuite/g++.dg/ext/mv11.C
    trunk/gcc/testsuite/g++.dg/ext/mv2.C
      - copied, changed from r195583, trunk/gcc/testsuite/g++.dg/mv2.C
    trunk/gcc/testsuite/g++.dg/ext/mv3.C
      - copied unchanged from r195583, trunk/gcc/testsuite/g++.dg/mv3.C
    trunk/gcc/testsuite/g++.dg/ext/mv4.C
      - copied unchanged from r195583, trunk/gcc/testsuite/g++.dg/mv4.C
    trunk/gcc/testsuite/g++.dg/ext/mv5.C
      - copied, changed from r195583, trunk/gcc/testsuite/g++.dg/mv5.C
    trunk/gcc/testsuite/g++.dg/ext/mv6.C
      - copied, changed from r195583, trunk/gcc/testsuite/g++.dg/mv6.C
    trunk/gcc/testsuite/g++.dg/ext/mv7.C
    trunk/gcc/testsuite/g++.dg/ext/mv8.C
    trunk/gcc/testsuite/g++.dg/ext/mv9.C
Removed:
    trunk/gcc/testsuite/g++.dg/mv1.C
    trunk/gcc/testsuite/g++.dg/mv2.C
    trunk/gcc/testsuite/g++.dg/mv3.C
    trunk/gcc/testsuite/g++.dg/mv4.C
    trunk/gcc/testsuite/g++.dg/mv5.C
    trunk/gcc/testsuite/g++.dg/mv6.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/config/i386/i386.c
    trunk/gcc/doc/tm.texi
    trunk/gcc/doc/tm.texi.in
    trunk/gcc/target.def
    trunk/gcc/testsuite/ChangeLog

[Bug c++/55742] [4.8 regression] __attribute__ in class function declaration cause "prototype does not match" errors.

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #45 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-30 18:17:21 UTC ---
Should be fixed now.  Beyond gcc/doc/ documentation and more testcases, I think
http://gcc.gnu.org/wiki/FunctionMultiVersioning and
http://gcc.gnu.org/gcc-4.8/changes.html need updating.