[PATCH 0/4] Fix various minor issues seen with cppcheck

[PATCH 0/4] Fix various minor issues seen with cppcheck

[Martin Liska]

Hi.

The series is about small issues that were spotted with cppcheck
and where David Binderman suggested a patch.

It's probably a stage1 material?

Martin

Martin Liska (4):
  Remove 2 dead variables in bid_internal.h.
  Use const for some function arguments.
  Put index check before use.
  Use const for template argument.

 gcc/alloc-pool.h                               | 2 +-
 gcc/bitmap.h                                   | 2 +-
 gcc/mem-stats.h                                | 4 ++--
 gcc/sese.h                                     | 4 ++--
 libgcc/config/libbid/bid_internal.h            | 4 ----
 liboffloadmic/runtime/offload_target.cpp       | 2 +-
 libstdc++-v3/include/parallel/multiway_merge.h | 4 ++--
 7 files changed, 9 insertions(+), 13 deletions(-)

-- 
2.25.0

[PATCH 3/4] Put index check before use.

[Martin Liska]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]


liboffloadmic/ChangeLog:

2020-02-04  Martin Liska  <mliska@suse.cz>

	PR other/89860.
	* runtime/offload_target.cpp: Put index check
	before its use.
---
 liboffloadmic/runtime/offload_target.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Put-index-check-before-use.patch --]
[-- Type: text/x-patch; name="0003-Put-index-check-before-use.patch", Size: 759 bytes --]

diff --git a/liboffloadmic/runtime/offload_target.cpp b/liboffloadmic/runtime/offload_target.cpp
index 8273faac13b..16ba4a32991 100644
--- a/liboffloadmic/runtime/offload_target.cpp
+++ b/liboffloadmic/runtime/offload_target.cpp
@@ -329,7 +329,7 @@ void OffloadDescriptor::merge_var_descs(
                 }
             }
             // instead of m_vars[i].type.dst we will use m_vars_extra[i].type_dst
-            if (m_vars[i].type.dst == c_extended_type && i < vars_total) {
+            if (i < vars_total && m_vars[i].type.dst == c_extended_type) {
                 VarDescExtendedType *etype =
                     reinterpret_cast<VarDescExtendedType*>(vars[i].into);
                 m_vars_extra[i].type_dst = etype->extended_type;

Re: [PATCH 3/4] Put index check before use.

[Jeff Law]

On Tue, 2020-02-04 at 14:57 +0100, Martin Liska wrote:
> liboffloadmic/ChangeLog:
> 
> 2020-02-04  Martin Liska  <mliska@suse.cz>
> 
> 	PR other/89860.
> 	* runtime/offload_target.cpp: Put index check
> 	before its use.
OK
jeff
> 

[PATCH 1/4] Remove 2 dead variables in bid_internal.h.

[Martin Liska]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]


libgcc/config/libbid/ChangeLog:

2020-02-04  Martin Liska  <mliska@suse.cz>

	PR libgcc/92565
	* bid_internal.h (handle_UF_128_rem): Remove unused variable.
	(handle_UF_128): Likewise.
---
 libgcc/config/libbid/bid_internal.h | 4 ----
 1 file changed, 4 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-2-dead-variables-in-bid_internal.h.patch --]
[-- Type: text/x-patch; name="0001-Remove-2-dead-variables-in-bid_internal.h.patch", Size: 687 bytes --]

diff --git a/libgcc/config/libbid/bid_internal.h b/libgcc/config/libbid/bid_internal.h
index cef36a9bb80..9baa098caac 100644
--- a/libgcc/config/libbid/bid_internal.h
+++ b/libgcc/config/libbid/bid_internal.h
@@ -1540,8 +1540,6 @@ handle_UF_128_rem (UINT128 * pres, UINT64 sgn, int expon, UINT128 CQ,
     __shr_128 (CQ, Qh, amount);
   }
 
-  expon = 0;
-
 #ifndef IEEE_ROUND_NEAREST_TIES_AWAY
 #ifndef IEEE_ROUND_NEAREST
   if (!(*prounding_mode))
@@ -1676,8 +1674,6 @@ handle_UF_128 (UINT128 * pres, UINT64 sgn, int expon, UINT128 CQ,
     __shr_128 (CQ, Qh, amount);
   }
 
-  expon = 0;
-
 #ifndef IEEE_ROUND_NEAREST_TIES_AWAY
 #ifndef IEEE_ROUND_NEAREST
   if (!(*prounding_mode))

Re: [PATCH 1/4] Remove 2 dead variables in bid_internal.h.

[Jeff Law]

On Tue, 2020-02-04 at 14:54 +0100, Martin Liska wrote:
> libgcc/config/libbid/ChangeLog:
> 
> 2020-02-04  Martin Liska  <mliska@suse.cz>
> 
> 	PR libgcc/92565
> 	* bid_internal.h (handle_UF_128_rem): Remove unused variable.
> 	(handle_UF_128): Likewise.
OK for master.  And patches that drop unused variables should be considered
obvious and don't need a review cycle.

jeff

[PATCH 2/4] Use const for some function arguments.

[Martin Liska]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]


gcc/ChangeLog:

2020-02-04  Martin Liska  <mliska@suse.cz>

	PR c/92472.
	* alloc-pool.h: Use const for some arguments.
	* bitmap.h: Likewise.
	* mem-stats.h: Likewise.
	* sese.h (get_entry_bb): Likewise.
	(get_exit_bb): Likewise.
---
 gcc/alloc-pool.h | 2 +-
 gcc/bitmap.h     | 2 +-
 gcc/mem-stats.h  | 4 ++--
 gcc/sese.h       | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Use-const-for-some-function-arguments.patch --]
[-- Type: text/x-patch; name="0002-Use-const-for-some-function-arguments.patch", Size: 2030 bytes --]

diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index 1686a8b5f91..fd7194bfea4 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -60,7 +60,7 @@ public:
 
   /* Dump usage coupled to LOC location, where TOTAL is sum of all rows.  */
   inline void
-  dump (mem_location *loc, mem_usage &total) const
+  dump (mem_location *loc, const mem_usage &total) const
   {
     char *location_string = loc->to_string ();
 
diff --git a/gcc/bitmap.h b/gcc/bitmap.h
index d52fd5bb905..b481f4b2606 100644
--- a/gcc/bitmap.h
+++ b/gcc/bitmap.h
@@ -237,7 +237,7 @@ public:
 
   /* Dump usage coupled to LOC location, where TOTAL is sum of all rows.  */
   inline void
-  dump (mem_location *loc, mem_usage &total) const
+  dump (mem_location *loc, const mem_usage &total) const
   {
     char *location_string = loc->to_string ();
 
diff --git a/gcc/mem-stats.h b/gcc/mem-stats.h
index 21d038bb370..4a3177dd4fc 100644
--- a/gcc/mem-stats.h
+++ b/gcc/mem-stats.h
@@ -70,7 +70,7 @@ public:
 
   /* Return true if the memory location is equal to OTHER.  */
   int
-  equal (mem_location &other)
+  equal (const mem_location &other)
   {
     return m_filename == other.m_filename && m_function == other.m_function
       && m_line == other.m_line;
@@ -203,7 +203,7 @@ public:
 
   /* Dump usage coupled to LOC location, where TOTAL is sum of all rows.  */
   inline void
-  dump (mem_location *loc, mem_usage &total) const
+  dump (mem_location *loc, const mem_usage &total) const
   {
     char *location_string = loc->to_string ();
 
diff --git a/gcc/sese.h b/gcc/sese.h
index 8afea28b07e..74d3fe3cd8a 100644
--- a/gcc/sese.h
+++ b/gcc/sese.h
@@ -45,7 +45,7 @@ void dump_sese (const sese_l &);
 /* Get the entry of an sese S.  */
 
 static inline basic_block
-get_entry_bb (sese_l &s)
+get_entry_bb (const sese_l &s)
 {
   return s.entry->dest;
 }
@@ -53,7 +53,7 @@ get_entry_bb (sese_l &s)
 /* Get the exit of an sese S.  */
 
 static inline basic_block
-get_exit_bb (sese_l &s)
+get_exit_bb (const sese_l &s)
 {
   return s.exit->src;
 }

Re: [PATCH 2/4] Use const for some function arguments.

[Segher Boessenkool]

On Tue, Feb 04, 2020 at 02:55:14PM +0100, Martin Liska wrote:
> 
> gcc/ChangeLog:
> 
> 2020-02-04  Martin Liska  <mliska@suse.cz>
> 
> 	PR c/92472.

That trailing dot should not be there (in some other patches as well).


Segher

Re: [PATCH 2/4] Use const for some function arguments.

[Martin Liška]

On 2/7/20 6:21 PM, Segher Boessenkool wrote:
> On Tue, Feb 04, 2020 at 02:55:14PM +0100, Martin Liska wrote:
>>
>> gcc/ChangeLog:
>>
>> 2020-02-04  Martin Liska  <mliska@suse.cz>
>>
>> 	PR c/92472.
> 
> That trailing dot should not be there (in some other patches as well).

Sure. I'll fix it.

@Richi: May I install the patch once stage1 opens, or will it require
another review?

Martin

> 
> 
> Segher
> 

Re: [PATCH 2/4] Use const for some function arguments.

[Richard Biener]

On Mon, Feb 17, 2020 at 11:05 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 2/7/20 6:21 PM, Segher Boessenkool wrote:
> > On Tue, Feb 04, 2020 at 02:55:14PM +0100, Martin Liska wrote:
> >>
> >> gcc/ChangeLog:
> >>
> >> 2020-02-04  Martin Liska  <mliska@suse.cz>
> >>
> >>      PR c/92472.
> >
> > That trailing dot should not be there (in some other patches as well).
>
> Sure. I'll fix it.
>
> @Richi: May I install the patch once stage1 opens, or will it require
> another review?

Yes, OK for stage1.

> Martin
>
> >
> >
> > Segher
> >
>

Re: [PATCH 2/4] Use const for some function arguments.

[Jeff Law]

On Tue, 2020-02-04 at 14:55 +0100, Martin Liska wrote:
> gcc/ChangeLog:
> 
> 2020-02-04  Martin Liska  <mliska@suse.cz>
> 
> 	PR c/92472.
> 	* alloc-pool.h: Use const for some arguments.
> 	* bitmap.h: Likewise.
> 	* mem-stats.h: Likewise.
> 	* sese.h (get_entry_bb): Likewise.
> 	(get_exit_bb): Likewise.
OK.  And constification should be considered obvious and doesn't need a review
cycle.

jeff
> 

[PATCH 4/4] Use const for template argument.

[Martin Liska]

[-- Attachment #1: Type: text/plain, Size: 275 bytes --]


libstdc++-v3/ChangeLog:

2020-02-04  Martin Liska  <mliska@suse.cz>

	PR c/92472.
	* include/parallel/multiway_merge.h:
	Use const for _Compare template argument.
---
 libstdc++-v3/include/parallel/multiway_merge.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0004-Use-const-for-template-argument.patch --]
[-- Type: text/x-patch; name="0004-Use-const-for-template-argument.patch", Size: 995 bytes --]

diff --git a/libstdc++-v3/include/parallel/multiway_merge.h b/libstdc++-v3/include/parallel/multiway_merge.h
index 983c7b2bd9a..97a9ce0feb7 100644
--- a/libstdc++-v3/include/parallel/multiway_merge.h
+++ b/libstdc++-v3/include/parallel/multiway_merge.h
@@ -118,7 +118,7 @@ namespace __gnu_parallel
        *  @return @c true if less. */
       friend bool
       operator<(_GuardedIterator<_RAIter, _Compare>& __bi1,
-		_GuardedIterator<_RAIter, _Compare>& __bi2)
+		_GuardedIterator<_RAIter, const _Compare>& __bi2)
       {
 	if (__bi1._M_current == __bi1._M_end)       // __bi1 is sup
 	  return __bi2._M_current == __bi2._M_end;  // __bi2 is not sup
@@ -188,7 +188,7 @@ namespace __gnu_parallel
        *  @return @c true if less. */
       friend bool
       operator<(_UnguardedIterator<_RAIter, _Compare>& __bi1,
-		_UnguardedIterator<_RAIter, _Compare>& __bi2)
+		_UnguardedIterator<_RAIter, const _Compare>& __bi2)
       {
 	// Normal compare.
 	return (__bi1.__comp)(*__bi1, *__bi2);

Re: [PATCH 4/4] Use const for template argument.

[Jeff Law]

On Tue, 2020-02-04 at 14:55 +0100, Martin Liska wrote:
> libstdc++-v3/ChangeLog:
> 
> 2020-02-04  Martin Liska  <mliska@suse.cz>
> 
> 	PR c/92472.
> 	* include/parallel/multiway_merge.h:
> 	Use const for _Compare template argument.
OK
jeff
> 

Re: [PATCH 4/4] Use const for template argument.

[Jonathan Wakely]

On 04/02/20 14:55 +0100, Martin Liska wrote:
>diff --git a/libstdc++-v3/include/parallel/multiway_merge.h b/libstdc++-v3/include/parallel/multiway_merge.h
>index 983c7b2bd9a..97a9ce0feb7 100644
>--- a/libstdc++-v3/include/parallel/multiway_merge.h
>+++ b/libstdc++-v3/include/parallel/multiway_merge.h
>@@ -118,7 +118,7 @@ namespace __gnu_parallel
>        *  @return @c true if less. */
>       friend bool
>       operator<(_GuardedIterator<_RAIter, _Compare>& __bi1,
>-               _GuardedIterator<_RAIter, _Compare>& __bi2)
>+               _GuardedIterator<_RAIter, const _Compare>& __bi2)
>       {
>        if (__bi1._M_current == __bi1._M_end)       // __bi1 is sup
>          return __bi2._M_current == __bi2._M_end;  // __bi2 is not sup
>@@ -188,7 +188,7 @@ namespace __gnu_parallel
>        *  @return @c true if less. */
>       friend bool
>       operator<(_UnguardedIterator<_RAIter, _Compare>& __bi1,
>-               _UnguardedIterator<_RAIter, _Compare>& __bi2)
>+               _UnguardedIterator<_RAIter, const _Compare>& __bi2)
>       {
>        // Normal compare.
>        return (__bi1.__comp)(*__bi1, *__bi2);


This is completely bogus, please revert.

The cppcheck warning is saying that it could be:

     const _UnguardedIterator<_RAIter, _Compare>&

which is completely different from:

     _UnguardedIterator<_RAIter, const _Compare>&

Also both parameters of operator< should have been changed, not just
one, and operator<= should have the same change.

But cppcheck is completely wrong anyway. The operator* member of
_GuardedIterator and _UnguardedIterator is not const, so trying to
dereference *__b1 and *__b2 would fail.

Nack nack nack.

Re: [PATCH 4/4] Use const for template argument.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]

On 06/05/20 11:01 +0100, Jonathan Wakely wrote:
>On 04/02/20 14:55 +0100, Martin Liska wrote:
>>diff --git a/libstdc++-v3/include/parallel/multiway_merge.h b/libstdc++-v3/include/parallel/multiway_merge.h
>>index 983c7b2bd9a..97a9ce0feb7 100644
>>--- a/libstdc++-v3/include/parallel/multiway_merge.h
>>+++ b/libstdc++-v3/include/parallel/multiway_merge.h
>>@@ -118,7 +118,7 @@ namespace __gnu_parallel
>>       *  @return @c true if less. */
>>      friend bool
>>      operator<(_GuardedIterator<_RAIter, _Compare>& __bi1,
>>-               _GuardedIterator<_RAIter, _Compare>& __bi2)
>>+               _GuardedIterator<_RAIter, const _Compare>& __bi2)
>>      {
>>       if (__bi1._M_current == __bi1._M_end)       // __bi1 is sup
>>         return __bi2._M_current == __bi2._M_end;  // __bi2 is not sup
>>@@ -188,7 +188,7 @@ namespace __gnu_parallel
>>       *  @return @c true if less. */
>>      friend bool
>>      operator<(_UnguardedIterator<_RAIter, _Compare>& __bi1,
>>-               _UnguardedIterator<_RAIter, _Compare>& __bi2)
>>+               _UnguardedIterator<_RAIter, const _Compare>& __bi2)
>>      {
>>       // Normal compare.
>>       return (__bi1.__comp)(*__bi1, *__bi2);
>
>
>This is completely bogus, please revert.
>
>The cppcheck warning is saying that it could be:
>
>    const _UnguardedIterator<_RAIter, _Compare>&
>
>which is completely different from:
>
>    _UnguardedIterator<_RAIter, const _Compare>&
>
>Also both parameters of operator< should have been changed, not just
>one, and operator<= should have the same change.
>
>But cppcheck is completely wrong anyway. The operator* member of
>_GuardedIterator and _UnguardedIterator is not const, so trying to
>dereference *__b1 and *__b2 would fail.
>
>Nack nack nack.

Here's a correct fix for the cppcheck complaint.

Tested powerpc64le-linux, 'make check check-parallel', committed to
master.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4339 bytes --]

commit 4cbc9d8b346b932f34828a51e8822881413951b7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu May 7 21:43:49 2020 +0100

    libstdc++: Make relational operators work with const guarded iterators (PR 92472)
    
    This is a correct fix for the incorrect cppcheck suggestion to make
    these parameters const. In order to that, the dereference operators need
    to be const. The conversions to the underlying iterator can be const
    too.
    
            PR c/92472
            * include/parallel/multiway_merge.h (_GuardedIterator::operator*)
            (_GuardedIterator::operator _RAIter, _UnguardedIterator::operator*)
            (_UnguardedIterator::operator _RAIter): Add const qualifier.
            (operator<(_GuardedIterator&, _GuardedIterator&)
            (operator<=(_GuardedIterator&, _GuardedIterator&)
            (operator<(_UnguardedIterator&, _UnguardedIterator&)
            (operator<=(_UnguardedIterator&, _UnguardedIterator&): Change
            parameters to const references.

diff --git a/libstdc++-v3/include/parallel/multiway_merge.h b/libstdc++-v3/include/parallel/multiway_merge.h
index 983c7b2bd9a..52a8b2ca9e7 100644
--- a/libstdc++-v3/include/parallel/multiway_merge.h
+++ b/libstdc++-v3/include/parallel/multiway_merge.h
@@ -104,12 +104,12 @@ namespace __gnu_parallel
       /** @brief Dereference operator.
       *  @return Referenced element. */
       typename std::iterator_traits<_RAIter>::value_type&
-      operator*()
+      operator*() const
       { return *_M_current; }
 
       /** @brief Convert to wrapped iterator.
       *  @return Wrapped iterator. */
-      operator _RAIter()
+      operator _RAIter() const
       { return _M_current; }
 
       /** @brief Compare two elements referenced by guarded iterators.
@@ -117,8 +117,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c true if less. */
       friend bool
-      operator<(_GuardedIterator<_RAIter, _Compare>& __bi1,
-		_GuardedIterator<_RAIter, _Compare>& __bi2)
+      operator<(const _GuardedIterator<_RAIter, _Compare>& __bi1,
+		const _GuardedIterator<_RAIter, _Compare>& __bi2)
       {
 	if (__bi1._M_current == __bi1._M_end)       // __bi1 is sup
 	  return __bi2._M_current == __bi2._M_end;  // __bi2 is not sup
@@ -132,8 +132,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c True if less equal. */
       friend bool
-      operator<=(_GuardedIterator<_RAIter, _Compare>& __bi1,
-		 _GuardedIterator<_RAIter, _Compare>& __bi2)
+      operator<=(const _GuardedIterator<_RAIter, _Compare>& __bi1,
+		 const _GuardedIterator<_RAIter, _Compare>& __bi2)
       {
 	if (__bi2._M_current == __bi2._M_end)       // __bi1 is sup
 	  return __bi1._M_current != __bi1._M_end;  // __bi2 is not sup
@@ -174,12 +174,12 @@ namespace __gnu_parallel
       /** @brief Dereference operator.
       *  @return Referenced element. */
       typename std::iterator_traits<_RAIter>::value_type&
-      operator*()
+      operator*() const
       { return *_M_current; }
 
       /** @brief Convert to wrapped iterator.
       *  @return Wrapped iterator. */
-      operator _RAIter()
+      operator _RAIter() const
       { return _M_current; }
 
       /** @brief Compare two elements referenced by unguarded iterators.
@@ -187,8 +187,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c true if less. */
       friend bool
-      operator<(_UnguardedIterator<_RAIter, _Compare>& __bi1,
-		_UnguardedIterator<_RAIter, _Compare>& __bi2)
+      operator<(const _UnguardedIterator<_RAIter, _Compare>& __bi1,
+		const _UnguardedIterator<_RAIter, _Compare>& __bi2)
       {
 	// Normal compare.
 	return (__bi1.__comp)(*__bi1, *__bi2);
@@ -199,8 +199,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c True if less equal. */
       friend bool
-      operator<=(_UnguardedIterator<_RAIter, _Compare>& __bi1,
-		 _UnguardedIterator<_RAIter, _Compare>& __bi2)
+      operator<=(const _UnguardedIterator<_RAIter, _Compare>& __bi1,
+		 const _UnguardedIterator<_RAIter, _Compare>& __bi2)
       {
 	// Normal compare.
 	return !(__bi1.__comp)(*__bi2, *__bi1);

Re: [PATCH 0/4] Fix various minor issues seen with cppcheck

[Richard Biener]

On Thu, Feb 6, 2020 at 12:07 PM Martin Liska <mliska@suse.cz> wrote:
>
> Hi.
>
> The series is about small issues that were spotted with cppcheck
> and where David Binderman suggested a patch.
>
> It's probably a stage1 material?

Yes.

> Martin
>
> Martin Liska (4):
>   Remove 2 dead variables in bid_internal.h.
>   Use const for some function arguments.
>   Put index check before use.
>   Use const for template argument.
>
>  gcc/alloc-pool.h                               | 2 +-
>  gcc/bitmap.h                                   | 2 +-
>  gcc/mem-stats.h                                | 4 ++--
>  gcc/sese.h                                     | 4 ++--
>  libgcc/config/libbid/bid_internal.h            | 4 ----
>  liboffloadmic/runtime/offload_target.cpp       | 2 +-
>  libstdc++-v3/include/parallel/multiway_merge.h | 4 ++--
>  7 files changed, 9 insertions(+), 13 deletions(-)
>
> --
> 2.25.0
>