diff mbox

Fix PR gcov-profile/46266

Message ID 26b657ee-eb87-d42d-e754-9ab96ce6d267@suse.cz
State New
Headers show

Commit Message

Martin Liška Sept. 27, 2016, 12:46 p.m. UTC 
On 09/27/2016 02:45 PM, Martin Liška wrote:
> On 09/27/2016 01:32 PM, Nathan Sidwell wrote:
>> On 09/27/16 07:26, Martin Liška wrote:
>>> Following patch prevents emission of gcno file (notes file) for statements
>>> that do not point to original source file, like:
>>>
>>> $ echo "int main(){}" > x.c
>>>
>>> In this case the location points to a builtin.
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>
>> I'm sure this is sensible. Is there a difficulty with a testcase -- what exactly is the failure mode?
> 
> Second version of the patch adds validation to gcov.exp, where $result is scanned for "File '<built-in>'".
> Luckily current test-case hit that verification:
> 
> FAIL: gcc.misc-tests/gcov-6.c gcov failed: <built-in>.gcov should not be created
> FAIL: gcc.misc-tests/gcov-7.c gcov failed: <built-in>.gcov should not be created
> 
>>
>> One thing I dislike is negated predicates though -- I think I'd find
>>    if (!gimple_has_reserved_location (gs))
>> to be more understandable (particularly as that matches the sense of RESERVED_LOCATION_P.
> 
> Agree with you, renamed to gimple_has_reserved_location.
> Ready with that change?
> 
> Martin
> 
>>
>> Richard?
>>
>> nathan
>>
> 

Adding missing patch.

M.

Comments

Nathan Sidwell Sept. 27, 2016, 12:57 p.m. UTC | #1 
On 09/27/16 08:46, Martin Liška wrote:

>> Second version of the patch adds validation to gcov.exp, where $result is scanned for "File '<built-in>'".
>> Luckily current test-case hit that verification:
>>
>> FAIL: gcc.misc-tests/gcov-6.c gcov failed: <built-in>.gcov should not be created
>> FAIL: gcc.misc-tests/gcov-7.c gcov failed: <built-in>.gcov should not be created

thanks.

>>> One thing I dislike is negated predicates though -- I think I'd find
>>>    if (!gimple_has_reserved_location (gs))
>>> to be more understandable (particularly as that matches the sense of RESERVED_LOCATION_P.
>>
>> Agree with you, renamed to gimple_has_reserved_location.
>> Ready with that change?

> Adding missing patch.

I think this needs Richard's approval for the gimple predicates etc.  But OK for me.

nathan
Richard Biener Sept. 27, 2016, 1:05 p.m. UTC | #2 
On Tue, Sep 27, 2016 at 2:57 PM, Nathan Sidwell <nathan@acm.org> wrote:
> On 09/27/16 08:46, Martin Liška wrote:
>
>>> Second version of the patch adds validation to gcov.exp, where $result is
>>> scanned for "File '<built-in>'".
>>> Luckily current test-case hit that verification:
>>>
>>> FAIL: gcc.misc-tests/gcov-6.c gcov failed: <built-in>.gcov should not be
>>> created
>>> FAIL: gcc.misc-tests/gcov-7.c gcov failed: <built-in>.gcov should not be
>>> created
>
>
> thanks.
>
>>>> One thing I dislike is negated predicates though -- I think I'd find
>>>>    if (!gimple_has_reserved_location (gs))
>>>> to be more understandable (particularly as that matches the sense of
>>>> RESERVED_LOCATION_P.
>>>
>>>
>>> Agree with you, renamed to gimple_has_reserved_location.
>>> Ready with that change?
>
>
>> Adding missing patch.
>
>
> I think this needs Richard's approval for the gimple predicates etc.  But OK
> for me.

Sorry for not chiming in earlier but I'd rather have you use

  if (RESERVED_LOCATION_P (gimple_location (...)))

and not add the gimple_has_not_reserved_location wrapper.  That's more in line
with the other uses you have.

Ok with that change (well, adding the RESERVED_LOCATION_P macro is ok).

Richard.

> nathan
Martin Liška Sept. 27, 2016, 1:22 p.m. UTC | #3 
On 09/27/2016 03:05 PM, Richard Biener wrote:
> On Tue, Sep 27, 2016 at 2:57 PM, Nathan Sidwell <nathan@acm.org> wrote:
>> On 09/27/16 08:46, Martin Liška wrote:
>>
>>>> Second version of the patch adds validation to gcov.exp, where $result is
>>>> scanned for "File '<built-in>'".
>>>> Luckily current test-case hit that verification:
>>>>
>>>> FAIL: gcc.misc-tests/gcov-6.c gcov failed: <built-in>.gcov should not be
>>>> created
>>>> FAIL: gcc.misc-tests/gcov-7.c gcov failed: <built-in>.gcov should not be
>>>> created
>>
>>
>> thanks.
>>
>>>>> One thing I dislike is negated predicates though -- I think I'd find
>>>>>    if (!gimple_has_reserved_location (gs))
>>>>> to be more understandable (particularly as that matches the sense of
>>>>> RESERVED_LOCATION_P.
>>>>
>>>>
>>>> Agree with you, renamed to gimple_has_reserved_location.
>>>> Ready with that change?
>>
>>
>>> Adding missing patch.
>>
>>
>> I think this needs Richard's approval for the gimple predicates etc.  But OK
>> for me.
> 
> Sorry for not chiming in earlier but I'd rather have you use
> 
>   if (RESERVED_LOCATION_P (gimple_location (...)))
> 
> and not add the gimple_has_not_reserved_location wrapper.  That's more in line
> with the other uses you have.
> 
> Ok with that change (well, adding the RESERVED_LOCATION_P macro is ok).
> 
> Richard.

Done that in r240536.
Thanks for the review.

Martin

> 
>> nathan
diff mbox

Patch

From a456386416c0d4d5a6acd8755ff81c601af01c9c Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 8 Aug 2016 17:38:07 +0200
Subject: [PATCH] Fix PR gcov-profile/46266

gcc/testsuite/ChangeLog:

2016-09-27  Martin Liska  <mliska@suse.cz>

	* lib/gcov.exp: Verify that <built-in>.gcov file is not
	considered.

gcc/ChangeLog:

2016-09-27  Martin Liska  <mliska@suse.cz>

	* gimple.h (gimple_has_reserved_location): New function.
	* input.h (RESERVED_LOCATION_P): New macro.
	* profile.c (branch_prob): Use RESERVED_LOCATION_P and
	gimple_has_reserved_location instread of comparison
	with UNKNOWN_LOCATION.
---
 gcc/gimple.h               | 8 ++++++++
 gcc/input.h                | 2 ++
 gcc/profile.c              | 9 ++++-----
 gcc/testsuite/lib/gcov.exp | 9 ++++++++-
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/gcc/gimple.h b/gcc/gimple.h
index 9fad15b..f6dab77 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1792,6 +1792,14 @@  gimple_has_location (const gimple *g)
   return LOCATION_LOCUS (gimple_location (g)) != UNKNOWN_LOCATION;
 }
 
+/* Return true if G has a location that is any from reserved.  */
+
+static inline bool
+gimple_has_reserved_location (const gimple *g)
+{
+  return RESERVED_LOCATION_P (gimple_location (g));
+}
+
 
 /* Return the file name of the location of STMT.  */
 
diff --git a/gcc/input.h b/gcc/input.h
index fe80605..6ce0b81 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -61,6 +61,8 @@  extern location_t input_location;
 #define LOCATION_BLOCK(LOC) \
   ((tree) ((IS_ADHOC_LOC (LOC)) ? get_data_from_adhoc_loc (line_table, (LOC)) \
    : NULL))
+#define RESERVED_LOCATION_P(LOC) \
+  (LOCATION_LOCUS (LOC) < RESERVED_LOCATION_COUNT)
 
 /* Return a positive value if LOCATION is the locus of a token that is
    located in a system header, O otherwise. It returns 1 if LOCATION
diff --git a/gcc/profile.c b/gcc/profile.c
index 791225b..3c00077 100644
--- a/gcc/profile.c
+++ b/gcc/profile.c
@@ -1042,7 +1042,7 @@  branch_prob (void)
 	       gsi_prev_nondebug (&gsi))
 	    {
 	      last = gsi_stmt (gsi);
-	      if (gimple_has_location (last))
+	      if (!gimple_has_reserved_location (last))
 		break;
 	    }
 
@@ -1053,7 +1053,7 @@  branch_prob (void)
 	     is not computed twice.  */
 	  if (last
 	      && gimple_has_location (last)
-	      && LOCATION_LOCUS (e->goto_locus) != UNKNOWN_LOCATION
+	      && !RESERVED_LOCATION_P (e->goto_locus)
 	      && !single_succ_p (bb)
 	      && (LOCATION_FILE (e->goto_locus)
 	          != LOCATION_FILE (gimple_location (last))
@@ -1262,15 +1262,14 @@  branch_prob (void)
 	  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	    {
 	      gimple *stmt = gsi_stmt (gsi);
-	      if (gimple_has_location (stmt))
+	      if (!gimple_has_reserved_location (stmt))
 		output_location (gimple_filename (stmt), gimple_lineno (stmt),
 				 &offset, bb);
 	    }
 
 	  /* Notice GOTO expressions eliminated while constructing the CFG.  */
 	  if (single_succ_p (bb)
-	      && LOCATION_LOCUS (single_succ_edge (bb)->goto_locus)
-		 != UNKNOWN_LOCATION)
+	      && !RESERVED_LOCATION_P (single_succ_edge (bb)->goto_locus))
 	    {
 	      expanded_location curr_location
 		= expand_location (single_succ_edge (bb)->goto_locus);
diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
index 02bc6b9..91f14e2 100644
--- a/gcc/testsuite/lib/gcov.exp
+++ b/gcc/testsuite/lib/gcov.exp
@@ -364,13 +364,20 @@  proc run-gcov { args } {
 	return
     }
 
+    set builtin_index [string first "File '<built-in>'" $result]
+    if { $builtin_index != -1 } {
+        fail "$testname gcov failed: <built-in>.gcov should not be created"
+        clean-gcov $testcase
+        return
+    }
+
     # Get the gcov output file after making sure it exists.
     set files [glob -nocomplain $testcase.gcov]
     if { $files == "" } {
 	if { $xfailed } {
 	    setup_xfail "*-*-*"
 	}
-        fail "$testname gcov failed: $testcase.gov does not exist"
+        fail "$testname gcov failed: $testcase.gcov does not exist"
         clean-gcov $testcase
         return
     }
-- 
2.9.2