search

SDL-Hercules-390 / hyperion

The SDL Hercules 4.x Hyperion version of the System/370, ESA/390, and z/Architecture Emulator
Other
240 stars 90 forks source link

C4789 Build Error in general1.c with VS2019 v16.11.10 #479

Closed Peter-J-Jansen closed 9 months ago

Peter-J-Jansen commented 2 years ago

Adding DISABLE_MSVC_WARNING(4789) avoids this warning/error:

   Creating library msvc.AMD64.obj\hengine.lib and object msvc.AMD64.obj\hengine.exp
Generating code
F:\Qsync\Source\Repos\SDL-hyperion\general1.c(2262) : error C2220: the following warning is treated as an error
F:\Qsync\Source\Repos\SDL-hyperion\general1.c(2262) : warning C4789: buffer 'tmp' of size 16 bytes will be overrun; 16 bytes will be written starting at offset 48
F:\Qsync\Source\Repos\SDL-hyperion\general1.c(2262) : warning C4789: buffer 'tmp' of size 16 bytes will be overrun; 16 bytes will be written starting at offset 32
F:\Qsync\Source\Repos\SDL-hyperion\general1.c(2262) : warning C4789: buffer 'tmp' of size 16 bytes will be overrun; 16 bytes will be written starting at offset 16
F:\Qsync\Source\Repos\SDL-hyperion\general1.c(2233) : warning C4789: buffer 'tmp' of size 16 bytes will be overrun; 16 bytes will be written starting at offset 48
F:\Qsync\Source\Repos\SDL-hyperion\general1.c(2233) : warning C4789: buffer 'tmp' of size 16 bytes will be overrun; 16 bytes will be written starting at offset 32
F:\Qsync\Source\Repos\SDL-hyperion\general1.c(2233) : warning C4789: buffer 'tmp' of size 16 bytes will be overrun; 16 bytes will be written starting at offset 16
LINK : fatal error LNK1257: code generation failed
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\link.EXE"' : return code '0x4e9'
Stop.

Cheers,

Peter

Fish-Git commented 2 years ago

I think Bill Lewis (@wrljet) reported this same issue to me privately a short while ago, and I'll tell you the same thing I told him:

Your warnings are being caused by a bullshit compiler bug, which was reported over a year ago:

which has since been fixed.

When I build Hercules using VS2019 (on Windows 7, but I doubt the operating system has anything to do with it), I do not see those obviously bullshit / bogus warnings.

(Anyone who is reasonably competent at 'C' should be able to examine the code for themselves and determine there is no way for any type of buffer overflow to occur, especially not the way the warnings say they're occurring.)

I updated my VS2019 from 16.10.3 --> 16.11.9 on January 25, and doing a cl /? reports the following:

**********************************************************************
** Visual Studio 2019 Developer Command Prompt v16.11.9
** Copyright (c) 2021 Microsoft Corporation
**********************************************************************

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community>cl /?
Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30139 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

And in my Hercules log itself I'm seeing:

HHC01415I Build date: Jan 25 2022 at 12:11:23
HHC01417I Built with: Microsoft Visual Studio 2019 (MSVC 192930139 0)

(Note: Compiler Version 19.29.30139 --> MSVC 192930139 0)

Check your version of VS2019 and update it if it's old. (VS2019 should automatically notify you whenever there's an update and all you have to do is click yes.) (and then die of old age while waiting for it to finish of course, but that's completely different issue.)

I noticed there was yet another update available now too (16.11.10), which is currently being installed even as I type this. I'll let you know in a followup comment how well (or how poorly) it works too.

FWIW, I also installed VS2022 too in a different snapshot *`()`**, and Hercules builds just fine (no warnings) with VS2022 too.

 

*`()`** I do all my modern VStudio testing in a VMware virtual machine because I don't want to break my existing live/production VS2008 installation on my real host because it has worked well for many, many years and I'm not about to jeopardize breaking it.

wrljet commented 2 years ago

I updated to the latest, as of the day we had discussed this, VS2019 16.11.10 (16.11.32126.315) which has CL 19.29.30140. And it still gives the error.

Bill

Fish-Git commented 2 years ago

Check your version of VS2019 and update it if it's old.

(DOH!)   I just now noticed the TITLE of your GitHub Issue! "Windows 10 VS2019 (v16.11.10) Build Error due to C4789 in general1.c"

Fish-Git commented 2 years ago

Fish wrote:

(DOH!)   I just now noticed the TITLE of your GitHub Issue! "Windows 10 VS2019 (v16.11.10) Build Error due to C4789 in general1.c"

Bill Lewis wrote:

I updated to the latest, as of the day we had discussed this, VS2019 16.11.10 (16.11.32126.315) which has CL 19.29.30140. And it still gives the error.

Yeah, it looks like we're going to need to insert a DISABLE_MSVC_WARNING(4789) statement to silence this BULLSHIT warning bug.   (SIGH!)   :(

But I'd rather it be conditional based on the version of the compiler. See e.g. vsvers.h header.

wrljet commented 2 years ago

Fish,

Can we apply this just for the function(s) in question? That warning might possibly show up again later, for an actual legit reason, and I wouldn't want to miss those.

Bill

Fish-Git commented 2 years ago

Can we apply this just for the function(s) in question? That warning might possibly show up again later, for an actual legit reason, and I wouldn't want to miss those.

Yes, absolutely. I agree 100%. That is in fact what I was suggesting, even though I admit to not making that intention clear.

Peter? (@Peter-J-Jansen) Since you opened this issue I'll let you fix it if you want. Otherwise I'll do it if you want me to. Let me know.

Fish-Git commented 2 years ago

FYI:   My upgrade to 16.11.10 just finished and yes, I'm now also seeing the same problem too.   :(

Fish-Git commented 2 years ago

p.s. as both Bill and I are already aware, the problem was apparently introduced in VS2019 v16.11.10, but never actually fixed until VS2022, so upgrading to VS2022 is an alternative workaround.

But we should nevertheless still apply the suggested VS2019 workaround anyway IMO.

wrljet commented 2 years ago

Actually, when I first noticed it, I was a couple releases back, on 16.11.8 (16.11.32002.261).

Fish-Git commented 2 years ago

Actually, when I first noticed it, I was a couple releases back, on 16.11.8 (16.11.32002.261).

Interesting.

I personally never experienced it myself until just now after finally upgrading my 16.11.9 install to 16.11.10. Previously, I was using 16.10.3 which, like 16.11.9, didn't have the problem. So I guess it was introduced in 16.11.8, fixed in 16.11.9, and then re-introduced (re-broken) in 16.11.10!

(sigh)   Microsoft...   >:-(

Fish-Git commented 2 years ago

Peter? (@Peter-J-Jansen) Since you opened this issue I'll let you fix it if you want. Otherwise I'll do it if you want me to. Let me know.

Hello? (@Peter-J-Jansen)

FYI:  The following seems to work:

--- hyperion-1/general1.c   2022-03-03 11:30:32.292684800 -0800
+++ hyperion-0/general1.c   2022-03-03 11:21:20.605130100 -0800
@@ -2118,6 +2118,10 @@
             (S32)regs->GR_L(r1) > (S32)n ? 2 : 0;
 }

+#if defined(_MSVC_) && (_MSC_VER >= VS2019) && (_MSC_VER < VS2022)
+PUSH_MSVC_WARNINGS()
+DISABLE_MSVC_WARNING( 4789 )
+#endif

 /*-------------------------------------------------------------------*/
 /* B21A CFC   - Compare and Form Codeword                        [S] */
@@ -2300,6 +2304,10 @@
 #endif
 }

+#if defined(_MSVC_) && (_MSC_VER >= VS2019) && (_MSC_VER < VS2022)
+POP_MSVC_WARNINGS()
+#endif
+
 /*-------------------------------------------------------------------*/
 /* BA   CS    - Compare and Swap                              [RS-a] */
 /*-------------------------------------------------------------------*/

If I don't hear back from you in another day or so, I'll go ahead and commit the fix myself. But I just wanted to give you the opportunity to do so yourself so you get the credit.

OR... YOU could do it Bill if you want to, since the problem was technically first identified by you (albeit "unofficially").

Either way is fine. You or Bill or me.

wrljet commented 2 years ago

Fixed (warning ignored) by commit https://github.com/SDL-Hercules-390/hyperion/commit/c84cda3c0517f1fea2b1b6a78aa7f7123c667661

Fish-Git commented 2 years ago

Fixed (warning ignored) by commit c84cda3

Excellent. Thanks Bill. Closing Issue.

Peter-J-Jansen commented 2 years ago

Bill and Fish,

Thanks for helping with this.  All is working fine again. :-)

Cheers,

Peter

Peter-J-Jansen commented 10 months ago

Bill and Fish,

The problem also happens for VS2022, hence that I propose to extend your correction for any VS later than VS2019 like this : 

--- SDL-hyperion/general1.c     2023-11-18 15:05:52.303185600 +0100
+++ SDL-hyperion_VS2022/general1.c        2023-11-18 15:23:21.309152823 +0100
@@ -2121,7 +2121,7 @@
             (S32)regs->GR_L(r1) > (S32)n ? 2 : 0;
 }

-#if defined(_MSVC_) && (_MSC_VER >= VS2019) && (_MSC_VER < VS2022)
+#if defined(_MSVC_) && (_MSC_VER >= VS2019) /* && (_MSC_VER < VS2022) */
 PUSH_MSVC_WARNINGS()
 DISABLE_MSVC_WARNING( 4789 )
 #endif
@@ -2307,7 +2307,7 @@
 #endif
 }

-#if defined(_MSVC_) && (_MSC_VER >= VS2019) && (_MSC_VER < VS2022)
+#if defined(_MSVC_) && (_MSC_VER >= VS2019) /* && (_MSC_VER < VS2022) */
 POP_MSVC_WARNINGS()
 #endif

OK?

Cheers,

Peter

wrljet commented 10 months ago

Hello Peter,

I'm curious why you're seeing these errors again.

I build Hercules many times a week and haven't run into it. (just did builds, for example, with VS2019 16.11.32, and VS2022 17.7.5)

What MSVC version are you running, and what Hercules commit? I'd like to try it out on those.

That said, I don't see a problem with your suggested change, except I would use (_MSC_VER <= VS2022).

Bill

Peter-J-Jansen commented 10 months ago

Hi Bill,

My Hercules commit is the latest development branch, commit 4f007805.

I encountered the C4789 problem with VS2022 17.8.0 (which has _MSC_VER=1938), under Windows 11 22H2 OS Build 22621.2715. I think I may have built successfully with 17.7.X as well in the past, which could mean that 17.8.0 re-introduced the problem and Microsoft's regressing tests didn't catch it.

I'd agree on principle that (_MSC_VER <= VS2022) would be a better fix than just commenting the whole thing out, but I think there is a problem in general with comparing _MSC_VER against strings like VS####. I think that _MSC_VER is a 4-digit number in the 1900-1999 range as some 'net searches quickly show. And there are a few other such _MSC_VER tests present, i.e. in decimal.c and fthreads.h. I'd therefore suggest this fix (which I tested) : 

--- SDL-hyperion/general1.c     2023-10-25 10:49:57.800926559 +0200
+++ SDL-hyperion_C4789_FIX/general1.c  2023-11-19 13:40:50.549837600 +0100
@@ -2121,7 +2121,7 @@
             (S32)regs->GR_L(r1) > (S32)n ? 2 : 0;
 }

-#if defined(_MSVC_) && (_MSC_VER >= VS2019) && (_MSC_VER < VS2022)
+#if defined(_MSVC_) && (_MSC_VER >= 1920) && (_MSC_VER <= 1938)
 PUSH_MSVC_WARNINGS()
 DISABLE_MSVC_WARNING( 4789 )
 #endif
@@ -2307,7 +2307,7 @@
 #endif
 }

-#if defined(_MSVC_) && (_MSC_VER >= VS2019) && (_MSC_VER < VS2022)
+#if defined(_MSVC_) && (_MSC_VER >= 1920) && (_MSC_VER <= 1938)
 POP_MSVC_WARNINGS()
 #endif

That decimal.c and fthreads.h should be fixed as well I'd leave for another day / issue.

Feedback welcome!

Cheers,

Peter

wrljet commented 10 months ago

Peter, I have reproduced this now with VS2022 17.8.0 !

(If I were Fish, I could add some colorful commentary here about MSFT's compiler) (and I would have to agree with Fish!)

I also agree with changing from "VS2019", etc. to specific build numbers, such as "1920".

Also agree with decimal.c and fthreads.h being left alone.

Your proposed changes look good to me.

If you want, I'll make those edits and check it into the GitHub.

Bill

wrljet commented 9 months ago

Should be fixed with commit https://github.com/SDL-Hercules-390/hyperion/commit/871258623ba500e30dfb92614e9aa589b7fda98d

Bill

wrljet commented 9 months ago

Since duplicate issue https://github.com/SDL-Hercules-390/hyperion/issues/610 has been tested and declared fixed, I'm going to go ahead and re-close this issue (again).

Bill

Fish-Git commented 8 months ago

Peter Jansen wrote:

...but I think there is a problem in general with comparing _MSC_VER against strings like VS####.

There is no problem, except maybe the compiler versions #defined in vsvers.h are not finely grained enough in this particular case. That is to say, in this particular case, we should probably add a new explicit version-specific #define for VS2022 v17.8.0:

#define VS2022_8    1938                /* Visual Studio 2022 */

and then use THAT constant instead.

The whole point of using e.g. VS2022 instead of e.g. 1930 is for self documenting purposes. When a developer sees (_MSC_VER >= 1920) && (_MSC_VER <= 1938), they have no idea what compiler versions are being compared for, whereas with (_MSC_VER >= VS2019) && (_MSC_VER <= VS2022_8) they can clearly see precisely what compiler versions are being compared for.

Bill Lewis wrote:

I also agree with changing from "VS2019", etc. to specific build numbers, such as "1920".

I don't.