diff mbox

Fix for PR sanitize/65000 introduces an unusual gcc_assert

Message ID alpine.LNX.2.20.1510282036530.13239@monopod.intra.ispras.ru
State New
Headers show

Commit Message

Alexander Monakov Oct. 28, 2015, 5:47 p.m. UTC 
Hello Richard,

Your commit to fix PR 65000 (pasted below) introduced
  gcc_assert (ri = (int)ri);

I'm unclear what is meant there; if equality test was meant, that looks
suspicious to me because truncating conversion is implementation-defined.
Can you please comment?

(I found this after noticing assignment-in-assertion in nvptx.c, but apart
from two instances there and this one, I didn't find others in GCC)

Thanks.
Alexander

PR sanitize/65000
 
 * tree-eh.c (mark_reachable_handlers): Mark source and destination
 regions of __builtin_eh_copy_values.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@220626 138bc75d-0d04-0410-961f-82ee72b054a4

Comments

Richard Henderson Oct. 28, 2015, 6:07 p.m. UTC | #1 
On 10/28/2015 10:47 AM, Alexander Monakov wrote:
> Hello Richard,
>
> Your commit to fix PR 65000 (pasted below) introduced
>    gcc_assert (ri = (int)ri);
>
> I'm unclear what is meant there; if equality test was meant, that looks
> suspicious to me because truncating conversion is implementation-defined.

Of course equality was meant.  And since the argument to bitmap_set_bit is of 
type int, the same truncation would happen there.  So this just verifies that 
we don't lose information during the conversion.

Since implementation defined doesn't mean undefined, I see no problem.


r~
Richard Biener Oct. 28, 2015, 7:09 p.m. UTC | #2 
On October 28, 2015 7:07:54 PM GMT+01:00, Richard Henderson <rth@redhat.com> wrote:
>On 10/28/2015 10:47 AM, Alexander Monakov wrote:
>> Hello Richard,
>>
>> Your commit to fix PR 65000 (pasted below) introduced
>>    gcc_assert (ri = (int)ri);
>>
>> I'm unclear what is meant there; if equality test was meant, that
>looks
>> suspicious to me because truncating conversion is
>implementation-defined.
>
>Of course equality was meant.  And since the argument to bitmap_set_bit
>is of 
>type int, the same truncation would happen there.  So this just
>verifies that 
>we don't lose information during the conversion.
>
>Since implementation defined doesn't mean undefined, I see no problem.

And ri == 0 cannot happen?

>
>r~
Richard Henderson Oct. 28, 2015, 7:39 p.m. UTC | #3 
On 10/28/2015 12:09 PM, Richard Biener wrote:
> On October 28, 2015 7:07:54 PM GMT+01:00, Richard Henderson <rth@redhat.com> wrote:
>> On 10/28/2015 10:47 AM, Alexander Monakov wrote:
>>> Hello Richard,
>>>
>>> Your commit to fix PR 65000 (pasted below) introduced
>>>     gcc_assert (ri = (int)ri);
>>>
>>> I'm unclear what is meant there; if equality test was meant, that
>> looks
>>> suspicious to me because truncating conversion is
>> implementation-defined.
>>
>> Of course equality was meant.  And since the argument to bitmap_set_bit
>> is of
>> type int, the same truncation would happen there.  So this just
>> verifies that
>> we don't lose information during the conversion.
>>
>> Since implementation defined doesn't mean undefined, I see no problem.
>
> And ri == 0 cannot happen?

IIRC, 0 is reserved for nothrow, so exception indicies are 1 based.

That said, I've fixed the assert in the obvious way.


r~
diff mbox

Patch

diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 159fa2b..3c45f37 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -3859,6 +3859,17 @@  mark_reachable_handlers (sbitmap *r_reachablep, sbitmap *lp_reachablep)
                              gimple_eh_dispatch_region (
                                 as_a <geh_dispatch *> (stmt)));    
              break;     
+           case GIMPLE_CALL:
+             if (gimple_call_builtin_p (stmt, BUILT_IN_EH_COPY_VALUES))
+               for (int i = 0; i < 2; ++i)
+                 {      
+                   tree rt = gimple_call_arg (stmt, i);              
+                   HOST_WIDE_INT ri = tree_to_shwi (rt);             
+
+                   gcc_assert (ri = (int)ri);
+                   bitmap_set_bit (r_reachable, ri);           
+                 }      
+             break;     
            default:     
              break;     
            }