[C++] Remove quick fix for c++/85553

Message ID 602ede9d-2165-551a-d61d-6fa711886eff@oracle.com
State New
Headers show
Series
  • [C++] Remove quick fix for c++/85553
Related show

Commit Message

Paolo Carlini Oct. 17, 2018, 5:20 p.m.
Hi,

as you probably remember, very close to the release of 8.1.0 we noticed 
that my fix for c++/70808 was causing c++/85553, which Jakub promptly 
fixed. However, we later found out that the real problem was a latent 
issue in convert, which I fixed in r259966. Thus, I think that in 
current trunk we can revert Jakub's quick fix, now redundant. Tested 
x86_64-linux.

Thanks! Paolo.

//////////////////
2018-10-17  Paolo Carlini  <paolo.carlini@oracle.com>

	* init.c (build_zero_init_1): Remove special casing for
	NULLPTR_TYPE_P (type), introduced by r259728 and made
	redundant by r259966.

Comments

Jakub Jelinek Oct. 17, 2018, 5:42 p.m. | #1
On Wed, Oct 17, 2018 at 07:20:53PM +0200, Paolo Carlini wrote:
> Hi,

> 

> as you probably remember, very close to the release of 8.1.0 we noticed that

> my fix for c++/70808 was causing c++/85553, which Jakub promptly fixed.

> However, we later found out that the real problem was a latent issue in

> convert, which I fixed in r259966. Thus, I think that in current trunk we

> can revert Jakub's quick fix, now redundant. Tested x86_64-linux.


Is there some desirable diagnostics you expect from the convert?
If not, build_int_cst is certainly cheaper.

> 2018-10-17  Paolo Carlini  <paolo.carlini@oracle.com>

> 

> 	* init.c (build_zero_init_1): Remove special casing for

> 	NULLPTR_TYPE_P (type), introduced by r259728 and made

> 	redundant by r259966.


> Index: init.c

> ===================================================================

> --- init.c	(revision 265241)

> +++ init.c	(working copy)

> @@ -180,10 +180,8 @@ build_zero_init_1 (tree type, tree nelts, bool sta

>         items with static storage duration that are not otherwise

>         initialized are initialized to zero.  */

>      ;

> -  else if (TYPE_PTR_OR_PTRMEM_P (type))

> +  else if (TYPE_PTR_OR_PTRMEM_P (type) || NULLPTR_TYPE_P (type))

>      init = fold (convert (type, nullptr_node));

> -  else if (NULLPTR_TYPE_P (type))

> -    init = build_int_cst (type, 0);

>    else if (SCALAR_TYPE_P (type))

>      init = fold (convert (type, integer_zero_node));

>    else if (RECORD_OR_UNION_CODE_P (TREE_CODE (type)))



	Jakub
Paolo Carlini Oct. 17, 2018, 7:38 p.m. | #2
Hi Jakub,

On 17/10/18 19:42, Jakub Jelinek wrote:
> On Wed, Oct 17, 2018 at 07:20:53PM +0200, Paolo Carlini wrote:

>> Hi,

>>

>> as you probably remember, very close to the release of 8.1.0 we noticed that

>> my fix for c++/70808 was causing c++/85553, which Jakub promptly fixed.

>> However, we later found out that the real problem was a latent issue in

>> convert, which I fixed in r259966. Thus, I think that in current trunk we

>> can revert Jakub's quick fix, now redundant. Tested x86_64-linux.

> Is there some desirable diagnostics you expect from the convert?

> If not, build_int_cst is certainly cheaper.


No, no diagnostics. But I'm still a bit nervous about the various way we 
handle those zero-initializations for pointer-ish types. Which is, if 
you wish, what fooled me in the first place when I simply assumed that 
convert was able to cope with the easy nullptr_node case. Say we do 
something like the below, can you see something wrong with it? Only 
lightly tested so far but appears to work fine at least on x86_64...

Thanks, Paolo.

///////////////////
Index: init.c
===================================================================
--- init.c	(revision 265241)
+++ init.c	(working copy)
@@ -180,10 +180,10 @@ build_zero_init_1 (tree type, tree nelts, bool sta
        items with static storage duration that are not otherwise
        initialized are initialized to zero.  */
     ;
-  else if (TYPE_PTR_OR_PTRMEM_P (type))
+  else if (TYPE_PTR_P (type) || NULLPTR_TYPE_P (type))
+    init = build_int_cst (type, 0);
+  else if (TYPE_PTRMEM_P (type))
     init = fold (convert (type, nullptr_node));
-  else if (NULLPTR_TYPE_P (type))
-    init = build_int_cst (type, 0);
   else if (SCALAR_TYPE_P (type))
     init = fold (convert (type, integer_zero_node));
   else if (RECORD_OR_UNION_CODE_P (TREE_CODE (type)))
Jason Merrill Oct. 24, 2018, 7:28 p.m. | #3
On 10/17/18 3:38 PM, Paolo Carlini wrote:
> Hi Jakub,

> 

> On 17/10/18 19:42, Jakub Jelinek wrote:

>> On Wed, Oct 17, 2018 at 07:20:53PM +0200, Paolo Carlini wrote:

>>> Hi,

>>>

>>> as you probably remember, very close to the release of 8.1.0 we 

>>> noticed that

>>> my fix for c++/70808 was causing c++/85553, which Jakub promptly fixed.

>>> However, we later found out that the real problem was a latent issue in

>>> convert, which I fixed in r259966. Thus, I think that in current 

>>> trunk we

>>> can revert Jakub's quick fix, now redundant. Tested x86_64-linux.

>> Is there some desirable diagnostics you expect from the convert?

>> If not, build_int_cst is certainly cheaper.

> 

> No, no diagnostics. But I'm still a bit nervous about the various way we 

> handle those zero-initializations for pointer-ish types. Which is, if 

> you wish, what fooled me in the first place when I simply assumed that 

> convert was able to cope with the easy nullptr_node case. Say we do 

> something like the below, can you see something wrong with it? Only 

> lightly tested so far but appears to work fine at least on x86_64...


This is fine.

Jason

Patch

Index: init.c
===================================================================
--- init.c	(revision 265241)
+++ init.c	(working copy)
@@ -180,10 +180,8 @@  build_zero_init_1 (tree type, tree nelts, bool sta
        items with static storage duration that are not otherwise
        initialized are initialized to zero.  */
     ;
-  else if (TYPE_PTR_OR_PTRMEM_P (type))
+  else if (TYPE_PTR_OR_PTRMEM_P (type) || NULLPTR_TYPE_P (type))
     init = fold (convert (type, nullptr_node));
-  else if (NULLPTR_TYPE_P (type))
-    init = build_int_cst (type, 0);
   else if (SCALAR_TYPE_P (type))
     init = fold (convert (type, integer_zero_node));
   else if (RECORD_OR_UNION_CODE_P (TREE_CODE (type)))