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

Jason Merrill jason@redhat.com
Wed Jun 11 02:00:00 GMT 2003


On Wed, 11 Jun 2003 08:02:40 +1000 (EST), Danny Smith <danny_r_smith_2001@yahoo.co.nz> wrote:

>  --- 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. 

There's no such thing.  Storage duration is either static, automatic (on
the stack) or dynamic (malloc).

> + 	/* `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.  */

I'm saying that the case you want to complain about is !TREE_PUBLIC.
TREE_STATIC is set for both
  int i;
and
  static int j;

You just don't want to set TREE_PUBLIC if it isn't already set.

>> !       /* 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:

Perhaps we're just parsing that sentence differently.  You intended it as
"definition of (decl previously marked as dllimport)" and I parsed it as
"(definition of decl) previously marked as dllimport"; parsed my way, it's
wrong, as it was the declaration which was marked as dllimport.

Perhaps "defining `%D' which was previously declared dllimport" would be
clearer.

>> !           /* 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).

Kind of.  Really, it's treated as itself; it has semantics defined by the
language (9.4.2).  It isn't a definition.

> 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.

Well, that's a bug in the other compiler, but I suppose giving a
portability warning is reasonable.  Please talk about that in the comment.

>> !       /* 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. 

IMO, ABI decisions shouldn't be made based on what's the simplest solution.
Incidentally, the DLL model isn't unique to Windows, though I suppose BeOS
doesn't count for much these days...:)

Why does this patch fix those bugs?

Jason



More information about the Gcc-patches mailing list