New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S_croak_memory_wrap breaks gcc warning flags detection #12824
Comments
From @doughera88Created by @doughera88This commit: commit f019c49 optimize memory wrap croaks, often used in MEM_WRAP_CHECK breaks the detection of gcc warning flags by cflags.SH to break. /tmp/cc2FFrOt.o: In function `S_croak_memory_wrap': Prior to this patch, the typical command line used with gcc is something CCCMD = cc -DPERL_CORE -c -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi -W -Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat -Wwrite-strings Now, it is something like CCCMD = cc -DPERL_CORE -c -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -Wall Note that all the extra warnings are missing. The new function S_croak_memory_wrap in inline.h is the culprit. Perl Info
|
From @nwc10On Thu, Feb 28, 2013 at 05:41:14AM -0800, Andy Dougherty wrote:
PL_memory_wrap isn't used on CPAN. At all. I think the solution is to replace use of PL_memory_wrap with a string (And also mark the variable as deprecated, but that doesn't have to be done Nicholas Clark |
The RT System itself - Status changed from 'new' to 'open' |
From @doughera88On Thu, 28 Feb 2013, Nicholas Clark via RT wrote:
That still leaves the undefined reference to Perl_croak. The _cflags.c test is being slightly naughty here. It's relying on the The S_croak_memory_wrap function is also being slightly naughty here. I did try simply reverting the patch, but it no longer reverts cleanly. I'm thinking of trying something like this: It patches the cflags.SH to Inline Patchdiff --git a/cflags.SH b/cflags.SH
index 899c465..9efd503 100755
--- a/cflags.SH
+++ b/cflags.SH
@@ -43,6 +43,9 @@ esac
cat >_cflags.c <<__EOT__
#include "EXTERN.h"
+/* Avoid perl's inline.h file, which needs to link to other files not
+ yet compiled. */
+#define PERL_INLINE_H 1
#include "perl.h"
/* The stdio.h, errno.h, and setjmp.h should be there in any ANSI C89. */
#include <stdio.h>
diff --git a/inline.h b/inline.h
index 6b5f93c..697f197 100644
--- a/inline.h
+++ b/inline.h
@@ -12,6 +12,9 @@
* Each section names the header file that the functions "belong" to.
*/
+#ifndef PERL_INLINE_H
+#define PERL_INLINE_H 1
+
/* ------------------------------- av.h ------------------------------- */
PERL_STATIC_INLINE I32
@@ -141,6 +144,7 @@ S_sv_or_pv_pos_u2b(pTHX_ SV *sv, const char *pv, STRLEN pos, STRLEN *lenp)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunused-function"
#endif
+/* Deliberately not inline. See f019c49e. */
static void
S_croak_memory_wrap(void)
{
@@ -170,3 +174,4 @@ S_isALNUM_lazy(pTHX_ const char* p)
return isALNUM_lazy_if(p,1);
}
+#endif /* PERL_INLINE_H */
-- Andy Dougherty doughera@lafayette.edu |
From @nwc10On Thu, Feb 28, 2013 at 10:03:02AM -0500, Andy Dougherty wrote:
I cheated. See below :-)
clang is smart enough that it doesn't pull it in. This was what I had from an earlier experiment: commit 8694dae014089be4f0c40c96514423aca1bbe2f9 WIP on blead: b4fbebd Data::Dumper is at 2.139 diff --cc cflags.SH I hadn't cleaned it up enough to be confident of testing it. In particular, I was trying also to add a self-check, so that with Nicholas Clark |
From @bulk88On Thu Feb 28 05:51:58 2013, nicholas wrote:
PL_memory_wrap is exported, darkpan code could be using it. More I think someone else once said for maintenance reason you can not
What about the Perl_croak func? Why doesn't the linker remove unused functions? I am not very familiar Why does cflags.SH include perl's headers without the linking lib? One Another choice is to make a macro that does or S_croak_memory_wrap depending on platform. Looking at my VC 2008 x64 blead perl just now, //from an VC x64 XS module FWIW, ppport.h and MS's Win32 API headers both define statics in .h files. *low level compiler design ahead, skip if you want, VC with -- |
From @doughera88On Thu, 28 Feb 2013, bulk88 via RT wrote:
Empirically, the linker does remove unused functions if a sufficiently
Because there is no "linking lib" yet. cflags.SH runs just after
I posted a patch that did this, but it has the drawback that the test
I didn't think of that, but it feels like it's trying to solve the One of the underlying issues is that S_croak_memory_wrap is an odd duck. The other underlying issue is that cflags.SH is doing some strange things -- |
From @ilmariCreated by @ilmariperl.h includes inline.h, which has inline functions referencing real ilmari@nurket:~/src/Time-HiRes-1.9725$ VERBOSE=1 perl Makefile.PL Perl Info
|
From @ilmariDagfinn Ilmari Mannsåker (via RT) <perlbug-followup@perl.org> writes:
Btw, Nicholas says this should block 5.18: <@Nicholas> ilmari: this is a bug we need to fix -- |
From @bulk88On Sun Mar 24 06:47:06 2013, ilmari wrote:
This sounds related to |
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Thu Feb 28 08:54:28 2013, doughera wrote:
Maybe -funit-at-a-time is what does it (remove unused statics). -- |
From @dmcbrideCreated by @dmcbrideThis seems to be a regression: $ perl5.16.3 -MTime::HiRes -le 'print Time::HiRes::clock_gettime (Time::HiRes::CLOCK_MONOTONIC ())' This is, I think, the root cause of the failure: http://www.cpantesters.org/cpan/report/fb45c356-93f5-11e2-8b69-0a4fc51cef80 Perl Info
|
From @ilmariDarin McBride (via RT) <perlbug-followup@perl.org> writes:
This is caused by the same header problem as: https://rt-archive.perl.org/perl5/Public/Bug/Display.html?id=117319 -- |
The RT System itself - Status changed from 'new' to 'open' |
From @nwc10On Mon, Mar 25, 2013 at 09:36:05AM +0100, Dagfinn Ilmari Mannsåker wrote:
I have merged all these tickets. I haven't yet been hit with inspiration on how to solve the problem whilst Nicholas Clark |
From @tonycozOn Mon Mar 25 02:12:34 2013, nicholas wrote:
Making croak_memory_map() into an exported API function would retain Tony |
From @doughera88On Tue, 26 Mar 2013, Tony Cook via RT wrote:
This is the tact I tried in commit f8fd8bfff8eb57156a0ab858d4c185701258b0e3 I'd welcome feedback. I'm no longer sure it ends up saving anything -- |
From @bulk88On Wed Mar 27 13:11:35 2013, doughera wrote:
Summary, on Win32, new way (8 bytes) is slightly worse than before (5 or On Win32 (not discussing any other OS), Perl_croak_memory_wrap is still As a static non-inline previously, *ideally* (C compiler dependent) it Visual C 2003 in 32 bit mode I remember did In theory, a C compiler could do (VC will do this in debug mode to ________________________________________________ But I guess there is a performance problem with doing that so it just -- |
From @jkeenanOn 3/25/13 1:26 AM, Darin McBride wrote:
Well, FWIW, it's not implemented on this older Darwin, either: ##### Platform: $ perl -MTime::HiRes -le 'print Time::HiRes::clock_gettime |
From @jkeenanOn 3/27/13 4:10 PM, Andy Dougherty wrote:
PASS all tests on Darwin/PPC (but see other post in thread). |
From @bulk88On Wed Mar 27 18:00:17 2013, bulk88 wrote:
Forgot to mention, the above numbers assume croak_memory_wrap() is a XS -- |
From @doughera88On Wed, 27 Mar 2013, Andy Dougherty wrote:
I've now applied this fix in commit 43387ee Remove the non-inline function S_croak_memory_wrap from inline.h. |
From @bulk88On Wed Mar 27 18:00:17 2013, bulk88 wrote:
As TonyC mentioned on IRC, the above is 6 bytes, not 5. My mistake. -- |
From @bulk88On Wed Mar 27 13:11:35 2013, doughera wrote:>
I know you already merged it, but what is SHA |
From @doughera88On Fri, 29 Mar 2013, bulk88 via RT wrote:
I'm sorry, but I have no idea what you mean here. -- |
From @bulk88On Fri Mar 29 06:53:21 2013, doughera wrote:
http://perl5.git.perl.org/perl.git/commit/f019c49e380f764c1ead36fe3602184804292711?f=inline.h
Goto Another topic, what is the criteria for closing this ticket? A patch was -- |
From @doughera88On Fri, 29 Mar 2013, bulk88 via RT wrote:
Ideally, I would like to have a test such that we can detect this sort of Alas, I have no time to create such a test right now. -- |
From @tonycozOn Wed Apr 03 04:25:21 2013, doughera wrote:
I've written a test for this issue, currently pushed to It's based on the Time-HiRes build code rather than cflags.SH, but does fail Tested locally on Win32, cygwin, Linux. Tony |
From @doughera88On Sat, 13 Apr 2013, Tony Cook via RT wrote:
Thank you for writing this test. Unfortunately, it fails on Solaris 11 porting/extrefs.t .. The test is fine, and is testing what I wanted to test. It's just that I didn't have a chance to check the smoke logs to see if any other (So others don't have to go digging: The test simply tries to compile the #include "EXTERN.h" int main(int argc, char **argv) { without linking to libperl.a.) I'm not sure what the best thing to do here is. This is definitely a I don't see a simple fix. Adding $Config{optimize} to the compile command I had originally hoped that adding static inline functions would allow us -- |
From @tonycozOn Wed, Apr 17, 2013 at 09:17:17AM -0400, Andy Dougherty wrote:
It failed on my Solaris smoke tester too: http://www.nntp.perl.org/group/perl.daily-build.reports/2013/04/msg140448.html which uses the Solaris compiler. This also means that the base bug isn't actually fixed - while
I wonder if it's realistic to use perl's header files without using This is still a regression though. Perhaps the inline.h header could be reverted for 5.18 with a notice Tony |
From @doughera88On Wed, 17 Apr 2013, Tony Cook via RT wrote:
I suspect that would be a big change too. I haven't had time to think it The following untested workaround patch might at least let us kick the can Inline Patchdiff --git a/cpan/Time-HiRes/Makefile.PL b/cpan/Time-HiRes/Makefile.PL
index 6f6a790..a0f9a01 100644
--- a/cpan/Time-HiRes/Makefile.PL
+++ b/cpan/Time-HiRes/Makefile.PL
@@ -115,7 +115,7 @@ __EOD__
}
}
- my $ccflags = $Config{'ccflags'} . ' ' . "-I$COREincdir";
+ my $ccflags = $Config{'ccflags'} . ' ' . $Config{'optimize'} . ' ' . "-I$COREincdir";
if ($^O eq 'VMS') {
$cccmd = "$Config{'cc'} /include=($COREincdir) $tmp.c";
diff --git a/t/porting/extrefs.t b/t/porting/extrefs.t
index 42045c2..a990639 100644
--- a/t/porting/extrefs.t
+++ b/t/porting/extrefs.t
@@ -66,7 +66,7 @@ sub try_compile_and_link {
my $COREincdir = File::Spec->catdir(File::Spec->updir);
- my $ccflags = $Config{'ccflags'} . ' ' . "-I$COREincdir";
+ my $ccflags = $Config{'ccflags'} . ' ' . $Config{'optimize'} . ' ' . "-I$COREincdir";
if ($^O eq "MSWin32") {
$ccflags .= " -I../win32 -I../win32/include";
-- Andy Dougherty doughera@lafayette.edu |
From @nwc10On Wed, Apr 24, 2013 at 05:45:14PM -0400, Andy Dougherty wrote:
Yes: ~/Perl/perl/Porting/bisect.pl --target perl --start v5.16.0 -- sh -c './miniperl -Ilib make_ext.pl MAKE=make Time::HiRes; grep TIME_HIRES_CLOCK cpan/Time-HiRes/xdefine' ... HEAD is now at 75e16a4 Use fast SvREFCNT_dec for non-GCC Use fast SvREFCNT_dec for non-GCC :100644 100644 cdcaf04a8f35a302f18c85b8a7c81da35794ab15 798b493e216e456864c0c69fcefbfae83f879b4f M inline.h That's the first commit that adds a static inline function definition which
There seem to be at least 3 use cases. 0) cflags.SH probing to work out what flags are valid. Note that for "probing", you actually have the perl library somewhere.
I fear that it's a big change.
This, however, doesn't work if your build doesn't have the optimiser enabled. The upshot is that even with this patch, you end up with different runtime I can't see a way to have everything. ie 1) static inline functions to replace macros combined with 2) defaulting to use them 3) not screwing up the "traditional" use without linking I think that the least worst option is to keep (1) and (2), and force (3) Hence, the attached patch, which adds PERL_NO_INLINE_FUNCTIONS for that Without it, at the tip of Tony's branch (or in blead): $ cat cpan/Time-HiRes/xdefine With it: $ cat cpan/Time-HiRes/xdefine which is the same as v5.16.0: Note that on both platforms static inline is real: $ grep ^optimize config.sh $ grep ^optimize config.sh so it's not some messy artefact of Configure faking "static inline" by Having had a big dig around CPAN, I think that there are only 3 (possibly 4) Time::HiRes and possibly Digest::MD5 and its copy-paste-edit variant. "Possibly", because So, in terms of least worst, I'm wondering if this is the way to go. Nicholas Clark PS Note that patch hasn't touched cflags.SH - it's not "finished" From eb4e33350076afce74a53d559559640b40a9166e Mon Sep 17 00:00:00 2001 cpan/Time-HiRes/Makefile.PL | 3 ++- Inline Patchdiff --git a/cpan/Time-HiRes/Makefile.PL b/cpan/Time-HiRes/Makefile.PL
index 6f6a790..f27439e 100644
--- a/cpan/Time-HiRes/Makefile.PL
+++ b/cpan/Time-HiRes/Makefile.PL
@@ -115,7 +115,8 @@ __EOD__
}
}
- my $ccflags = $Config{'ccflags'} . ' ' . "-I$COREincdir";
+ my $ccflags = $Config{'ccflags'} . ' ' . "-I$COREincdir"
+ . ' -DPERL_NO_INLINE_FUNCTIONS';
if ($^O eq 'VMS') {
$cccmd = "$Config{'cc'} /include=($COREincdir) $tmp.c";
diff --git a/perl.h b/perl.h
index 5639f1c..f2a5b52 100644
--- a/perl.h
+++ b/perl.h
@@ -5239,8 +5239,17 @@ EXTCONST bool PL_valid_types_NV_set[];
#endif
-/* Static inline funcs that depend on includes and declarations above */
-#include "inline.h"
+#ifndef PERL_NO_INLINE_FUNCTIONS
+/* Static inline funcs that depend on includes and declarations above.
+ Some of these reference functions in the perl object files, and some
+ compilers aren't smart enough to eliminate unused static inline functions,
+ so including this file in source code can cause link errors even if the
+ source code uses none of the functions. Hence including these can be be
+ suppressed by setting PERL_NO_INLINE_FUNCTIONS. Doing this will (obviously)
+ result in unworkable XS code, but permits simple probing code to continue
+ to work. */
+# include "inline.h"
+#endif
#include "overload.h"
diff --git a/t/porting/extrefs.t b/t/porting/extrefs.t
index c19eb56..95c59b3 100644
--- a/t/porting/extrefs.t
+++ b/t/porting/extrefs.t
@@ -26,6 +26,8 @@ use Cwd;
plan(tests => 1);
+my $VERBOSE = grep {/^-v$/} @ARGV;
+
ok(try_compile_and_link(<<'CODE'));
#include "EXTERN.h"
#include "perl.h"
@@ -44,7 +46,6 @@ sub try_compile_and_link {
my $ld_exeext = ($^O eq 'cygwin' || $^O eq 'MSWin32' ||
$^O eq 'os2' && $Config{ldflags} =~ /-Zexe\b/) ? '.exe' :
(($^O eq 'vos') ? $Config{exe_ext} : '');
- my $VERBOSE = 0;
my ($ok) = 0;
my $tempdir = tempfile();
@@ -66,7 +67,8 @@ sub try_compile_and_link {
my $COREincdir = File::Spec->catdir(File::Spec->updir);
- my $ccflags = $Config{'ccflags'} . ' ' . "-I$COREincdir";
+ my $ccflags = $Config{'ccflags'} . ' ' . "-I$COREincdir"
+ . ' -DPERL_NO_INLINE_FUNCTIONS';
if ($^O eq "MSWin32") {
$ccflags .= " -I../win32 -I../win32/include";
--
1.7.9.2 |
From @khwilliamsonOn 04/25/2013 01:14 PM, Nicholas Clark wrote:
This sounds sensible to me |
From @nwc10On Thu, Apr 25, 2013 at 03:23:31PM -0600, Karl Williamson wrote:
I've pushed this, and Tony's test case as smoke-me/nicholas/extrefs I think that if there aren't any problems, we should merge this for v5.18.0 It actually works well enough that this commit could be reverted: commit 43387ee Remove the non-inline function S_croak_memory_wrap from inline.h. I don't think that we should do it before 5.18.0, but it now seems to be an Nicholas Clark |
From @doughera88On Fri, 26 Apr 2013, Nicholas Clark via RT wrote:
I still haven't had time to reflect on this, but it sounds like you've Since the define is a no-op right now, CPAN modules can actually be
That was a "get-something-working-now" patch based on an incomplete -- |
From @rjbsOn Fri Apr 26 08:13:41 2013, nicholas wrote:
I don't see many smoke results on this, sadly, but I think we should land it unless there is -- |
From @nwc10On Tue, Apr 30, 2013 at 03:14:10PM -0700, Ricardo SIGNES via RT wrote:
There were smoke results for the previous iteration, which showed that There are now smoke results for this iteration. Everything is happy with I haven't submitted patches to the 3 modules on CPAN that are affected. Nicholas Clark |
From @SmylersNicholas Clark writes:
I think you want the rjbs/perldelta-5.18 branch. Smylers |
From @nwc10On Fri, May 03, 2013 at 01:35:39PM +0100, Smylers wrote:
Yes, but in turn I think that that branch wants to appear in blead first. Nicholas Clark |
From @rjbsOn Fri May 03 05:39:14 2013, nicholas wrote:
I've merged that branch. The delta is still in Porting until, probably, tomorrow. Maybe Tuesday. Meanwhile, marking this not-blocking. -- |
From @tonycozOn Fri May 03 05:39:14 2013, nicholas wrote:
I'm pretty sure this ticket can be closed now. I'll close it in a few days unless someone objects. Tony |
From @tonycozOn Fri Apr 26 08:13:41 2013, nicholas wrote:
Should it be reverted? I have no real opinion either way, but either we could revert it and Tony |
From @bulk88On Sun Jul 14 21:35:38 2013, tonyc wrote:
Commit message at -- |
From @nwc10On Sun, Jul 14, 2013 at 09:35:38PM -0700, Tony Cook via RT wrote:
As we now have the mechanisms in place to make a non-inline static function Where "put it back" is: On Tue, Jul 16, 2013 at 03:35:28PM -0700, bulk88 via RT wrote:
Nicholas Clark |
From @tonycozOn Tue Jul 16 15:35:28 2013, bulk88 wrote:
http://perl5.git.perl.org/perl.git/commit/28fbb451174e1613d5b90a4961cfb629936bea55
Applied with the SHA fix. Tony |
@tonycoz - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#116989 (status was 'resolved')
Searchable as RT116989$
The text was updated successfully, but these errors were encountered: