Re: [PATCH] middle-end/114599 - fix bitmap allocation for check_ifunc_callee_symtab_nodes

["Jørgen Kvalsvik" <j@lambda.is>]

On 05/04/2024 16:05, H.J. Lu wrote:
> On Fri, Apr 5, 2024 at 6:52 AM Richard Biener <rguenther@suse.de> wrote:
>>
>>
>>
>>> Am 05.04.2024 um 15:46 schrieb H.J. Lu <hjl.tools@gmail.com>:
>>>
>>> On Fri, Apr 5, 2024 at 1:21 AM Richard Biener <rguenther@suse.de> wrote:
>>>>
>>>> There's no default bitmap obstack during global CTORs, so allocate the
>>>> bitmap locally.
>>>>
>>>> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>>>>
>>>> Richard.
>>>>
>>>>         PR middle-end/114599
>>>>         * symtab.cc (ifunc_ref_map): Do not use auto_bitmap.
>>>>         (is_caller_ifunc_resolver): Optimize bitmap_bit_p/bitmap_set_bit
>>>>         pair.
>>>>         (symtab_node::check_ifunc_callee_symtab_nodes): Properly
>>>>         allocate ifunc_ref_map here.
>>>> ---
>>>> gcc/symtab.cc | 11 +++++++----
>>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/gcc/symtab.cc b/gcc/symtab.cc
>>>> index 3256133891d..3b018ab3ea2 100644
>>>> --- a/gcc/symtab.cc
>>>> +++ b/gcc/symtab.cc
>>>> @@ -1383,7 +1383,7 @@ check_ifunc_resolver (cgraph_node *node, void *data)
>>>>    return false;
>>>> }
>>>>
>>>> -static auto_bitmap ifunc_ref_map;
>>>> +static bitmap ifunc_ref_map;
>>>>
>>>> /* Return true if any caller of NODE is an ifunc resolver.  */
>>>>
>>>> @@ -1404,9 +1404,8 @@ is_caller_ifunc_resolver (cgraph_node *node)
>>>>
>>>>        /* Skip if it has been visited.  */
>>>>        unsigned int uid = e->caller->get_uid ();
>>>> -      if (bitmap_bit_p (ifunc_ref_map, uid))
>>>> +      if (!bitmap_set_bit (ifunc_ref_map, uid))
>>>>         continue;
>>>> -      bitmap_set_bit (ifunc_ref_map, uid);
>>>>
>>>>        if (is_caller_ifunc_resolver (e->caller))
>>>>         {
>>>> @@ -1437,6 +1436,9 @@ symtab_node::check_ifunc_callee_symtab_nodes (void)
>>>> {
>>>>    symtab_node *node;
>>>>
>>>> +  bitmap_obstack_initialize (NULL);
>>>> +  ifunc_ref_map = BITMAP_ALLOC (NULL);
>>>> +
>>>>    FOR_EACH_SYMBOL (node)
>>>>      {
>>>>        cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
>>>> @@ -1455,7 +1457,8 @@ symtab_node::check_ifunc_callee_symtab_nodes (void)
>>>>         cnode->called_by_ifunc_resolver = true;
>>>>      }
>>>>
>>>> -  bitmap_clear (ifunc_ref_map);
>>>> +  BITMAP_FREE (ifunc_ref_map);
>>>> +  bitmap_obstack_release (NULL);
>>>> }
>>>>
>>>> /* Verify symbol table for internal consistency.  */
>>>> --
>>>> 2.35.3
>>>
>>> The bug isn't fixed:
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114599#c5
>>
>> Ah, I reproduced with -coverage and that is fixed now.  The still existing bug must be something unrelated.
>>
> 
> It is a bug in the -fcondition-coverage patch.
> 

It is. What seems to happen is that the -O2 causes a different path 
through gimplification so the gcond is not associated with the 
expression, and the condition coverage just assumes that all if it is 
even invoked, all gconds it finds will be associated. Since the cond_uid 
map is created on the first association (which does not happen) there is 
a nullptr deref when the cond id is looked up.

I think the fix is to simply guard the cond_uid access and fall back to 
the same path as on key misses, which is to skip the expression 
altogether. I would like to understand _why_ the gcond isn't associated 
with the expr in the first place, if anything to make a better decision 
here.

Thanks,
Jørgen