Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] PERL_UNUSED_CONTEXT audit #13935

Closed
p5pRT opened this issue Jun 17, 2014 · 10 comments
Closed

[PATCH] PERL_UNUSED_CONTEXT audit #13935

p5pRT opened this issue Jun 17, 2014 · 10 comments

Comments

@p5pRT
Copy link

p5pRT commented Jun 17, 2014

Migrated from rt.perl.org#122122 (status was 'rejected')

Searchable as RT122122$

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2014

From @bulk88

Created by @bulk88

See attached patch.

Perl Info

Flags:
        category=core
        severity=low

Site configuration information for perl 5.21.1:

Configured by Owner at Wed May 28 03:57:28 2014.

Summary of my perl5 (revision 5 version 21 subversion 1) configuration:
      Local Commit: 1abbcfa06576bf8a6937c566bb4d18ba803b59d8
      Ancestor: 234105dd8a732de5fb48ccb1838c99281f89f669
      Platform:
        osname=MSWin32, osvers=5.1, archname=MSWin32-x86-multi-thread
        uname=''
        config_args='undef'
        hint=recommended, useposix=true, d_sigaction=undef
        useithreads=define, usemultiplicity=define
        use64bitint=undef, use64bitall=undef, uselongdouble=undef
        usemymalloc=n, bincompat5005=undef
      Compiler:
        cc='cl', ccflags ='-nologo -GF -W3 -O1 -MD -Zi -DNDEBUG -G7 -GL
-DWIN32 -D_CONSOLE -DNO_STRICT  -DPERL_TEXTMODE_SCRIPTS
-DPERL_HASH_FUNC_ONE_AT_A_TIME -DPERL_IMPLICIT_CONTEXT
-DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T',
        optimize='-O1 -MD -Zi -DNDEBUG -G7 -GL',
        cppflags='-DWIN32'
        ccversion='13.10.6030', gccversion='', gccosandvers=''
        intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
        d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
        ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64',
lseeksize=8
        alignbytes=8, prototype=define
      Linker and Libraries:
        ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf
-ltcg  -libpath:"c:\perl521\lib\CORE"  -machine:x86'
        libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib"
        libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib
comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib
netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  version.lib
odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
        perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib
winspool.lib  comdlg32.lib advapi32.lib shell32.lib ole32.lib
oleaut32.lib  netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib
version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
        libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl521.lib
        gnulibc_version=''
      Dynamic Linking:
        dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
        cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug
-opt:ref,icf -ltcg  -libpath:"c:\perl521\lib\CORE"  -machine:x86'

Locally applied patches:
        7494266ea378a3cdc4bfd51725012c1e432db0f1
        61961437d9453dd0d4053ad100e97a029a24edbb
        cd30b936fc5177ce169d776445d09c9898c15da1
        1abbcfa06576bf8a6937c566bb4d18ba803b59d8


@INC for perl 5.21.1:
        C:/perl521/site/lib
        C:/perl521/lib
        .


Environment for perl 5.21.1:
        HOME (unset)
        LANG (unset)
        LANGUAGE (unset)
        LD_LIBRARY_PATH (unset)
        LOGDIR (unset)
        PATH=C:\perl521\bin;C:\Program Files\Microsoft Visual Studio .NET
2003\Common7\IDE;C:\Program Files\Microsoft Visual Studio .NET
2003\VC7\BIN;C:\Program Files\Microsoft Visual Studio .NET
2003\Common7\Tools;C:\Program Files\Microsoft Visual Studio .NET
2003\Common7\Tools\bin\prerelease;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;
        PERL_BADLANG (unset)
        SHELL (unset)







@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2014

From @bulk88

0001-PERL_UNUSED_CONTEXT-audit.patch
From fa479836a7bb587357407b408c00998da7b2bc56 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Tue, 17 Jun 2014 18:58:53 -0400
Subject: [PATCH] PERL_UNUSED_CONTEXT audit

make PERL_UNUSED_CONTEXT be poisoned, if the context will truly never be
used, then make sure it wont, an assignment to var that is never referenced
again will optimize away by CC

-Perl_refcounted_he_free added in commit 57ca3b03ba, a few days after
 commit b6bbf3fa3e and 71ad1b0c38 started not using context in some build
 permutations
-PerlIOUnix_setfd is positioned to avoid whitespace changes+C89
-rest of perlio, is vtable calls under PERL_IMPLICIT_SYS
-*rxres* use my_perl under COW
-pp_leavegiven, PERL_UNUSED_CONTEXT was wrong from its creation at
 commit 96a5add60f , POPBLOCK and PL_curpm and PUTBACK all use my_perl
-Perl_dirp_dup, PERL_UNUSED_CONTEXT was correct from its creation at
 commit 96a5add60f but made obsolete partially at commit 11a11ecf4b and
 fully at commit 60b22aca14 , ptr_table_* uses my_perl

most changes are related to PERL_IMPLICIT_SYS redirecting memory allocation
through interp instance specific vtables
---
 hv.c      |    2 ++
 perl.h    |    2 +-
 perlio.c  |   26 +++++++++++++++++++++++++-
 pp_ctl.c  |    5 ++++-
 regcomp.c |    2 ++
 sv.c      |    2 --
 util.c    |    5 ++++-
 7 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/hv.c b/hv.c
index db3ce63..09c5494 100644
--- a/hv.c
+++ b/hv.c
@@ -3462,7 +3462,9 @@ no action occurs in this case.
 void
 Perl_refcounted_he_free(pTHX_ struct refcounted_he *he) {
     dVAR;
+#if ! defined(PERL_IMPLICIT_SYS) && defined(USE_ITHREADS)
     PERL_UNUSED_CONTEXT;
+#endif
 
     while (he) {
 	struct refcounted_he *copy;
diff --git a/perl.h b/perl.h
index f59eb6e..116b344 100644
--- a/perl.h
+++ b/perl.h
@@ -322,7 +322,7 @@
 #endif
 
 #ifdef USE_ITHREADS
-#  define PERL_UNUSED_CONTEXT PERL_UNUSED_ARG(my_perl)
+#  define PERL_UNUSED_CONTEXT (my_perl = NULL)
 #else
 #  define PERL_UNUSED_CONTEXT
 #endif
diff --git a/perlio.c b/perlio.c
index d41c2f5..1f9e819 100644
--- a/perlio.c
+++ b/perlio.c
@@ -2597,10 +2597,12 @@ PerlIOUnix_setfd(pTHX_ PerlIO *f, int fd, int imode)
 	}
     }
 #endif
+#if !defined(WIN32) || !defined(PERL_IMPLICIT_SYS)
+    PERL_UNUSED_CONTEXT;
+#endif
     s->fd = fd;
     s->oflags = imode;
     PerlIOUnix_refcnt_inc(fd);
-    PERL_UNUSED_CONTEXT;
 }
 
 IV
@@ -2628,7 +2630,9 @@ PerlIOUnix_seek(pTHX_ PerlIO *f, Off_t offset, int whence)
 {
     const int fd = PerlIOSelf(f, PerlIOUnix)->fd;
     Off_t new_loc;
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
     if (PerlIOBase(f)->flags & PERLIO_F_NOTREG) {
 #ifdef  ESPIPE
 	SETERRNO(ESPIPE, LIB_INVARG);
@@ -2791,7 +2795,9 @@ PerlIOUnix_write(pTHX_ PerlIO *f, const void *vbuf, Size_t count)
 Off_t
 PerlIOUnix_tell(pTHX_ PerlIO *f)
 {
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     return PerlLIO_lseek(PerlIOSelf(f, PerlIOUnix)->fd, 0, SEEK_CUR);
 }
@@ -2880,7 +2886,9 @@ typedef struct {
 IV
 PerlIOStdio_fileno(pTHX_ PerlIO *f)
 {
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     if (PerlIOValid(f)) {
 	FILE * const s = PerlIOSelf(f, PerlIOStdio)->stdio;
@@ -3419,7 +3427,9 @@ IV
 PerlIOStdio_seek(pTHX_ PerlIO *f, Off_t offset, int whence)
 {
     FILE * const stdio = PerlIOSelf(f, PerlIOStdio)->stdio;
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     return PerlSIO_fseek(stdio, offset, whence);
 }
@@ -3428,7 +3438,9 @@ Off_t
 PerlIOStdio_tell(pTHX_ PerlIO *f)
 {
     FILE * const stdio = PerlIOSelf(f, PerlIOStdio)->stdio;
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     return PerlSIO_ftell(stdio);
 }
@@ -3437,7 +3449,9 @@ IV
 PerlIOStdio_flush(pTHX_ PerlIO *f)
 {
     FILE * const stdio = PerlIOSelf(f, PerlIOStdio)->stdio;
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     if (PerlIOBase(f)->flags & PERLIO_F_CANWRITE) {
 	return PerlSIO_fflush(stdio);
@@ -3465,7 +3479,9 @@ PerlIOStdio_flush(pTHX_ PerlIO *f)
 IV
 PerlIOStdio_eof(pTHX_ PerlIO *f)
 {
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     return PerlSIO_feof(PerlIOSelf(f, PerlIOStdio)->stdio);
 }
@@ -3473,7 +3489,9 @@ PerlIOStdio_eof(pTHX_ PerlIO *f)
 IV
 PerlIOStdio_error(pTHX_ PerlIO *f)
 {
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     return PerlSIO_ferror(PerlIOSelf(f, PerlIOStdio)->stdio);
 }
@@ -3481,7 +3499,9 @@ PerlIOStdio_error(pTHX_ PerlIO *f)
 void
 PerlIOStdio_clearerr(pTHX_ PerlIO *f)
 {
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     PerlSIO_clearerr(PerlIOSelf(f, PerlIOStdio)->stdio);
 }
@@ -3489,7 +3509,9 @@ PerlIOStdio_clearerr(pTHX_ PerlIO *f)
 void
 PerlIOStdio_setlinebuf(pTHX_ PerlIO *f)
 {
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
 #ifdef HAS_SETLINEBUF
     PerlSIO_setlinebuf(PerlIOSelf(f, PerlIOStdio)->stdio);
@@ -3573,7 +3595,9 @@ PerlIOStdio_fill(pTHX_ PerlIO *f)
 {
     FILE * stdio;
     int c;
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
     if (PerlIO_lockcnt(f)) /* in use: abort ungracefully */
 	return -1;
     stdio = PerlIOSelf(f, PerlIOStdio)->stdio;
diff --git a/pp_ctl.c b/pp_ctl.c
index 9cb6aba..8909dfd 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -397,7 +397,9 @@ S_rxres_restore(pTHX_ void **rsp, REGEXP *rx)
     U32 i;
 
     PERL_ARGS_ASSERT_RXRES_RESTORE;
+#ifndef PERL_ANY_COW
     PERL_UNUSED_CONTEXT;
+#endif
 
     RX_MATCH_COPY_FREE(rx);
     RX_MATCH_COPIED_set(rx, *p);
@@ -427,7 +429,9 @@ S_rxres_free(pTHX_ void **rsp)
     UV * const p = (UV*)*rsp;
 
     PERL_ARGS_ASSERT_RXRES_FREE;
+#ifndef PERL_ANY_COW
     PERL_UNUSED_CONTEXT;
+#endif
 
     if (p) {
 	void *tmp = INT2PTR(char*,*p);
@@ -4459,7 +4463,6 @@ PP(pp_leavegiven)
     I32 gimme;
     SV **newsp;
     PMOP *newpm;
-    PERL_UNUSED_CONTEXT;
 
     POPBLOCK(cx,newpm);
     assert(CxTYPE(cx) == CXt_GIVEN);
diff --git a/regcomp.c b/regcomp.c
index 205c840..eb58a99 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -15963,7 +15963,9 @@ Perl_re_intuit_string(pTHX_ REGEXP * const r)
     GET_RE_DEBUG_FLAGS_DECL;
 
     PERL_ARGS_ASSERT_RE_INTUIT_STRING;
+#ifndef DEBUGGING
     PERL_UNUSED_CONTEXT;
+#endif
 
     DEBUG_COMPILE_r(
 	{
diff --git a/sv.c b/sv.c
index 8e3c4cf..e2e1a25 100644
--- a/sv.c
+++ b/sv.c
@@ -11977,8 +11977,6 @@ Perl_dirp_dup(pTHX_ DIR *const dp, CLONE_PARAMS *const param)
     STRLEN len = 0;
     long pos;
 #endif
-
-    PERL_UNUSED_CONTEXT;
     PERL_ARGS_ASSERT_DIRP_DUP;
 
     if (!dp)
diff --git a/util.c b/util.c
index 195402a..509023f 100644
--- a/util.c
+++ b/util.c
@@ -1105,8 +1105,9 @@ Perl_savesharedpv(pTHX_ const char *pv)
 {
     char *newaddr;
     STRLEN pvlen;
-
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
 
     if (!pv)
 	return NULL;
@@ -1999,7 +2000,9 @@ Perl_new_warnings_bitfield(pTHX_ STRLEN *buffer, const char *const bits,
 			   STRLEN size) {
     const MEM_SIZE len_wanted =
 	sizeof(STRLEN) + (size > WARNsize ? size : WARNsize);
+#ifndef PERL_IMPLICIT_SYS
     PERL_UNUSED_CONTEXT;
+#endif
     PERL_ARGS_ASSERT_NEW_WARNINGS_BITFIELD;
 
     buffer = (STRLEN*)
-- 
1.7.9.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2014

From @bulk88

This was very poorly smoked since my unthreaded and no-PERL_IMPLICIT_SYS threaded perls were crashing due to another open ticket due to a Dynaloder/LoadLibrary bug that is open in another ticket.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jun 19, 2014

From @tonycoz

On Tue Jun 17 16​:05​:37 2014, bulk88 wrote​:

This was very poorly smoked since my unthreaded and no-
PERL_IMPLICIT_SYS threaded perls were crashing due to another open
ticket due to a Dynaloder/LoadLibrary bug that is open in another
ticket.

This change​:

--- a/perl.h
+++ b/perl.h
@​@​ -322,7 +322,7 @​@​
#endif

#ifdef USE_ITHREADS
-# define PERL_UNUSED_CONTEXT PERL_UNUSED_ARG(my_perl)
+# define PERL_UNUSED_CONTEXT (my_perl = NULL)
#else
# define PERL_UNUSED_CONTEXT
#endif

would need to be limited to -DDEBUGGING builds.

I expected gcc to warn about this assignment if the value isn't used, but couldn't get it to happen.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 19, 2014

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

@p5pRT
Copy link
Author

p5pRT commented Jun 25, 2014

From @tonycoz

On Tue Jun 17 16​:05​:37 2014, bulk88 wrote​:

This was very poorly smoked since my unthreaded and no-
PERL_IMPLICIT_SYS threaded perls were crashing due to another open
ticket due to a Dynaloder/LoadLibrary bug that is open in another
ticket.

It fails fairly badly on POSIXish systems​:

cc -fstack-protector -L/usr/local/lib -o miniperl \
  perlmini.o opmini.o miniperlmain.o gv.o toke.o perly.o pad.o regcomp.o dump.o util.o mg.o reentr.o mro.o keywords.o hv.o av.o run.o pp_hot.o sv.o pp.o scope.o pp_ctl.o pp_sys.o doop.o doio.o regexec.o utf8.o taint.o deb.o universal.o globals.o perlio.o perlapi.o numeric.o mathoms.o locale.o pp_pack.o pp_sort.o caretx.o -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>' || sh -c 'echo >&2 Failed to build miniperl. Please run make minitest; exit 1'
Segmentation fault
Failed to build miniperl. Please run make minitest

tony@​mars​:.../git/perl2$ gdb --args ./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>'
GNU gdb (GDB) 7.4.1-debian
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+​: GNU GPL version 3 or later <http​://gnu.org/licenses/gpl.html>
This is free software​: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see​:
<http​://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/tony/dev/perl/git/perl2/miniperl...done.
(gdb) r
Starting program​: /home/tony/dev/perl/git/perl2/miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e \<\?\>
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
Perl_reentrant_init (my_perl=0x0, my_perl@​entry=0x8ba010) at reentr.c​:145
145 Newx(PL_reentrant_buffer, 1, REENTR);
(gdb) bt
#0 Perl_reentrant_init (my_perl=0x0, my_perl@​entry=0x8ba010) at reentr.c​:145
#1 0x000000000040a96b in perl_construct (my_perl=0x8ba010) at perl.c​:283
#2 0x0000000000406afe in main (argc=7, argv=0x7fffffffe888,
  env=0x7fffffffe8c8) at miniperlmain.c​:114
(gdb)

This particular crash is fixable with some changes to regen/reentr.pl to move the PERL_UNUSED_CONTEXT into a #else, but it continues to crash elsewhere after that is fixed​:

Program received signal SIGSEGV, Segmentation fault.
Perl_opslab_free (my_perl=0x0, slab=0x8e7ec0) at op.c​:379
379 DEBUG_S_warn((aTHX_ "freeing slab %p", (void*)slab));

So this patch isn't currently suitable for blead.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 28, 2014

From @jhi

So this patch isn't currently suitable for blead.

bulk88, if you take a look at the latest blead, I have been actually also working towards removing unused contexts (or making them conditional on whatever) (because I was turning on as many gcc -Wflags as I could easily). I haven't looked at your patch in detail, but I think you will find we covered many of the same spots.

See for example​:

http​://perl5.git.perl.org/perl.git/commit/23491f1decae17401c5c08e7f8a11e0d30f4f0e2
http​://perl5.git.perl.org/perl.git/commit/dd791c72c136e634557810ed390db9ed30f1576f
http​://perl5.git.perl.org/perl.git/commit/8161153450000b2b806ca000eae18af7e430d3d9

(hugmeir also did some similar work in early June, but I think that was your original inspiration)

Also I removed unused dVAR (which I think under some cases are actually also dTHX)​:

http​://perl5.git.perl.org/perl.git/commit/23491f1decae17401c5c08e7f8a11e0d30f4f0e2
http​://perl5.git.perl.org/perl.git/commit/c97ab48957e7f73939e2a77e6312b44dc2408398

@p5pRT
Copy link
Author

p5pRT commented Jun 28, 2014

From [Unknown Contact. See original ticket]

So this patch isn't currently suitable for blead.

bulk88, if you take a look at the latest blead, I have been actually also working towards removing unused contexts (or making them conditional on whatever) (because I was turning on as many gcc -Wflags as I could easily). I haven't looked at your patch in detail, but I think you will find we covered many of the same spots.

See for example​:

http​://perl5.git.perl.org/perl.git/commit/23491f1decae17401c5c08e7f8a11e0d30f4f0e2
http​://perl5.git.perl.org/perl.git/commit/dd791c72c136e634557810ed390db9ed30f1576f
http​://perl5.git.perl.org/perl.git/commit/8161153450000b2b806ca000eae18af7e430d3d9

(hugmeir also did some similar work in early June, but I think that was your original inspiration)

Also I removed unused dVAR (which I think under some cases are actually also dTHX)​:

http​://perl5.git.perl.org/perl.git/commit/23491f1decae17401c5c08e7f8a11e0d30f4f0e2
http​://perl5.git.perl.org/perl.git/commit/c97ab48957e7f73939e2a77e6312b44dc2408398

@p5pRT
Copy link
Author

p5pRT commented Nov 11, 2017

From zefram@fysh.org

Poisoning my_perl in PERL_UNUSED_CONTEXT is a bad idae, and this patch
should be entirely rejected. The semantics of PERL_UNUSED_* are not
that the programmer guarantees that the subject is entirely unused; they
are that the subject *might* be unused, so should not be warned about.
It is quite common to apply a PERL_UNUSED_* macro to something that is
used in some builds depending on macro flags.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Nov 12, 2017

@cpansprout - Status changed from 'open' to 'rejected'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant