From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx.kolabnow.com (mx.kolabnow.com [212.103.80.155]) by sourceware.org (Postfix) with ESMTPS id 1D76A3858C32 for ; Sun, 7 Apr 2024 07:29:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1D76A3858C32 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=lambda.is Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lambda.is ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 1D76A3858C32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=212.103.80.155 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712474949; cv=none; b=lP8Ebv4IwcXaEhrjqCM51qMyJd12wOl7kJjEPDY0ZHG3U7fN59Z2occGyv/sn/q6Mdl1cHqX/HFDPh/h2EZ5TKcYv9VzIYr3IQs88Km8s7K6wVWR+tkYOmFAkhY+mdv+3JqEowOoVgB8pQG/a22Ki1ZDfkFk8R8YPGtu4SR2Mu4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712474949; c=relaxed/simple; bh=BwqkCjCOi7FGtrwHbTjFdKUAdoA0bKXfEl5MxnPVn00=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=arRDFSDpWJWs1Mojb8MwK8s0g981mCt+qt4RcdRZ2hiTzYWpS0zOGKbu9WMl9e8htMD5bgeJAHgPQiPoYh/7Ec1AqJcZUkEDVFA42Gil3tjB9bdNx0+nsDVj9r+NbAaQXGff/TXcjlTNEZzaSVC90COb4II8LWFz+Fh5wvdLshI= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from localhost (unknown [127.0.0.1]) by mx.kolabnow.com (Postfix) with ESMTP id 0802E2096DC4; Sun, 7 Apr 2024 09:29:05 +0200 (CEST) Authentication-Results: ext-mx-out011.mykolab.com (amavis); dkim=pass (4096-bit key) reason="pass (just generated, assumed good)" header.d=kolabnow.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kolabnow.com; h= content-transfer-encoding:content-type:content-type:in-reply-to :from:from:content-language:references:subject:subject :mime-version:date:date:message-id:received:received:received; s=dkim20160331; t=1712474942; x=1714289343; bh=LoUTyJthPEv9GlY3 Y/lo6lPZjtvySUCK4C0xCQF+Yvk=; b=XYt3Ge3BwqHXQhrONnRBNt8OK5g0gVEv hRSOPJxvRn770k5pV/zaN0SxwtbAa1uz0IH7nGYwMbZ1f+3citzgTpt3Dq45CvSi 78wmHzoF7O+qNIJQ5SPy7FDmjekL3Tnvx+qMVpnWT0MGQeRdstoOcUj261R2aVT+ aAszs5BbUia2bJQPPvrNZSHac2Zbl6gi7n2NHEeNpTF2SrVjFFaVBfcWSagVCJVg sXc30rSp24QOpS7yv7PB+mU9AWitIJDnKosCQHDlsBUl8o758vUDcmoVf69qoaXo 7hGCPA0rbI73vL1odZWETLhQke5M5/k31OZ6XhXdW6qAnT3S8dRnePq9EZe66ulh F3yEXPhliWqguQ8m+TIgyIyFPA0MqFemJqG3pcXyQ7F7613eWh2XzeWSSI8eNT2t lFZUBzGM3/fcikqudbXwL+Oy8tgwSNgPgyGWVlJSuPNCdUtOW1BmUJSiH8+shCaU v3xbEo6r1ovZX8o0y0xQZiM9TOioAtDZB88kLbqX0OwbwTylAOO4OkjY4H8hjgry XOKJ2PZTFypDwxqbcQbbfmHWHb0jX6j4vCK6qHeLw9B6V1RPzu7zLyERlJTHqBvi aqMsU7b/6gID+MTaymJL5JncmU91POrTF8IwAwfMfn1kC9zY3o7W2D7Z0qtZ9H9j e40GoqBIw3U= X-Virus-Scanned: amavis at mykolab.com X-Spam-Score: -1 X-Spam-Level: X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,KAM_SHORT,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 Received: from mx.kolabnow.com ([127.0.0.1]) by localhost (ext-mx-out011.mykolab.com [127.0.0.1]) (amavis, port 10024) with ESMTP id hoaMLdNckuuI; Sun, 7 Apr 2024 09:29:02 +0200 (CEST) Received: from int-mx011.mykolab.com (unknown [10.9.13.11]) by mx.kolabnow.com (Postfix) with ESMTPS id C44972096DC2; Sun, 7 Apr 2024 09:29:00 +0200 (CEST) Received: from ext-subm010.mykolab.com (unknown [10.9.6.10]) by int-mx011.mykolab.com (Postfix) with ESMTPS id 469E430FC17A; Sun, 7 Apr 2024 09:29:00 +0200 (CEST) Message-ID: <73e90dbd-a58e-4079-baed-41332d5ab5ca@lambda.is> Date: Sun, 7 Apr 2024 09:28:58 +0200 MIME-Version: 1.0 Subject: Re: [PATCH 0/2] Condition coverage fixes To: Richard Biener Cc: gcc-patches@gcc.gnu.org, hjl.tools@gmail.com, hubicka@ucw.cz, ro@cebitec.uni-bielefeld.de References: Content-Language: en-US From: =?UTF-8?Q?J=C3=B8rgen_Kvalsvik?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 07/04/2024 08:26, Richard Biener wrote: > > >> Am 06.04.2024 um 22:41 schrieb Jørgen Kvalsvik : >> >> On 06/04/2024 13:15, Jørgen Kvalsvik wrote: >>>> On 06/04/2024 07:50, Richard Biener wrote: >>>> >>>> >>>>> Am 05.04.2024 um 21:59 schrieb Jørgen Kvalsvik : >>>>> >>>>> Hi, >>>>> >>>>> I propose these fixes for the current issues with the condition >>>>> coverage. >>>>> >>>>> Rainer, I propose to simply delete the test with __sigsetjmp. I don't >>>>> think it actually detects anything reasonable any more, I kept it around >>>>> to prevent a regression. Since then I have built a lot of programs (with >>>>> optimization enabled) and not really seen this problem. >>>>> >>>>> H.J., the problem you found with -O2 was really a problem of >>>>> tree-inlining, which was actually caught earlier by Jan [1]. It probably >>>>> warrants some more testing, but I could reproduce by tuning your test >>>>> case to use always_inline and not -O2 and trigger the error. >>>>> >>>>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html >>>> >>>> Ok >>> Thanks, committed. >>> I am wondering if the fn->cond_uids access should always be guarded (in tree-profile.cc) should always be guarded. Right now there is the assumption that if condition coverage is requested the will exist and be populated, but as this shows there may be other circumstances where this is not true. >>> Or perhaps there should be a gcc_assert to (reliably) detect cases where the map is not constructed properly? >>> Thanks, >>> Jørgen >> >> I gave this some more thought, and realised I was too eager to fix the segfault. While trunk no longer crashes (at least on my x86_64 linux) the fix itself is bad. It copies the gcond -> uid mappings into the caller, but the stmts are deep copied into the caller, so no gcond will ever be a hit when we look up the condition_uids in tree-profile.cc. >> >> I did a very quick prototype to confirm. By applying this patch: >> >> @@ -2049,6 +2049,9 @@ copy_bb (copy_body_data *id, basic_block bb, >> >> copy_gsi = gsi_start_bb (copy_basic_block); >> >> + if (!cfun->cond_uids && id->src_cfun->cond_uids) >> + cfun->cond_uids = new hash_map (); >> + >> for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) >> { >> gimple_seq stmts; >> @@ -2076,6 +2079,12 @@ copy_bb (copy_body_data *id, basic_block bb, >> if (gimple_nop_p (stmt)) >> continue; >> >> + if (id->src_cfun->cond_uids && is_a (orig_stmt)) >> + { >> + unsigned *v = id->src_cfun->cond_uids->get (as_a (orig_stmt)); >> + if (v) cfun->cond_uids->put (as_a (stmt), *v); >> + } >> + >> >> >> and this test program: >> >> __attribute__((always_inline)) >> inline int >> inlinefn (int a) >> { >> if (a > 5) >> { >> printf ("a > 5\n"); >> return a; >> } >> else >> printf ("a < 5, was %d\n", a); >> return a * a - 2; >> } >> >> int >> mcdc027e (int a, int b) >> { >> int y = inlinefn (a); >> return y + b; >> } >> >> >> gcov reports: >> >> 2: 18:mcdc027e (int a, int b) >> condition outcomes covered 1/2 >> condition 0 not covered (true) >> -: 19:{ >> 2: 20: int y = inlinefn (a); >> 2: 21: return y + b; >> -: 22:} >> >> but without the patch, gcov prints nothing. >> >> I am not sure if this approach is even ideal. Probably the most problematic is the source line mapping which is all messed up. I checked with gcov --branch-probabilities and it too reports the callee at the top of the caller. >> >> If you think it is a good strategy I can clean up the prototype and submit a patch. I suppose the function _totals_ should be accurate, even if the source mapping is a bit surprising. >> >> What do you think? I am open to other strategies, too > > I think the most important bit is that the segfault is gone. The interaction of coverage with inlining or even other optimization when applying optimization to coverage should be documented better. > > Does condition coverage apply ontop of regular coverage counting or is it an either/or? On top, it is perfectly reasonable (and desirable) to measure statement/line coverage in addition to condition coverage. That being said, if you achieve MC/DC you also achieve branch coverage, but gcc -fprofile-arcs + --branch-counts/--branch-probabilities measure more than just taken/not taken, so -fcondition-coverage does not completely replace it. You might also not care about MC/DC, only branch coverage. Personally, I have come around to this strategy being alright. It can, and even might be, documented that inlined functions will be anchored to the top of the calling function, and the summaries will be useful still. A future project could be to improve the source mapping also through inlining. In practice this is ok because code under test tends to not be inlined so much in practice. Thanks, Jørgen > > Thanks, > Richard > >> Thanks, >> Jørgen >> >>>> >>>> Thanks, >>>> Richard >>>> >>>> >>>>> Thanks, >>>>> Jørgen >>>>> >>>>> Jørgen Kvalsvik (2): >>>>> Remove unecessary and broken MC/DC compile test >>>>> Copy condition->expr map when inlining [PR114599] >>>>> >>>>> gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- >>>>> gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ >>>>> gcc/tree-inline.cc | 20 +++++++++++++++- >>>>> 3 files changed, 44 insertions(+), 12 deletions(-) >>>>> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c >>>>> >>>>> -- >>>>> 2.30.2 >>>>> >>