Re: [PATCH] gcov: Fix -fprofile-update=atomic

[Sebastian Huber <sebastian.huber@embedded-brains.de>]

On 13/12/2022 15:30, Richard Biener wrote:
> On Fri, Dec 9, 2022 at 2:56 PM Sebastian Huber
> <sebastian.huber@embedded-brains.de>  wrote:
>> The code coverage support uses counters to determine which edges in the control
>> flow graph were executed.  If a counter overflows, then the code coverage
>> information is invalid.  Therefore the counter type should be a 64-bit integer.
>> In multithreaded applications, it is important that the counter increments are
>> atomic.  This is not the case by default.  The user can enable atomic counter
>> increments through the -fprofile-update=atomic and
>> -fprofile-update=prefer-atomic options.
>>
>> If the hardware supports 64-bit atomic operations, then everything is fine.  If
>> not and -fprofile-update=prefer-atomic was chosen by the user, then non-atomic
>> counter increments will be used.  However, if the hardware does not support the
>> required atomic operations and -fprofile-atomic=update was chosen by the user,
>> then a warning was issued and as a forced fall-back to non-atomic operations
>> was done.  This is probably not what a user wants.  There is still hardware on
>> the market which does not have atomic operations and is used for multithreaded
>> applications.  A user which selects -fprofile-update=atomic wants consistent
>> code coverage data and not random data.
>>
>> This patch removes the fall-back to non-atomic operations for
>> -fprofile-update=atomic.  If atomic operations in hardware are not available,
>> then a library call to libatomic is emitted.  To mitigate potential performance
>> issues an optimization for systems which only support 32-bit atomic operations
>> is provided.  Here, the edge counter increments are done like this:
>>
>>    low = __atomic_add_fetch_4 (&counter.low, 1, MEMMODEL_RELAXED);
>>    high_inc = low == 0 ? 1 : 0;
>>    __atomic_add_fetch_4 (&counter.high, high_inc, MEMMODEL_RELAXED);
> You check for compare_and_swapsi and the old code checks
> TYPE_PRECISION (gcov_type_node) > 32 to determine whether 8 byte or 4 byte
> gcov_type is used.  But you do not seem to handle the case where
> neither SImode nor DImode atomic operations are available?  Can we instead
> do
> 
>    if (gcov_type_size == 4)
>      can_support_atomic4
>        = HAVE_sync_compare_and_swapsi || HAVE_atomic_compare_and_swapsi;
>    else if (gcov_type_size == 8)
>      can_support_atomic8
>        = HAVE_sync_compare_and_swapdi || HAVE_atomic_compare_and_swapdi;
> 
>    if (flag_profile_update == PROFILE_UPDATE_ATOMIC
>        && !can_support_atomic4 && !can_support_atomic8)
>      {
>        warning (0, "target does not support atomic profile update, "
>                 "single mode is selected");
>        flag_profile_update = PROFILE_UPDATE_SINGLE;
>      }
> 
> thus retain the diagnostic & fallback for this case?  

No, if you select -fprofile-update=atomic, then the updates shall be 
atomic from my point of view. If a fallback is acceptable, then you can 
use -fprofile-update=prefer-atomic. Using the fallback in 
-fprofile-update=atomic is a bug which prevents the use of gcov for 
multi-threaded applications on the lower end targets which do not have 
atomic operations in hardware.

> The existing
> code also suggests
> there might be targets with HImode or TImode counters?

Sorry, I don't know.

> 
> In another mail you mentioned that for gen_time_profiler this doesn't
> work but its
> code relies on flag_profile_update as well.  So do we need to split the flag
> somehow, or continue using the PROFILE_UPDATE_SINGLE fallback when
> we are doing more than just edge profiling?

If atomic operations are not available in hardware, then with this patch 
calls to libatomic are emitted. In gen_time_profiler() this is not an 
issue at all, since the atomic increment is only done if counters[0] == 
0 (if I read the code correctly).

For example for

void f(void) {}

we get on riscv:

gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic

         lui     a4,%hi(__gcov0.f)
         li      a3,1
         addi    a4,a4,%lo(__gcov0.f)
         amoadd.w a5,a3,0(a4)
         lui     a4,%hi(__gcov0.f+4)
         addi    a5,a5,1
         seqz    a5,a5
         addi    a4,a4,%lo(__gcov0.f+4)
         amoadd.w zero,a5,0(a4)
         ret

gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic -mbig-endian

         lui     a4,%hi(__gcov0.f+4)
         li      a3,1
         addi    a4,a4,%lo(__gcov0.f+4)
         amoadd.w a5,a3,0(a4)
         lui     a4,%hi(__gcov0.f)
         addi    a5,a5,1
         seqz    a5,a5
         addi    a4,a4,%lo(__gcov0.f)
         amoadd.w zero,a5,0(a4)
         ret

gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic -march=rv64g 
-mabi=lp64

         lui     a5,%hi(__gcov0.f)
         li      a4,1
         addi    a5,a5,%lo(__gcov0.f)
         amoadd.d zero,a4,0(a5)
         ret

gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic -march=rv32id

         lui     a0,%hi(__gcov0.f)
         li      a3,0
         li      a1,1
         li      a2,0
         addi    a0,a0,%lo(__gcov0.f)
         tail    __atomic_fetch_add_8

gcc --coverage -O2 -S -o - test.c -fprofile-update=prefer-atomic 
-march=rv32id

         lui     a4,%hi(__gcov0.f)
         lw      a5,%lo(__gcov0.f)(a4)
         lw      a2,%lo(__gcov0.f+4)(a4)
         addi    a3,a5,1
         sltu    a5,a3,a5
         add     a5,a5,a2
         sw      a3,%lo(__gcov0.f)(a4)
         sw      a5,%lo(__gcov0.f+4)(a4)
         ret

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/