Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.9k stars 540 forks source link

t/re/fold_grind.t spews tons of "Attempt to free temp prematurely" warnings on DEBUGGING but ultimately passes #13363

Closed p5pRT closed 10 years ago

p5pRT commented 10 years ago

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

Searchable as RT120314$

p5pRT commented 10 years ago

From @bulk88

Created by @bulk88

I dont think I've ever seen this before\, but I almost never make DEBUGGING builds. I am also using a compiler I haven't used before\, VC6. I made a DEBUGGING build with blead today. Did a nmake test. The nmake test\, ultimately passed\, but at one point I noticed something strange in the console. t/re/fold_grind.t spews warnings about "Attempt to free temp prematurely" and "Attempt to free unreferenced scalar" when it runs but the TAP output passes. I've attached a manual run ("C​:\perl519\src\t>..\perl.exe -I..\lib re/fold_grind.t > re-fold_grind.t-free-temp-warnings.txt 2>&1") of the file. The interleaving of STDERR and STDOUT is *NOT* the way it is when printed to console. They are much more mixed in the console than the attached file. File is zipped since its 3 MB of repetitive text.

The bug is also seen on the George Greer smoker at http​://m-l.org/~perl/smoke/perl/win32/blead/log46879fadd034dcb5d6f720076845767f8ccf152d.log.gz

Perl Info ``` Flags: category=core severity=low Site configuration information for perl 5.19.6: Configured by Owner at Tue Oct 22 16:20:33 2013. Summary of my perl5 (revision 5 version 19 subversion 6) configuration: Derived from: 082f4888e1e059512e8a6f9abb3c230090974236 Ancestor: 363338367009f8156f4d49f6a24226eaf02db24c 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 useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef use64bitint=undef, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cl', ccflags ='-nologo -GF -W3 -O1 -MD -Zi -DDEBUGGING -DWIN32 -D_CONSOLE -DNO_STRICT -DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T', optimize='-O1 -MD -Zi -DDEBUGGING', cppflags='-DWIN32' ccversion='12.00.8168', 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 -libpath:"c:\perl519\lib\CORE" -machine:x86' libpth=C:\PROGRA~1\MIAF9D~1\VC98\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=perl519.lib gnulibc_version='' Dynamic Linking: dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' ' cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug -opt:ref,icf -libpath:"c:\perl519\lib\CORE" -machine:x86' Locally applied patches: uncommitted-changes 082f4888e1e059512e8a6f9abb3c230090974236 @INC for perl 5.19.6: ..\lib C:/perl519/src/lib . Environment for perl 5.19.6: HOME (unset) LANG (unset) LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=*removed* PERL_BADLANG (unset) PERL_JSON_BACKEND=JSON::XS PERL_YAML_BACKEND=YAML SHELL (unset) ```
p5pRT commented 10 years ago

From @bulk88

re-fold_grind.t-free-temp-warnings.zip

p5pRT commented 10 years ago

From @bulk88

On Tue Oct 22 15​:02​:49 2013\, bulk88 wrote​:

This is a bug report for perl from bulk88@​hotmail.com\, generated with the help of perlbug 1.39 running under perl 5.19.6.

----------------------------------------------------------------- [Please describe your issue here]

I dont think I've ever seen this before\, but I almost never make DEBUGGING builds. I am also using a compiler I haven't used before\, VC6.

This isn't VC6 specific. I also got the same results from fold_grind.t on VC 2008 DEBUGGING Perl.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @bulk88

Using PERL_DEBUG_FULL_TEST=1\, fold_grind.t became more verbose. I captured a 75 MB output of the test script\, which is attached.

The first test in the file\, I tried manually in a one liner\, and it failed. ________________________________________ C​:\perl519\src\t>..\perl.exe -I..\lib -e "my $c = \"\x{003A}\"; my $p = qr/(?l​:[ ^\x{003A}]?)/i; $c =~ $p;" Attempt to free temp prematurely​: SV 0x8fde0c\, Perl interpreter​: 0x366014 at -e line 1. Attempt to free unreferenced scalar​: SV 0x8fde0c\, Perl interpreter​: 0x366014.

C​:\perl519\src\t> ________________________________________

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @bulk88

re-fold_grind.t-free-temp-warnings.txt.bz2

p5pRT commented 10 years ago

From @bulk88

On Tue Oct 22 17​:07​:56 2013\, bulk88 wrote​:

Using PERL_DEBUG_FULL_TEST=1\, fold_grind.t became more verbose. I captured a 75 MB output of the test script\, which is attached.

The first test in the file\, I tried manually in a one liner\, and it failed. ________________________________________ C​:\perl519\src\t>..\perl.exe -I..\lib -e "my $c = \"\x{003A}\"; my $p = qr/(?l​:[ ^\x{003A}]?)/i; $c =~ $p;" Attempt to free temp prematurely​: SV 0x8fde0c\, Perl interpreter​: 0x366014 at -e line 1. Attempt to free unreferenced scalar​: SV 0x8fde0c\, Perl interpreter​: 0x366014.

C​:\perl519\src\t> ________________________________________

forward to ML

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @bulk88

a call stack\, this is an -O1 build\, so not all params are correct ______________________________   perl519.dll!Perl_sv_clear(interpreter * my_perl=0x003644a4\, sv * const orig_sv=0x008fead4) Line 6527 C   perl519.dll!Perl_sv_free2(interpreter * my_perl=0x003644a4\, sv * const sv=0x008fead4\, const unsigned long rc=1) Line 6714 C   perl519.dll!Perl_free_tmps(interpreter * my_perl=0x003644a4) Line 169 + 0x1a C

perl519.dll!S_parse_body(interpreter * my_perl=0x008f498c\, char * * env=0x00362918\, void (interpreter *)* xsinit=0x280ed19d) Line 2379 + 0xf C   perl519.dll!perl_parse(interpreter * my_perl=0x003644a4\, void (interpreter *)* xsinit=0x280ed19d\, int argc=4\, char * * argv=0x00362508\, char * * env=0x00362918) Line 1667 C   perl519.dll!RunPerl(int argc=4\, char * * argv=0x00362508\, char * * env=0x01362d00) Line 263 + 0x12 C++   perl.exe!main(int argc=4\, char * * argv=0x00362508\, char * * env=0x00362d00) Line 22 + 0x12 C   perl.exe!_mainCRTStartup() + 0xe3
  kernel32.dll!_BaseProcessStart@​4() + 0x23
______________________________________________________   if (PL_do_undump)   my_unexec();

  if (isWARN_ONCE) {   SAVECOPFILE(PL_curcop);   SAVECOPLINE(PL_curcop);   gv_check(PL_defstash);   }

  LEAVE;   FREETMPS; \<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\< Line 2379 S_parse_body

#ifdef MYMALLOC   {   const char *s;   if ((s=PerlEnv_getenv("PERL_DEBUG_MSTATS")) && atoi(s) >= 2)   dump_mstats("after compilation​:");   } #endif

  ENTER;   PL_restartjmpenv = NULL;   PL_restartop = 0;   return NULL; }

_________________________________________________________   if (PL_main_root) {   op_free(PL_main_root);   PL_main_root = NULL;   }   PL_main_start = NULL;   SvREFCNT_dec(PL_main_cv);   PL_main_cv = NULL;

  time(&PL_basetime);   oldscope = PL_scopestack_ix;   PL_dowarn = G_WARN_OFF;

  JMPENV_PUSH(ret);   switch (ret) {   case 0​:   parse_body(env\,xsinit); \<\<\<\<\<\<\<\<\<\<\<\<Line 1667 perl_parse   if (PL_unitcheckav) {   call_list(oldscope\, PL_unitcheckav);   }   if (PL_checkav) {   PERL_SET_PHASE(PERL_PHASE_CHECK);   call_list(oldscope\, PL_checkav);   }   ret = 0;   break;   case 1​:   STATUS_ALL_FAILURE;   /* FALL THROUGH */   case 2​:   /* my_exit() was called */   while (PL_scopestack_ix > oldscope)   LEAVE;   FREETMPS;   SET_CURSTASH(PL_defstash);   if (PL_unitcheckav) {   call_list(oldscope\, PL_unitcheckav);   }   if (PL_checkav) {   PERL_SET_PHASE(PERL_PHASE_CHECK);   call_list(oldscope\, PL_checkav);   }   ret = STATUS_EXIT;   break;   case 3​:   PerlIO_printf(Perl_error_log\, "panic​: top_env\n");   ret = 1;   break;   }   JMPENV_POP;   return ret; } _______________________________________________

A different call stack that tried to do a "Attempt to free temp prematurely​:"

_______________________________________________

perl519.dll!Perl_ck_warner_d(interpreter * my_perl=0x003644a4\, unsigned long err=22\, const char * pat=0x2811c970\, ...) Line 1726 C   perl519.dll!Perl_sv_free2(interpreter * my_perl=0x003644a4\, sv * const sv=0x008ff324\, const unsigned long rc=1) Line 6705 C

perl519.dll!Perl__invlist_intersection_maybe_complement_2nd(interpreter * my_perl=0x003644a4\, sv * const a=0x008ff324\, sv * const b=0x008ff064\, const char complement_b=''\, sv * * i=0x0012f2ac) Line 8231 + 0x18 C   perl519.dll!S_get_ANYOF_cp_list_for_ssc(interpreter * my_perl=0x008ff324\, const RExC_state_t * pRExC_state=0x0012f928\, const regnode_charclass_class * const node=0x009072a0) Line 993 + 0x15 C   perl519.dll!S_ssc_and(interpreter * my_perl=0x003644a4\, const RExC_state_t * pRExC_state=0x0012f928\, regnode_ssc * ssc=0x0012f644\, const regnode_ssc * and_with=0x009072a0) Line 1070 + 0xc C   perl519.dll!S_study_chunk(interpreter * my_perl=0x003644a4\, RExC_state_t * pRExC_state=0x0012f928\, regnode * * scanp=0x0012f74c\, int * minlenp=0x0012fa10\, int * deltap=0x0012f700\, regnode * last=0x009072d8\, scan_data_t * data=0x1500ffff\, long stopparen=-1\, unsigned char * recursed=0x00000000\, regnode_ssc * and_withp=0x00000000\, unsigned long flags=10240\, unsigned long depth=1) Line 4535 C   perl519.dll!S_study_chunk(interpreter * my_perl=0x003644a4\, RExC_state_t * pRExC_state=0x0012f928\, regnode * * scanp=0x0012fa50\, int * minlenp=0x0012fa10\, int * deltap=0x0012f8d0\, regnode * last=0x009072d8\, scan_data_t * data=0x00000000\, long stopparen=-1\, unsigned char * recursed=0x00000000\, regnode_ssc * and_withp=0x00000000\, unsigned long flags=11264\, unsigned long depth=0) Line 4156 + 0x2f C   perl519.dll!Perl_re_op_compile(interpreter * my_perl=0x003644a4\, sv * * const patternp=0x00000800\, int pat_count=781185092\, op * expr=0x0090727c\, const regexp_engine * eng=0x00907298\, p5rx * old_re=0x00000000\, char * is_bare_re=0x008ff2a4\, unsigned long orig_rx_flags=771751940\, unsigned long pm_flags=134217732) Line 6548 + 0x4e C   perl519.dll!Perl_pmruntime(interpreter * my_perl=0x003644a4\, op * o=0x00907101\, op * expr=0x00907100\, char isreg=''\, long floor=0) Line 4827 + 0x1d C   perl519.dll!Perl_yyparse(interpreter * my_perl=0x00905a4c\, int gramtype=381) Line 1400 C   perl519.dll!S_parse_body(interpreter * my_perl=0x008f498c\, char * * env=0x00362918\, void (interpreter *)* xsinit=0x280ed19d) Line 2354 + 0xd C   perl519.dll!perl_parse(interpreter * my_perl=0x003644a4\, void (interpreter *)* xsinit=0x280ed19d\, int argc=4\, char * * argv=0x00362508\, char * * env=0x00362918) Line 1667 C   perl519.dll!RunPerl(int argc=4\, char * * argv=0x00362508\, char * * env=0x01362d00) Line 263 + 0x12 C++   perl.exe!main(int argc=4\, char * * argv=0x00362508\, char * * env=0x00362d00) Line 22 + 0x12 C   perl.exe!_mainCRTStartup() + 0xe3
  kernel32.dll!_BaseProcessStart@​4() + 0x23
______________________________________________________

_________________________________________________________________

void Perl__invlist_intersection_maybe_complement_2nd(pTHX_ SV* const a\, SV* const b\, const bool complement_b\, SV** i) {   /* Take the intersection of two inversion lists and point \ to it. *i   * SHOULD BE DEFINED upon input\, and if it points to one of the two lists\,   * the reference count to that list will be decremented if not already a   * temporary (mortal); otherwise *i will be made correspondingly mortal.   * The first list\, \\, may be NULL\, in which case an empty list is   * returned. If \<complement_b> is TRUE\, the result will be the   * intersection of \ and the complement (or inversion) of \ instead of   * \ directly.   *   * The basis for this comes from "Unicode Demystified" Chapter 13 by   * Richard Gillam\, published by Addison-Wesley\, and explained at some   * length there. The preface says to incorporate its examples into your   * code at your own risk. In fact\, it had bugs   *   * The algorithm is like a merge sort\, and is essentially the same as the   * union above   */

  const UV* array_a; /* a's array */   const UV* array_b;   UV len_a; /* length of a's array */   UV len_b;

  SV* r; /* the resulting intersection */   UV* array_r;   UV len_r;

  UV i_a = 0; /* current index into a's array */   UV i_b = 0;   UV i_r = 0;

  /* running count\, as explained in the algorithm source book; items are   * stopped accumulating and are output when the count changes to/from 2.   * The count is incremented when we start a range that's in the set\, and   * decremented when we start a range that's not in the set. So its range   * is 0 to 2. Only when the count is 2 is something in the intersection.   */   UV count = 0;

  PERL_ARGS_ASSERT__INVLIST_INTERSECTION_MAYBE_COMPLEMENT_2ND;   assert(a != b);

  /* Special case if either one is empty */   len_a = (a == NULL) ? 0 : _invlist_len(a);   if ((len_a == 0) || ((len_b = _invlist_len(b)) == 0)) {   bool make_temp = FALSE;

  if (len_a != 0 && complement_b) {

  /* Here\, 'a' is not empty\, therefore from the above 'if'\, 'b' must   * be empty. Here\, also we are using 'b's complement\, which hence   * must be every possible code point. Thus the intersection is   * simply 'a'. */   if (*i != a) {   if (*i == b) {   if (! (make_temp = SvTEMP(b))) {   SvREFCNT_dec_NN(b);   }   }

  *i = invlist_clone(a);   }   /* else *i is already 'a' */

  if (make_temp) {   sv_2mortal(*i);   }   return;   }

  /* Here\, 'a' or 'b' is empty and not using the complement of 'b'. The   * intersection must be empty */   if (*i == a) {   if (! (make_temp = SvTEMP(a))) {   SvREFCNT_dec_NN(a);   }   }   else if (*i == b) {   if (! (make_temp = SvTEMP(b))) {   SvREFCNT_dec_NN(b); \<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\< Perl__invlist_intersection_maybe_complement_2nd Line 8231   }   }   *i = _new_invlist(0);   if (make_temp) {   sv_2mortal(*i);   }

  return;   }

_________________________________________________________________

STATIC SV* S_get_ANYOF_cp_list_for_ssc(pTHX_ const RExC_state_t *pRExC_state\,   const regnode_charclass_posixl* const node) {   /* Returns a mortal inversion list defining which code points are matched   * by 'node'\, which is of type ANYOF. Handles complementing the result if   * appropriate. If some code points aren't knowable at this time\, the   * returned list must\, and will\, contain every possible code point. */

  SV* invlist = sv_2mortal(_new_invlist(0));   unsigned int i;   const U32 n = ARG(node);

  PERL_ARGS_ASSERT_GET_ANYOF_CP_LIST_FOR_SSC;

  /* Look at the data structure created by S_set_ANYOF_arg() */   if (n != ANYOF_NONBITMAP_EMPTY) {   SV * const rv = MUTABLE_SV(RExC_rxi->data->data[n]);   AV * const av = MUTABLE_AV(SvRV(rv));   SV **const ary = AvARRAY(av);   assert(RExC_rxi->data->what[n] == 's');

  if (ary[1] && ary[1] != &PL_sv_undef) { /* Has compile-time swash */   invlist = sv_2mortal(invlist_clone(_get_swash_invlist(ary[1])));   }   else if (ary[0] && ary[0] != &PL_sv_undef) {

  /* Here\, no compile-time swash\, and there are things that won't be   * known until runtime -- we have to assume it could be anything */   return _add_range_to_invlist(invlist\, 0\, UV_MAX);   }   else {

  /* Here no compile-time swash\, and no run-time only data. Use the   * node's inversion list */   invlist = sv_2mortal(invlist_clone(ary[2]));   }   }

  /* An ANYOF node contains a bitmap for the first 256 code points\, and an   * inversion list for the others\, but if there are code points that should   * match only conditionally on the target string being UTF-8\, those are   * placed in the inversion list\, and not the bitmap. Since there are   * circumstances under which they could match\, they are included in the   * SSC. But if the ANYOF node is to be inverted\, we have to exclude them   * here\, so that when we invert below\, the end result actually does include   * them. (Think about "\xe0" =~ /[^\xc0]/di;). We have to do this here   * before we add the unconditionally matched code points */   if (ANYOF_FLAGS(node) & ANYOF_INVERT) {   _invlist_intersection_complement_2nd(invlist\,   PL_UpperLatin1\,   &invlist); \<\<\<\<\<\<\< S_get_ANYOF_cp_list_for_ssc Line 993   }

________________________________________________________________ STATIC void S_ssc_and(pTHX_ const RExC_state_t *pRExC_state\, regnode_ssc *ssc\,   const regnode_ssc *and_with) {   /* Accumulate into SSC 'ssc' its 'AND' with 'and_with'\, which is either   * another SSC or a regular ANYOF class. Can create false positives. */

  SV* anded_cp_list;   U8 anded_flags;

  PERL_ARGS_ASSERT_SSC_AND;

  assert(OP(ssc) == ANYOF_SYNTHETIC);

  /* 'and_with' is used as-is if it too is an SSC; otherwise have to extract   * the code point inversion list and just the relevant flags */   if (OP(and_with) == ANYOF_SYNTHETIC) {   anded_cp_list = and_with->invlist;   anded_flags = ANYOF_FLAGS(and_with);   }   else {   anded_cp_list = get_ANYOF_cp_list_for_ssc(pRExC_state\,   (regnode_charclass_posixl*) and_with); \<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<S_ssc_and Line 1070   anded_flags = ANYOF_FLAGS(and_with) & ANYOF_LOCALE_FLAGS;   } _________________________________________________________________

I think I'll try making a DEBUG_LEAKING_SCALARS Perl with -Od. IDK when I'll get around to it.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @bulk88

On Tue Oct 22 17​:39​:10 2013\, bulk88 wrote​:

I think I'll try making a DEBUG_LEAKING_SCALARS Perl with -Od. IDK when I'll get around to it.

_______________________________________________________________ C​:\perl519\src\t>..\perl.exe -I..\lib -e "my $c = \"\x{003A}\"; my $p = qr/(?l​:[ ^\x{003A}]?)/i; $c =~ $p;" Attempt to free temp prematurely​: SV 0x9099c4\, Perl interpreter​: 0x366014 at -e line 1. ALLOCATED at -e​:1 by (none) (parent 0x0); serial 428 SV = INVLIST(0x908838) at 0x9099c4   REFCNT = 0   FLAGS = ()   PV = 0x90ba4c   CUR = 0   LEN = 12 Attempt to free unreferenced scalar​: SV 0x9099c4\, Perl interpreter​: 0x366014.

C​:\perl519\src\t> _______________________________________________________________

The answer is plain as day in

http​://www.nntp.perl.org/group/perl.daily-build.reports/2013/10/msg152778.html

I won't provide a patch to protest discrimination against C89 :p

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @iabyn

On Tue\, Oct 22\, 2013 at 07​:46​:39PM -0700\, bulk88 via RT wrote​:

On Tue Oct 22 17​:39​:10 2013\, bulk88 wrote​:

I think I'll try making a DEBUG_LEAKING_SCALARS Perl with -Od. IDK when I'll get around to it.

_______________________________________________________________ C​:\perl519\src\t>..\perl.exe -I..\lib -e "my $c = \"\x{003A}\"; my $p = qr/(?l​:[ ^\x{003A}]?)/i; $c =~ $p;" Attempt to free temp prematurely​: SV 0x9099c4\, Perl interpreter​: 0x366014 at -e line 1. ALLOCATED at -e​:1 by (none) (parent 0x0); serial 428 SV = INVLIST(0x908838) at 0x9099c4 REFCNT = 0 FLAGS = () PV = 0x90ba4c CUR = 0 LEN = 12 Attempt to free unreferenced scalar​: SV 0x9099c4\, Perl interpreter​: 0x366014.

C​:\perl519\src\t> _______________________________________________________________

The answer is plain as day in

http​://www.nntp.perl.org/group/perl.daily-build.reports/2013/10/msg152778.html

I won't provide a patch to protest discrimination against C89 :p

It may be plain to you\, but not to me. Care to elaborate?

-- A walk of a thousand miles begins with a single step... then continues for another 1\,999\,999 or so.

p5pRT commented 10 years ago

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

p5pRT commented 10 years ago

From @bulk88

On Wed Oct 23 03​:35​:04 2013\, davem wrote​:

The answer is plain as day in

http​://www.nntp.perl.org/group/perl.daily- build.reports/2013/10/msg152778.html

I won't provide a patch to protest discrimination against C89 :p

It may be plain to you\, but not to me. Care to elaborate?

A lack of knowledge of C89\, since AFAIK\, most P5Pers use GCC in C++ mode here\, and if my C++ knowledge is correct\, they will never see the bug.

Commit http​://perl5.git.perl.org/perl.git/commit/db17973a7879fcb6ada1024d1c72e99a944746d1 is responsible. I'll leave it to khw to realize his mistake in that commit and fix all the warnings in regcomp.c in http​://www.nntp.perl.org/group/perl.daily-build.reports/2013/10/msg152778.html .

I am upset because the only reason I saw this is because I looked at my screen because my TV show went to commercials. The smoke report still "passes" even though its outputting severe warnings. The timing of commercials on the TV is not a reliable way to check for bugs in a make test.

IMO if it wasn't for a TV commercial this would have silently gone into 5.20. Anyone agree or disagree?

Maybe those SV leak warnings should be fatal on DEBUGGING builds\, not warnings?

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @cpansprout

On Wed Oct 23 07​:08​:38 2013\, bulk88 wrote​:

On Wed Oct 23 03​:35​:04 2013\, davem wrote​:

The answer is plain as day in

http​://www.nntp.perl.org/group/perl.daily- build.reports/2013/10/msg152778.html

I won't provide a patch to protest discrimination against C89 :p

It may be plain to you\, but not to me. Care to elaborate?

A lack of knowledge of C89\, since AFAIK\, most P5Pers use GCC in C++ mode here\, and if my C++ knowledge is correct\, they will never see the bug.

Commit

http​://perl5.git.perl.org/perl.git/commit/db17973a7879fcb6ada1024d1c72e99a944746d1 is responsible. I'll leave it to khw to realize his mistake in that commit and fix all the warnings in regcomp.c in http​://www.nntp.perl.org/group/perl.daily- build.reports/2013/10/msg152778.html .

I am upset because the only reason I saw this is because I looked at my screen because my TV show went to commercials. The smoke report still "passes" even though its outputting severe warnings. The timing of commercials on the TV is not a reliable way to check for bugs in a make test.

IMO if it wasn't for a TV commercial this would have silently gone into 5.20. Anyone agree or disagree?

Maybe those SV leak warnings should be fatal on DEBUGGING builds\, not warnings?

I use GCC without C++ mode\, and it seems to imply !! when casting to a bool. That used not to be the case. What I have now is version 4.2.1. I don’t remember what I used before\, or whether the GCC version has anything to do with it. Could stdbool.h have something to do with it?

(My complaint is that I used to catch this type of bug regularly\, and now I don’t\, and can’t easily.)

--

Father Chrysostomos

p5pRT commented 10 years ago

From @cpansprout

On Wed Oct 23 09​:25​:17 2013\, sprout wrote​:

On Wed Oct 23 07​:08​:38 2013\, bulk88 wrote​:

On Wed Oct 23 03​:35​:04 2013\, davem wrote​:

The answer is plain as day in

http​://www.nntp.perl.org/group/perl.daily- build.reports/2013/10/msg152778.html

I won't provide a patch to protest discrimination against C89 :p

It may be plain to you\, but not to me. Care to elaborate?

A lack of knowledge of C89\, since AFAIK\, most P5Pers use GCC in C++ mode here\, and if my C++ knowledge is correct\, they will never see the bug.

Commit

http​://perl5.git.perl.org/perl.git/commit/db17973a7879fcb6ada1024d1c72e99a944746d1

is responsible. I'll leave it to khw to realize his mistake in that commit and fix all the warnings in regcomp.c in http​://www.nntp.perl.org/group/perl.daily- build.reports/2013/10/msg152778.html .

I think it is a little unfair to say that that people are unaware of the issue. Within the last two months I made a similar mistake\, caught the mistake in my code before committing it\, but then accidentally reverted the correction before pushing to perl.git. It’s a really easy mistake to make\, even for those aware of it\, especially if the build hides it.

I am upset because the only reason I saw this is because I looked at my screen because my TV show went to commercials. The smoke report still "passes" even though its outputting severe warnings. The timing of commercials on the TV is not a reliable way to check for bugs in a make test.

IMO if it wasn't for a TV commercial this would have silently gone into 5.20. Anyone agree or disagree?

That’s a hard question. :-)

Maybe those SV leak warnings should be fatal on DEBUGGING builds\, not warnings?

If so\, we don’t want to suppress multiple warnings. Maybe croak at the end if any of those warnings have occurred?

I use GCC without C++ mode\, and it seems to imply !! when casting to a bool. That used not to be the case. What I have now is version 4.2.1. I don’t remember what I used before\, or whether the GCC version has anything to do with it. Could stdbool.h have something to do with it?

(My complaint is that I used to catch this type of bug regularly\, and now I don’t\, and can’t easily.)

It is stdbool.h doing it. On Darwin it is hard to avoid\, because perl.c includes mach-o/dyld.h​:

#ifdef USE_NSGETEXECUTABLEPATH # include \<mach-o/dyld.h> #endif

and dyld.h include stdbool.h.

If I disable I_STDBOOL in config.h and apply this (there are two stdbools.h on Mac OS X\, one for GCC and another for clang\, each using a different #define)​:

Inline Patch ```diff diff --git a/perl.c b/perl.c index e9cf22a..b9ae8d0 100644 --- a/perl.c +++ b/perl.c @@ -43,6 +43,8 @@ #endif #ifdef USE_NSGETEXECUTABLEPATH +# define _STDBOOL_H_ +# define _STDBOOL_H # include #endif ```

then I get this:

$ ./miniperl -Ilib -e 'my $c = "\x{003A}"; my $p = qr/(?l​:[^\x{003A}]?)/i; $c =~ $p;' Attempt to free temp prematurely​: SV 0x7fa4540316a0 at -e line 1. Attempt to free unreferenced scalar​: SV 0x7fa4540316a0.

(Defining _STDBOOL_H like that is inherently risky\, as the system’s header files may depend on that bool definition.)

I think defining bool as char for debugging builds is a good idea. But the order of the headers will have to change a bit (for Darwin)\, since perl.c #includes perl.h (which includes config.h and handy.h; config.h defines USE_NSGETEXECUTABLEPATH) and then uses USE_NSGETEXECUTABLEPATH to determine whether to #include mach-o/dyld.h.

I think the only way around it is something like this​:

Inline Patch ```diff diff --git a/handy.h b/handy.h index f80ba2c..3ae9302 100644 --- a/handy.h +++ b/handy.h @@ -69,7 +69,7 @@ Null SV pointer. (No longer available when ```

C is defined.) #define MUTABLE_IO(p) ((IO *)MUTABLE_PTR(p)) #define MUTABLE_SV(p) ((SV *)MUTABLE_PTR(p))

-#ifdef I_STDBOOL +#if defined(I_STDBOOL) && !defined(DEBUGGING) # include \<stdbool.h> # ifndef HAS_BOOL # define HAS_BOOL 1 @​@​ -85\,9 +85\,11 @​@​ Null SV pointer. (No longer available when C\<PERL_CORE> is defined.)   Andy Dougherty February 2000 */ #ifdef __GNUG__ /* GNU g++ has bool built-in */ +# ifndef DEBUGGING # ifndef HAS_BOOL # define HAS_BOOL 1 # endif +# endif #endif

/* The NeXT dynamic loader headers will not build with the bool macro @​@​ -104\,6 +106\,9 @​@​ Null SV pointer. (No longer available when C\<PERL_CORE> is defined.) #endif /* NeXT || __NeXT__ */

#ifndef HAS_BOOL +# ifdef bool +# undef bool +# endif # if defined(VMS) # define bool int # else

Inline Patch ```diff diff --git a/perl.c b/perl.c index e9cf22a..5ddad89 100644 --- a/perl.c +++ b/perl.c @@ -42,10 +42,6 @@ # include #endif -#ifdef USE_NSGETEXECUTABLEPATH -# include -#endif - #ifdef DEBUG_LEAKING_SCALARS_FORK_DUMP # ifdef I_SYSUIO # include diff --git a/perl.h b/perl.h index b40f141..1405019 100644 --- a/perl.h +++ b/perl.h @@ -2300,6 +2300,12 @@ typedef SV PADNAME; # define PERL_SAWAMPERSAND #endif +/* Include mach-o/dyld.h here for perl.c’s sake, since it may #define bool, + and handy.h needs to be able to re#define it. */ +#if defined(USE_NSGETEXECUTABLEPATH) && defined(PERL_IN_PERL_C) +# include +#endif + #include "handy.h" #if defined(USE_LARGE_FILES) && !defined(NO_64_BIT_RAWIO) ```

Is this patch a good idea? (I think it is. It also brings broken-bool goodness to C++\, but I have not tested that.)

--

Father Chrysostomos

p5pRT commented 10 years ago

From @cpansprout

On Wed Oct 23 12​:45​:13 2013\, sprout wrote​:

Is this patch a good idea? (I think it is. It also brings broken- bool goodness to C++\, but I have not tested that.)

And now I have tested it. I get this with g++\, too​:

$ ./miniperl -Ilib -e 'my $c = "\x{003A}"; my $p = qr/(?l​:[^\x{003A}]?)/i; $c =~ $p;' Attempt to free temp prematurely​: SV 0x7fe4dc0316a0 at -e line 1. Attempt to free unreferenced scalar​: SV 0x7fe4dc0316a0.

--

Father Chrysostomos

p5pRT commented 10 years ago

From @Leont

On Wed\, Oct 23\, 2013 at 4​:08 PM\, bulk88 via RT \perlbug\-followup@&#8203;perl\.orgwrote​:

A lack of knowledge of C89\, since AFAIK\, most P5Pers use GCC in C++ mode here\, and if my C++ knowledge is correct\, they will never see the bug.

I think the only platform that uses gcc in C++ mode is Strawberry Perl. It's usually more like C99 ;-).

Leon

p5pRT commented 10 years ago

From @craigberry

On Wed\, Oct 23\, 2013 at 4​:11 PM\, Leon Timmermans \fawaka@&#8203;gmail\.com wrote​:

On Wed\, Oct 23\, 2013 at 4​:08 PM\, bulk88 via RT \perlbug\-followup@&#8203;perl\.org wrote​:

A lack of knowledge of C89\, since AFAIK\, most P5Pers use GCC in C++ mode here\, and if my C++ knowledge is correct\, they will never see the bug.

I think the only platform that uses gcc in C++ mode is Strawberry Perl. It's usually more like C99 ;-).

But people do manual builds with g++ fairly regularly as a stricter\, trouble-seeking C. I think it's just that in the unusual case of bool\, C is where the trouble is.

p5pRT commented 10 years ago

From perl@profvince.com

But people do manual builds with g++ fairly regularly as a stricter\, trouble-seeking C. I think it's just that in the unusual case of bool\, C is where the trouble is.

I think you mean "stricter non-C". :)

The only legitimate use of a C++ compiler to build perl is to check
that the headers are both valid C and C++. As for mingw\, I believe
that only the linker needs to be g++.

Vincent

p5pRT commented 10 years ago

From @craigberry

On Thu\, Oct 24\, 2013 at 11​:32 AM\, Vincent Pit \perl@&#8203;profvince\.com wrote​:

But people do manual builds with g++ fairly regularly as a stricter\, trouble-seeking C. I think it's just that in the unusual case of bool\, C is where the trouble is.

I think you mean "stricter non-C". :)

When you throw C code at a C++ compiler and it finds real problems that a C compiler didn't catch\, then for all practical purposes it's a stricter C compiler.

The only legitimate use of a C++ compiler to build perl is to check that the headers are both valid C and C++.

In that case\, illegitimacy is more fun\, or at least of more practical benefit. See paragraph 4 of \<http​://www.nntp.perl.org/group/perl.perl5.porters/2011/07/msg174792.html>.

p5pRT commented 10 years ago

From @cpansprout

On Thu Oct 24 19​:55​:58 2013\, craig.a.berry@​gmail.com wrote​:

On Thu\, Oct 24\, 2013 at 11​:32 AM\, Vincent Pit \perl@&#8203;profvince\.com wrote​:

But people do manual builds with g++ fairly regularly as a stricter\, trouble-seeking C. I think it's just that in the unusual case of bool\, C is where the trouble is.

I think you mean "stricter non-C". :)

When you throw C code at a C++ compiler and it finds real problems that a C compiler didn't catch\, then for all practical purposes it's a stricter C compiler.

The only legitimate use of a C++ compiler to build perl is to check that the headers are both valid C and C++.

In that case\, illegitimacy is more fun\, or at least of more practical benefit. See paragraph 4 of \<http​://www.nntp.perl.org/group/perl.perl5.porters/2011/07/msg174792.html>.

OK\, but could someone comment on my patch that makes even C++ have a C-style bool under -DDEBUGGING? :-)

--

Father Chrysostomos

p5pRT commented 10 years ago

From @bulk88

On Thu Oct 24 21​:24​:40 2013\, sprout wrote​:

OK\, but could someone comment on my patch that makes even C++ have a C-style bool under -DDEBUGGING? :-)

I like the idea\, but I dont use any of the platforms you want to patch.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From PeterCMartini@GMail.com

On Fri\, Oct 25\, 2013 at 12​:24 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Thu Oct 24 19​:55​:58 2013\, craig.a.berry@​gmail.com wrote​:

On Thu\, Oct 24\, 2013 at 11​:32 AM\, Vincent Pit \perl@&#8203;profvince\.com wrote​:

But people do manual builds with g++ fairly regularly as a stricter\, trouble-seeking C. I think it's just that in the unusual case of bool\, C is where the trouble is.

I think you mean "stricter non-C". :)

When you throw C code at a C++ compiler and it finds real problems that a C compiler didn't catch\, then for all practical purposes it's a stricter C compiler.

The only legitimate use of a C++ compiler to build perl is to check that the headers are both valid C and C++.

In that case\, illegitimacy is more fun\, or at least of more practical benefit. See paragraph 4 of \<http​://www.nntp.perl.org/group/perl.perl5.porters/2011/07/msg174792.html>.

OK\, but could someone comment on my patch that makes even C++ have a C-style bool under -DDEBUGGING? :-)

A couple of comments​:

My understanding of the intent of stdbool.h is that its sole purpose is to define bool\, true\, and false properly. If we want to go this route\, where DEBUGGING forces the traditional behavior\, would it be clearer to have Configure unset I_STDBOOL if DEBUGGING was passed?

The header file stdbool.h was added in C99 for the sole purpose of defining bool\, true\, and false. It was done as a header file rather than adding new keywords\, so that any existing defines of those tokens didn't break\, and anything that wanted the new C99 behavior for those files could just include stdbool.h - anything that wanted to maintain C89 compatibility could opt out of those three keywords by not including the stdbool.h header file. Incidentally\, if we never included stdbool.h\, we'd also be missing out on 'true' and 'false'\, which would catch a similar class of error (that only a smoke on George's VC6 caught when I made it).

On another note\, can anyone provide context as to why our final fallback to 'define bool char'\, except for VMS? Couldn't we just change *that* to be int everywhere\, unless otherwise defined?

p5pRT commented 10 years ago

From @cpansprout

On Sat Oct 26 02​:11​:01 2013\, pcm wrote​:

On Fri\, Oct 25\, 2013 at 12​:24 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Thu Oct 24 19​:55​:58 2013\, craig.a.berry@​gmail.com wrote​:

On Thu\, Oct 24\, 2013 at 11​:32 AM\, Vincent Pit \perl@&#8203;profvince\.com wrote​:

But people do manual builds with g++ fairly regularly as a stricter\, trouble-seeking C. I think it's just that in the unusual case of bool\, C is where the trouble is.

I think you mean "stricter non-C". :)

When you throw C code at a C++ compiler and it finds real problems that a C compiler didn't catch\, then for all practical purposes it's a stricter C compiler.

The only legitimate use of a C++ compiler to build perl is to check that the headers are both valid C and C++.

In that case\, illegitimacy is more fun\, or at least of more practical benefit. See paragraph 4 of \<http​://www.nntp.perl.org/group/perl.perl5.porters/2011/07/msg174792.html>.

OK\, but could someone comment on my patch that makes even C++ have a C-style bool under -DDEBUGGING? :-)

A couple of comments​:

My understanding of the intent of stdbool.h is that its sole purpose is to define bool\, true\, and false properly. If we want to go this route\, where DEBUGGING forces the traditional behavior\, would it be clearer to have Configure unset I_STDBOOL if DEBUGGING was passed?

I suppose so\, but we would still have to shift defines around for Mac OS X\, as mach-o/dyld.h includes stdbool.h. (stdbool.h support was added for its sake; see \http&#8203;://www\.nntp\.perl\.org/group/perl\.perl5\.porters/;msgid=20110914202736\.GN23881@&#8203;plum\.flirble\.org).

The header file stdbool.h was added in C99 for the sole purpose of defining bool\, true\, and false. It was done as a header file rather than adding new keywords\, so that any existing defines of those tokens didn't break\, and anything that wanted the new C99 behavior for those files could just include stdbool.h - anything that wanted to maintain C89 compatibility could opt out of those three keywords by not including the stdbool.h header file. Incidentally\, if we never included stdbool.h\, we'd also be missing out on 'true' and 'false'\, which would catch a similar class of error (that only a smoke on George's VC6 caught when I made it).

On another note\, can anyone provide context as to why our final fallback to 'define bool char'\, except for VMS? Couldn't we just change *that* to be int everywhere\, unless otherwise defined?

That would increase memory usage on C89 compilers.

(Why does VMS need int?)

--

Father Chrysostomos

p5pRT commented 10 years ago

From PeterCMartini@GMail.com

On Sat\, Oct 26\, 2013 at 5​:27 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Sat Oct 26 02​:11​:01 2013\, pcm wrote​:

On Fri\, Oct 25\, 2013 at 12​:24 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Thu Oct 24 19​:55​:58 2013\, craig.a.berry@​gmail.com wrote​:

On Thu\, Oct 24\, 2013 at 11​:32 AM\, Vincent Pit \perl@&#8203;profvince\.com wrote​:

But people do manual builds with g++ fairly regularly as a stricter\, trouble-seeking C. I think it's just that in the unusual case of bool\, C is where the trouble is.

I think you mean "stricter non-C". :)

When you throw C code at a C++ compiler and it finds real problems that a C compiler didn't catch\, then for all practical purposes it's a stricter C compiler.

The only legitimate use of a C++ compiler to build perl is to check that the headers are both valid C and C++.

In that case\, illegitimacy is more fun\, or at least of more practical benefit. See paragraph 4 of \<http​://www.nntp.perl.org/group/perl.perl5.porters/2011/07/msg174792.html>.

OK\, but could someone comment on my patch that makes even C++ have a C-style bool under -DDEBUGGING? :-)

A couple of comments​:

My understanding of the intent of stdbool.h is that its sole purpose is to define bool\, true\, and false properly. If we want to go this route\, where DEBUGGING forces the traditional behavior\, would it be clearer to have Configure unset I_STDBOOL if DEBUGGING was passed?

I suppose so\, but we would still have to shift defines around for Mac OS X\, as mach-o/dyld.h includes stdbool.h. (stdbool.h support was added for its sake; see \http&#8203;://www\.nntp\.perl\.org/group/perl\.perl5\.porters/;msgid=20110914202736\.GN23881@&#8203;plum\.flirble\.org).

Ah\, thanks for that history\, that clears things up. That means to use the API needed for relocatable @​INC on Mac OS X\, we *require* C99 compatibility. The code that uses that is in perl.c\, but it's not hot code\, so it could potentially be moved to a separate .c file where we could limit the scope of the include to its own private file rather than a header that everything uses.

Does anyone know what the C99 standard says about _Bool? I'm having trouble finding it in Google\, and don't have a copy of the standard.

The header file stdbool.h was added in C99 for the sole purpose of defining bool\, true\, and false. It was done as a header file rather than adding new keywords\, so that any existing defines of those tokens didn't break\, and anything that wanted the new C99 behavior for those files could just include stdbool.h - anything that wanted to maintain C89 compatibility could opt out of those three keywords by not including the stdbool.h header file. Incidentally\, if we never included stdbool.h\, we'd also be missing out on 'true' and 'false'\, which would catch a similar class of error (that only a smoke on George's VC6 caught when I made it).

On another note\, can anyone provide context as to why our final fallback to 'define bool char'\, except for VMS? Couldn't we just change *that* to be int everywhere\, unless otherwise defined?

That would increase memory usage on C89 compilers.

bulk88​: could you change the define for bool from char to int in your setup? If it's as simple as that\, could you tell us what kind of effect it has on the size? I can't think of a better person for that kind of comparison.

(Why does VMS need int?)

--

Father Chrysostomos

--- via perlbug​: queue​: perl5 status​: open https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120314

p5pRT commented 10 years ago

From @mauke

On 26.10.2013 12​:16\, Peter Martini wrote​:

Does anyone know what the C99 standard says about _Bool? I'm having trouble finding it in Google\, and don't have a copy of the standard.

http​://www.open-std.org/jtc1/sc22/wg14/www/standards.html

"The latest publicly available version of the C99 standard is the combined C99 + TC1 + TC2 + TC3\, WG14 N1256\, dated 2007-09-07. This is a WG14 working paper\, but it reflects the consolidated standard at the time of issue." http​://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

-- Lukas Mai \plokinom@&#8203;gmail\.com

p5pRT commented 10 years ago

From PeterCMartini@GMail.com

On Sat\, Oct 26\, 2013 at 6​:28 AM\, Lukas Mai \plokinom@&#8203;gmail\.com wrote​:

On 26.10.2013 12​:16\, Peter Martini wrote​:

Does anyone know what the C99 standard says about _Bool? I'm having trouble finding it in Google\, and don't have a copy of the standard.

http​://www.open-std.org/jtc1/sc22/wg14/www/standards.html

"The latest publicly available version of the C99 standard is the combined C99 + TC1 + TC2 + TC3\, WG14 N1256\, dated 2007-09-07. This is a WG14 working paper\, but it reflects the consolidated standard at the time of issue." http​://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Thanks! So\, the standard essentially says _Bool is a byte (technically\, that it's rank is less than char)\, and that anywhere any scalar is converted to a bool\, a non-zero value is converted to true (1) instead of truncated\, like char would be. Which also implies that cBOOL is unnecessary when assigning to bools if stdbool.h is in use. I'm not suggesting changing anything though\, just thinking out loud :-)

p5pRT commented 10 years ago

From @craigberry

On Sat\, Oct 26\, 2013 at 4​:27 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Sat Oct 26 02​:11​:01 2013\, pcm wrote​:

On Fri\, Oct 25\, 2013 at 12​:24 AM\, Father Chrysostomos via RT

On another note\, can anyone provide context as to why our final fallback to 'define bool char'\, except for VMS? Couldn't we just change *that* to be int everywhere\, unless otherwise defined?

That would increase memory usage on C89 compilers.

(Why does VMS need int?)

It's been that way for 16 years​:

\<http​://perl5.git.perl.org/perl.git/blobdiff/39e571d41067215a80f26089b260f1418caeb36b..61bb59065bf1b12edab39b124e7373fb357e2d73​:/handy.h>

My memory (not from then but from a subsequent discussion) is that there was an ancient C++ compiler on VMS that used int for bool and we needed to make it compatible with that when compiling with C. But that reason is long since gone\, especially since support for building Perl with that C++ compiler was never completed and no recent (last decade or more) C++ compiler uses int for bool.

Note that we don't even hit the ifdef path in question since admitting to having stdbool.h a couple of years ago​:

\<http​://perl5.git.perl.org/perl.git/commit/f29eb9c41faae36e349122c83174fb3ebfb7b350?f=configure.com>

but we've actually had stdbool.h since C 6.4\, which came out in 2001.

It seems to me we are completely safe doing the following\, but I will do a test run or two before committing​:

Inline Patch ```diff --- handy.h;-0 2013-09-19 08:51:06 -0500 +++ handy.h 2013-10-26 09:58:14 -0500 @@ -104,11 +104,7 @@ Null SV pointer. (No longer available wh #endif /* NeXT || __NeXT__ */ #ifndef HAS_BOOL -# if defined(VMS) -# define bool int -# else -# define bool char -# endif +# define bool char # define HAS_BOOL 1 #endif [end] ```
p5pRT commented 10 years ago

From @bulk88

On Sat Oct 26 03​:17​:47 2013\, pcm wrote​:

On Sat\, Oct 26\, 2013 at 5​:27 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​: ...........

On another note\, can anyone provide context as to why our final fallback to 'define bool char'\, except for VMS? Couldn't we just change *that* to be int everywhere\, unless otherwise defined?

That would increase memory usage on C89 compilers.

bulk88​: could you change the define for bool from char to int in your setup? If it's as simple as that\, could you tell us what kind of effect it has on the size? I can't think of a better person for that kind of comparison.

TLDR​: need a couple days to compile a new perl and will give stats and a perlbench run at that time\, also some compilers could theoretically stuff multiple bools/chars into 1 int\, but VC 2003 doesn't

All opinion is from VC 2003. Also doing this somewhat from memory since my favorite asm book isn't next to me ATM. I've never seen Visual C 2003 put 2 bools/chars into a 4 byte C stack slot or 2 chars into a register although it is possible with registers EA*-ED* using AH/AL\, etc. If a bool isn't stored across a function call\, it will often be optimized to a EFLAGS check and a conditional jump and machine instructions will be reordered to not overwrite EFLAGS\, therefore a bool across function calls is what I will discuss. The problem is\, only one of the 4 registers that are byte addressable are non-volatile\, EBX. The 2 main non-vol registers on x86\, ESI and EDI\, are NOT splitable. EBP (stack frame or GP depending on compiler design)\, also isn't splitable. Not splitable means it can only be manipulated as one 32 bit value and the lower 16 bit value (upper bit manipulation requires |= and \<\<).

On x64 ALL registers can be addresses as lowest byte. Even RSI/RDI. But there is no new AH/BH/CH/DH style operands in x64. AH/BH/CH/DH still exist on x64. So it is technically possible to put 2 chars into a int. Making the "bool" be an "int" would likely prevent that optimization if it would happen. The other side of the story is\, it is possible VC's devels decided bitfields and *H registers and splitting of registers is a very bad idea due to data dependencies in the CPU\, so it won't paralelize during execution. *H registers could also be much slower if they are software emulated by the CPU. I once did tried to do a &= on 16 bit SP instead of copying ESP to EAX\, then a &= on 8 bit AL in hand written asm\, then copying EAX to ESP. The function was 2 instructions shorter but doubled in execution time. 16 bit SP is obviously software emulated on an Intel Core 2 the same way all of real mode (16 bit machine code) is microcode emulated by the CPU.

So the question is\, will the CPU split bitfields or AH/AL into separate phantom registers and performance improves (2 bools became 1 machine code register\, other random int is 2nd machine code register\, the whole thing becomes 3 internal registers and executes simultaneously) over using 3 ints (2 bools become 2 machine code registers\, 3rd int is other random int and goes on C stack\, executes as 2 internal registers and a L1 fetch simultanously\, other random int executed in next cycle after L1)\, or are AH and AL implemented in 2 to 3 32 bit operations in software and they are slower than 2 full size E*X regs?

The interp struct will probably skyrocket in size by making bool into an int since there are sequences of 4 and 8 bools in a row in the struct.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @bulk88

On Sat Oct 26 03​:17​:47 2013\, pcm wrote​:

bulk88​: could you change the define for bool from char to int in your setup? If it's as simple as that\, could you tell us what kind of effect it has on the size? I can't think of a better person for that kind of comparison.

char perl against itself


C​:\sources\perlbench-0.93>perl "C​:\sources\perlbench-0.93\perlbench-run" -c 1000 00 "C​:\perl519\boolischar\bin\perl.exe" "C​:\perl519\boolischar\bin\perl.exe" Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 75. A)   version = 5.019006   path = C​:\perl519\boolischar\bin\perl.exe

Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 75. B)   version = 5.019006   path = C​:\perl519\boolischar\bin\perl.exe

  A B   --- --- arith/mixed 100 101 arith/trig 100 100 array/copy 100 100 array/foreach 100 100 array/index 100 99 array/pop 100 95 array/shift 100 100 array/sort-num 100 100 array/sort 100 100 call/0arg 100 101 call/1arg 100 99 call/2arg 100 102 call/9arg 100 100 call/empty 100 100 call/fib 100 101 call/method 100 99 call/wantarray 100 101 hash/copy 100 98 hash/each 100 100 hash/foreach-sort 100 101 hash/foreach 100 100 hash/get 100 96 hash/set 100 101 loop/for-c 100 101 loop/for-range-const 100 101 loop/for-range 100 100 loop/getline 100 99 loop/while-my 100 100 loop/while 100 101 re/const 100 99 re/w 100 100 startup/fewmod 100 - startup/lotsofsub - - startup/noprog 100 107 string/base64 100 100 string/htmlparser 100 101 string/index-const 100 100 string/index-var 100 98 string/ipol 100 100 string/tr 100 101

AVERAGE 100 100Use of uninitialized value $str in substitut ion (s///) at C​:\sources\perlbench-0.93\perlbench-run line 375. Use of uninitialized value $str in substitution (s///) at C​:\sources\perlbench-0 .93\perlbench-run line 376. Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 287. Use of uninitialized value $str in substitution (s///) at C​:\sources\perlbench-0 .93\perlbench-run line 375. Use of uninitialized value $str in substitution (s///) at C​:\sources\perlbench-0 .93\perlbench-run line 376. Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 287.

Results saved in file​:///C|/sources/perlbench-0.93/benchres-004/index.html

C​:\sources\perlbench-0.93>


interp struct size


C​:\sources\perlbench-0.93>"C​:\perl519\boolisint\bin\perl.exe" -E "say UNIVERSAL​: :interpsize()" 2776

C​:\sources\perlbench-0.93>"C​:\perl519\boolischar\bin\perl.exe" -E "say UNIVERSAL :​:interpsize()" 2688


int vs char perl\, done with change in handy.h


C​:\sources\perlbench-0.93>perl "C​:\sources\perlbench-0.93\perlbench-run" -c 1000 00 "C​:\perl519\boolisint\bin\perl.exe" "C​:\perl519\boolischar\bin\perl.exe" Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 75. A)   version = 5.019006   path = C​:\perl519\boolisint\bin\perl.exe

Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 75. B)   version = 5.019006   path = C​:\perl519\boolischar\bin\perl.exe

  A B   --- --- arith/mixed 100 88 arith/trig 100 97 array/copy 100 101 array/foreach 100 99 array/index 100 105 array/pop 100 98 array/shift 100 98 array/sort-num 100 100 array/sort 100 100 call/0arg 100 110 call/1arg 100 107 call/2arg 100 96 call/9arg 100 111 call/empty 100 99 call/fib 100 101 call/method 100 104 call/wantarray 100 110 hash/copy 100 109 hash/each 100 104 hash/foreach-sort 100 99 hash/foreach 100 99 hash/get 100 104 hash/set 100 101 loop/for-c 100 108 loop/for-range-const 100 98 loop/for-range 100 100 loop/getline 100 100 loop/while-my 100 115 loop/while 100 117 re/const 100 103 re/w 100 101 startup/fewmod - - startup/lotsofsub - - startup/noprog 100 84 string/base64 100 101 string/htmlparser 100 102 string/index-const 100 106 string/index-var 100 82 string/ipol 100 102 string/tr 100 105

AVERAGE 100 102Use of uninitialized value $str in substitut ion (s///) at C​:\sources\perlbench-0.93\perlbench-run line 375. Use of uninitialized value $str in substitution (s///) at C​:\sources\perlbench-0 .93\perlbench-run line 376. Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 287. Use of uninitialized value $str in substitution (s///) at C​:\sources\perlbench-0 .93\perlbench-run line 375. Use of uninitialized value $str in substitution (s///) at C​:\sources\perlbench-0 .93\perlbench-run line 376. Use of uninitialized value in concatenation (.) or string at C​:\sources\perlbenc h-0.93\perlbench-run line 287.

Results saved in file​:///C|/sources/perlbench-0.93/benchres-003/index.html


bool is char perl519.dll .text (machine code) section of dll size\, 0xc1aff bool is int perl519.dll .text (machine code) section of dll size\, 0xc1fdf

Interesting results. Some were faster\, some were slower. IDK what to think.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @cpansprout

On Sun Oct 27 14​:07​:29 2013\, bulk88 wrote​:

int vs char perl\, done with change in handy.h ... Interesting results. Some were faster\, some were slower. IDK what to think.

Do you see any speed different when running

./miniperl -Ilib lib/unicore/mktables -C lib/unicore -P pod -maketest -makelist -q -w

?

--

Father Chrysostomos

p5pRT commented 10 years ago

From @bulk88

On Sun Oct 27 14​:47​:51 2013\, sprout wrote​:

On Sun Oct 27 14​:07​:29 2013\, bulk88 wrote​:

int vs char perl\, done with change in handy.h ... Interesting results. Some were faster\, some were slower. IDK what to think.

Do you see any speed different when running

./miniperl -Ilib lib/unicore/mktables -C lib/unicore -P pod -maketest -makelist -q -w

?

I don't have the miniperls anymore.

I did try this


C​:\perl519\src>perl runbool.pl Benchmark​: timing 4 iterations of char\, int...   char​: 120.076 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU)   (warning​: too few iterations for a reliable count)   int​: 121.722 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU)   (warning​: too few iterations for a reliable count)

C​:\perl519\src>


use Benchmark '​:all'\, '​:hireswallclock'; timethese(4\, {   'char' => sub {   system('"C​:\perl519\boolischar\bin\perl.exe" -Ilib lib/unicore/mktables -C lib/unicore -P pod -maketest -makelist -q -w');   }\,   'int' => sub {   system('"C​:\perl519\boolisint\bin\perl.exe" -Ilib lib/unicore/mktables -C lib/unicore -P pod -maketest -makelist -q -w');   }\,   });


-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From @craigberry

On Thu\, Oct 24\, 2013 at 11​:24 PM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

could someone comment on my patch that makes even C++ have a C-style bool under -DDEBUGGING? :-)

If I'm reading it right\, it seems to me that when DEBUGGING is defined you're redefining bool to char even if the compiler already defines it (either via stdbool.h for a C compiler or as a native type for a C++ compiler). Wouldn't that introduce a lot of bugs if you built an extension that used external libraries that depended on bool being a particular size that didn't happen to be the same size as a char?

p5pRT commented 10 years ago

From @bulk88

On Sat Oct 26 08​:25​:53 2013\, craig.a.berry@​gmail.com wrote​:

It's been that way for 16 years​:

\<http​://perl5.git.perl.org/perl.git/blobdiff/39e571d41067215a80f26089b260f1418caeb36b..61bb59065bf1b12edab39b124e7373fb357e2d73​:/handy.h>

My memory (not from then but from a subsequent discussion) is that there was an ancient C++ compiler on VMS that used int for bool and we needed to make it compatible with that when compiling with C. But that reason is long since gone\, especially since support for building Perl with that C++ compiler was never completed and no recent (last decade or more) C++ compiler uses int for bool.

Note that we don't even hit the ifdef path in question since admitting to having stdbool.h a couple of years ago​:

\<http​://perl5.git.perl.org/perl.git/commit/f29eb9c41faae36e349122c83174fb3ebfb7b350?f=configure.com>

but we've actually had stdbool.h since C 6.4\, which came out in 2001.

It seems to me we are completely safe doing the following\, but I will do a test run or two before committing​:

--- handy.h;-0 2013-09-19 08​:51​:06 -0500 +++ handy.h 2013-10-26 09​:58​:14 -0500 @​@​ -104\,11 +104\,7 @​@​ Null SV pointer. (No longer available wh #endif /* NeXT || __NeXT__ */

#ifndef HAS_BOOL -# if defined(VMS) -# define bool int -# else -# define bool char -# endif +# define bool char # define HAS_BOOL 1 #endif

[end]

http​://perl5.git.perl.org/perl.git/commitdiff/70d5cb32 It would have been better if this ticket number was somewhere in the commit message for historical reasons.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 10 years ago

From PeterCMartini@GMail.com

On Sat\, Oct 26\, 2013 at 5​:27 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

I suppose so\, but we would still have to shift defines around for Mac OS X\, as mach-o/dyld.h includes stdbool.h. (stdbool.h support was added for its sake; see \http&#8203;://www\.nntp\.perl\.org/group/perl\.perl5\.porters/;msgid=20110914202736\.GN23881@&#8203;plum\.flirble\.org).

Can we instead move S_set_caret_X from perl.c to\, say\, caretx.c\, and the dependency on mach-o/dyld.h and stdbool.h with it? That way we could turn off I_STDBOOL for testing builds without worrying about special casing Mac OS X.

I'd have a patch attached if I had a better idea for a name :-)

p5pRT commented 10 years ago

From @khwilliamson

On 10/24/2013 11​:03 PM\, Karl Williamson wrote​:

# Test process timed out - terminating

On 10/23/2013 08​:08 AM\, bulk88 via RT wrote​:

On Wed Oct 23 03​:35​:04 2013\, davem wrote​:

The answer is plain as day in

http​://www.nntp.perl.org/group/perl.daily- build.reports/2013/10/msg152778.html

I won't provide a patch to protest discrimination against C89 :p

It may be plain to you\, but not to me. Care to elaborate?

A lack of knowledge of C89\, since AFAIK\, most P5Pers use GCC in C++ mode here\, and if my C++ knowledge is correct\, they will never see the bug.

Commit http​://perl5.git.perl.org/perl.git/commit/db17973a7879fcb6ada1024d1c72e99a944746d1

is responsible. I'll leave it to khw to realize his mistake in that commit and fix all the warnings in regcomp.c in http​://www.nntp.perl.org/group/perl.daily-build.reports/2013/10/msg152778.html

.

I am upset because the only reason I saw this is because I looked at my screen because my TV show went to commercials. The smoke report still "passes" even though its outputting severe warnings. The timing of commercials on the TV is not a reliable way to check for bugs in a make test.

IMO if it wasn't for a TV commercial this would have silently gone into 5.20. Anyone agree or disagree?

Maybe those SV leak warnings should be fatal on DEBUGGING builds\, not warnings?

It should follow then that you wouldn't have been upset if you hadn't jumped to unwarranted conclusions.

First of all\, this is arguably not a bug at all since the correct answer is always computed and nothing bad happens. The warnings are saying that if Perl were less clever\, then freeing the same memory twice could happen (I didn't see any leak messages.) But Perl is clever enough not to do that. That's why it's OK for the warnings to only be in debugging builds.

Second\, I had already noticed that I had made a commit which was causing Win32 compiler warnings\, and so I had fixed them (and similar ones that I hadn't caused) locally. But I had not pushed this yet because I didn't realize that it was actually causing run-time warnings\, and I have had only a very minimum of time that I can devote to Perl due to rebuilding after a natural disaster in my area. Thus\, it turns out that your TV commercial did not affect the ultimate outcome in this case. When I found out that they were causing warnings\, I stayed up late one night and split the original patch into two\, one for just the warnings specified in this ticket; and the other for the similar warnings from elsewhere. After smoking on Win32\, it turns out that fixing these compiler warnings also fixed the run-time warnings. (Until then it really wasn't known if they were actually related.)

That's not to say that blind luck doesn't play a role in finding and averting problems. It just wasn't a factor in this case. I'm grateful whenever luck happens; sometimes it doesn't\, and code is shipped whose defectiveness only becomes apparent from field reports. Naturally\, a project should be managed to try to avoid (as much as possible) relying on luck. In this particular case\, I think the lesson is that we definitely want to minimize compiler warnings as much as possible before shipping a release. My mistake was to not notice those warnings before I made the original commit.

I don't submit a patch unless it passes all tests on my (Linux) machine under both C++\, and C with C89 strict enabled. However\, I don't usually do a debugging build for the latter. That catches many goofs I might otherwise make\, but it doesn't catch everything. I've been burned enough that for almost all non-trivial changes to C code\, I also wait for the output of the Win32 smoker.

p5pRT commented 10 years ago

From @khwilliamson

Fixed by commitd4ec33202c97c980bc66fc90ba442a70003ec5ff -- Karl Williamson

p5pRT commented 10 years ago

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

p5pRT commented 10 years ago

From @cpansprout

On Tue Oct 29 07​:21​:13 2013\, pcm wrote​:

On Sat\, Oct 26\, 2013 at 5​:27 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

I suppose so\, but we would still have to shift defines around for Mac OS X\, as mach-o/dyld.h includes stdbool.h. (stdbool.h support was added for its sake; see \http&#8203;://www\.nntp\.perl\.org/group/perl\.perl5\.porters/;msgid=20110914202736\.GN23881@&#8203;plum\.flirble\.org).

Can we instead move S_set_caret_X from perl.c to\, say\, caretx.c\, and the dependency on mach-o/dyld.h and stdbool.h with it? That way we could turn off I_STDBOOL for testing builds without worrying about special casing Mac OS X.

I'd have a patch attached if I had a better idea for a name :-)

I think allowing a way for even C++ compilers to use char for bool (which your proposal would not cover) is a good idea. (I know I’ll use it; I also suggest the smokers use it.)

When you sent your last message\, I had already written the attached patch. What do you think of this approach?

--

Father Chrysostomos

p5pRT commented 10 years ago

From @cpansprout

From f22112f54799d1c3d4c05a8cda5281185374eda2 Mon Sep 17 00​:00​:00 2001 From​: Father Chrysostomos \sprout@&#8203;cpan\.org Date​: Wed\, 30 Oct 2013 05​:37​:43 -0700 Subject​: [PATCH] Introduce PERL_BOOL_AS_CHAR define MIME-Version​: 1.0 Content-Type​: text/plain; charset=UTF-8 Content-Transfer-Encoding​: 8bit

This allows compilers that do support real booleans (C++ or anything with stdbool.h) to emulate those that don’t.

See ticket #120314.

Inline Patch ```diff diff --git a/handy.h b/handy.h index 131ede2..d345abe 100644 --- a/handy.h +++ b/handy.h @@ -69,7 +69,7 @@ Null SV pointer. (No longer available when C is defined.) #define MUTABLE_IO(p) ((IO *)MUTABLE_PTR(p)) #define MUTABLE_SV(p) ((SV *)MUTABLE_PTR(p)) -#ifdef I_STDBOOL +#if defined(I_STDBOOL) && !defined(PERL_BOOL_AS_CHAR) # include # ifndef HAS_BOOL # define HAS_BOOL 1 @@ -85,9 +85,11 @@ Null SV pointer. (No longer available when C is defined.) Andy Dougherty February 2000 */ #ifdef __GNUG__ /* GNU g++ has bool built-in */ +# ifndef PERL_BOOL_AS_CHAR # ifndef HAS_BOOL # define HAS_BOOL 1 # endif +# endif #endif /* The NeXT dynamic loader headers will not build with the bool macro @@ -104,6 +106,9 @@ Null SV pointer. (No longer available when C is defined.) #endif /* NeXT || __NeXT__ */ #ifndef HAS_BOOL +# ifdef bool +# undef bool +# endif # define bool char # define HAS_BOOL 1 #endif diff --git a/perl.c b/perl.c index 4c80edb..15adb8a 100644 --- a/perl.c +++ b/perl.c @@ -42,10 +42,6 @@ # include #endif -#ifdef USE_NSGETEXECUTABLEPATH -# include -#endif - #ifdef DEBUG_LEAKING_SCALARS_FORK_DUMP # ifdef I_SYSUIO # include diff --git a/perl.h b/perl.h index dc140bd..0a8beda 100644 --- a/perl.h +++ b/perl.h @@ -2298,6 +2298,13 @@ typedef SV PADNAME; # define PERL_SAWAMPERSAND #endif +/* Include mach-o/dyld.h here for perl.c’s sake, since it may #define bool, + and handy.h needs to be able to re#define it under + -Accflags=-DPERL_BOOL_AS_CHAR. */ +#if defined(USE_NSGETEXECUTABLEPATH) && defined(PERL_IN_PERL_C) +# include +#endif + #include "handy.h" #if defined(USE_LARGE_FILES) && !defined(NO_64_BIT_RAWIO) diff --git a/pod/perlhacktips.pod b/pod/perlhacktips.pod index 7851db9..1f55daf 100644 --- a/pod/perlhacktips.pod +++ b/pod/perlhacktips.pod @@ -1329,6 +1329,16 @@ op slabs belonging to C blocks. However, as an 80% solution it is still effective, as it has caught bugs in the past. +=head2 When is a bool not a bool? + +On some platforms, C is defined as equivalent to C. +Consequently assignment of any larger type to a C is unsafe and may +be truncated. The C macro exists to cast it correctly. + +On those platforms and compilers where C really is a boolean (C++, +F), it is easy to forget the cast. You can force C to be +a C by compiling with C<-Accflags=-DPERL_BOOL_AS_CHAR>. + =head2 The .i Targets You can expand the macros in a F file by saying ```
p5pRT commented 10 years ago

From @craigberry

On Wed\, Oct 30\, 2013 at 7​:41 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

When you sent your last message\, I had already written the attached patch. What do you think of this approach?

I like it; it's a clear\, surgical exception to what the compiler would do on its own.

+On some platforms\, C\ is defined as equivalent to C\.

s/some/pre-C99/

+Consequently assignment of any larger type to a C\ is unsafe and may +be truncated. The C\ macro exists to cast it correctly. + +On those platforms and compilers where C\ really is a boolean (C++\, +F\<stdbool.h>)\, it is easy to forget the cast. You can force C\ to be +a C\ by compiling with C\<-Accflags=-DPERL_BOOL_AS_CHAR>.

You may also wish to add C\<-Werror=conversion> or local equivalent to make it impossible to ignore the unsafe truncations.

p5pRT commented 10 years ago

From PeterCMartini@GMail.com

I think allowing a way for even C++ compilers to use char for bool (which your proposal would not cover) is a good idea. (I know I’ll use it; I also suggest the smokers use it.)

When you sent your last message\, I had already written the attached patch. What do you think of this approach?

Sorry\, I didnt mean to imply my approach was the whole solution :-). I was suggesting that as a part of your approach\, instead of moving the mach include to perl.c\, the set caret x function and the mach dependency be moved together. So\, instead of moving it from the header to perl.c\, it would be moved to its own c file\, so perl.c would still get whatever handy.h sets bool to.

(I hope thats clearer)

--Sent from my phone

p5pRT commented 10 years ago

From @cpansprout

On Wed Oct 30 07​:39​:36 2013\, craig.a.berry@​gmail.com wrote​:

On Wed\, Oct 30\, 2013 at 7​:41 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

When you sent your last message\, I had already written the attached patch. What do you think of this approach?

I like it; it's a clear\, surgical exception to what the compiler would do on its own.

+On some platforms\, C\ is defined as equivalent to C\.

s/some/pre-C99/

+Consequently assignment of any larger type to a C\ is unsafe and may +be truncated. The C\ macro exists to cast it correctly. + +On those platforms and compilers where C\ really is a boolean (C++\, +F\<stdbool.h>)\, it is easy to forget the cast. You can force C\ to be +a C\ by compiling with C\<-Accflags=-DPERL_BOOL_AS_CHAR>.

You may also wish to add C\<-Werror=conversion> or local equivalent to make it impossible to ignore the unsafe truncations.

Revised patch attached.

--

Father Chrysostomos

p5pRT commented 10 years ago

From @cpansprout

From 3a4b5b8e5f88d34ec4162bead9f201f564df32b3 Mon Sep 17 00​:00​:00 2001 From​: Father Chrysostomos \sprout@&#8203;cpan\.org Date​: Wed\, 30 Oct 2013 05​:37​:43 -0700 Subject​: [PATCH] Introduce PERL_BOOL_AS_CHAR define MIME-Version​: 1.0 Content-Type​: text/plain; charset=UTF-8 Content-Transfer-Encoding​: 8bit

This allows compilers that do support real booleans (C++ or anything with stdbool.h) to emulate those that don’t.

See ticket #120314.

Inline Patch ```diff diff --git a/handy.h b/handy.h index 131ede2..d345abe 100644 --- a/handy.h +++ b/handy.h @@ -69,7 +69,7 @@ Null SV pointer. (No longer available when C is defined.) #define MUTABLE_IO(p) ((IO *)MUTABLE_PTR(p)) #define MUTABLE_SV(p) ((SV *)MUTABLE_PTR(p)) -#ifdef I_STDBOOL +#if defined(I_STDBOOL) && !defined(PERL_BOOL_AS_CHAR) # include # ifndef HAS_BOOL # define HAS_BOOL 1 @@ -85,9 +85,11 @@ Null SV pointer. (No longer available when C is defined.) Andy Dougherty February 2000 */ #ifdef __GNUG__ /* GNU g++ has bool built-in */ +# ifndef PERL_BOOL_AS_CHAR # ifndef HAS_BOOL # define HAS_BOOL 1 # endif +# endif #endif /* The NeXT dynamic loader headers will not build with the bool macro @@ -104,6 +106,9 @@ Null SV pointer. (No longer available when C is defined.) #endif /* NeXT || __NeXT__ */ #ifndef HAS_BOOL +# ifdef bool +# undef bool +# endif # define bool char # define HAS_BOOL 1 #endif diff --git a/perl.c b/perl.c index 4c80edb..15adb8a 100644 --- a/perl.c +++ b/perl.c @@ -42,10 +42,6 @@ # include #endif -#ifdef USE_NSGETEXECUTABLEPATH -# include -#endif - #ifdef DEBUG_LEAKING_SCALARS_FORK_DUMP # ifdef I_SYSUIO # include diff --git a/perl.h b/perl.h index dc140bd..0a8beda 100644 --- a/perl.h +++ b/perl.h @@ -2298,6 +2298,13 @@ typedef SV PADNAME; # define PERL_SAWAMPERSAND #endif +/* Include mach-o/dyld.h here for perl.c’s sake, since it may #define bool, + and handy.h needs to be able to re#define it under + -Accflags=-DPERL_BOOL_AS_CHAR. */ +#if defined(USE_NSGETEXECUTABLEPATH) && defined(PERL_IN_PERL_C) +# include +#endif + #include "handy.h" #if defined(USE_LARGE_FILES) && !defined(NO_64_BIT_RAWIO) diff --git a/pod/perlhacktips.pod b/pod/perlhacktips.pod index 7851db9..1f55daf 100644 --- a/pod/perlhacktips.pod +++ b/pod/perlhacktips.pod @@ -1329,6 +1329,16 @@ op slabs belonging to C blocks. However, as an 80% solution it is still effective, as it has caught bugs in the past. +=head2 When is a bool not a bool? + +On some platforms, C is defined as equivalent to C. +Consequently assignment of any larger type to a C is unsafe and may +be truncated. The C macro exists to cast it correctly. + +On those platforms and compilers where C really is a boolean (C++, +F), it is easy to forget the cast. You can force C to be +a C by compiling with C<-Accflags=-DPERL_BOOL_AS_CHAR>. + =head2 The .i Targets You can expand the macros in a F file by saying ```
p5pRT commented 10 years ago

From @cpansprout

On Wed Oct 30 08​:02​:10 2013\, pcm wrote​:

I think allowing a way for even C++ compilers to use char for bool (which your proposal would not cover) is a good idea. (I know I’ll use it; I also suggest the smokers use it.)

When you sent your last message\, I had already written the attached patch. What do you think of this approach?

Sorry\, I didnt mean to imply my approach was the whole solution :-). I was suggesting that as a part of your approach\, instead of moving the mach include to perl.c\, the set caret x function and the mach dependency be moved together. So\, instead of moving it from the header to perl.c\, it would be moved to its own c file\, so perl.c would still get whatever handy.h sets bool to.

(I hope thats clearer)

I see. Care to write a patch against mine? :-)

I think the name caretx.c is fine. It would need a Tolkien quote.

Speaking of path names\, it has been a long time since I read LOTR\, but I seem to remember a discussion in which Faramir asked if Cirith Ungol was the name of the path Gollum spoke of. Could someone find that quote\, or maybe something mentioning the Paths of the Dead?

--

Father Chrysostomos

p5pRT commented 10 years ago

From @cpansprout

On Wed Oct 30 13​:27​:30 2013\, sprout wrote​:

On Wed Oct 30 07​:39​:36 2013\, craig.a.berry@​gmail.com wrote​:

On Wed\, Oct 30\, 2013 at 7​:41 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

When you sent your last message\, I had already written the attached patch. What do you think of this approach?

I like it; it's a clear\, surgical exception to what the compiler would do on its own.

+On some platforms\, C\ is defined as equivalent to C\.

s/some/pre-C99/

+Consequently assignment of any larger type to a C\ is unsafe and may +be truncated. The C\ macro exists to cast it correctly. + +On those platforms and compilers where C\ really is a boolean (C++\, +F\<stdbool.h>)\, it is easy to forget the cast. You can force C\ to be +a C\ by compiling with C\<-Accflags=-DPERL_BOOL_AS_CHAR>.

You may also wish to add C\<-Werror=conversion> or local equivalent to make it impossible to ignore the unsafe truncations.

Revised patch attached.

No\, that was the same patch. Revised patch *now* attached.

--

Father Chrysostomos

p5pRT commented 10 years ago

From @cpansprout

From b6d27fda1d958e036f3260b1fce6f69978572cfb Mon Sep 17 00​:00​:00 2001 From​: Father Chrysostomos \sprout@&#8203;cpan\.org Date​: Wed\, 30 Oct 2013 05​:37​:43 -0700 Subject​: [PATCH] Introduce PERL_BOOL_AS_CHAR define MIME-Version​: 1.0 Content-Type​: text/plain; charset=UTF-8 Content-Transfer-Encoding​: 8bit

This allows compilers that do support real booleans (C++ or anything with stdbool.h) to emulate those that don’t.

See ticket #120314.

Inline Patch ```diff diff --git a/handy.h b/handy.h index 131ede2..d345abe 100644 --- a/handy.h +++ b/handy.h @@ -69,7 +69,7 @@ Null SV pointer. (No longer available when C is defined.) #define MUTABLE_IO(p) ((IO *)MUTABLE_PTR(p)) #define MUTABLE_SV(p) ((SV *)MUTABLE_PTR(p)) -#ifdef I_STDBOOL +#if defined(I_STDBOOL) && !defined(PERL_BOOL_AS_CHAR) # include # ifndef HAS_BOOL # define HAS_BOOL 1 @@ -85,9 +85,11 @@ Null SV pointer. (No longer available when C is defined.) Andy Dougherty February 2000 */ #ifdef __GNUG__ /* GNU g++ has bool built-in */ +# ifndef PERL_BOOL_AS_CHAR # ifndef HAS_BOOL # define HAS_BOOL 1 # endif +# endif #endif /* The NeXT dynamic loader headers will not build with the bool macro @@ -104,6 +106,9 @@ Null SV pointer. (No longer available when C is defined.) #endif /* NeXT || __NeXT__ */ #ifndef HAS_BOOL +# ifdef bool +# undef bool +# endif # define bool char # define HAS_BOOL 1 #endif diff --git a/perl.c b/perl.c index 4c80edb..15adb8a 100644 --- a/perl.c +++ b/perl.c @@ -42,10 +42,6 @@ # include #endif -#ifdef USE_NSGETEXECUTABLEPATH -# include -#endif - #ifdef DEBUG_LEAKING_SCALARS_FORK_DUMP # ifdef I_SYSUIO # include diff --git a/perl.h b/perl.h index dc140bd..0a8beda 100644 --- a/perl.h +++ b/perl.h @@ -2298,6 +2298,13 @@ typedef SV PADNAME; # define PERL_SAWAMPERSAND #endif +/* Include mach-o/dyld.h here for perl.c’s sake, since it may #define bool, + and handy.h needs to be able to re#define it under + -Accflags=-DPERL_BOOL_AS_CHAR. */ +#if defined(USE_NSGETEXECUTABLEPATH) && defined(PERL_IN_PERL_C) +# include +#endif + #include "handy.h" #if defined(USE_LARGE_FILES) && !defined(NO_64_BIT_RAWIO) diff --git a/pod/perlhacktips.pod b/pod/perlhacktips.pod index 7851db9..32739ff 100644 --- a/pod/perlhacktips.pod +++ b/pod/perlhacktips.pod @@ -1329,6 +1329,18 @@ op slabs belonging to C blocks. However, as an 80% solution it is still effective, as it has caught bugs in the past. +=head2 When is a bool not a bool? + +On pre-C99 compilers, C is defined as equivalent to C. +Consequently assignment of any larger type to a C is unsafe and may +be truncated. The C macro exists to cast it correctly. + +On those platforms and compilers where C really is a boolean (C++, +C99), it is easy to forget the cast. You can force C to be a C +by compiling with C<-Accflags=-DPERL_BOOL_AS_CHAR>. You may also wish to +add C<-Accflags=-Werror=conversion> or your compiler's equivalent to make +it impossible to ignore the unsafe truncations. + =head2 The .i Targets You can expand the macros in a F file by saying ```
p5pRT commented 10 years ago

From @craigberry

On Wed\, Oct 30\, 2013 at 3​:31 PM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Wed Oct 30 13​:27​:30 2013\, sprout wrote​:

Revised patch attached.

No\, that was the same patch. Revised patch *now* attached.

Looks good to me.

p5pRT commented 10 years ago

From @cpansprout

On Wed Oct 30 19​:09​:06 2013\, craig.a.berry@​gmail.com wrote​:

On Wed\, Oct 30\, 2013 at 3​:31 PM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Wed Oct 30 13​:27​:30 2013\, sprout wrote​:

Revised patch attached.

No\, that was the same patch. Revised patch *now* attached.

Looks good to me.

Now in as f789f6a4bdb.

--

Father Chrysostomos

p5pRT commented 10 years ago

From @khwilliamson

On 10/30/2013 10​:36 PM\, Father Chrysostomos via RT wrote​:

On Wed Oct 30 19​:09​:06 2013\, craig.a.berry@​gmail.com wrote​:

On Wed\, Oct 30\, 2013 at 3​:31 PM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Wed Oct 30 13​:27​:30 2013\, sprout wrote​:

Revised patch attached.

No\, that was the same patch. Revised patch *now* attached.

Looks good to me.

Now in as f789f6a4bdb.

I don't think we should be recommending -Accflags=-Werror=conversion until blead can be compiled using it\, starting with Configure. Here's the output of make -k (after Configure)

p5pRT commented 10 years ago

From @khwilliamson

/bin/sh Makefile.SH Extracting Makefile (with variable substitutions) make depend MAKEDEPEND= make[1]​: Entering directory `/home/khw/perl/mktables' sh ./makedepend MAKE=make cflags make[2]​: Entering directory `/home/khw/perl/mktables' echo av.c scope.c op.c doop.c doio.c dump.c gv.c hv.c mg.c reentr.c mro.c perl.c perly.c pp.c pp_hot.c pp_ctl.c pp_sys.c regcomp.c regexec.c utf8.c sv.c taint.c toke.c util.c deb.c run.c universal.c pad.c globals.c keywords.c perlio.c perlapi.c numeric.c mathoms.c locale.c pp_pack.c pp_sort.c miniperlmain.c opmini.c perlmini.c | tr ' ' '\n' >.clist make[2]​: Leaving directory `/home/khw/perl/mktables' Finding dependencies for av.o. Finding dependencies for scope.o. Finding dependencies for op.o. Finding dependencies for doop.o. Finding dependencies for doio.o. Finding dependencies for dump.o. Finding dependencies for gv.o. Finding dependencies for hv.o. Finding dependencies for mg.o. Finding dependencies for reentr.o. Finding dependencies for mro.o. Finding dependencies for perl.o. Finding dependencies for perly.o. Finding dependencies for pp.o. Finding dependencies for pp_hot.o. Finding dependencies for pp_ctl.o. Finding dependencies for pp_sys.o. Finding dependencies for regcomp.o. Finding dependencies for regexec.o. Finding dependencies for utf8.o. Finding dependencies for sv.o. Finding dependencies for taint.o. Finding dependencies for toke.o. Finding dependencies for util.o. Finding dependencies for deb.o. Finding dependencies for run.o. Finding dependencies for universal.o. Finding dependencies for pad.o. Finding dependencies for globals.o. Finding dependencies for keywords.o. Finding dependencies for perlio.o. Finding dependencies for perlapi.o. Finding dependencies for numeric.o. Finding dependencies for mathoms.o. Finding dependencies for locale.o. Finding dependencies for pp_pack.o. Finding dependencies for pp_sort.o. Finding dependencies for miniperlmain.o. Finding dependencies for opmini.o. Finding dependencies for perlmini.o. Updating makefile... make[1]​: Leaving directory `/home/khw/perl/mktables' perl regen.pl /usr/bin/perl regen/mg_vtable.pl /usr/bin/perl regen/opcode.pl /usr/bin/perl regen/overload.pl /usr/bin/perl regen/reentr.pl /usr/bin/perl regen/regcomp.pl /usr/bin/perl regen/warnings.pl /usr/bin/perl regen/embed.pl /usr/bin/perl regen/feature.pl perl regen/uconfig_h.pl Extracting uconfig.h-new (with variable substitutions) `sh cflags "optimize='-ggdb3 -O0'" perlmini.o` -DPERL_IS_MINIPERL -DPERL_EXTERNAL_GLOB perlmini.c `sh cflags "optimize='-ggdb3 -O0'" opmini.o` -DPERL_IS_MINIPERL -DPERL_EXTERNAL_GLOB opmini.c `sh cflags "optimize='-ggdb3 -O0'" miniperlmain.o` miniperlmain.c   CCCMD = g++ -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE -DPERL_BOOL_AS_CHAR -Werror=conversion -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -ggdb3 -O0 -Wall -W -Wextra -Wendif-labels -Wwrite-strings -Wno-unused-variable -Wno-unused-parameter   CCCMD = g++ -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE -DPERL_BOOL_AS_CHAR -Werror=conversion -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -ggdb3 -O0 -Wall -W -Wextra -Wendif-labels -Wwrite-strings -Wno-unused-variable -Wno-unused-parameter   CCCMD = g++ -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE -DPERL_BOOL_AS_CHAR -Werror=conversion -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -ggdb3 -O0 -Wall -W -Wextra -Wendif-labels -Wwrite-strings -Wno-unused-variable -Wno-unused-parameter In file included from perl.h​:5068\,   from miniperlmain.c​:46​: inline.h​: In function ‘void S_append_utf8_from_native_byte(U8\, U8**)’​: inline.h​:264​: error​: conversion to ‘unsigned char’ from ‘int’ may alter its value inline.h​:265​: error​: conversion to ‘unsigned char’ from ‘int’ may alter its value In file included from perl.h​:5068\,   from miniperlmain.c​:46​: inline.h​: In function ‘char S_isIDFIRST_lazy(PerlInterpreter*\, const char*)’​: inline.h​:277​: error​: conversion to ‘char’ from ‘int’ may alter its value In file included from perl.h​:5068\,   from miniperlmain.c​:46​: inline.h​: In function ‘char S_isALNUM_lazy(PerlInterpreter*\, const char*)’​: inline.h​:285​: error​: conversion to ‘char’ from ‘int’ may alter its value make​: *** [miniperlmain.o] Error 1 make​: *** Waiting for unfinished jobs.... In file included from perl.h​:5068\,   from perl.c​:34​: inline.h​: In function ‘void S_append_utf8_from_native_byte(U8\, U8**)’​: inline.h​:264​: error​: conversion to ‘unsigned char’ from ‘int’ may alter its value inline.h​:265​: error​: conversion to ‘unsigned char’ from ‘int’ may alter its value In file included from perl.h​:5068\,   from perl.c​:34​: inline.h​: In function ‘char S_isIDFIRST_lazy(PerlInterpreter*\, const char*)’​: inline.h​:277​: error​: conversion to ‘char’ from ‘int’ may alter its value In file included from perl.h​:5068\,   from perl.c​:34​: inline.h​: In function ‘char S_isALNUM_lazy(PerlInterpreter*\, const char*)’​: inline.h​:285​: error​: conversion to ‘char’ from ‘int’ may alter its value perl.c​: In function ‘int perl_destruct(PerlInterpreter*)’​: perl.c​:542​: error​: conversion to ‘signed char’ from ‘int’ may alter its value perl.c​:546​: error​: conversion to ‘signed char’ from ‘int’ may alter its value perl.c​: In function ‘I32 Perl_call_sv(PerlInterpreter*\, SV*\, I32)’​: perl.c​:2774​: error​: conversion to ‘U8’ from ‘long int’ may alter its value perl.c​: In function ‘I32 Perl_eval_sv(PerlInterpreter*\, SV*\, I32)’​: perl.c​:2919​: error​: conversion to ‘U8’ from ‘long int’ may alter its value perl.c​: In function ‘void S_init_ids(PerlInterpreter*)’​: perl.c​:3949​: error​: conversion to ‘char’ from ‘int’ may alter its value perl.c​: In function ‘void Perl_my_exit(PerlInterpreter*\, U32)’​: perl.c​:4983​: warning​: format ‘%u’ expects type ‘unsigned int’\, but argument 3 has type ‘U32’ perl.c​:4984​: error​: conversion to ‘U8’ from ‘int’ may alter its value perl.c​: In function ‘void Perl_my_failure_exit(PerlInterpreter*)’​: perl.c​:5088​: warning​: format ‘%u’ expects type ‘unsigned int’\, but argument 3 has type ‘I32’ perl.c​:5089​: error​: conversion to ‘U8’ from ‘int’ may alter its value make​: *** [perlmini.o] Error 1 In file included from perl.h​:5068\,   from op.c​:104​: inline.h​: In function ‘void S_append_utf8_from_native_byte(U8\, U8**)’​: inline.h​:264​: error​: conversion to ‘unsigned char’ from ‘int’ may alter its value inline.h​:265​: error​: conversion to ‘unsigned char’ from ‘int’ may alter its value In file included from perl.h​:5068\,   from op.c​:104​: inline.h​: In function ‘char S_isIDFIRST_lazy(PerlInterpreter*\, const char*)’​: inline.h​:277​: error​: conversion to ‘char’ from ‘int’ may alter its value In file included from perl.h​:5068\,   from op.c​:104​: inline.h​: In function ‘char S_isALNUM_lazy(PerlInterpreter*\, const char*)’​: inline.h​:285​: error​: conversion to ‘char’ from ‘int’ may alter its value op.c​: In function ‘void S_no_bareword_allowed(PerlInterpreter*\, OP*)’​: op.c​:557​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘void Perl_op_clear(PerlInterpreter*\, OP*)’​: op.c​:765​: error​: conversion to ‘short unsigned int​:9’ from ‘Optype’ may alter its value op.c​: In function ‘OP* Perl_scalar(PerlInterpreter*\, OP*)’​: op.c​:1246​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_scalarvoid(PerlInterpreter*\, OP*)’​: op.c​:1349​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* S_scalarseq(PerlInterpreter*\, OP*)’​: op.c​:1751​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_op_lvalue_flags(PerlInterpreter*\, OP*\, I32\, U32)’​: op.c​:2050​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:2058​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:2322​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_doref(PerlInterpreter*\, OP*\, I32\, char)’​: op.c​:2444​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:2449​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:2468​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:2502​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_bind_match(PerlInterpreter*\, I32\, OP*\, OP*)’​: op.c​:2965​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* S_op_integerize(PerlInterpreter*\, OP*)’​: op.c​:3389​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* S_gen_constant_list(PerlInterpreter*\, OP*)’​: op.c​:3582​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_convert(PerlInterpreter*\, I32\, I32\, OP*)’​: op.c​:3608​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:3620​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​:3622​: error​: conversion to ‘U8’ from ‘long int’ may alter its value op.c​: In function ‘OP* Perl_op_append_list(PerlInterpreter*\, I32\, OP*\, OP*)’​: op.c​:3705​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_op_prepend_elem(PerlInterpreter*\, I32\, OP*\, OP*)’​: op.c​:3755​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_newLISTOP(PerlInterpreter*\, I32\, I32\, OP*\, OP*)’​: op.c​:4068​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* Perl_newOP(PerlInterpreter*\, I32\, I32)’​: op.c​:4122​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* Perl_newUNOP(PerlInterpreter*\, I32\, I32\, OP*)’​: op.c​:4174​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* Perl_newBINOP(PerlInterpreter*\, I32\, I32\, OP*\, OP*)’​: op.c​:4214​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* S_pmtrans(PerlInterpreter*\, OP*\, OP*\, OP*)’​: op.c​:4418​: error​: conversion to ‘U32’ from ‘UV’ may alter its value op.c​:4450​: error​: conversion to ‘U32’ from ‘long long unsigned int’ may alter its value op.c​: In function ‘OP* Perl_newPMOP(PerlInterpreter*\, I32\, I32)’​: op.c​:4614​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​:4634​: error​: conversion to ‘U32’ from ‘long long int’ may alter its value op.c​:4656​: error​: conversion to ‘PADOFFSET’ from ‘long long int’ may alter its value op.c​: In function ‘OP* Perl_pmruntime(PerlInterpreter*\, OP*\, OP*\, char\, I32)’​: op.c​:4956​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_newSVOP(PerlInterpreter*\, I32\, I32\, SV*)’​: op.c​:5063​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* Perl_newPADOP(PerlInterpreter*\, I32\, I32\, SV*)’​: op.c​:5105​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* Perl_newPVOP(PerlInterpreter*\, I32\, I32\, char*)’​: op.c​:5176​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* Perl_newASSIGNOP(PerlInterpreter*\, I32\, OP*\, I32\, OP*)’​: op.c​:5696​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:5814​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* S_new_logop(PerlInterpreter*\, I32\, I32\, OP**\, OP**)’​: op.c​:6182​: error​: conversion to ‘U16’ from ‘unsigned int’ may alter its value op.c​:6208​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘OP* Perl_newLOOPOP(PerlInterpreter*\, I32\, I32\, OP*\, OP*)’​: op.c​:6457​: error​: conversion to ‘U8’ from ‘long int’ may alter its value op.c​: In function ‘OP* Perl_newWHILEOP(PerlInterpreter*\, I32\, I32\, LOOP*\, OP*\, OP*\, OP*\, I32)’​: op.c​:6585​: error​: conversion to ‘U8’ from ‘long int’ may alter its value op.c​:6586​: error​: conversion to ‘U8’ from ‘long int’ may alter its value op.c​: In function ‘OP* Perl_newFOROP(PerlInterpreter*\, I32\, OP*\, OP*\, OP*\, OP*)’​: op.c​:6692​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* S_ref_array_or_hash(PerlInterpreter*\, OP*)’​: op.c​:6821​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* S_newGIVWHENOP(PerlInterpreter*\, OP*\, OP*\, I32\, I32\, PADOFFSET)’​: op.c​:6853​: error​: conversion to ‘short unsigned int​:9’ from ‘Optype’ may alter its value op.c​: In function ‘OP* Perl_ck_rvconst(PerlInterpreter*\, OP*)’​: op.c​:8758​: error​: conversion to ‘U8’ from ‘long unsigned int’ may alter its value op.c​:8760​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_ck_fun(PerlInterpreter*\, OP*)’​: op.c​:8951​: error​: conversion to ‘U8’ from ‘long int’ may alter its value op.c​:9172​: error​: conversion to ‘U8’ from ‘long int’ may alter its value op.c​:9197​: error​: conversion to ‘U8’ from ‘long int’ may alter its value op.c​: In function ‘OP* Perl_ck_glob(PerlInterpreter*\, OP*)’​: op.c​:9270​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_ck_grep(PerlInterpreter*\, OP*)’​: op.c​:9306​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:9322​: error​: conversion to ‘short unsigned int​:9’ from ‘const U16’ may alter its value op.c​: In function ‘OP* Perl_ck_open(PerlInterpreter*\, OP*)’​: op.c​:9697​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_ck_select(PerlInterpreter*\, OP*)’​: op.c​:9825​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘void S_simplify_sort(PerlInterpreter*\, OP*)’​: op.c​:10005​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_ck_entersub_args_proto(PerlInterpreter*\, OP*\, GV*\, SV*)’​: op.c​:10350​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘void Perl_cv_set_call_checker(PerlInterpreter*\, CV*\, OP* (*)(PerlInterpreter*\, OP*\, GV*\, SV*)\, SV*)’​: op.c​:10738​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_ck_subr(PerlInterpreter*\, OP*)’​: op.c​:10767​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:10769​: error​: conversion to ‘U8’ from ‘long unsigned int’ may alter its value op.c​:10773​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:10777​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:10781​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_ck_trunc(PerlInterpreter*\, OP*)’​: op.c​:10848​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​: In function ‘OP* Perl_ck_each(PerlInterpreter*\, OP*)’​: op.c​:10905​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​:10916​: error​: conversion to ‘short unsigned int​:9’ from ‘U16’ may alter its value op.c​: In function ‘void Perl_rpeep(PerlInterpreter*\, OP*)’​: op.c​:11327​: warning​: comparison is always false due to limited range of data type op.c​:11385​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:11438​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:11463​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:11482​: error​: conversion to ‘U8’ from ‘int’ may alter its value op.c​:11784​: error​: conversion to ‘U8’ from ‘int’ may alter its value make​: *** [opmini.o] Error 1

p5pRT commented 10 years ago

From @craigberry

On Thu\, Oct 31\, 2013 at 11​:27 AM\, Karl Williamson \public@&#8203;khwilliamson\.com wrote​:

On 10/30/2013 10​:36 PM\, Father Chrysostomos via RT wrote​:

On Wed Oct 30 19​:09​:06 2013\, craig.a.berry@​gmail.com wrote​:

On Wed\, Oct 30\, 2013 at 3​:31 PM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

On Wed Oct 30 13​:27​:30 2013\, sprout wrote​:

Revised patch attached.

No\, that was the same patch. Revised patch *now* attached.

Looks good to me.

Now in as f789f6a4bdb.

I don't think we should be recommending -Accflags=-Werror=conversion until blead can be compiled using it\, starting with Configure. Here's the output of make -k (after Configure)

Sigh. I should've known the tiny test program I used that beautifully displayed the shortening problem didn't represent the real world. Even -Wconversion (without escalating the warning to an error) includes a huge pile of different\, unrelated potential problems when thrown at the Perl sources. The closest we could come might be​:

-Accflags='-Wconversion -Wno-sign-conversion -Wno-shorten-64-to-32'

which is kind of a mouthful and still generates so many warnings with existing code that it would be hard to see any new ones\, even if people could develop the habit of reviewing and acting on warnings\, which experience shows usually won't happen.

So I agree with you\, but I don't know what else to recommend.

Side note​: -Wshorten-64-to-32 might be useful when chasing the I32 bug on a platform with -Duse64bitint enabled.