[PATCH] MSVC Perl doesn't use noreturn on noreturn functions

[p5pRT]

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

Searchable as RT114144$

[p5pRT]

From @bulk88

Created by @bulk88

I'll post the body through RT web interface.

Perl Info

Flags:
    category=core
    severity=wishlist

Site configuration information for perl 5.17.2:

Configured by Owner at Sun Jul 15 15:12:08 2012.

Summary of my perl5 (revision 5 version 17 subversion 2) configuration:
   
  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 -Od -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='-Od -MD -Zi -DDEBUGGING',
    cppflags='-DWIN32'
    ccversion='13.10.6030', gccversion='', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='link', ldflags ='-nologo -nodefaultlib -debug  -libpath:"c:\perl517\lib\CORE"  -machine:x86'
    libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib"
    libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib  comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib  netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib  comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib  netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl517.lib
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug  -libpath:"c:\perl517\lib\CORE"  -machine:x86'

Locally applied patches:
    


@INC for perl 5.17.2:
    C:/p517/perl/lib
    .


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

 		 	   		  

[p5pRT]

From @bulk88

I was looking at the assembly code output of XS modules today. I noticed
XS libraries always clean up after croaks and other noreturn functions.

From an ActivePerl 5.10, with -O1 XS module.
_____________________________________________
  522​: if (items != 3)
00361D5C 83 FE 03 cmp esi,3
00361D5F 74 10 je XS_Win32__API_MoveMemory+4Fh
(361D71h)
  523​: croak_xs_usage(cv, "Destination, Source, Length");
00361D61 8B 4D 0C mov ecx,dword ptr [cv]
00361D64 68 1C 43 36 00 push offset string "Destination,
Source, Length" (36431Ch)
00361D69 53 push ebx
00361D6A E8 9B F4 FF FF call S_croak_xs_usage (36120Ah)
00361D6F 59 pop ecx
00361D70 59 pop ecx
_____________________________________________
122​: /* prototype to pass -Wmissing-prototypes */
  123​: STATIC void
  124​: S_croak_xs_usage(pTHX_ const CV *const cv, const char *const
params);
  125​:
  126​: STATIC void
  127​: S_croak_xs_usage(pTHX_ const CV *const cv, const char *const params)
  128​: {
0036120A 55 push ebp
0036120B 8B EC mov ebp,esp
  129​: const GV *const gv = CvGV(cv);
0036120D 8B 01 mov eax,dword ptr [ecx]
0036120F 8B 40 28 mov eax,dword ptr [eax+28h]
  130​:
  131​: PERL_ARGS_ASSERT_CROAK_XS_USAGE;
  132​:
  133​: if (gv) {
00361212 85 C0 test eax,eax
00361214 74 4D je S_croak_xs_usage+59h (361263h)
  134​: const char *const gvname = GvNAME(gv);
00361216 8B 00 mov eax,dword ptr [eax]
00361218 8B 48 10 mov ecx,dword ptr [eax+10h]
  135​: const HV *const stash = GvSTASH(gv);
0036121B 8B 00 mov eax,dword ptr [eax]
0036121D 83 C1 08 add ecx,8
  136​: const char *const hvname = stash ? HvNAME(stash) : NULL;
00361220 85 C0 test eax,eax
00361222 74 1B je S_croak_xs_usage+35h (36123Fh)
00361224 F6 40 0B 02 test byte ptr [eax+0Bh],2
00361228 74 15 je S_croak_xs_usage+35h (36123Fh)
0036122A 8B 10 mov edx,dword ptr [eax]
0036122C 8B 52 0C mov edx,dword ptr [edx+0Ch]
0036122F 8B 40 0C mov eax,dword ptr [eax+0Ch]
00361232 8B 44 90 04 mov eax,dword ptr [eax+edx*4+4]
00361236 85 C0 test eax,eax
00361238 74 05 je S_croak_xs_usage+35h (36123Fh)
0036123A 83 C0 08 add eax,8
0036123D EB 02 jmp S_croak_xs_usage+37h (361241h)
0036123F 33 C0 xor eax,eax
  137​:
  138​: if (hvname)
00361241 85 C0 test eax,eax
  139​: Perl_croak(aTHX_ "Usage​: %s​::%s(%s)", hvname,
gvname, params);
00361243 FF 75 0C push dword ptr [params]
00361246 51 push ecx
00361247 74 13 je S_croak_xs_usage+52h (36125Ch)
00361249 50 push eax
0036124A 68 10 42 36 00 push offset string "Usage​: %s​::%s(%s)"
(364210h)
0036124F FF 75 08 push dword ptr [my_perl]
00361252 E8 C3 1C 00 00 call Perl_croak (362F1Ah)
00361257 83 C4 14 add esp,14h
  145​: }
  146​: }
0036125A 5D pop ebp
0036125B C3 ret
  140​: else
  141​: Perl_croak(aTHX_ "Usage​: %s(%s)", gvname, params);
0036125C 68 00 42 36 00 push offset string "Usage​: %s(%s)"
(364200h)
  142​: } else {
00361261 EB 09 jmp S_croak_xs_usage+62h (36126Ch)
  143​: /* Pants. I don't think that it should be possible to
get here. */
  144​: Perl_croak(aTHX_ "Usage​: CODE(0x%"UVxf")(%s)",
PTR2UV(cv), params);
00361263 FF 75 0C push dword ptr [params]
00361266 51 push ecx
00361267 68 E8 41 36 00 push offset string "Usage​:
CODE(0x%lx)(%s)" (3641E8h)
0036126C FF 75 08 push dword ptr [my_perl]
0036126F E8 A6 1C 00 00 call Perl_croak (362F1Ah)
00361274 83 C4 10 add esp,10h
  145​: }
  146​: }
00361277 5D pop ebp
00361278 C3 ret
____________________________________________

Same thing happens with the Perl 5.17 that this perlbug was filed with.
I won't post its assembly since its -Od and an unfair comparison.
Microsoft's docs on noreturn are here
http​://msdn.microsoft.com/en-us/library/k6ktzx3s%28v=vs.80%29.aspx .
It doesn't macro-wise fit in at all with perl macro
"__attribute__noreturn__" which for GCC builds is defined to
"__attribute__((noreturn))" which is the GCC token to mark a function
noreturn. I tried defining __attribute__noreturn__ to
__declspec(noreturn) but it syntax errored everywhere. So, how do I or
we get MSVC's noreturn support working? The fix will probably involve
regen/embed.pl since it generates proto.h, but I'm not sure whats the
best fix. I haven't full looked through how embed.pl works. Should
embed.pl be compiler aware or should the __attribute__noreturn__ macro
be changed from a object-like macro to a function-like macro or something?

[p5pRT]

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

[p5pRT]

From @bulk88

Made a (my first ever, not sure if I did it right) git patch to
implement Visual C noreturn to Perl. I think this should be applied to
maintenance 5.16/14/etc since its a small change yet a large reduction
of object code in the interp and all XS modules. It shouldn't break any
ABI compatibility since croak never has and never will return, and GCC
Perl already does noreturn.

From post preprocessor XS module,
_________________________________________
__declspec (dllimport, noreturn)
  void Perl_croak (PerlInterpreter * my_perl, const char *pat, ...);

__declspec (dllimport, noreturn)
  void Perl_croak_no_modify (PerlInterpreter * my_perl);

__declspec (dllimport, noreturn)
  void Perl_croak_sv (PerlInterpreter * my_perl, SV * baseex);

__declspec (dllimport, noreturn)
  void Perl_croak_xs_usage (PerlInterpreter * my_perl, const CV *
const cv, const char *const params);

_________________________________________
and I checked the asm and the stack clean up code was gone on my local
5.17 in a XS module.

[p5pRT]

From @bulk88

commit-83133db

[p5pRT]

@bulk88 - Status changed from 'open' to 'new'

[p5pRT]

From @bulk88

Subject​: [perl #114144] [PATCH] MSVC Perl doesn't use noreturn on noreturn functions
From​: perlbug-followup@​perl.org
To​: bulk88@​hotmail.com
Date​: Mon, 16 Jul 2012 13​:12​:33 -0700

Made a (my first ever, not sure if I did it right) git patch to
implement Visual C noreturn to Perl. I think this should be applied to
maintenance 5.16/14/etc since its a small change yet a large reduction
of object code in the interp and all XS modules. It shouldn't break any
ABI compatibility since croak never has and never will return, and GCC
Perl already does noreturn.

From post preprocessor XS module,
_________________________________________
__declspec (dllimport, noreturn)
void Perl_croak (PerlInterpreter * my_perl, const char *pat, ...);

__declspec (dllimport, noreturn)
void Perl_croak_no_modify (PerlInterpreter * my_perl);

__declspec (dllimport, noreturn)
void Perl_croak_sv (PerlInterpreter * my_perl, SV * baseex);

__declspec (dllimport, noreturn)
void Perl_croak_xs_usage (PerlInterpreter * my_perl, const CV *
const cv, const char *const params);

_________________________________________
and I checked the asm and the stack clean up code was gone on my local
5.17 in a XS module.

I'm forwarding this to p5p since I originally didn't plan to write a patch to fix the problem, but then I decided to write one, and I wasn't sure whether to file a new ticket or not for the patch, (I changed the title though of this ticket) and I don't want this ticket now having a patch attached to get lost.
 

[p5pRT]

From @bulk88

commit-83133db

[p5pRT]

From @nwc10

On Wed, Jul 18, 2012 at 03​:17​:34PM -0400, bulk 88 wrote​:

I'm forwarding this to p5p since I originally didn't plan to write a patch to fix the problem, but then I decided to write one, and I wasn't sure whether to file a new ticket or not for the patch, (I changed the title though of this ticket) and I don't want this ticket now having a patch attached to get lost.

Thanks. This is useful.

However, I really don't know MSVC, and I'm a bit troubled by the code, as
it's is conditional on _MSC_VER. It's analogous to using just __GNUC__
and hence assuming that a feature has been in gcc since the beginning, which
isn't always safe there.

Does this code work on all versions of MSVC? At least all that are new
enough to build perl? ActiveState deliberately uses a fairly old version,
doesn't it, meaning that fairly old versions are still used.

Inline Patch

diff --git a/win32/win32.h b/win32/win32.h
index e906266..2c821eb 100644
--- a/win32/win32.h
+++ b/win32/win32.h
@@ -66,8 +66,14 @@
 #if !defined(PERLDLL) && !defined(PERL_EXT_RE_BUILD)
 #  ifdef __cplusplus
 #    define PERL_CALLCONV extern "C" __declspec(dllimport)
+#    ifdef _MSC_VER
+#      define PERL_CALLCONV_NO_RET extern "C" __declspec(dllimport, noreturn)
+#    endif
 #  else
 #    define PERL_CALLCONV __declspec(dllimport)
+#    ifdef _MSC_VER
+#      define PERL_CALLCONV_NO_RET __declspec(dllimport, noreturn)
+#    endif
 #  endif
 #endif


Nicholas Clark

[p5pRT]

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

[p5pRT]

From @bulk88

On Wed Jul 18 12​:27​:10 2012, nicholas wrote​:

On Wed, Jul 18, 2012 at 03​:17​:34PM -0400, bulk 88 wrote​:

I'm forwarding this to p5p since I originally didn't plan to write a
patch to fix the problem, but then I decided to write one, and I
wasn't sure whether to file a new ticket or not for the patch, (I
changed the title though of this ticket) and I don't want this
ticket now having a patch attached to get lost.

Thanks. This is useful.

However, I really don't know MSVC, and I'm a bit troubled by the code,
as
it's is conditional on _MSC_VER. It's analogous to using just __GNUC__
and hence assuming that a feature has been in gcc since the beginning,
which
isn't always safe there.

Does this code work on all versions of MSVC? At least all that are new
enough to build perl? ActiveState deliberately uses a fairly old
version,
doesn't it, meaning that fairly old versions are still used.

I use Visual Studio/C 2003, ActiveState uses Visual C 6 with Platform
SDK circa 2003 headers for 32 bit builds (VC 6 is too old to know what
Win 2000/XP are out of the box) ( see
http​://code.activestate.com/lists/ppm/671/ ), and a Platform SDK circa
March 2005 for x64 (VS 2005 was released Oct 2005), so my setup is
pretty similar to ActiveState's I presume.

Looking at this
http​://fog.googlecode.com/svn-history/r754/trunk/Fog/Fog/Core/C++/CompilerMsc.h
and this
http​://books.google.com/books?id=Q4iP1mkfdtsC&lpg=PT745&ots=wZk5aAN-3t&dq=__declspec%20noreturn%20%20borland&pg=PT745#v=onepage&q=noreturn%20%20&f=false
(borland supported it, Perl doesn't support Borland anymore, and all
support code was removed a few years ago) makes me think declspec
noreturn was in VC 6 and older and has been there since the mid or early
1990s. VC 6 was released in 1998. On the other hand I did find a link
that says VC 6 didn't support no return,
http​://xapian.org/docs/sourcedoc/html/noreturn_8h_source.html . I think
someone trying to build it with an actual VC 6 is the only way to find
out. Wouldn't one of the smokers catch it?

I put the msc_ver checks into win32.h since there was already a ton of
them anyway in win32.h
http​://perl5.git.perl.org/perl.git/blob/HEAD​:/win32/win32.h#l195 .
Windows Perl has no equivalent of "unix configure" which can probe
compiler feature (I think thats what unix configure does) during the
build process.

I didn't test my patch on Win32 GCC Perl or any other compilers other
than my Studio 2003 although the change is so small that if the fallback
fails (PERL_CALLCONV_NO_RET to PERL_CALLCONV), it should be trivial to
fix it. I'm not sure how compatible my patch is with "I have a VC
ActivePerl, but I use Mingw to compile my XS modules" setup. I think the
declspec noreturn will just disappear at that point since proto.h
contains the macro (PERL_CALL__*__) and not the compiler specific expansion.

At the moment for some personal XS code, I "fixed" the problem by having
a separate compiland that creates a "my_croak" that doesn't use Perl's
headers but is a wrapper around vcroak, but its a ugly hack, fixing
Perl's headers is the correct thing to do.

[p5pRT]

From @bulk88

A link that says VC 6 has noreturn
http​://xapian.org/docs/sourcedoc/html/noreturn_8h_source.html , a table
to convert MS C compiler version numbers to retail names,
http​://stackoverflow.com/questions/70013/how-to-detect-if-im-compiling-code-under-visual-studio-8
.

[p5pRT]

From @jandubois

On Wed, 18 Jul 2012, Nicholas Clark wrote​:

On Wed, Jul 18, 2012 at 03​:17​:34PM -0400, bulk 88 wrote​:

I'm forwarding this to p5p since I originally didn't plan to write a
patch to fix the problem, but then I decided to write one, and I
wasn't sure whether to file a new ticket or not for the patch, (I
changed the title though of this ticket) and I don't want this
ticket now having a patch attached to get lost.

Thanks. This is useful.

However, I really don't know MSVC, and I'm a bit troubled by the code,
as it's is conditional on _MSC_VER. It's analogous to using just
__GNUC__ and hence assuming that a feature has been in gcc since the
beginning, which isn't always safe there.

It works with VC6 and later, and VC6 is the oldest version we support,
so I've committed it. I had to run `make regen` to regenerate proto.h,
which I included in the commit​:

  http​://perl5.git.perl.org/perl.git/commitdiff/12a2785c7

Cheers,
-Jan

[p5pRT]

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

[p5pRT]

From @bulk88

On Wed Jul 18 15​:37​:53 2012, jdb wrote​:

It works with VC6 and later, and VC6 is the oldest version we support,
so I've committed it. I had to run `make regen` to regenerate proto.h,
which I included in the commit​:

http​://perl5.git.perl.org/perl.git/commitdiff/12a2785c7

Cheers,
-Jan

I realized the noreturn support I wrote was incomplete, it only worked
for external DLL XS modules. This patch makes noreturn work inside the
perl interp. perl517.dll dropped a couple KB (before and after both -O1,
no DEBUGGING) after this patch and I checked the asm. I am pretty sure
the ticket can be closed after this 2nd patch is applied.

Strangely, I noticed under -Od with VS 2003, MSVC will write in C stack
clean up opcodes inside perl517.dll in the caller, if the noreturn
callee is the very last function call in the caller, on the other hand,
if the noreturn callee is in the middle or beginning, no c stack cleanup
opcodes appear, so I looked at it as -O1 for KB comparison instead.
There is nothing that can be done about this strange behavior with MSVC
noreturns on -Od. Its irrelevant since production VC/ActivePerl's are
compiled as -O1.

[p5pRT]

From @bulk88

commit-e09f2f3

[p5pRT]

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

[p5pRT]

From @jandubois

On Wed Jul 18 15​:37​:53 2012, jdb wrote​:

It works with VC6 and later, and VC6 is the oldest version we support,
so I've committed it. I had to run `make regen` to regenerate proto.h,
which I included in the commit​:

http​://perl5.git.perl.org/perl.git/commitdiff/12a2785c7

Not sure if I made some mistake when I tested the original commit, but
the code no longer compiles for me with VC6 (on the same machine where
it seemed to work earlier).

I've fixed the change with

http​://perl5.git.perl.org/perl.git/commitdiff/03c98af06f

[p5pRT]

From @jandubois

On Thu Jul 19 11​:49​:12 2012, bulk88. wrote​:

I realized the noreturn support I wrote was incomplete, it only worked
for external DLL XS modules. This patch makes noreturn work inside the
perl interp. perl517.dll dropped a couple KB (before and after both -O1,
no DEBUGGING) after this patch and I checked the asm. I am pretty sure
the ticket can be closed after this 2nd patch is applied.

Thanks, applied as

http​://perl5.git.perl.org/perl.git/commitdiff/50e8e1f58

[p5pRT]

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