From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id DE3083858C83 for ; Tue, 19 Apr 2022 14:22:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DE3083858C83 Received: by mail-pg1-x52f.google.com with SMTP id u2so24002249pgq.10 for ; Tue, 19 Apr 2022 07:22:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=mwlJLRJ5ICmgd09ZCCwQwpasullwRMkfLYzuwhp8F7E=; b=AudZZdxSUa3YFzXkrxWgRStScFgLDs9a4y56Cb5K31VOpj6oc1757dIKGz7RCQOaQr PHm7fMAVnMZuTOxV+rETSlCkYotJyKCqEdCBIeE7xcY0j0WOZGnfYs7RY8LYrWUOYvlW vvY6F46hXFIm7EdL8q2ZjXoJv1h+v8q2gcfm0fE/RUOJdl0sEvT+Nai9B17H2IbjkUjS QX2ooqpN+YKZyxYia4/o9RkP5M6M5ClCxTXMI7xmN04wyqCyHdKo56t8415+HirS8ltd uCVqAczSzgrHdIoa0wG3LXXKRNY3JlaGVTqpx+oAkNG1XGcH9d3SlRcXK12QGY9YbBGr pBfg== X-Gm-Message-State: AOAM533FOpfZ295xGAYK71NwuROysy1sKOgz/bt7v39MqNWq1y/NwPEA gR2l8Y9h8U/sfgYXtOLbtWcGoG1Jn+RF2YU2 X-Google-Smtp-Source: ABdhPJzhOaKrpMDlMndpIUkrimq0PwS/9zV4BgePEs6tPNbZjuwt1ytmRI6hpAe5vv8A2O29E2/93w== X-Received: by 2002:a65:46cf:0:b0:399:13b3:dd8b with SMTP id n15-20020a6546cf000000b0039913b3dd8bmr14889797pgr.585.1650378172795; Tue, 19 Apr 2022 07:22:52 -0700 (PDT) Received: from [172.16.185.109] ([103.175.111.222]) by smtp.gmail.com with ESMTPSA id w123-20020a623081000000b005056a4d71e3sm16503409pfw.77.2022.04.19.07.22.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 19 Apr 2022 07:22:52 -0700 (PDT) Message-ID: Date: Tue, 19 Apr 2022 16:22:43 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH] Add condition coverage profiling Content-Language: en-US To: =?UTF-8?Q?Martin_Li=c5=a1ka?= , gcc-patches@gcc.gnu.org References: <8041d06d-219d-c83a-ff8f-9c75227cb072@woven-planet.global> <8ea26b0e-40e2-9e7c-4470-5add7a02a074@woven-planet.global> From: =?UTF-8?Q?J=c3=b8rgen_Kvalsvik?= In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Apr 2022 14:22:57 -0000 On 07/04/2022 14:04, Martin Liška wrote: > On 3/28/22 16:40, Jørgen Kvalsvik via Gcc-patches wrote: >> ... And with another tiny change that fixes Martin's while (1); case. > > Hello. > > Back to this ;) Thank you for the updated version of the patch. I have a couple > of comments/requests: Sorry for the late response, this email was eaten by spam filtering for some reason. > 1) I noticed the following cannot be linked: > > $ cat errors.C > char trim_filename_name; > int r; > > void trim_filename() { >   if (trim_filename_name) >     r = 123; >   while (trim_filename_name) >     ; > } > > int > main() {} > > $ g++ errors.C -fprofile-conditions -O2 > mold: error: undefined symbol: /tmp/ccmZANB5.o: __gcov8._Z13trim_filenamev > collect2: error: ld returned 1 exit status I was able to reproduce this, and I think I have it fixed (that is, I cannot reproduce it anymore). I looks like the counters were allocated, but never used in the absence of an output file. I changed it so that the writes are guarded behind an output check and the counter instrumentation is unconditional which makes it go away. The coverage data dependencies are more interleaved (at least currently) than all the other counters, which makes the flow awkward. I am not convinced a proper fix is actually worth it, or if anything it would be an separate next step. > Btw. how much have you tested the patch set so far? Have you tried building > something bigger > with -fprofile-conditions enabled? > My own test suite plus zlib, and it looks like linux is compiling fine. I haven't rigorously studied the _output_ in these projects yet, and it is very time consuming without a benchmark. > 2) As noticed by Sebastian, please support the new tag in gcov-dump: > > $ gcov-dump -l a.gcno > ... > a.gcno:    01450000:  28:LINES > a.gcno:                  block 7:`a.c':11 > a.gcno:    01470000:   8:UNKNOWN > Will do. > 3) Then I have some top-level formatting comments: > > a) please re-run check_GNU_style.py, I still see: > === ERROR type #1: blocks of 8 spaces should be replaced with tabs (35 error(s)) > === > ... > > b) write comments for each newly added function (and separate it by one empty > line from > the function definition) > > c) do not create visual alignment, we don't use it: > > +       cond->set ("count",   new json::integer_number (count)); > +       cond->set ("covered", new json::integer_number (covered)); > > and similar examples > > d) finish multiple comments after a dot on the same line: > > +    /* Number of expressions found - this value is the number of entries in the > +       gcov output and the parameter to gcov_counter_alloc (). > +       */ > > should be ... gcov_counter_alloc ().  */ > > e) for long lines with a function call like: > > +           const int n = find_conditions_masked_by > +               (block, expr, flags + k, path, CONDITIONS_MAX_TERMS); > > do rather > const int n >   = find_conditions_masked_by (block, expr, >                                next_arg, ...); > > f) for function params: > > +int > +collect_reachable_conditionals ( > +    basic_block pre, > +    basic_block post, > +    basic_block *out, > +    int maxsize, > +    sbitmap expr) > > use rather: > > int > collect_reachable_conditionals (basic_block pre, >                                 second_arg,... > Consider it done for the next revision. > In the next round, I'm going to take a look at the CFG algorithm that identifies > and instruments the sub-expressions. Thank you.