Reject dllimport on scalar typedefs (bug 33963)

Reject dllimport on scalar typedefs (bug 33963)

[Joseph S. Myers]

Bug 33963 is a regression in 4.3 and later where dllimport and
dllexport attributes are wrongly accepted on scalar types.

This patch fixes this bug.  Tested with no regressions with cross to
i686-mingw32, where it fixes:

FAIL: gcc.dg/attr-invalid.c  (test for warnings, line 88)
FAIL: gcc.dg/attr-invalid.c  (test for warnings, line 90)

OK to commit?  (If OK, I'll also commit to 4.3 branch after 4.3.0 is
released.)

2008-02-26  Joseph Myers  <joseph@codesourcery.com>

	PR target/33963
	* tree.c (handle_dll_attribute): Disallow TYPE_DECLs for types
	other than structures and unions.

Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 132664)
+++ gcc/tree.c	(working copy)
@@ -4067,6 +4067,16 @@
       return NULL_TREE;
     }
 
+  if (TREE_CODE (node) == TYPE_DECL
+      && TREE_CODE (TREE_TYPE (node)) != RECORD_TYPE
+      && TREE_CODE (TREE_TYPE (node)) != UNION_TYPE)
+    {
+      *no_add_attrs = true;
+      warning (OPT_Wattributes, "%qs attribute ignored",
+	       IDENTIFIER_POINTER (name));
+      return NULL_TREE;
+    }
+
   /* Report error on dllimport ambiguities seen now before they cause
      any damage.  */
   else if (is_attribute_p ("dllimport", name))

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Reject dllimport on scalar typedefs (bug 33963)

[Mark Mitchell]

Joseph S. Myers wrote:

> 2008-02-26  Joseph Myers  <joseph@codesourcery.com>
> 
> 	PR target/33963
> 	* tree.c (handle_dll_attribute): Disallow TYPE_DECLs for types
> 	other than structures and unions.

OK, assuming no objections from Windows maintainers within 24 hours. (I 
know this is target-independent code, but Windows folks are experts on 
dllimport.)

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

Re: Reject dllimport on scalar typedefs (bug 33963)

[Danny Smith]

On Wed, Feb 27, 2008 at 10:40 AM, Mark Mitchell <mark@codesourcery.com> wrote:
> Joseph S. Myers wrote:
>
>  > 2008-02-26  Joseph Myers  <joseph@codesourcery.com>
>  >
>  >       PR target/33963
>  >       * tree.c (handle_dll_attribute): Disallow TYPE_DECLs for types
>  >       other than structures and unions.
>
>  OK, assuming no objections from Windows maintainers within 24 hours. (I
>  know this is target-independent code, but Windows folks are experts on
>  dllimport.)
>

Why do we allow dllimport/dllexport for typedefs of structs and
unions? That usage is not documented in MSDN
http://msdn2.microsoft.com/en-us/library/3y1sfaz2%28VS.71%29.aspx

AFAICT even though GCC allows the attribute,
it doesn't do anything

typedef struct _foo {
  int i;
} Foo __attribute__((dllimport));;

Foo Z;

int t() {
   return Z.i;
};

gets

globl _t
	.def	_t;	.scl	2;	.type	32;	.endef
_t:
	pushl	%ebp
	movl	_Z, %eax
	movl	%esp, %ebp
	popl	%ebp
	ret
	.comm	_Z, 16	 # 4

Doesn't that deserve a warning?  Without a warning, users  may well
think they are  import an external Z from a dll.

Using the C++ syntax for dllimported struct for a typedef, like so

typedef struct __attribute__((dllimport)) _foo {
  int i;
} Foo;

also does nothing.

Danny

Re: Reject dllimport on scalar typedefs (bug 33963)

[Joseph S. Myers]

On Wed, 27 Feb 2008, Danny Smith wrote:

> On Wed, Feb 27, 2008 at 10:40 AM, Mark Mitchell <mark@codesourcery.com> wrote:
> > Joseph S. Myers wrote:
> >
> >  > 2008-02-26  Joseph Myers  <joseph@codesourcery.com>
> >  >
> >  >       PR target/33963
> >  >       * tree.c (handle_dll_attribute): Disallow TYPE_DECLs for types
> >  >       other than structures and unions.
> >
> >  OK, assuming no objections from Windows maintainers within 24 hours. (I
> >  know this is target-independent code, but Windows folks are experts on
> >  dllimport.)
> >
> 
> Why do we allow dllimport/dllexport for typedefs of structs and
> unions? That usage is not documented in MSDN
> http://msdn2.microsoft.com/en-us/library/3y1sfaz2%28VS.71%29.aspx

I presume TYPE_DECL is arising for some C++ cases other than explicit 
typedefs, relating to visibility for classes (otherwise Mark wouldn't have 
needed to make the TYPE_DECL change in the first place).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Reject dllimport on scalar typedefs (bug 33963)

[Mark Mitchell]

Joseph S. Myers wrote:

>> Why do we allow dllimport/dllexport for typedefs of structs and
>> unions? That usage is not documented in MSDN
>> http://msdn2.microsoft.com/en-us/library/3y1sfaz2%28VS.71%29.aspx
> 
> I presume TYPE_DECL is arising for some C++ cases other than explicit 
> typedefs, relating to visibility for classes (otherwise Mark wouldn't have 
> needed to make the TYPE_DECL change in the first place).

See, for example, the g++.dg/ext/visibility/visibility-9.C, which was 
one of the tests checked in with the patch that allowed these attributes 
on TYPE_DECLs.  The issue is probably not the dllimport per se, but, 
rather __attribute__((visibility(...))) -- and variants of that 
spelling, like __declspec(notshared).

Thanks,

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

Re: Reject dllimport on scalar typedefs (bug 33963)

[Joseph S. Myers]

On Wed, 27 Feb 2008, Mark Mitchell wrote:

> Joseph S. Myers wrote:
> 
> > > Why do we allow dllimport/dllexport for typedefs of structs and
> > > unions? That usage is not documented in MSDN
> > > http://msdn2.microsoft.com/en-us/library/3y1sfaz2%28VS.71%29.aspx
> > 
> > I presume TYPE_DECL is arising for some C++ cases other than explicit
> > typedefs, relating to visibility for classes (otherwise Mark wouldn't have
> > needed to make the TYPE_DECL change in the first place).
> 
> See, for example, the g++.dg/ext/visibility/visibility-9.C, which was one of
> the tests checked in with the patch that allowed these attributes on
> TYPE_DECLs.  The issue is probably not the dllimport per se, but, rather
> __attribute__((visibility(...))) -- and variants of that spelling, like
> __declspec(notshared).

So is this patch OK or not?  As far as I can tell it fixes the testsuite 
regression - but there may be other cases that have also regressed, not 
covered by the testsuite, that would best have separate bugs filed and 
marked as regressions.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Reject dllimport on scalar typedefs (bug 33963)

[Mark Mitchell]

Joseph S. Myers wrote:

> So is this patch OK or not?  As far as I can tell it fixes the testsuite 
> regression - but there may be other cases that have also regressed, not 
> covered by the testsuite, that would best have separate bugs filed and 
> marked as regressions.

Yes, this patch is OK.  I think we all agree the behavior is a step 
forward.  If Danny thinks that we shouldn't accept these attributes on 
any types, even class types, then that's a further restriction, easily 
implemented on top of your patch.

Thanks,

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

Re: Reject dllimport on scalar typedefs (bug 33963)

[Danny Smith]

On Thu, Feb 28, 2008 at 4:37 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Joseph S. Myers wrote:
>
>  > So is this patch OK or not?  As far as I can tell it fixes the testsuite
>  > regression - but there may be other cases that have also regressed, not
>  > covered by the testsuite, that would best have separate bugs filed and
>  > marked as regressions.
>
>  Yes, this patch is OK.  I think we all agree the behavior is a step
>  forward.  If Danny thinks that we shouldn't accept these attributes on
>  any types, even class types, then that's a further restriction, easily
>  implemented on top of your patch.

Yes OK.
The lack of warning on things like

struct /* __attribute__((dllimport))*/ _foo {
  static int i;
};
typedef _foo Foo __attribute__((dllimport));  // dllimport attribute ignored

is not a regression.

Thanks for fixing this Joseph.

Danny