[PATCH] Coverity: Avoid "unused sp" #13829

p5pRT · 2014-05-13T15:54:42Z

Migrated from rt.perl.org#121860 (status was 'resolved')

Searchable as RT121860$

p5pRT · 2014-05-13T15:54:42Z

From @jhi

Sometimes the "sp" assigned to by EXTEND (or MEXTEND) is the last thing
mentioning the sp. The existing cast to void helps to ignore *the value
of the assignment expression* but it doesn't help to ignore *the
variable sp*. So the patch splits the assignment and the void casting
in two expressions, with a comma operator in between.

(Aside: what a horrible API EXTEND has... it is declared void, but it
unconditionally sets the sp which it expects to be in scope.)

An alternative solution could've been to define 'EXTEND_nosp' (and
MEXTEND_nosp) that do not set the sp, but we have a gnarly API such as
it is...

p5pRT · 2014-05-13T15:54:42Z

From @jhi

0001-Avoid-unused-sp-if-EXTEND-is-the-last-mentioning-sp.patch

From bdcb14a0b69120a34faded06c3e3c53e69b7a639 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Tue, 6 May 2014 15:49:59 -0400
Subject: [PATCH] Avoid "unused sp" if EXTEND is the last mentioning sp.

Move the (void)sp later to fake use of the sp (like PERL_UNUSED_VAR does),
in case the EXTEND (or MEXTEND) is the last thing mentioning the sp.

Addresses Coverity perl5 CIDs 29199..29201, 29204, 29205, 29206,
29208, 29210, 29212, 29213, 29215, 29216, 29219..29221.
---
 pp.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/pp.h b/pp.h
index 97738c2..445b303 100644
--- a/pp.h
+++ b/pp.h
@@ -271,21 +271,21 @@ Does not use C<TARG>.  See also C<XPUSHu>, C<mPUSHu> and C<PUSHu>.
 */
 
 #ifdef STRESS_REALLOC
-# define EXTEND(p,n)	(void)(sp = stack_grow(sp,p, (SSize_t)(n)))
+# define EXTEND(p,n)	((sp = stack_grow(sp,p, (SSize_t)(n))),(void)sp)
 /* Same thing, but update mark register too. */
 # define MEXTEND(p,n)	STMT_START {					\
 			    const int markoff = mark - PL_stack_base;	\
-			    sp = stack_grow(sp,p,(SSize_t) (n));	\
+			    (sp = stack_grow(sp,p,(SSize_t) (n))),(void)sp; \
 			    mark = PL_stack_base + markoff;		\
 			} STMT_END
 #else
-# define EXTEND(p,n)   (void)(UNLIKELY(PL_stack_max - p < (SSize_t)(n)) &&     \
-			    (sp = stack_grow(sp,p, (SSize_t) (n))))
+# define EXTEND(p,n)   (UNLIKELY(PL_stack_max - p < (SSize_t)(n)) &&     \
+                        ((sp = stack_grow(sp,p, (SSize_t) (n)))),(void)sp)
 
 /* Same thing, but update mark register too. */
 # define MEXTEND(p,n)  STMT_START {if (UNLIKELY(PL_stack_max - p < (int)(n))) {\
                            const int markoff = mark - PL_stack_base;           \
-                           sp = stack_grow(sp,p,(SSize_t) (n));                \
+                           (sp = stack_grow(sp,p,(SSize_t) (n))),(void)sp;	\
                            mark = PL_stack_base + markoff;                     \
                        } } STMT_END
 #endif
-- 
1.9.2

p5pRT · 2014-05-20T02:04:11Z

From @tonycoz

On Tue May 13 08:54:42 2014, jhi wrote:

Sometimes the "sp" assigned to by EXTEND (or MEXTEND) is the last thing
mentioning the sp. The existing cast to void helps to ignore *the value
of the assignment expression* but it doesn't help to ignore *the
variable sp*. So the patch splits the assignment and the void casting
in two expressions, with a comma operator in between.

(Aside: what a horrible API EXTEND has... it is declared void, but it
unconditionally sets the sp which it expects to be in scope.)

An alternative solution could've been to define 'EXTEND_nosp' (and
MEXTEND_nosp) that do not set the sp, but we have a gnarly API such as
it is...

The change as it is introduces many unused value warnings under gcc 4.7.2, eg:

cc -c -DPERL_CORE -D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -g -Wall -ansi -W -Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat -Wwrite-strings mg.c
mg.c: In function ‘Perl_magic_methcall’:
mg.c:1785:5: warning: value computed is not used [-Wunused-value]
mg.c: In function ‘Perl_sighandler’:
mg.c:3215:6: warning: value computed is not used [-Wunused-value]

Tony

p5pRT · 2014-05-20T02:04:12Z

The RT System itself - Status changed from 'new' to 'open'

p5pRT · 2014-05-20T11:50:20Z

From @jhi

On Monday-201405-19, 22:04, Tony Cook via RT wrote:

On Tue May 13 08:54:42 2014, jhi wrote:

Sometimes the "sp" assigned to by EXTEND (or MEXTEND) is the last thing
mentioning the sp. The existing cast to void helps to ignore *the value
of the assignment expression* but it doesn't help to ignore *the
variable sp*. So the patch splits the assignment and the void casting
in two expressions, with a comma operator in between.

(Aside: what a horrible API EXTEND has... it is declared void, but it
unconditionally sets the sp which it expects to be in scope.)

An alternative solution could've been to define 'EXTEND_nosp' (and
MEXTEND_nosp) that do not set the sp, but we have a gnarly API such as
it is...

The change as it is introduces many unused value warnings under gcc 4.7.2, eg:

cc -c -DPERL_CORE -D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -g -Wall -ansi -W -Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat -Wwrite-strings mg.c
mg.c: In function ‘Perl_magic_methcall’:
mg.c:1785:5: warning: value computed is not used [-Wunused-value]
mg.c: In function ‘Perl_sighandler’:
mg.c:3215:6: warning: value computed is not used [-Wunused-value]

Argh. Fighting different compilers' ideas about what value is unused...
giving in and using PERL_UNUSED_VAR(sp). This means that EXTEND() is
now a block, but since it's void, that works. Patch attached.

p5pRT · 2014-05-20T11:50:20Z

From @jhi

0001-Avoid-unused-sp-if-EXTEND-is-the-last-mentioning-sp.patch

From c004e0931d476a3e26fd44abee0f14539979b881 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Tue, 20 May 2014 07:44:06 -0400
Subject: [PATCH] Avoid "unused sp" if EXTEND is the last mentioning sp.

Add PERL_UNUSED_VAR(sp) in case the EXTEND (or MEXTEND)
is the last thing mentioning the sp.

Addresses Coverity perl5 CIDs 29199..29201, 29204, 29205, 29206,
29208, 29210, 29212, 29213, 29215, 29216, 29219..29221.
---
 pp.h | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/pp.h b/pp.h
index 97738c2..09d8bb1 100644
--- a/pp.h
+++ b/pp.h
@@ -271,23 +271,31 @@ Does not use C<TARG>.  See also C<XPUSHu>, C<mPUSHu> and C<PUSHu>.
 */
 
 #ifdef STRESS_REALLOC
-# define EXTEND(p,n)	(void)(sp = stack_grow(sp,p, (SSize_t)(n)))
+# define EXTEND(p,n)   STMT_START {                                     \
+                           sp = stack_grow(sp,p,(SSize_t) (n));         \
+                           PERL_UNUSED_VAR(sp);                         \
+                       } } STMT_END
 /* Same thing, but update mark register too. */
-# define MEXTEND(p,n)	STMT_START {					\
-			    const int markoff = mark - PL_stack_base;	\
-			    sp = stack_grow(sp,p,(SSize_t) (n));	\
-			    mark = PL_stack_base + markoff;		\
-			} STMT_END
+# define MEXTEND(p,n)   STMT_START {                                    \
+                            const int markoff = mark - PL_stack_base;   \
+                            sp = stack_grow(sp,p,(SSize_t) (n)));       \
+                            mark = PL_stack_base + markoff;             \
+                            PERL_UNUSED_VAR(sp);                        \
+                        } STMT_END
 #else
-# define EXTEND(p,n)   (void)(UNLIKELY(PL_stack_max - p < (SSize_t)(n)) &&     \
-			    (sp = stack_grow(sp,p, (SSize_t) (n))))
-
+# define EXTEND(p,n)   STMT_START {                                     \
+                         if (UNLIKELY(PL_stack_max - p < (int)(n))) {   \
+                           sp = stack_grow(sp,p,(SSize_t) (n));         \
+                           PERL_UNUSED_VAR(sp);                         \
+                         } } STMT_END
 /* Same thing, but update mark register too. */
-# define MEXTEND(p,n)  STMT_START {if (UNLIKELY(PL_stack_max - p < (int)(n))) {\
-                           const int markoff = mark - PL_stack_base;           \
-                           sp = stack_grow(sp,p,(SSize_t) (n));                \
-                           mark = PL_stack_base + markoff;                     \
-                       } } STMT_END
+# define MEXTEND(p,n)  STMT_START {                                     \
+                         if (UNLIKELY(PL_stack_max - p < (int)(n))) {   \
+                           const int markoff = mark - PL_stack_base;    \
+                           sp = stack_grow(sp,p,(SSize_t) (n));         \
+                           mark = PL_stack_base + markoff;              \
+                           PERL_UNUSED_VAR(sp);                         \
+                         } } STMT_END
 #endif
 
 #define PUSHs(s)	(*++sp = (s))
-- 
1.8.5.2 (Apple Git-48)

p5pRT · 2014-05-28T08:23:53Z

From @tsee

On Tue May 13 08:54:42 2014, jhi wrote:

Sometimes the "sp" assigned to by EXTEND (or MEXTEND) is the last thing
mentioning the sp. The existing cast to void helps to ignore *the value
of the assignment expression* but it doesn't help to ignore *the
variable sp*. So the patch splits the assignment and the void casting
in two expressions, with a comma operator in between.

(Aside: what a horrible API EXTEND has... it is declared void, but it
unconditionally sets the sp which it expects to be in scope.)

An alternative solution could've been to define 'EXTEND_nosp' (and
MEXTEND_nosp) that do not set the sp, but we have a gnarly API such as
it is...

IMO we can, albeit slowly and carefully, evolve said horrific API.

p5pRT · 2014-05-28T10:51:57Z

From @tsee

Thanks, applied as aad79b3.

p5pRT · 2014-05-28T10:51:58Z

@tsee - Status changed from 'open' to 'resolved'

p5pRT · 2014-06-04T03:43:30Z

From @bulk88

On Tue May 13 08:54:42 2014, jhi wrote:

Sometimes the "sp" assigned to by EXTEND (or MEXTEND) is the last thing
mentioning the sp. The existing cast to void helps to ignore *the value
of the assignment expression* but it doesn't help to ignore *the
variable sp*. So the patch splits the assignment and the void casting
in two expressions, with a comma operator in between.

(Aside: what a horrible API EXTEND has... it is declared void, but it
unconditionally sets the sp which it expects to be in scope.)

An alternative solution could've been to define 'EXTEND_nosp' (and
MEXTEND_nosp) that do not set the sp, but we have a gnarly API such as
it is...

This broke some code of mine where I used EXTEND as an expression with a ",", not ";". CPAN Grep shows nothing that isn't core http:// grep.cpan.me/ ?q=EXTEND\%28%28sp|SP%29%2C%28\d%2B|\w%2B%29\%29%2C aka "EXTEND$(sp|SP),(\d+|\w+)$,". I do see this commit though http://perl5.git.perl.org/perl.git/commitdiff/0acfb02fed689f2745813114bae77c22a2211cc7 . Someone else with darkpan code may come complaining in the future. So this is a heads up.

--
bulk88 ~ bulk88 at hotmail.com

p5pRT closed this as completed May 28, 2014

p5pRT added Severity Low hasPatch labels Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH] Coverity: Avoid "unused sp" #13829

[PATCH] Coverity: Avoid "unused sp" #13829

p5pRT commented May 13, 2014

p5pRT commented May 13, 2014

p5pRT commented May 13, 2014

p5pRT commented May 20, 2014

p5pRT commented May 20, 2014

p5pRT commented May 20, 2014

p5pRT commented May 20, 2014

p5pRT commented May 28, 2014

p5pRT commented May 28, 2014

p5pRT commented May 28, 2014

p5pRT commented Jun 4, 2014

[PATCH] Coverity: Avoid "unused sp" #13829

[PATCH] Coverity: Avoid "unused sp" #13829

Comments

p5pRT commented May 13, 2014

p5pRT commented May 13, 2014

From @jhi

p5pRT commented May 13, 2014

From @jhi

p5pRT commented May 20, 2014

From @tonycoz

p5pRT commented May 20, 2014

p5pRT commented May 20, 2014

From @jhi

p5pRT commented May 20, 2014

From @jhi

p5pRT commented May 28, 2014

From @tsee

p5pRT commented May 28, 2014

From @tsee

p5pRT commented May 28, 2014

p5pRT commented Jun 4, 2014

From @bulk88