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

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Dec 15, 2022 at 9:34 AM Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> 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.

Ah, OK.  So the fallback there is libatomic calls as well then?  Note
not all targets support libatomic, for those the failure mode is likely
a link error (which might be fine, but we eventually might want to
amend documentation to indicate this failure mode).

> > The existing
> > code also suggests
> > there might be targets with HImode or TImode counters?
>
> Sorry, I don't know.

can you conditionalize the split_atomic_increment init on
gcov_type_size == 8 please?

The patch is missing adjusments to the -fprofile-update documentation.
I think the prefer-atomic vs. atomic needs more explanation with this
change.

otherwise the patch looks OK to me.

Thanks,
Richard.

> >
> > 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/