diff mbox series

Fix dllimport attribute handling on C++ static data members (PR c/88568)

Message ID 20190305201438.GD7611@tucnak
State New
Headers show
Series Fix dllimport attribute handling on C++ static data members (PR c/88568) | expand

Commit Message

Jakub Jelinek March 5, 2019, 8:14 p.m. UTC 
On Thu, Jan 10, 2019 at 11:07:37AM +0100, Jakub Jelinek wrote:
> 2019-01-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/88568
> 	* attribs.c (handle_dll_attribute): Clear TREE_STATIC after setting
> 	DECL_EXTERNAL.
> 
> 	* gcc.dg/pr88568.c: New test.
> 
> --- gcc/attribs.c.jj	2019-01-05 12:06:12.055124090 +0100
> +++ gcc/attribs.c	2019-01-07 12:57:09.739782281 +0100
> @@ -1691,6 +1691,8 @@ handle_dll_attribute (tree * pnode, tree
>  	     a function global scope, unless declared static.  */
>  	  if (current_function_decl != NULL_TREE && !TREE_STATIC (node))
>  	    TREE_PUBLIC (node) = 1;
> +	  /* Clear TREE_STATIC because DECL_EXTERNAL is set.  */
> +	  TREE_STATIC (node) = 0;
>  	}
>  
>        if (*no_add_attrs == false)

The above change apparently broke handling of dllimport C++ static data
members (on both trunk in 8.3), where the C++ FE requires that TREE_STATIC
is set on the static data members, on the other side handles well the case
when it is TREE_STATIC and DECL_EXTERNAL at the same time.

So, the following patch partially reverts the above change, though for
variables in RECORD/UNION_TYPE DECL_CONTEXTs only.  That should keep the
gcc.dg/pr88568.c testcase working, unbreaks the new testcase as well.

Unfortunately, I have no way to test this on mingw or cygwin, on the other
side it likely should work fine because for the static data members it
will do what it used to do before the above patch and no issues were
reported with those, except the pr88568.c C testcase.

Ok for trunk (and after a while for 8.4 too)?

2019-03-05  Jakub Jelinek  <jakub@redhat.com>

	PR c/88568
	* attribs.c (handle_dll_attribute): Don't clear TREE_STATIC for
	dllimport on VAR_DECLs with RECORD_TYPE or UNION_TYPE DECL_CONTEXT.

	* g++.dg/other/pr88568.C: New test.



	Jakub

Comments

Jason Merrill March 5, 2019, 9:03 p.m. UTC | #1 
On 3/5/19 3:14 PM, Jakub Jelinek wrote:
> On Thu, Jan 10, 2019 at 11:07:37AM +0100, Jakub Jelinek wrote:
>> 2019-01-10  Jakub Jelinek  <jakub@redhat.com>
>>
>> 	PR c/88568
>> 	* attribs.c (handle_dll_attribute): Clear TREE_STATIC after setting
>> 	DECL_EXTERNAL.
>>
>> 	* gcc.dg/pr88568.c: New test.
>>
>> --- gcc/attribs.c.jj	2019-01-05 12:06:12.055124090 +0100
>> +++ gcc/attribs.c	2019-01-07 12:57:09.739782281 +0100
>> @@ -1691,6 +1691,8 @@ handle_dll_attribute (tree * pnode, tree
>>   	     a function global scope, unless declared static.  */
>>   	  if (current_function_decl != NULL_TREE && !TREE_STATIC (node))
>>   	    TREE_PUBLIC (node) = 1;
>> +	  /* Clear TREE_STATIC because DECL_EXTERNAL is set.  */
>> +	  TREE_STATIC (node) = 0;
>>   	}
>>   
>>         if (*no_add_attrs == false)
> 
> The above change apparently broke handling of dllimport C++ static data
> members (on both trunk in 8.3), where the C++ FE requires that TREE_STATIC
> is set on the static data members, on the other side handles well the case
> when it is TREE_STATIC and DECL_EXTERNAL at the same time.

Yes, that flag combination seems entirely reasonable to me: it's a 
static-storage-duration variable that doesn't happen to be defined in 
this translation unit (yet).

In several cases the C++ front end leaves DECL_EXTERNAL set on things 
that have definitions until EOF, when we decide what actually needs to 
be emitted.  This is obsolete since the advent of cgraph, but I haven't 
found the time to rip it out.  It looks like we don't do that for 
namespace-scope variable templates, though, so they should be OK.

The patch is OK with me.

Jason
Jason Merrill March 5, 2019, 9:04 p.m. UTC | #2 
On 3/5/19 4:03 PM, Jason Merrill wrote:
> On 3/5/19 3:14 PM, Jakub Jelinek wrote:
>> On Thu, Jan 10, 2019 at 11:07:37AM +0100, Jakub Jelinek wrote:
>>> 2019-01-10  Jakub Jelinek  <jakub@redhat.com>
>>>
>>>     PR c/88568
>>>     * attribs.c (handle_dll_attribute): Clear TREE_STATIC after setting
>>>     DECL_EXTERNAL.
>>>
>>>     * gcc.dg/pr88568.c: New test.
>>>
>>> --- gcc/attribs.c.jj    2019-01-05 12:06:12.055124090 +0100
>>> +++ gcc/attribs.c    2019-01-07 12:57:09.739782281 +0100
>>> @@ -1691,6 +1691,8 @@ handle_dll_attribute (tree * pnode, tree
>>>            a function global scope, unless declared static.  */
>>>         if (current_function_decl != NULL_TREE && !TREE_STATIC (node))
>>>           TREE_PUBLIC (node) = 1;
>>> +      /* Clear TREE_STATIC because DECL_EXTERNAL is set.  */
>>> +      TREE_STATIC (node) = 0;
>>>       }
>>>         if (*no_add_attrs == false)
>>
>> The above change apparently broke handling of dllimport C++ static data
>> members (on both trunk in 8.3), where the C++ FE requires that 
>> TREE_STATIC
>> is set on the static data members, on the other side handles well the 
>> case
>> when it is TREE_STATIC and DECL_EXTERNAL at the same time.
> 
> Yes, that flag combination seems entirely reasonable to me: it's a 
> static-storage-duration variable that doesn't happen to be defined in 
> this translation unit (yet).

Or, more specifically, that we aren't (yet) planning to emit in this 
translation unit even if we have a definition.

Jason
diff mbox series

Patch

--- gcc/attribs.c.jj	2019-01-10 11:44:07.122511397 +0100
+++ gcc/attribs.c	2019-03-05 13:59:51.745924578 +0100
@@ -1691,8 +1691,11 @@  handle_dll_attribute (tree * pnode, tree
 	     a function global scope, unless declared static.  */
 	  if (current_function_decl != NULL_TREE && !TREE_STATIC (node))
 	    TREE_PUBLIC (node) = 1;
-	  /* Clear TREE_STATIC because DECL_EXTERNAL is set.  */
-	  TREE_STATIC (node) = 0;
+	  /* Clear TREE_STATIC because DECL_EXTERNAL is set, unless
+	     it is a C++ static data member.  */
+	  if (DECL_CONTEXT (node) == NULL_TREE
+	      || !RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (node)))
+	    TREE_STATIC (node) = 0;
 	}
 
       if (*no_add_attrs == false)
--- gcc/testsuite/g++.dg/other/pr88568.C.jj	2019-03-05 14:03:20.132509560 +0100
+++ gcc/testsuite/g++.dg/other/pr88568.C	2019-03-05 14:01:39.674155860 +0100
@@ -0,0 +1,13 @@ 
+// PR c/88568
+// { dg-do compile }
+// { dg-require-dll "" }
+
+struct S {
+  __attribute__((dllimport)) static const char foo[];
+};
+
+int
+foo (int x)
+{
+  return S::foo[x];
+}