RE: [Patch] Fix PR 21275, PR21597 cygwin/mingw32 bootstrap regressionson trunk

[Danny Smith <dannysmith at clear dot net dot nz>]

> -----Original Message-----
> From: Mark Mitchell
> Sent: Wednesday, August 03, 2005 11:18 AM
> 
> Danny Smith wrote:
> > Hi, the attached patch fixes dllimport related bootstrap
> regressions
> > PR 21275 and PR 21597 on minw32 and cygwin.  It also fixes PR 19704 
> > for C++ class members that get dllimport status from class type 
> > attribute.
> 
> Yay!  This patch looks pretty good; a few comments below.  When these 
> are address, copy me, and I'll review/approve.
> 

Sorry for the delay.

> > Setting non_addr_const_p flag earlier, when we first handle the 
> > dllimport attribute, fixes most of the problems for C code,
> but still
> > fails to allow dllimport to be overriden by a later
> declaration sans
> > attribute or an earlier declaration with dllexport
> attribute. It also
> > fails to handle dllimport of C++ class members that acquire
> dllimport
> > status via a class type attribute rather than by a decl attribute.
> 
> Why doesn't that work in C++?  It looks to me like it should.  In 
> particular, determine_visibility should be called for the members at 
> some point, and it should set their visibility based on the class 
> type.
>   Is it just that this function doesn't handle
> DECL_DLLIMPORT?  If so, 
> that would be a natural place to put that.


determine_visibility is called when members are defined, to set the
visibility of the definition.  dllimport needs to be set earlier, at
declaration, so that references to the external definitions use the
right sematics.

> 
> > In varasm.c, uses the DECL_DLLIMPORT flag to test whether  
> ADDR_EXPR's 
> > of both function and var decls are constant.
> 
> OK, but use DECL_DLLIMPORT_P.

I've changed the name to DECL_DLIMPORT_P.  Since I've reverted the idea
to set staticp for dllimported vars, I can leave  the VAR_DECL case
alone. 

> 
>  > Maybe I misunderstand what staticp means, but
> > dllimported vars do have static storage -- it's just that we don't 
> > know their addresses until the library is loaded and the 
> fixups to the import
> > address table are made.   If we do need to keep a test for
> > DECL_DLLIMPORT in staticp, then we also need to treat 
> dllimports like 
> > thread local variables in 
> > tree.c:recompute_tree_invarant_for_addr_expr.
> 

I did (still do) misunderstand how staticp is used.  Saying that
addresses of dllimported vars should be treated as constant breaks
optimization of things that use imported arrays, eg 'extern
__attribute__ ((dllimport)) int _ctype_[]' as used by ctype is* macro's
in regex.c

> I think we ought to keep the check, and, as you say, add it to 
> r_t_i_f_a_e.  It's a little hard to know exactly how staticp will be 
> used, so absent a very clear statement to the contrary, I think we 
> should err on the side of caution.  DLLIMPORT is pretty similar to 
> thread-local, in the sense that once the variable exists it's 
> at a fixed 
> address, but not until that point.
> 

A big difference between thread-local and dllimport semantics is that
dllimport attribute is allowed to be overriden by a later redeclaration.

This is a problem that has been blocking me:

If we do this (which is acceptable by MS rules)
          extern int __attribute__ ((dllimport)) foo;
	   int* bar () {return &foo;}
	   int foo; 

the second declaration of foo should cause the dllimport to be lost.
But if I remove it then, I get the error message 'constant not
recomputed when ADDR_EXPR changed' in bar().  So, in
merge_dllimport_decl_attributes, I just kept the old DECL_DLLIMPORT_P if
the the decl's address has already been used. Keeping the
DECL_DLLIMPORT_P flag means that &foo cannot be used as constant
initializer.

I don't like this, I would prefer just saying: "You can't do this," and
emit an error, but MS allows it.

> > + 	      /* FIXME, should we warn about inconsistency in
> > + 		 dll linkage here, or just silently override?  */	
> 
> You need to decide, rather than check in this FIXME. :-)  I think my 
> default answer would be to do whatever MSVC does; they've 
> thought about 
> dllimport more than all the rest of us put together, and 
> users probably 
> expect that behavior.

OK, I've put warnings into merge_dllimport_decl_attributes.  MS does
warn.  Adding the warnings identified potential problems in libiberty
strerror() code (the code is never used by gcc or binutils, so it never
was noticed).  These problems would be flagged later by the linker, but
the compiler can give better diagnostics.

Putting the warnings here, means that we get the same warnings in C and
C++, except for those that apply specifically to
C++ classes (static data members, virtual methods).
 
> 
> > + 
> > +   bool (* valid_dllimport_attribute_p) (tree);
> 
> Needs comment.

Done.

> 
> > 
> > +     /* Use this hook If the target needs check type 
> attributes of a class
> > +        and apply to member decls.  */  
> > +     void (*check_class_attributes) (tree);
> 
> Needs better comment. :-)  Something like:
> 
> The argument is a C++ class (i.e., RECORD_TYPE or UNION_TYPE) 
> that has 
> just been defined.  The back end can make adjustments to the type.
> 

Done.

> This will probably be (ab)used for more than just attributes, 
> so maybe 
> this should just be after_class_hook?
> 

I called it 'adjust_class_at_definition'.  The adjustments are made as
soon as we have a complete class definition.

> > *** doc/tm.texi	21 Jul 2005 00:55:20 -0000	1.443
> > --- doc/tm.texi	2 Aug 2005 04:59:52 -0000
> 
> There's only documentation for the C++ hook here.
> 
> > +   targetm.cxx.check_class_attributes (t);
> > + 

Added documentation for valid_dllimport_attribute_p,

> 
> I'd rather you call this right after the call to warn_hidden, 
> instead of 
> right before.  And, add a comment:
> 
> /* Class layout, assignment of virtual table slots, etc., is now 
> complete.  Give the back end a chance to tweak the visibility of the 
> class, or perform any other modifications required.  */

Done.


Tested on i686-pc-mingw32,
../gcc/configure  --with-gcc --with-as=/mingw/mingw32/bin/as.exe
--with-ld=/mingw/mingw32/bin/ld.exe --with-nm=/mingw/mingw32/bin/nm.exe
--host=mingw32 --build=mingw32 --target=mingw32  --prefix=/mingw
--enable-threads --disable-nls --enable-languages=c,c++,objc,ada,f95
--disable-win32-registry --disable-shared --enable-sjlj-exceptions

and i686-pc-cygwin
../gcc/configure --prefix=/usr --enable-threads=posix --disable-libgcj
--enable-sjlj-exceptions --with-system-zlib --enable-nls
--disable-libmudflap --enable-version-specific-runtime-libs
--without-included-gettext --disable-shared
--enable-languages=c,c++,java,objc,f95 --disable-win32-registry

with no new regressions.

  

ChangeLog

        PR middle-end/21275, middle-end/21597
        * target.h (struct gcc_target): Add valid_dllimport_attribute_p
        target hook.
        (struct cxx): Add adjust_class_at_definition target hook.
        * target-def.h: (TARGET_VALID_DLLIMPORT_ATTRIBUTE_P): New
define,
        defaulting to hook_bool_tree_true. Add to TARGET_INITIALIZER
        (TARGET_CXX_ADJUST_CLASS_AT_DEFINITION): New define, defaulting
to 
        hook_void_tree. Add to TARGET_CXX.
        * tree.h (struct decl_with_vis): Rename non_addr_const_p field
to
        dllimport_flag.
        (DECL_NON_ADDR_CONSTANT_P): Replace with DECL_DLLIMPORT_P macro.
        * tree.c (merge_dllimport_decl_attributes): Check
DECL_DLLIMPORT_P
        instead of attribute. Check for dllexport override.  Warn if
        inconsistent dll linkage. Don't lose old dllimport if decl has
        had had address referenced.   Tweak lookup of dllimport
atribute.  
        (handle_dll_attribute): Check
targetm.valid_dllimport_attribute_p
        for target specific rules.  Don't add dllimport attribute if
        DECL_DECLARED_ONLINE_P.  Set DECL_DLLIMPORT_P when adding
dllimport
        attribute. 
        (staticp): Replace DECL_NON_ADDR_CONSTANT_P with
DECL_DLLIMPORT_P.
        * varasm.c (initializer_constant_valid_p): Replace
DECL_NON_ADDR_CONSTANT_P
        with DECL_DLLIMPORT_P.
        * config.gcc (i[34567]86-*-cygwin*): Add winnt-cxx.o to 
        'cxx_target_objs', winnt-stubs,o to 'extra_objs'.
        (i[34567]86-*-mingw32*): Likewise.

        * doc/tm.texi (TARGET_CXX_ADJUST_CLASS_AT_DEFINITION): Document.
        (TARGET_VALID_DLLIMPORT_ATTRIBUTE_P): Document.
                        
        * config/i386/winnt.c (i386_pe_dllimport_p): Factor out C++
specific code.
        Change return value to bool. 
        (i386_pe_dllimport_p): Likewise.
        (associated_type): Simplify and make language-independent
        (i386_pe_encode_section_info): Replace override of ambiguous
dllimport symbol
        refs with a gcc_assert.
        (i386_pe_valid_dllimport_attribute_p): Define.
        * config/i386/winnt-cxx.c: New file. Define
i386_pe_type_dllimport_p,
        i386_pe_type_dllexport_p, i386_pe_adjust_class_at_definition.
        * config/i386/winnt-stubs.c: New file. Define stub versions of
lang-specific
        functions.
        config/i386/i386-protos.h: Declare winnt-[cxx|stubs].c functions
        i386_pe_type_dllimport_p, i386_pe_type_dllexport_p, 
        i386_pe_adjust_class_at_definition.
        (i386_pe_valid_dllimport_attribute_p): Declare. 
        * config/i386/cygming.h (TARGET_VALID_DLLIMPORT_ATTRIBUTE_P):
Define.
        (TARGET_CXX_ADJUST_CLASS_AT_DEFINITION): Define.
        * config/i386/t-cygming: Add rules for winnt-cxx.o,
winnt-stubs.o.

        PR target/19704
        config/i386/i386.c (ix86_function_ok_for_sibcall): Replace test
for
        dllimport attribute with test of DECL_DLLIMPORT_P flag.        


cp/ChangeLog 

        PR middle-end/21275, middle-end/21597
        class.c (finish_struct_1): Call
targetm.cxx.adjust_class_at_definition.


testsuite/ChangeLog

        gcc.dg/dll-2.c: Add tests for warnings.
        gcc.dg/dll-3.c: Likewise.
        gcc.dg/dll-4.c: Likewise.

        g++.dg/ext/dllimport1.C: Adjust tests for warnings.
        g++.dg/ext/dllimport2.C: Likewise.
        g++.dg/ext/dllimport3.C: Likewise.
        g++.dg/ext/dllimport7.C: Likewise.
        g++.dg/ext/dllimport8.C: Likewise.
        g++.dg/ext/dllimport9.C: Likewise.

Attachment: dllimport_p.diff
Description: Binary data