Re: Unreviewed cygwin/mingw dllimport patch for PRs 5287. 7910, 11021

[Danny Smith <danny_r_smith_2001 at yahoo dot co dot nz>]

 --- Jason Merrill <jason@redhat.com> wrote: > + 	/* `extern' needn't be specified with dllimport.
Specify
> + 	   `extern' now and hope for the best. Be careful that we don't
> + 	   set TREE_PUBLIC for variables declared static (class static
> + 	   data is already public)  so we can catch syntax ambiguity
> + 	   below.  */
> + 	  if (!TREE_STATIC (node))
> + 	    TREE_PUBLIC (node) = 1;
> 
> This is wrong; TREE_STATIC means static storage duration, not C 'static'.
> The way to test for internal vs. external linkage is to look at
> TREE_PUBLIC.


Okay, the comment is misleading.  I do want to test for local temporal
scope. 

+ 	/* `extern' needn't be specified with dllimport. Specify
+ 	   `extern' now and hope for the best. Be careful that we don't
+ 	   set TREE_PUBLIC for variables with static storage duration
+	   (class static data is already public),  so we can catch syntax
+	   ambiguity  below.  */


Here is what I want to complain about (dllimport5,C):

// { dg-do compile { target i?86-*-cygwin* i?86-*-mingw*} }
//  Report error if static symbol definition has dllimport attribute.


__attribute__((dllimport))
 static int static_impvar;	// { dg-error "external linkage required" }

__attribute__((dllexport))
 static int static_expvar;	// { dg-error "external linkage required" }

__attribute__((dllimport))
static void static_impfun(void);	// { dg-error "external linkage required" }

void foo()
{
  __attribute__((dllimport)) int foovar; // OK,  implicit "extern" 
  foovar++;
}

void bar()
{
  __attribute__((dllexport)) int barvar;	// { dg-error "external linkage required" }
  barvar++;
}


> 
> !       /* Don't mark defined functions as dllimport.  If the definition
> ! 	 itself was marked with dllimport, then an error is raised. This
> ! 	 handles the case when the definition overrides an earlier
> ! 	 declaration.  */
> !       if (TREE_CODE (decl) ==  FUNCTION_DECL && DECL_INITIAL (decl)
> ! 	  && !DECL_INLINE (decl))
> ! 	{
> ! 	  warning ("%Hdefinition of '%D' previously marked as dllimport: attribute ignored",
> ! 		   &DECL_SOURCE_LOCATION (decl), decl);
> ! 	  return 0;
> ! 	}
> 
> I think the warning should say "declaration", not "definition".

No I meant definition as in:

dlimport1.C

//  PR c++/7910
// { dg-do compile { target i?86-*-cygwin* i?86-*-mingw*} }
// { dg-options { -Wall -W } }

class __attribute__((dllimport)) Foo
{
 public:
  virtual void dummy_foo_func(void)
    {}	// { dg-warning "inline function" }
  virtual void dummy_foo_fun2(void);
  virtual ~Foo();  //  avoid warning  
};

void Foo::dummy_foo_fun2()
{	//  { dg-warning "definition" }
}

class Bar : public Foo
{
public:
  ~Bar();
  void dummy_bar_func();
};

Bar::~Bar()
{}

void Bar::dummy_bar_func()
{}

// { dg-final { scan-assembler-not "__imp___ZN3Foo14dummy_foo_fun" } }


> 
> !           /* Should in-class initialization of static consts raise an
> ! 	     error too, or just ignore the attribute, as here?  */
> ! 	  else if (DECL_INITIAL (decl)
> ! 		   && !TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (decl)))
> ! 	    {
> ! 	      warning ("%Hin-class initialization of '%D': dllimport attribute ignored.", 
> ! 		       &DECL_SOURCE_LOCATION (decl), decl);	
> ! 	      return 0;
> ! 	    }
> 
> In-class initialization of static consts should not cause them to be
> treated differently from variables without in-class initializers.


It isn't except for the warning. If a definition appears later, we still
report an error. The warning is there because the handling of in-class
initializers varies with compiler.
The in-class-initializer-without-definition is effectively treated like
a scope-qualified enum by g++ (that's my understanding anyway). At least
one other compiler reports an error for such in-class initialization in
a dllimported class, saying that is a definition. It's not a definition,
but its not really portable in the windows world either - hence
the warning. 


> 
> !       /* Turn off importing C++ virtual methods to able to create 
> ! 	 vtables using thunks. */
> !       else if (TREE_CODE (TREE_TYPE (decl)) == METHOD_TYPE
> ! 	       && DECL_VIRTUAL_P (decl))
> !         {
> !           return 0;
> !         }
> 
> I don't understand the rationale for this; please hold this bit back until
> your next patch to handle multiple inheritance.

OK, My empirical rationale is PRs c++/5287 and c++/11021 which fail without 
this hunk and succeed with it.  I've gone back and looked through all the dialogue
between Jason and Mumit Khan ca. 1998/1999, and I confess I do not understand it all. 
But -- given the minimal cost of ignoring the attibute for virtual methods -- 
this seemed like the simplest solution and one that is "local" to windows ports. 

> 
> Incidentally, talking about "throw"ing an error is misleading, since we
> don't use exception handling for error reporting in C++.  I'd probably say
> "report an error" or "complain".
> 

Okay.

Thanks for your comments. 

Danny

> Jason 

http://mobile.yahoo.com.au - Yahoo! Mobile
- Check & compose your email via SMS on your Telstra or Vodafone mobile.