diff mbox series

ipa: silent -Wodr notes with -w

Message ID c210778d-e7d8-5d00-7255-329f7dfec052@suse.cz
State Accepted
Headers
Series ipa: silent -Wodr notes with -w |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Martin Liška Dec. 2, 2022, 11:27 a.m. UTC 
  If -w is used, warn_odr properly sets *warned = false and
so it should be preserved when calling warn_types_mismatch.

Noticed that during a LTO reduction where I used -w.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
	value if set.
---
 gcc/ipa-devirt.cc | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Martin Liška Dec. 9, 2022, 8:27 a.m. UTC | #1 
PING^1

On 12/2/22 12:27, Martin Liška wrote:
> If -w is used, warn_odr properly sets *warned = false and
> so it should be preserved when calling warn_types_mismatch.
> 
> Noticed that during a LTO reduction where I used -w.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
> 	value if set.
> ---
>  gcc/ipa-devirt.cc | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
> index 265d07bb354..bcdc50c5bd7 100644
> --- a/gcc/ipa-devirt.cc
> +++ b/gcc/ipa-devirt.cc
> @@ -1300,7 +1300,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	      warn_odr (t1, t2, NULL, NULL, warn, warned,
>  			G_("it is defined as a pointer to different type "
>  			   "in another translation unit"));
> -	      if (warn && warned)
> +	      if (warn && (warned == NULL || *warned))
>  	        warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
>  				     loc1, loc2);
>  	      return false;
> @@ -1315,7 +1315,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>  		    G_("a different type is defined "
>  		       "in another translation unit"));
> -	  if (warn && warned)
> +	  if (warn && (warned == NULL || *warned))
>  	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>  	  return false;
>  	}
> @@ -1333,7 +1333,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	    warn_odr (t1, t2, NULL, NULL, warn, warned,
>  		      G_("a different type is defined in another "
>  			 "translation unit"));
> -	    if (warn && warned)
> +	    if (warn && (warned == NULL || *warned))
>  	      warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>  	  }
>  	gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
> @@ -1375,7 +1375,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>  		    G_("has different return value "
>  		       "in another translation unit"));
> -	  if (warn && warned)
> +	  if (warn && (warned == NULL || *warned))
>  	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>  	  return false;
>  	}
> @@ -1398,7 +1398,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  		  warn_odr (t1, t2, NULL, NULL, warn, warned,
>  			    G_("has different parameters in another "
>  			       "translation unit"));
> -		  if (warn && warned)
> +		  if (warn && (warned == NULL || *warned))
>  		    warn_types_mismatch (TREE_VALUE (parms1),
>  					 TREE_VALUE (parms2), loc1, loc2);
>  		  return false;
> @@ -1484,7 +1484,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  		    warn_odr (t1, t2, f1, f2, warn, warned,
>  			      G_("a field of same name but different type "
>  				 "is defined in another translation unit"));
> -		    if (warn && warned)
> +		    if (warn && (warned == NULL || *warned))
>  		      warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
>  		    return false;
>  		  }
Martin Liška Dec. 22, 2022, 12:15 p.m. UTC | #2 
PING^2

On 12/9/22 09:27, Martin Liška wrote:
> PING^1
> 
> On 12/2/22 12:27, Martin Liška wrote:
>> If -w is used, warn_odr properly sets *warned = false and
>> so it should be preserved when calling warn_types_mismatch.
>>
>> Noticed that during a LTO reduction where I used -w.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
>> 	value if set.
>> ---
>>  gcc/ipa-devirt.cc | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
>> index 265d07bb354..bcdc50c5bd7 100644
>> --- a/gcc/ipa-devirt.cc
>> +++ b/gcc/ipa-devirt.cc
>> @@ -1300,7 +1300,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>  	      warn_odr (t1, t2, NULL, NULL, warn, warned,
>>  			G_("it is defined as a pointer to different type "
>>  			   "in another translation unit"));
>> -	      if (warn && warned)
>> +	      if (warn && (warned == NULL || *warned))
>>  	        warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
>>  				     loc1, loc2);
>>  	      return false;
>> @@ -1315,7 +1315,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>  	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>>  		    G_("a different type is defined "
>>  		       "in another translation unit"));
>> -	  if (warn && warned)
>> +	  if (warn && (warned == NULL || *warned))
>>  	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>  	  return false;
>>  	}
>> @@ -1333,7 +1333,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>  	    warn_odr (t1, t2, NULL, NULL, warn, warned,
>>  		      G_("a different type is defined in another "
>>  			 "translation unit"));
>> -	    if (warn && warned)
>> +	    if (warn && (warned == NULL || *warned))
>>  	      warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>  	  }
>>  	gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
>> @@ -1375,7 +1375,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>  	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>>  		    G_("has different return value "
>>  		       "in another translation unit"));
>> -	  if (warn && warned)
>> +	  if (warn && (warned == NULL || *warned))
>>  	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>  	  return false;
>>  	}
>> @@ -1398,7 +1398,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>  		  warn_odr (t1, t2, NULL, NULL, warn, warned,
>>  			    G_("has different parameters in another "
>>  			       "translation unit"));
>> -		  if (warn && warned)
>> +		  if (warn && (warned == NULL || *warned))
>>  		    warn_types_mismatch (TREE_VALUE (parms1),
>>  					 TREE_VALUE (parms2), loc1, loc2);
>>  		  return false;
>> @@ -1484,7 +1484,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>  		    warn_odr (t1, t2, f1, f2, warn, warned,
>>  			      G_("a field of same name but different type "
>>  				 "is defined in another translation unit"));
>> -		    if (warn && warned)
>> +		    if (warn && (warned == NULL || *warned))
>>  		      warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
>>  		    return false;
>>  		  }
>
Martin Liška Jan. 13, 2023, 9:09 a.m. UTC | #3 
PING^3

On 12/22/22 13:15, Martin Liška wrote:
> PING^2
> 
> On 12/9/22 09:27, Martin Liška wrote:
>> PING^1
>>
>> On 12/2/22 12:27, Martin Liška wrote:
>>> If -w is used, warn_odr properly sets *warned = false and
>>> so it should be preserved when calling warn_types_mismatch.
>>>
>>> Noticed that during a LTO reduction where I used -w.
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>> Thanks,
>>> Martin
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
>>> 	value if set.
>>> ---
>>>   gcc/ipa-devirt.cc | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
>>> index 265d07bb354..bcdc50c5bd7 100644
>>> --- a/gcc/ipa-devirt.cc
>>> +++ b/gcc/ipa-devirt.cc
>>> @@ -1300,7 +1300,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>   	      warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>   			G_("it is defined as a pointer to different type "
>>>   			   "in another translation unit"));
>>> -	      if (warn && warned)
>>> +	      if (warn && (warned == NULL || *warned))
>>>   	        warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
>>>   				     loc1, loc2);
>>>   	      return false;
>>> @@ -1315,7 +1315,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>   	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>   		    G_("a different type is defined "
>>>   		       "in another translation unit"));
>>> -	  if (warn && warned)
>>> +	  if (warn && (warned == NULL || *warned))
>>>   	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>   	  return false;
>>>   	}
>>> @@ -1333,7 +1333,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>   	    warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>   		      G_("a different type is defined in another "
>>>   			 "translation unit"));
>>> -	    if (warn && warned)
>>> +	    if (warn && (warned == NULL || *warned))
>>>   	      warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>   	  }
>>>   	gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
>>> @@ -1375,7 +1375,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>   	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>   		    G_("has different return value "
>>>   		       "in another translation unit"));
>>> -	  if (warn && warned)
>>> +	  if (warn && (warned == NULL || *warned))
>>>   	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>   	  return false;
>>>   	}
>>> @@ -1398,7 +1398,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>   		  warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>   			    G_("has different parameters in another "
>>>   			       "translation unit"));
>>> -		  if (warn && warned)
>>> +		  if (warn && (warned == NULL || *warned))
>>>   		    warn_types_mismatch (TREE_VALUE (parms1),
>>>   					 TREE_VALUE (parms2), loc1, loc2);
>>>   		  return false;
>>> @@ -1484,7 +1484,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>   		    warn_odr (t1, t2, f1, f2, warn, warned,
>>>   			      G_("a field of same name but different type "
>>>   				 "is defined in another translation unit"));
>>> -		    if (warn && warned)
>>> +		    if (warn && (warned == NULL || *warned))
>>>   		      warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
>>>   		    return false;
>>>   		  }
>>
>
Martin Liška Jan. 24, 2023, 1:34 p.m. UTC | #4 
PING^4

On 1/13/23 10:09, Martin Liška wrote:
> PING^3
> 
> On 12/22/22 13:15, Martin Liška wrote:
>> PING^2
>>
>> On 12/9/22 09:27, Martin Liška wrote:
>>> PING^1
>>>
>>> On 12/2/22 12:27, Martin Liška wrote:
>>>> If -w is used, warn_odr properly sets *warned = false and
>>>> so it should be preserved when calling warn_types_mismatch.
>>>>
>>>> Noticed that during a LTO reduction where I used -w.
>>>>
>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>
>>>> Ready to be installed?
>>>> Thanks,
>>>> Martin
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>     * ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
>>>>     value if set.
>>>> ---
>>>>   gcc/ipa-devirt.cc | 12 ++++++------
>>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
>>>> index 265d07bb354..bcdc50c5bd7 100644
>>>> --- a/gcc/ipa-devirt.cc
>>>> +++ b/gcc/ipa-devirt.cc
>>>> @@ -1300,7 +1300,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>             warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>               G_("it is defined as a pointer to different type "
>>>>                  "in another translation unit"));
>>>> -          if (warn && warned)
>>>> +          if (warn && (warned == NULL || *warned))
>>>>               warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
>>>>                        loc1, loc2);
>>>>             return false;
>>>> @@ -1315,7 +1315,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>         warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>               G_("a different type is defined "
>>>>                  "in another translation unit"));
>>>> -      if (warn && warned)
>>>> +      if (warn && (warned == NULL || *warned))
>>>>           warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>>         return false;
>>>>       }
>>>> @@ -1333,7 +1333,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>           warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>                 G_("a different type is defined in another "
>>>>                "translation unit"));
>>>> -        if (warn && warned)
>>>> +        if (warn && (warned == NULL || *warned))
>>>>             warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>>         }
>>>>       gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
>>>> @@ -1375,7 +1375,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>         warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>               G_("has different return value "
>>>>                  "in another translation unit"));
>>>> -      if (warn && warned)
>>>> +      if (warn && (warned == NULL || *warned))
>>>>           warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>>         return false;
>>>>       }
>>>> @@ -1398,7 +1398,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>             warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>                   G_("has different parameters in another "
>>>>                      "translation unit"));
>>>> -          if (warn && warned)
>>>> +          if (warn && (warned == NULL || *warned))
>>>>               warn_types_mismatch (TREE_VALUE (parms1),
>>>>                        TREE_VALUE (parms2), loc1, loc2);
>>>>             return false;
>>>> @@ -1484,7 +1484,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>               warn_odr (t1, t2, f1, f2, warn, warned,
>>>>                     G_("a field of same name but different type "
>>>>                    "is defined in another translation unit"));
>>>> -            if (warn && warned)
>>>> +            if (warn && (warned == NULL || *warned))
>>>>                 warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
>>>>               return false;
>>>>             }
>>>
>>
>
Martin Liška Feb. 1, 2023, 1:13 p.m. UTC | #5 
PING^5

On 1/24/23 14:34, Martin Liška wrote:
> PING^4
> 
> On 1/13/23 10:09, Martin Liška wrote:
>> PING^3
>>
>> On 12/22/22 13:15, Martin Liška wrote:
>>> PING^2
>>>
>>> On 12/9/22 09:27, Martin Liška wrote:
>>>> PING^1
>>>>
>>>> On 12/2/22 12:27, Martin Liška wrote:
>>>>> If -w is used, warn_odr properly sets *warned = false and
>>>>> so it should be preserved when calling warn_types_mismatch.
>>>>>
>>>>> Noticed that during a LTO reduction where I used -w.
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>
>>>>> Ready to be installed?
>>>>> Thanks,
>>>>> Martin
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>>      * ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
>>>>>      value if set.
>>>>> ---
>>>>>    gcc/ipa-devirt.cc | 12 ++++++------
>>>>>    1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
>>>>> index 265d07bb354..bcdc50c5bd7 100644
>>>>> --- a/gcc/ipa-devirt.cc
>>>>> +++ b/gcc/ipa-devirt.cc
>>>>> @@ -1300,7 +1300,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>>              warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>>                G_("it is defined as a pointer to different type "
>>>>>                   "in another translation unit"));
>>>>> -          if (warn && warned)
>>>>> +          if (warn && (warned == NULL || *warned))
>>>>>                warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
>>>>>                         loc1, loc2);
>>>>>              return false;
>>>>> @@ -1315,7 +1315,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>>          warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>>                G_("a different type is defined "
>>>>>                   "in another translation unit"));
>>>>> -      if (warn && warned)
>>>>> +      if (warn && (warned == NULL || *warned))
>>>>>            warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>>>          return false;
>>>>>        }
>>>>> @@ -1333,7 +1333,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>>            warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>>                  G_("a different type is defined in another "
>>>>>                 "translation unit"));
>>>>> -        if (warn && warned)
>>>>> +        if (warn && (warned == NULL || *warned))
>>>>>              warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>>>          }
>>>>>        gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
>>>>> @@ -1375,7 +1375,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>>          warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>>                G_("has different return value "
>>>>>                   "in another translation unit"));
>>>>> -      if (warn && warned)
>>>>> +      if (warn && (warned == NULL || *warned))
>>>>>            warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>>>>>          return false;
>>>>>        }
>>>>> @@ -1398,7 +1398,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>>              warn_odr (t1, t2, NULL, NULL, warn, warned,
>>>>>                    G_("has different parameters in another "
>>>>>                       "translation unit"));
>>>>> -          if (warn && warned)
>>>>> +          if (warn && (warned == NULL || *warned))
>>>>>                warn_types_mismatch (TREE_VALUE (parms1),
>>>>>                         TREE_VALUE (parms2), loc1, loc2);
>>>>>              return false;
>>>>> @@ -1484,7 +1484,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>>>>>                warn_odr (t1, t2, f1, f2, warn, warned,
>>>>>                      G_("a field of same name but different type "
>>>>>                     "is defined in another translation unit"));
>>>>> -            if (warn && warned)
>>>>> +            if (warn && (warned == NULL || *warned))
>>>>>                  warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
>>>>>                return false;
>>>>>              }
>>>>
>>>
>>
>
Martin Jambor Feb. 1, 2023, 2:26 p.m. UTC | #6 
Hi,

On Fri, Dec 02 2022, Martin Liška wrote:
> If -w is used, warn_odr properly sets *warned = false and
> so it should be preserved when calling warn_types_mismatch.
>
> Noticed that during a LTO reduction where I used -w.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
> 	value if set.

Sorry for skipping this for so long, usually ODR stuff is... interesting
to the point I gladly leave it to Honza.

Please go ahead and commit the patch.  The way I read the code, your
version must have been the intended behavior and the dereference is
missing.

Thanks,

Martin
Martin Liška Feb. 1, 2023, 2:44 p.m. UTC | #7 
On 2/1/23 15:26, Martin Jambor wrote:
> Hi,
> 
> On Fri, Dec 02 2022, Martin Liška wrote:
>> If -w is used, warn_odr properly sets *warned = false and
>> so it should be preserved when calling warn_types_mismatch.
>>
>> Noticed that during a LTO reduction where I used -w.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
>> 	value if set.
> 

Hi.

> Sorry for skipping this for so long, usually ODR stuff is... interesting
> to the point I gladly leave it to Honza.

Makes sense, however, he's not much active when it comes to patch review.

> 
> Please go ahead and commit the patch.  The way I read the code, your
> version must have been the intended behavior and the dereference is
> missing.

Yep, the patch seems to me quite straightforward.

Thanks,
Martin

> 
> Thanks,
> 
> Martin
>
Jan Hubicka Feb. 7, 2023, 12:35 a.m. UTC | #8 
> On 2/1/23 15:26, Martin Jambor wrote:
> > Hi,
> > 
> > On Fri, Dec 02 2022, Martin Liška wrote:
> > > If -w is used, warn_odr properly sets *warned = false and
> > > so it should be preserved when calling warn_types_mismatch.
> > > 
> > > Noticed that during a LTO reduction where I used -w.
> > > 
> > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > > 
> > > Ready to be installed?
> > > Thanks,
> > > Martin
> > > 
> > > gcc/ChangeLog:
> > > 
> > > 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
> > > 	value if set.
> > 
> 
> Hi.
> 
> > Sorry for skipping this for so long, usually ODR stuff is... interesting
> > to the point I gladly leave it to Honza.
> 
> Makes sense, however, he's not much active when it comes to patch review.

Sorry, I was confused by the patch and delayed reply to figure out what
you are trying to fix.  Indeed the dererence is missing here, however
every caller that sets warn to true should also set warned to non-NULL.
So indeed derefernce is missing, but I think the check for
warned == NULL should not be necessary.

Honza
> 
> > 
> > Please go ahead and commit the patch.  The way I read the code, your
> > version must have been the intended behavior and the dereference is
> > missing.
> 
> Yep, the patch seems to me quite straightforward.
> 
> Thanks,
> Martin
> 
> > 
> > Thanks,
> > 
> > Martin
> > 
>
Jan Hubicka Feb. 9, 2023, 12:10 a.m. UTC | #9 
> > On 2/1/23 15:26, Martin Jambor wrote:
> > > Hi,
> > > 
> > > On Fri, Dec 02 2022, Martin Liška wrote:
> > > > If -w is used, warn_odr properly sets *warned = false and
> > > > so it should be preserved when calling warn_types_mismatch.
> > > > 
> > > > Noticed that during a LTO reduction where I used -w.
> > > > 
> > > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > > > 
> > > > Ready to be installed?
> > > > Thanks,
> > > > Martin
> > > > 
> > > > gcc/ChangeLog:
> > > > 
> > > > 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
> > > > 	value if set.
> > > 
> > 
> > Hi.
> > 
> > > Sorry for skipping this for so long, usually ODR stuff is... interesting
> > > to the point I gladly leave it to Honza.
> > 
> > Makes sense, however, he's not much active when it comes to patch review.
> 
> Sorry, I was confused by the patch and delayed reply to figure out what
> you are trying to fix.  Indeed the dererence is missing here, however
> every caller that sets warn to true should also set warned to non-NULL.
> So indeed derefernce is missing, but I think the check for
> warned == NULL should not be necessary.

This seems to bootstrap with LTO.  I am not sure what testcase you
looked in, but unless there as a good reason to include the NULL check,
I would rmeove it as it makes it harder to see what is going on.

honza

diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
index 14cf132c767..819860258d1 100644
--- a/gcc/ipa-devirt.cc
+++ b/gcc/ipa-devirt.cc
@@ -1221,6 +1221,9 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 			hash_set<type_pair> *visited,
 			location_t loc1, location_t loc2)
 {
+  /* If we are asked to warn, we need warned to keep track if warning was
+     output.  */
+  gcc_assert (!warn || warned);
   /* Check first for the obvious case of pointer identity.  */
   if (t1 == t2)
     return true;
@@ -1300,7 +1303,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	      warn_odr (t1, t2, NULL, NULL, warn, warned,
 			G_("it is defined as a pointer to different type "
 			   "in another translation unit"));
-	      if (warn && (warned == NULL || *warned))
+	      if (warn && *warned)
 	        warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
 				     loc1, loc2);
 	      return false;
@@ -1315,7 +1318,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	  warn_odr (t1, t2, NULL, NULL, warn, warned,
 		    G_("a different type is defined "
 		       "in another translation unit"));
-	  if (warn && (warned == NULL || *warned))
+	  if (warn && *warned)
 	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
 	  return false;
 	}
@@ -1333,7 +1336,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	    warn_odr (t1, t2, NULL, NULL, warn, warned,
 		      G_("a different type is defined in another "
 			 "translation unit"));
-	    if (warn && (warned == NULL || *warned))
+	    if (warn && *warned)
 	      warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
 	  }
 	gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
@@ -1375,7 +1378,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	  warn_odr (t1, t2, NULL, NULL, warn, warned,
 		    G_("has different return value "
 		       "in another translation unit"));
-	  if (warn && (warned == NULL || *warned))
+	  if (warn && *warned)
 	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
 	  return false;
 	}
@@ -1398,7 +1401,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 		  warn_odr (t1, t2, NULL, NULL, warn, warned,
 			    G_("has different parameters in another "
 			       "translation unit"));
-		  if (warn && (warned == NULL || *warned))
+		  if (warn && *warned)
 		    warn_types_mismatch (TREE_VALUE (parms1),
 					 TREE_VALUE (parms2), loc1, loc2);
 		  return false;
@@ -1484,7 +1487,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 		    warn_odr (t1, t2, f1, f2, warn, warned,
 			      G_("a field of same name but different type "
 				 "is defined in another translation unit"));
-		    if (warn && (warned == NULL || *warned))
+		    if (warn && *warned)
 		      warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
 		    return false;
 		  }
Martin Liška Feb. 9, 2023, 7:44 a.m. UTC | #10 
On 2/9/23 01:10, Jan Hubicka wrote:
>>> On 2/1/23 15:26, Martin Jambor wrote:
>>>> Hi,
>>>>
>>>> On Fri, Dec 02 2022, Martin Liška wrote:
>>>>> If -w is used, warn_odr properly sets *warned = false and
>>>>> so it should be preserved when calling warn_types_mismatch.
>>>>>
>>>>> Noticed that during a LTO reduction where I used -w.
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>
>>>>> Ready to be installed?
>>>>> Thanks,
>>>>> Martin
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> 	* ipa-devirt.cc (odr_types_equivalent_p): Respect *warned
>>>>> 	value if set.
>>>>
>>>
>>> Hi.
>>>
>>>> Sorry for skipping this for so long, usually ODR stuff is... interesting
>>>> to the point I gladly leave it to Honza.
>>>
>>> Makes sense, however, he's not much active when it comes to patch review.
>>
>> Sorry, I was confused by the patch and delayed reply to figure out what
>> you are trying to fix.  Indeed the dererence is missing here, however
>> every caller that sets warn to true should also set warned to non-NULL.
>> So indeed derefernce is missing, but I think the check for
>> warned == NULL should not be necessary.
> 
> This seems to bootstrap with LTO.  I am not sure what testcase you
> looked in, but unless there as a good reason to include the NULL check,
> I would rmeove it as it makes it harder to see what is going on.

Hi.

Thanks for the patch. Apparently, I noticed that during reduction of a test-case
where I used -w in order to silent all warnings.

So go ahead with your patch.

Martin

> 
> honza
> 
> diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
> index 14cf132c767..819860258d1 100644
> --- a/gcc/ipa-devirt.cc
> +++ b/gcc/ipa-devirt.cc
> @@ -1221,6 +1221,9 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  			hash_set<type_pair> *visited,
>  			location_t loc1, location_t loc2)
>  {
> +  /* If we are asked to warn, we need warned to keep track if warning was
> +     output.  */
> +  gcc_assert (!warn || warned);
>    /* Check first for the obvious case of pointer identity.  */
>    if (t1 == t2)
>      return true;
> @@ -1300,7 +1303,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	      warn_odr (t1, t2, NULL, NULL, warn, warned,
>  			G_("it is defined as a pointer to different type "
>  			   "in another translation unit"));
> -	      if (warn && (warned == NULL || *warned))
> +	      if (warn && *warned)
>  	        warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
>  				     loc1, loc2);
>  	      return false;
> @@ -1315,7 +1318,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>  		    G_("a different type is defined "
>  		       "in another translation unit"));
> -	  if (warn && (warned == NULL || *warned))
> +	  if (warn && *warned)
>  	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>  	  return false;
>  	}
> @@ -1333,7 +1336,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	    warn_odr (t1, t2, NULL, NULL, warn, warned,
>  		      G_("a different type is defined in another "
>  			 "translation unit"));
> -	    if (warn && (warned == NULL || *warned))
> +	    if (warn && *warned)
>  	      warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>  	  }
>  	gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
> @@ -1375,7 +1378,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  	  warn_odr (t1, t2, NULL, NULL, warn, warned,
>  		    G_("has different return value "
>  		       "in another translation unit"));
> -	  if (warn && (warned == NULL || *warned))
> +	  if (warn && *warned)
>  	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
>  	  return false;
>  	}
> @@ -1398,7 +1401,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  		  warn_odr (t1, t2, NULL, NULL, warn, warned,
>  			    G_("has different parameters in another "
>  			       "translation unit"));
> -		  if (warn && (warned == NULL || *warned))
> +		  if (warn && *warned)
>  		    warn_types_mismatch (TREE_VALUE (parms1),
>  					 TREE_VALUE (parms2), loc1, loc2);
>  		  return false;
> @@ -1484,7 +1487,7 @@ odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
>  		    warn_odr (t1, t2, f1, f2, warn, warned,
>  			      G_("a field of same name but different type "
>  				 "is defined in another translation unit"));
> -		    if (warn && (warned == NULL || *warned))
> +		    if (warn && *warned)
>  		      warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
>  		    return false;
>  		  }
diff mbox series

Patch

diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
index 265d07bb354..bcdc50c5bd7 100644
--- a/gcc/ipa-devirt.cc
+++ b/gcc/ipa-devirt.cc
@@ -1300,7 +1300,7 @@  odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	      warn_odr (t1, t2, NULL, NULL, warn, warned,
 			G_("it is defined as a pointer to different type "
 			   "in another translation unit"));
-	      if (warn && warned)
+	      if (warn && (warned == NULL || *warned))
 	        warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2),
 				     loc1, loc2);
 	      return false;
@@ -1315,7 +1315,7 @@  odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	  warn_odr (t1, t2, NULL, NULL, warn, warned,
 		    G_("a different type is defined "
 		       "in another translation unit"));
-	  if (warn && warned)
+	  if (warn && (warned == NULL || *warned))
 	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
 	  return false;
 	}
@@ -1333,7 +1333,7 @@  odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	    warn_odr (t1, t2, NULL, NULL, warn, warned,
 		      G_("a different type is defined in another "
 			 "translation unit"));
-	    if (warn && warned)
+	    if (warn && (warned == NULL || *warned))
 	      warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
 	  }
 	gcc_assert (TYPE_STRING_FLAG (t1) == TYPE_STRING_FLAG (t2));
@@ -1375,7 +1375,7 @@  odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 	  warn_odr (t1, t2, NULL, NULL, warn, warned,
 		    G_("has different return value "
 		       "in another translation unit"));
-	  if (warn && warned)
+	  if (warn && (warned == NULL || *warned))
 	    warn_types_mismatch (TREE_TYPE (t1), TREE_TYPE (t2), loc1, loc2);
 	  return false;
 	}
@@ -1398,7 +1398,7 @@  odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 		  warn_odr (t1, t2, NULL, NULL, warn, warned,
 			    G_("has different parameters in another "
 			       "translation unit"));
-		  if (warn && warned)
+		  if (warn && (warned == NULL || *warned))
 		    warn_types_mismatch (TREE_VALUE (parms1),
 					 TREE_VALUE (parms2), loc1, loc2);
 		  return false;
@@ -1484,7 +1484,7 @@  odr_types_equivalent_p (tree t1, tree t2, bool warn, bool *warned,
 		    warn_odr (t1, t2, f1, f2, warn, warned,
 			      G_("a field of same name but different type "
 				 "is defined in another translation unit"));
-		    if (warn && warned)
+		    if (warn && (warned == NULL || *warned))
 		      warn_types_mismatch (TREE_TYPE (f1), TREE_TYPE (f2), loc1, loc2);
 		    return false;
 		  }