preserve parameter attributes during C++ decl smashing

preserve parameter attributes during C++ decl smashing

[Taras Glek]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

I'm sending this patch on Dave's behalf.

class Foo {
  void foo (char __attribute__((user("param"))) c);
};

void Foo::foo(char c) {
}

In above example the attribute is not preserved. Since there is no way 
to obtain the forward declaration signature once Foo::foo implementation 
is parsed, the attribute should be preserved.


Attached patch fixes that.


gcc/cp/ChangeLog:

2009-04-22  David Mandelin <dmandelin@mozilla.com>:
       * decl.c (duplicate_decls): Preserve parameter attributes

Thanks,
Taras

[-- Attachment #2: parm_attrs.diff --]
[-- Type: text/x-patch, Size: 854 bytes --]

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1982,6 +1982,16 @@ duplicate_decls (tree newdecl, tree oldd
 	}
       if (! types_match || new_defines_function)
 	{
+          /* Make sure parameter attributes
+             are retained from the initial declaration. */
+          tree oldarg, newarg;
+          for (oldarg = DECL_ARGUMENTS(olddecl), 
+                 newarg = DECL_ARGUMENTS(newdecl);
+               oldarg && newarg;
+               oldarg = TREE_CHAIN(oldarg), newarg = TREE_CHAIN(newarg)) {
+            DECL_ATTRIBUTES (newarg)
+              = (*targetm.merge_decl_attributes) (oldarg, newarg);
+          }
 	  /* These need to be copied so that the names are available.
 	     Note that if the types do match, we'll preserve inline
 	     info and other bits, but if not, we won't.  */

Re: preserve parameter attributes during C++ decl smashing

[Jason Merrill]

This seems like a reasonable change, but it doesn't cover a 
redeclaration that is not a definition which adds attributes, i.e.

void f(int i);
void f(int i __attribute ((foo)));
void f(int i) { ... }

I think in this situation the "foo" attribute will get silently dropped 
on the floor.  Better to always merge the attributes, I think.

Jason

Re: preserve parameter attributes during C++ decl smashing

[Dave Korn]

Jason Merrill wrote:
> This seems like a reasonable change, but it doesn't cover a
> redeclaration that is not a definition which adds attributes, i.e.
> 
> void f(int i);
> void f(int i __attribute ((foo)));
> void f(int i) { ... }
> 
> I think in this situation the "foo" attribute will get silently dropped
> on the floor.  Better to always merge the attributes, I think.

  Is that correct for "__attribute__ ((weak))"?  Or should the version without
the attribute be taken as a strong declaration?

    cheers,
      DaveK

Re: preserve parameter attributes during C++ decl smashing

[Mark Mitchell]

Dave Korn wrote:
> Jason Merrill wrote:
>> This seems like a reasonable change, but it doesn't cover a
>> redeclaration that is not a definition which adds attributes, i.e.
>>
>> void f(int i);
>> void f(int i __attribute ((foo)));
>> void f(int i) { ... }
>>
>> I think in this situation the "foo" attribute will get silently dropped
>> on the floor.  Better to always merge the attributes, I think.

I agree.

>   Is that correct for "__attribute__ ((weak))"?  Or should the version without
> the attribute be taken as a strong declaration?

If we emit a definition of "f", I don't think we should emit it as weak.
 AFAIK, it only makes sense for a symbol to be weak when we are emitted
a reference to it, not a definition of it.  But, that's not really a
question about merging attributes; the same question applies to:

   __attribute__((weak)) void f(int i) { ... }

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: preserve parameter attributes during C++ decl smashing

[H.J. Lu]

On Tue, Apr 28, 2009 at 3:40 PM, Mark Mitchell <mark@codesourcery.com> wrote:
>
> If we emit a definition of "f", I don't think we should emit it as weak.
>  AFAIK, it only makes sense for a symbol to be weak when we are emitted
> a reference to it, not a definition of it.  But, that's not really a
> question about merging attributes; the same question applies to:
>
>   __attribute__((weak)) void f(int i) { ... }
>

Try

#  readelf -s --wide libstdc++.a| grep WEAK

on Linux. You may find many weak definitions.


-- 
H.J.

Re: preserve parameter attributes during C++ decl smashing

[Ian Lance Taylor]

Mark Mitchell <mark@codesourcery.com> writes:

> If we emit a definition of "f", I don't think we should emit it as weak.
>  AFAIK, it only makes sense for a symbol to be weak when we are emitted
> a reference to it, not a definition of it.

That turns out not to be the case.  Weak definitions are meaningful and
widely used for static links.  For example, glibc provides a weak
definition of __pthread_initialize_minimal which is used unless a strong
definition is linked in from libpthread.a.

Ian

Re: preserve parameter attributes during C++ decl smashing

[Mark Mitchell]

Ian Lance Taylor wrote:
> Mark Mitchell <mark@codesourcery.com> writes:
> 
>> If we emit a definition of "f", I don't think we should emit it as weak.
>>  AFAIK, it only makes sense for a symbol to be weak when we are emitted
>> a reference to it, not a definition of it.
> 
> That turns out not to be the case.  Weak definitions are meaningful and
> widely used for static links.  For example, glibc provides a weak
> definition of __pthread_initialize_minimal which is used unless a strong
> definition is linked in from libpthread.a.

I stand corrected, then.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: preserve parameter attributes during C++ decl smashing

[David Mandelin]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

H.J. Lu wrote:
> On Tue, Apr 28, 2009 at 3:40 PM, Mark Mitchell <mark@codesourcery.com> wrote:
>   
>> If we emit a definition of "f", I don't think we should emit it as weak.
>>  AFAIK, it only makes sense for a symbol to be weak when we are emitted
>> a reference to it, not a definition of it.  But, that's not really a
>> question about merging attributes; the same question applies to:
>>
>>   __attribute__((weak)) void f(int i) { ... }
>>
>>     
>
> Try
>
> #  readelf -s --wide libstdc++.a| grep WEAK
>
> on Linux. You may find many weak definitions.
>
>
>   

Here is an updated version of the patch that always merges the 
attributes. I found that I needed to set the attributes on both the 
'old' and the 'new' declaration because it was unpredictable which set 
of PARM_DECL nodes would be used in the result of merging the 
FUNCTION_DECLs.

I'm not sure if it's OK to share the DECL_ATTRIBUTES(...) properties 
like that, so please check that out specifically in reviewing the patch. 
Let me know if anything else is needed--this is my first submission to 
gcc-patches.

--
Dave

[-- Attachment #2: parm_attrs.diff --]
[-- Type: text/plain, Size: 738 bytes --]

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1920,6 +1920,17 @@ duplicate_decls (tree newdecl, tree oldd
 
   if (TREE_CODE (newdecl) == FUNCTION_DECL)
     {
+      /* Merge parameter attributes. */
+      tree oldarg, newarg;
+      for (oldarg = DECL_ARGUMENTS(olddecl), 
+             newarg = DECL_ARGUMENTS(newdecl);
+           oldarg && newarg;
+           oldarg = TREE_CHAIN(oldarg), newarg = TREE_CHAIN(newarg)) {
+        DECL_ATTRIBUTES (newarg)
+          = (*targetm.merge_decl_attributes) (oldarg, newarg);
+        DECL_ATTRIBUTES (oldarg) = DECL_ATTRIBUTES (newarg);
+      }
+
       if (DECL_TEMPLATE_INSTANTIATION (olddecl)
 	  && !DECL_TEMPLATE_INSTANTIATION (newdecl))
 	{

Re: preserve parameter attributes during C++ decl smashing

[Jason Merrill]

David Mandelin wrote:
> I'm not sure if it's OK to share the DECL_ATTRIBUTES(...) properties 
> like that, so please check that out specifically in reviewing the patch. 

That should be fine, since one of the decls is going to be garbage anyway.

> Let me know if anything else is needed--this is my first submission to 
> gcc-patches.

This needs a ChangeLog entry, like Taras included with the earlier 
patch.  Also, have you completed a copyright assignment?

Thanks,
Jason

Re: preserve parameter attributes during C++ decl smashing

[David Mandelin]

Jason Merrill wrote:
> David Mandelin wrote:
>> I'm not sure if it's OK to share the DECL_ATTRIBUTES(...) properties 
>> like that, so please check that out specifically in reviewing the patch. 
>
> That should be fine, since one of the decls is going to be garbage 
> anyway.
Good. Thanks.
> This needs a ChangeLog entry, like Taras included with the earlier 
> patch.  Also, have you completed a copyright assignment?
>
The same one Taras used before is fine:

gcc/cp/ChangeLog:

2009-04-22  David Mandelin <dmandelin@mozilla.com>:
       * decl.c (duplicate_decls): Preserve parameter attributes

We have a copyright assignment that covers all our work as Mozilla 
employees, including this work.

--
Dave

Re: preserve parameter attributes during C++ decl smashing

[Gerald Pfeifer]

On Fri, 1 May 2009, David Mandelin wrote:
> We have a copyright assignment that covers all our work as Mozilla 
> employees, including this work.

For the sake of all maintainers:  I had a quick look and the Mozilla 
Foundation has a global assignment for GCC on file with the FSF.

Gerald

Re: preserve parameter attributes during C++ decl smashing

[Taras Glek]

Gerald Pfeifer wrote:
> On Fri, 1 May 2009, David Mandelin wrote:
>   
>> We have a copyright assignment that covers all our work as Mozilla 
>> employees, including this work.
>>     
>
> For the sake of all maintainers:  I had a quick look and the Mozilla 
> Foundation has a global assignment for GCC on file with the FSF.
>   
Jason, is Dave's latest revision the patch OK to land now?

Taras

Re: preserve parameter attributes during C++ decl smashing

[Jason Merrill]

I was just about to check this in, and then noticed that there's no 
testcase.  Do you have one handy?

Jason

Re: preserve parameter attributes during C++ decl smashing

[Taras Glek]

Jason Merrill wrote:
> I was just about to check this in, and then noticed that there's no 
> testcase.  Do you have one handy?
We use our plugin framework to test this at Mozilla. I'm not sure of how 
this could be tested otherwise. Is there some way to dump the ast that 
GCC uses for attribute tests?

For me, the easiest thing to do would be to include a test for this in 
the testcase for

http://gcc.gnu.org/ml/gcc-patches/2009-05/msg00629.html

such that both features would be tested in the same testcase. Is that 
reasonable?

Taras

Re: preserve parameter attributes during C++ decl smashing

[Jason Merrill]

Taras Glek wrote:
> For me, the easiest thing to do would be to include a test for this in 
> the testcase for
> 
> http://gcc.gnu.org/ml/gcc-patches/2009-05/msg00629.html
> 
> such that both features would be tested in the same testcase. Is that 
> reasonable?

That's fine.

Jason

Re: preserve parameter attributes during C++ decl smashing

[Jason Merrill]

Jason Merrill wrote:
> Taras Glek wrote:
>> For me, the easiest thing to do would be to include a test for this in 
>> the testcase for
>>
>> http://gcc.gnu.org/ml/gcc-patches/2009-05/msg00629.html
>>
>> such that both features would be tested in the same testcase. Is that 
>> reasonable?
> 
> That's fine.

...but in that case this patch should be applied along with that one.

Jason

Re: preserve parameter attributes during C++ decl smashing

[Taras Glek]

Jason Merrill wrote:
> Jason Merrill wrote:
>> Taras Glek wrote:
>>> For me, the easiest thing to do would be to include a test for this 
>>> in the testcase for
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2009-05/msg00629.html
>>>
>>> such that both features would be tested in the same testcase. Is 
>>> that reasonable?
>>
>> That's fine.
>
> ...but in that case this patch should be applied along with that one.
Cool, so am I OKed to commit this at the same time as 
http://gcc.gnu.org/ml/gcc-patches/2009-05/msg00718.html ?

Taras

Re: preserve parameter attributes during C++ decl smashing

[Diego Novillo]

On Wed, May 13, 2009 at 15:05, Taras Glek <tglek@mozilla.com> wrote:

> Cool, so am I OKed to commit this at the same time as
> http://gcc.gnu.org/ml/gcc-patches/2009-05/msg00718.html ?

Yes, thanks.


Diego.