op.c: 'clobbered' warning becomes un-silenced with g++-8

[p5pRT]

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

Searchable as RT133575$

[p5pRT]

From @jkeenan

In Oct 2016 a commit made a one-line modification to op.c so as to
silence one compile-time warning​:

#####
$ git show --format=fuller 9af62dc | cat
commit 9af62dc
Author​: Lukas Mai <l.mai@​web.de>
AuthorDate​: Sat Oct 22 17​:48​:03 2016 +0200
Commit​: Lukas Mai <l.mai@​web.de>
CommitDate​: Sat Oct 22 17​:48​:03 2016 +0200

  op.c​: silence compiler warning in fold_constants()

  op.c​: In function ‘S_fold_constants’​:
  op.c​:4374​:28​: warning​: argument ‘o’ might be clobbered by ‘longjmp’
or ‘vfork’ [-Wclobbered]
  S_fold_constants(pTHX_ OP *o)
  ^

  This warning occurs for non-volatile local variables where the
compiler can't prove that their value doesn't change between setjmp and
longjmp (because the modified value may only be stored in a register
which longjmp overwrites). Adding 'const' apparently convinces the
compiler that no such modification occurs.

Inline Patch

diff --git a/op.c b/op.c
index ebbbf81a5a..cf1399ebbe 100644
--- a/op.c
+++ b/op.c
@@ -4371,7 +4371,7 @@ S_op_integerize(pTHX_ OP *o)
  }

  static OP *
-S_fold_constants(pTHX_ OP *o)
+S_fold_constants(pTHX_ OP *const o)
  {
      dVAR;
      OP * VOL curop;

#####

Unfortunately, if you compile with g++8, this warning reappears. From a
build at 51684e3 with '-Dcc=g++-8' on
Linux​:

#####
$ grep -B3 -A2 -E '^op\.c' make.g++-8.output.txt
echo @​`sh cflags "optimize='-O2'" opmini.o` -DPERL_IS_MINIPERL
-DPERL_EXTERNAL_GLOB opmini.c
@​g++-8 -c -DPERL_CORE -fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64 -ansi -O2 -Wall
-Werror=declaration-after-statement -Werror=pointer-arith -Wextra
-Wwrite-strings -DPERL_IS_MINIPERL -DPERL_EXTERNAL_GLOB opmini.c
op.c​: In function ‘OP* S_fold_constants(OP*)’​:
op.c​:5468​:34​: warning​: argument ‘o’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Wclobbered]
  S_fold_constants(pTHX_ OP *const o)
  ~~~~~~~~~~^
--
cp DynaLoader.o ../../DynaLoader.o
make[1]​: Leaving directory '/home/jkeenan/gitwork/perl/ext/DynaLoader'
g++-8 -c -DPERL_CORE -fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64 -ansi -O2 -Wall
-Werror=declaration-after-statement -Werror=pointer-arith -Wextra
-Wwrite-strings op.c
op.c​: In function ‘OP* S_fold_constants(OP*)’​:
op.c​:5468​:34​: warning​: argument ‘o’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Wclobbered]
  S_fold_constants(pTHX_ OP *const o)
  ~~~~~~~~~~^
#####

Can this be alleviated?

Thank you very much.
Jim Keenan

[p5pRT]

From @jkeenan

Summary of my perl5 (revision 5 version 29 subversion 4) configuration​:
  Commit id​: 51684e3
  Platform​:
  osname=linux
  osvers=4.15.0-36-generic
  archname=x86_64-linux
  uname='linux zareason 4.15.0-36-generic #39-ubuntu smp mon sep 24 16​:19​:09 utc 2018 x86_64 x86_64 x86_64 gnulinux '
  config_args='-des -Dusedevel -Dcc=g++-8'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  default_inc_excludes_dot=define
  bincompat5005=undef
  Compiler​:
  cc='g++-8'
  ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
  optimize='-O2'
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='8.2.0'
  gccosandvers=''
  intsize=4
  longsize=8
  ptrsize=8
  doublesize=8
  byteorder=12345678
  doublekind=3
  d_longlong=define
  longlongsize=8
  d_longdbl=define
  longdblsize=16
  longdblkind=3
  ivtype='long'
  ivsize=8
  nvtype='double'
  nvsize=8
  Off_t='off_t'
  lseeksize=8
  alignbytes=8
  prototype=define
  Linker and Libraries​:
  ld='g++-8'
  ldflags =' -fstack-protector-strong -L/usr/local/lib'
  libpth=/usr/include/c++/8 /usr/include/x86_64-linux-gnu/c++/8 /usr/include/c++/8/backward /usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/8/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /lib64 /usr/lib64
  libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.27.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.27'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl)​:
  Compile-time options​:
  HAS_TIMES
  PERLIO_LAYERS
  PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_MALLOC_WRAP
  PERL_OP_PARENT
  PERL_PRESERVE_IVUV
  PERL_USE_DEVEL
  USE_64_BIT_ALL
  USE_64_BIT_INT
  USE_LARGE_FILES
  USE_LOCALE
  USE_LOCALE_COLLATE
  USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC
  USE_LOCALE_TIME
  USE_PERLIO
  USE_PERL_ATOF
  Built under linux
  Compiled at Oct 10 2018 14​:56​:45
  %ENV​:
  PERL2DIR="/home/jkeenan/gitwork/perl2"
  PERLBREW_HOME="/home/jkeenan/.perlbrew"
  PERLBREW_MANPATH="/home/jkeenan/perl5/perlbrew/perls/perl-5.28.0/man"
  PERLBREW_PATH="/home/jkeenan/perl5/perlbrew/bin​:/home/jkeenan/perl5/perlbrew/perls/perl-5.28.0/bin"
  PERLBREW_PERL="perl-5.28.0"
  PERLBREW_ROOT="/home/jkeenan/perl5/perlbrew"
  PERLBREW_SHELLRC_VERSION="0.84"
  PERLBREW_VERSION="0.84"
  PERL_WORKDIR="/home/jkeenan/gitwork/perl"
  @​INC​:
  lib
  /usr/local/lib/perl5/site_perl/5.29.4/x86_64-linux
  /usr/local/lib/perl5/site_perl/5.29.4
  /usr/local/lib/perl5/5.29.4/x86_64-linux
  /usr/local/lib/perl5/5.29.4

[p5pRT]

From @iabyn

On Wed, Oct 10, 2018 at 12​:12​:51PM -0700, James E Keenan (via RT) wrote​:

Unfortunately, if you compile with g++8, this warning reappears.
op.c​: In function ‘OP* S_fold_constants(OP*)’​:
op.c​:5468​:34​: warning​: argument ‘o’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Wclobbered]
S_fold_constants(pTHX_ OP *const o)

In general I think our approach to the longjmp warnings should be
to move the JMPENV_PUSH...JMPENV_POP block of code into its own
small static function that does the barest minimum necessary to do the
exception catching, and thus has very few local variables. It them becomes
a lot more tractable to fix such warnings.

--
I don't want to achieve immortality through my work...
I want to achieve it through not dying.
  -- Woody Allen

[p5pRT]

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

[p5pRT]

From @tonycoz

On Mon, 15 Oct 2018 04​:01​:54 -0700, davem wrote​:

On Wed, Oct 10, 2018 at 12​:12​:51PM -0700, James E Keenan (via RT) wrote​:

Unfortunately, if you compile with g++8, this warning reappears.
op.c​: In function ‘OP* S_fold_constants(OP*)’​:
op.c​:5468​:34​: warning​: argument ‘o’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Wclobbered]
S_fold_constants(pTHX_ OP *const o)

In general I think our approach to the longjmp warnings should be
to move the JMPENV_PUSH...JMPENV_POP block of code into its own
small static function that does the barest minimum necessary to do the
exception catching, and thus has very few local variables. It them becomes
a lot more tractable to fix such warnings.

Something like the attached?

Tony

[p5pRT]

From @tonycoz

0001-perl-133575-prevent-set-longjmp-clobbering-locals-in.patch

From 7ba450c0cc472a49f30902abd01e5ca021aa264b Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 3 Jan 2019 10:48:05 +1100
Subject: (perl #133575) prevent set/longjmp clobbering locals in
 S_fold_constants

My original approach moved the whole switch into the new function,
but that was a lot messier, and I don't think it's necessary.

pad_swipe() can throw, but only for panics, and in DESTROY if
refadjust is true, which isn't the case here.

CLEAR_ERRSV() might throw if the code called by CALLRUNOPS()
puts an object that dies in DESTROY in $@, but I think that
might cause an infinite loop in the original code.
---
 op.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/op.c b/op.c
index 146407ba70..0b46b348cb 100644
--- a/op.c
+++ b/op.c
@@ -5464,15 +5464,34 @@ S_op_integerize(pTHX_ OP *o)
     return o;
 }
 
+/* This function exists solely to provide a scope to limit
+   setjmp/longjmp() messing with auto variables.
+ */
+PERL_STATIC_INLINE int
+S_fold_constants_eval(pTHX) {
+    int ret = 0;
+    dJMPENV;
+
+    JMPENV_PUSH(ret);
+
+    if (ret == 0) {
+	CALLRUNOPS(aTHX);
+    }
+
+    JMPENV_POP;
+
+    return ret;
+}
+
 static OP *
 S_fold_constants(pTHX_ OP *const o)
 {
     dVAR;
-    OP * volatile curop;
+    OP *curop;
     OP *newop;
-    volatile I32 type = o->op_type;
+    I32 type = o->op_type;
     bool is_stringify;
-    SV * volatile sv = NULL;
+    SV *sv = NULL;
     int ret = 0;
     OP *old_next;
     SV * const oldwarnhook = PL_warnhook;
@@ -5480,7 +5499,6 @@ S_fold_constants(pTHX_ OP *const o)
     COP not_compiling;
     U8 oldwarn = PL_dowarn;
     I32 old_cxix;
-    dJMPENV;
 
     PERL_ARGS_ASSERT_FOLD_CONSTANTS;
 
@@ -5582,15 +5600,15 @@ S_fold_constants(pTHX_ OP *const o)
     assert(IN_PERL_RUNTIME);
     PL_warnhook = PERL_WARNHOOK_FATAL;
     PL_diehook  = NULL;
-    JMPENV_PUSH(ret);
 
     /* Effective $^W=1.  */
     if ( ! (PL_dowarn & G_WARN_ALL_MASK))
 	PL_dowarn |= G_WARN_ON;
 
+    ret = S_fold_constants_eval(aTHX);
+
     switch (ret) {
     case 0:
-	CALLRUNOPS(aTHX);
 	sv = *(PL_stack_sp--);
 	if (o->op_targ && sv == PAD_SV(o->op_targ)) {	/* grab pad temp? */
 	    pad_swipe(o->op_targ,  FALSE);
@@ -5608,7 +5626,6 @@ S_fold_constants(pTHX_ OP *const o)
 	o->op_next = old_next;
 	break;
     default:
-	JMPENV_POP;
 	/* Don't expect 1 (setjmp failed) or 2 (something called my_exit)  */
 	PL_warnhook = oldwarnhook;
 	PL_diehook  = olddiehook;
@@ -5616,7 +5633,6 @@ S_fold_constants(pTHX_ OP *const o)
 	 * the stack - eg any nested evals */
 	Perl_croak(aTHX_ "panic: fold_constants JMPENV_PUSH returned %d", ret);
     }
-    JMPENV_POP;
     PL_dowarn   = oldwarn;
     PL_warnhook = oldwarnhook;
     PL_diehook  = olddiehook;
-- 
2.11.0

[p5pRT]

From @jkeenan

On Thu, 03 Jan 2019 00​:29​:16 GMT, tonyc wrote​:

On Mon, 15 Oct 2018 04​:01​:54 -0700, davem wrote​:

On Wed, Oct 10, 2018 at 12​:12​:51PM -0700, James E Keenan (via RT) wrote​:

Unfortunately, if you compile with g++8, this warning reappears.
op.c​: In function ‘OP* S_fold_constants(OP*)’​:
op.c​:5468​:34​: warning​: argument ‘o’ might be clobbered by ‘longjmp’ or
‘vfork’ [-Wclobbered]
S_fold_constants(pTHX_ OP *const o)

In general I think our approach to the longjmp warnings should be
to move the JMPENV_PUSH...JMPENV_POP block of code into its own
small static function that does the barest minimum necessary to do the
exception catching, and thus has very few local variables. It them becomes
a lot more tractable to fix such warnings.

Something like the attached?

Made available for smoke-testing in this branch​:

smoke-me/jkeenan/tonyc/133575-longjmp-clobbering

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

From @jkeenan

On Thu, 03 Jan 2019 15​:51​:38 GMT, jkeenan wrote​:

[snip]

Made available for smoke-testing in this branch​:

smoke-me/jkeenan/tonyc/133575-longjmp-clobbering

Configuring on FreeBSD-11.2 as follows​:

#####
$ grep '^config_args=' config.sh
config_args='-des -Dusedevel -Dcc=g++8 -Duseithreads'
#####

... I built perl on blead and in the smoke-me/jkeenan/tonyc/133575-longjmp-clobbering branch. Here is a diff of the 'make' output​:

#####
$ diff g++8.make.output.blead.txt g++8.make.output.clobberbranch.txt
3,6d2
< op.c​: In function 'OP* S_fold_constants(PerlInterpreter*, OP*)'​:
< op.c​:5468​:34​: warning​: argument 'o' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]
< S_fold_constants(pTHX_ OP *const o)
< ~~~~~~~~~~^
444,447d439
< op.c​: In function 'OP* S_fold_constants(PerlInterpreter*, OP*)'​:
< op.c​:5468​:34​: warning​: argument 'o' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]
< S_fold_constants(pTHX_ OP *const o)
< ~~~~~~~~~~^
#####

So the '-Wclobbered' warning has been successfully squelched in the branch. See also this smoke-test report where I built the branch on Linux with gcc-8 and g++-8 and avoided all '-Wclobbered' warnings​:

http​://perl5.test-smoke.org/report/77558

So, unless there's some flaw in the C-code I don't see, I think the patch should be applied.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

From @tonycoz

On Sat, 05 Jan 2019 05​:36​:24 -0800, jkeenan wrote​:

So, unless there's some flaw in the C-code I don't see, I think the
patch should be applied.

Applied as 35ad013.

Tony

[p5pRT]

@tonycoz - Status changed from 'open' to 'pending release'

[p5pRT]

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0, this and 160 other issues have been
resolved.

Perl 5.30.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists, feel free to reopen this ticket.

[p5pRT]

@khwilliamson - Status changed from 'pending release' to 'resolved'