This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PR 28034: -fsection-anchors and coverage counters


Mark Mitchell <mark@codesourcery.com> writes:
> Richard Sandiford wrote:
>>       /* Generate and save a copy of this so it can be shared.  */
>>       /* We don't know the size yet; make it big enough that nobody
>>          will make any clever transformation on it.  */
>>       char buf[20];
>>       tree gcov_type_node = get_gcov_type ();
>>       tree domain_tree
>>         = build_index_type (build_int_cst (NULL_TREE, 1000)); /* replaced later */
>>       tree gcov_type_array_type
>>         = build_array_type (gcov_type_node, domain_tree);
>
> I think this code should be considered wrong, in current GCC.  Your
> concern about the optimizers getting confused if we access too many
> elements is reasonable.  I don't think it's safe to rely on the type not
> being shared with other variables, either.
>
> I think it would be better to give the variable the type "T []" (with no
> array bounds) at this point, and to leave DECL_SIZE set to NULL_TREE, as
> we would with "extern int i[]".  Then, set the type, and DECL_SIZE, when
> the final size of the array is known.
>
> Do you think such a patch would be hard?  If it is, then your patch
> certainly looks to be a reasonable fallback.

As discussed off-list, that was my first instinct too, but I was
reluctant to do it because

  (a) It's a locally-defined object.  I thought the optimisers might be
      surprised that a locally-defined object had no known size.

  (b) It almost seemed too obvious.  I felt sure the original author
      must have rejected it for some reason (perhaps (a)).

However, it bootstraps & regression tests on x86_64-linux-gnu without
problems, and it does fix the testcase.  I also couldn't see a specific
reason for using a fixed size in the original patch submission:

    http://article.gmane.org/gmane.comp.gcc.patches/55165

So, OK to install?

Note: I used integer_type_node because the final index type is set
using build_int_cst (NULL, N).  The final index type will therefore
be a variant of integer_type_node, so I thought the initial type should
be consistent with it.

Richard


gcc/
	PR middle-end/28034
	* coverage.c (coverage_counter_alloc): Use an open index range for
	the counter tables.

Index: gcc/coverage.c
===================================================================
--- gcc/coverage.c	(revision 114768)
+++ gcc/coverage.c	(working copy)
@@ -388,13 +388,13 @@ coverage_counter_alloc (unsigned counter
 
   if (!tree_ctr_tables[counter])
     {
-      /* Generate and save a copy of this so it can be shared.  */
-      /* We don't know the size yet; make it big enough that nobody
-	 will make any clever transformation on it.  */
+      /* Generate and save a copy of this so it can be shared.  Leave
+	 the array's upper bound unspecified for now; it will be set
+	 after all functions have been compiled.  */
       char buf[20];
       tree gcov_type_node = get_gcov_type ();
       tree domain_tree
-        = build_index_type (build_int_cst (NULL_TREE, 1000)); /* replaced later */
+        = build_range_type (integer_type_node, integer_zero_node, NULL);
       tree gcov_type_array_type
         = build_array_type (gcov_type_node, domain_tree);
       tree_ctr_tables[counter]


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]