diff mbox series

[RFC] Fix UBSAN in postreload-gcse.c (PR rtl-optimization/87868).

Message ID c39959b0-d0ac-9887-abb5-e2fddfb5be0c@suse.cz
State New
Headers show
Series [RFC] Fix UBSAN in postreload-gcse.c (PR rtl-optimization/87868). | expand

Commit Message

Martin Liška Nov. 6, 2018, 2:05 p.m. UTC 
Hi.

The patch is adding a check overflow in  eliminate_partially_redundant_load.
Question is whether the usage of conditional compilation of __builtin_mul_overflow
is fine?

Thanks,
Martin

gcc/ChangeLog:

2018-11-06  Martin Liska  <mliska@suse.cz>

	PR rtl-optimization/87868
	* postreload-gcse.c (eliminate_partially_redundant_load): Set
	threshold to max_count if we would overflow.
	* profile-count.h: Make max_count a public constant.
---
 gcc/postreload-gcse.c | 14 ++++++++++++--
 gcc/profile-count.h   |  2 +-
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Jeff Law Nov. 6, 2018, 6:55 p.m. UTC | #1 
On 11/6/18 7:05 AM, Martin Liška wrote:
> Hi.
> 
> The patch is adding a check overflow in  eliminate_partially_redundant_load.
> Question is whether the usage of conditional compilation of __builtin_mul_overflow
> is fine?
> 
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 2018-11-06  Martin Liska  <mliska@suse.cz>
> 
> 	PR rtl-optimization/87868
> 	* postreload-gcse.c (eliminate_partially_redundant_load): Set
> 	threshold to max_count if we would overflow.
> 	* profile-count.h: Make max_count a public constant.
OK.  Though I do worry about how many of these things we'll have to
sprinkle over the sources over time.  I suspect there's all kinds of
overflows just waiting to happen, some are obviously more important than
others.

jeff
Martin Liška Nov. 7, 2018, 9:31 a.m. UTC | #2 
On 11/6/18 7:55 PM, Jeff Law wrote:
> On 11/6/18 7:05 AM, Martin Liška wrote:
>> Hi.
>>
>> The patch is adding a check overflow in  eliminate_partially_redundant_load.
>> Question is whether the usage of conditional compilation of __builtin_mul_overflow
>> is fine?
>>
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2018-11-06  Martin Liska  <mliska@suse.cz>
>>
>> 	PR rtl-optimization/87868
>> 	* postreload-gcse.c (eliminate_partially_redundant_load): Set
>> 	threshold to max_count if we would overflow.
>> 	* profile-count.h: Make max_count a public constant.
> OK.  Though I do worry about how many of these things we'll have to
> sprinkle over the sources over time.  I suspect there's all kinds of
> overflows just waiting to happen, some are obviously more important than
> others.

Sure! Btw. I've been preparing patch that will limit some of --param values as
they tent to overlap if a user selects a big-enough value.

Martin

> 
> jeff
>
diff mbox series

Patch

diff --git a/gcc/postreload-gcse.c b/gcc/postreload-gcse.c
index b56993183d0..399970c368a 100644
--- a/gcc/postreload-gcse.c
+++ b/gcc/postreload-gcse.c
@@ -1170,8 +1170,18 @@  eliminate_partially_redundant_load (basic_block bb, rtx_insn *insn,
   if (ok_count.to_gcov_type ()
       < GCSE_AFTER_RELOAD_PARTIAL_FRACTION * not_ok_count.to_gcov_type ())
     goto cleanup;
-  if (ok_count.to_gcov_type ()
-      < GCSE_AFTER_RELOAD_CRITICAL_FRACTION * critical_count.to_gcov_type ())
+
+  gcov_type threshold;
+#if (GCC_VERSION >= 5000)
+  if (__builtin_mul_overflow (GCSE_AFTER_RELOAD_CRITICAL_FRACTION,
+			      critical_count.to_gcov_type (), &threshold))
+    threshold = profile_count::max_count;
+#else
+  threshold
+    = GCSE_AFTER_RELOAD_CRITICAL_FRACTION * critical_count.to_gcov_type ();
+#endif
+
+  if (ok_count.to_gcov_type () < threshold)
     goto cleanup;
 
   /* Generate moves to the loaded register from where
diff --git a/gcc/profile-count.h b/gcc/profile-count.h
index f4d0c340a0a..5d3bcc75f6d 100644
--- a/gcc/profile-count.h
+++ b/gcc/profile-count.h
@@ -641,8 +641,8 @@  public:
      type to hold various extra stages.  */
 
   static const int n_bits = 61;
-private:
   static const uint64_t max_count = ((uint64_t) 1 << n_bits) - 2;
+private:
   static const uint64_t uninitialized_count = ((uint64_t) 1 << n_bits) - 1;
 
   uint64_t m_val : n_bits;